This patch is a breakdown of https://reviews.llvm.org/D89071, adding support for only the HDR, ESD and END GOFF records.
The convention here is to wait a week or so before a ping, and the same between pings.
Doesn't this allow for malformed GOFF where a set of continued records isn't properly terminated?
Also, because the last continued record can be recognized, it isn't necessary for this function to know how much data to expect. This implies that the different card types don't need to send down information on how many continued records are needed. They can get out of the continuation record business totally.
So how does one set a name that is too long for one record?
Why are we dealing with continuation records at this level still?
END looks complete enough for now. I hope you'll flesh it out more in a later patch. Same with HDR cards.
Yeah, I was kidding.
GOFF supports ED symbols larger than zero that could have TXT cards but don't. Said ED is allowed to have LD symbols. At run time space in memory is allocated for the ED, the LD's work correctly, and the memory is filled with the fill byte (typically zero). Which is like how BSS works.
But since GOFF doesn't require that ED's with no TXT be grouped together, the Unix concept of BSS doesn't really apply. Thus returning false is correct.
Doesn't this allow for "rogue" continuation records? Records that are marked as continuation records but have no first record appear to be allowed and silently ignored.
Granted, this sort of bug should be rare in practice. It seems most likely with a new GOFF reader/writer. But, of course, that's exactly what we have here. So a check for the error here would be helpful.
Trying to debug bad GOFF where the only diagnostic is a binder abend is... not much fun. That's why I'd like to see easy checks for errors here.
Thanks for letting me know, I wasn't aware of this.
It is possible that the last continuation is not completely filled with data - this is why I pass how much data to expect to this function. I did add a check to ensure the last continuation is terminated properly.
This function is not needed for this patch. I removed it and will update the functionality for the future patches.
I've added the error checking.
There's still not much testing here, and there is plenty to test. The continuation records, for example. Tests can verify that all the invalid cases are rejected. Tests can also verify that GOFF is rejected if it is missing an HDR card or missing an END card. And since GOFF objects can be concatenated you can test that an object deck that is HDR+ESD+END+HDR+ESD+END is handled properly.
I have found that round tripping is a good, efficient way to test. If I can't write out what I just read in without changing it then there might be a problem.
It looks like this allows for invalid GOFF where more data is expected but the last card of the continuation is hit.
Nit: add trailing . to the sentence.
Any particular reason you've used (char)0x00 rather than simply \0?
This and several other methods within this file look like they belong in the .cpp file. We should avoid pointing non-trivial functions in the header.
"begin" -> either "beginning" or "start"
"begin" is a verb, not a noun.
I'm just trying to understand these assertions here. Could a malformed input potentially cause these assertions to trigger, or will such malformed data be detected earlier in the program and this code path avoided? If these asserts could fire with malformed data, then they should not be assertions, but proper error checks.
Addressed some comments and added the following tests:
Check for invalid symbol types
Check for invalid external reference executable types
Check if a record is a continuation, then the previous record should be marked as continued
Check that the last continuation record is terminated correctly
Why did you change the name casing here? LLVM style is UpperCamelCase.
No need for braces for single-line if.
This message could be improved, I believe. If I understand it correctly, this error is trying to say that a symbol's section is not found, rather than that a "symbol section" is not found. I think you should record more information in the message, i.e. which symbol is missing its section, and what should identify that section (index/name/ID or whatever - I don't really follow the actual logic).
Nit: this could be EXPECT_THAT_EXPECTED. The test doesn't need to abort, if this check fails.
This can be EXPECT_THAT_EXPECTED, right? There's no need to stop this test if it has a problem in this case, I'd think.
Ping this comment?
Please keep the old naming scheme. If you aren't familiar with it, check out the LLVM coding guidelines (https://llvm.org/docs/CodingStandards.html), which includes documentation on naming functions and variables (functions lowerCamelCase, variables UpperCamelCase being the gist).
Same comment as above. Also, these SmallString diagnostic errors might imply you're missing an include, so other usages of this header might have issues.
Have you built this code? This error looks like it would break the build...
Were you using '\0' (with quotes)? If so, getting an error doesn't seem right. The return type of (char)0x00 is identical to '\0'. What is the error that you are getting? What compiler version are you using?
Similar to my other comments, this and the following error messages don't really provide enough context for a user to addresse them. See https://llvm.org/docs/CodingStandards.html#error-and-warning-messages for explanation and examples.
Ping this comment? (It was referring to the "no symbol section found" error message).
Tip: if you have not yet addressed a comment, but have plans to do so in a future update, say so when you update the patch.
Same as my other comments.
I removed the setters from this file as they are not required for this patch. The need for this change no longer applies.
I'm not sure which include I'm missing. Shouldn't a missing include cause a build failure?
Thanks, I will add more details to this error message and the others you pointed out.
The pre-merge checks show build failures for Windows and Darwin. Please review them and fix as appropriate.
The file that defines SmallString is ADT/SmallString.h. I went through the include tree and I couldn't find any reference to this header. A build error for a missing include may not happen, depending on how this file is included. For example, if your cpp file includes are:
#include "ADT/SmallString.h" #include "Object/GOFF.h"
SmallString will be defined before GOFF.h is included, so the contents of GOFF.h will see it and not have a problem using it (remember, C and C++ includes are simply copy-and-paste of contents, so the final result is essentially equivalent to one big file).
If in the future, you created a source file that only included Object/GOFF.h, then you'd likely see the build error. At the moment, the only usage you have of Object/GOFF.h is the Object/GOFFObjectFile.h file, which includes other headers before this one. As such, I suspect that one of the headers that file includes will include SmallString.h, and therefore avoid the build error.
I therefore think you need to include SmallString.h in this file, to avoid confusing errors later.