Page MenuHomePhabricator

[SystemZ][z/OS] Add GOFFObjectFile class support for HDR, ESD and END records
Needs RevisionPublic

Authored by yusra.syeda on Mar 11 2021, 10:12 AM.

Details

Summary

This patch is a breakdown of https://reviews.llvm.org/D89071, adding support for only the HDR, ESD and END GOFF records.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Double ping :) All comments have been addressed. Reviewers, please have a look.

kpn added a comment.Apr 6 2021, 11:17 AM

The convention here is to wait a week or so before a ping, and the same between pings.

llvm/include/llvm/Object/GOFF.h
55

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.

248

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?

420

END looks complete enough for now. I hope you'll flesh it out more in a later patch. Same with HDR cards.

llvm/include/llvm/Object/GOFFObjectFile.h
98

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.

llvm/lib/Object/GOFFObjectFile.cpp
60

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.

Added more error checking

yusra.syeda marked 5 inline comments as done.Apr 9 2021, 11:39 AM
In D98437#2672051, @kpn wrote:

The convention here is to wait a week or so before a ping, and the same between pings.

Thanks for letting me know, I wasn't aware of this.

llvm/include/llvm/Object/GOFF.h
55

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.

248

This function is not needed for this patch. I removed it and will update the functionality for the future patches.

llvm/lib/Object/GOFFObjectFile.cpp
60

I've added the error checking.

yusra.syeda marked 3 inline comments as done.Apr 9 2021, 11:39 AM
anirudhp added inline comments.
llvm/include/llvm/Object/GOFF.h
74

Couldn't we do away the conditional assignment to Value and just pass Continuation directly to setBits?

79

Same comment as above.

101

minor nit: not too sure why "MSB" is capitalized here.

225

Same comment about using bool value directly in the next function call

Address comment regarding removing unnecessary variables
Address clang tidy suggestion

yusra.syeda marked 4 inline comments as done.Apr 14 2021, 2:49 PM
yusra.syeda added inline comments.
llvm/include/llvm/Object/GOFF.h
74

Yes, I updated this here and below.

yusra.syeda marked an inline comment as done.Apr 14 2021, 2:50 PM

Clean up some formatting

kpn added a comment.Tue, Apr 20, 10:46 AM

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.

llvm/include/llvm/Object/GOFF.h
60

It looks like this allows for invalid GOFF where more data is expected but the last card of the continuation is hit.

Added the following tests:
Invalid cases of continutation records
Check for missing HDR record
Check for missing END record
Handling of concatenated GOFF object files

yusra.syeda added inline comments.Fri, Apr 30, 8:19 AM
llvm/include/llvm/Object/GOFF.h
60

This shouldn't be the case due to the check if (DataLength <= 77). This guarantees that the remaining data will fit in the last continuation.

kpn added a comment.Mon, May 3, 10:48 AM

OK, I have nothing more. I'll leave formal signoff to @jhenderson.

jhenderson added inline comments.Tue, May 4, 3:21 AM
llvm/include/llvm/BinaryFormat/GOFF.h
13

Nit: add trailing . to the sentence.

llvm/include/llvm/Object/GOFF.h
36

Any particular reason you've used (char)0x00 rather than simply \0?

40–41

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.

53

"begin" -> either "beginning" or "start"

"begin" is a verb, not a noun.

55–60

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.

llvm/lib/Object/GOFFObjectFile.cpp
277–280

Test case?

293–296

Test case?

336–337

Test case?

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

yusra.syeda marked 6 inline comments as done.Tue, May 11, 11:25 AM
jhenderson added inline comments.Wed, May 12, 12:55 AM
llvm/include/llvm/Object/GOFF.h
154–156
185

Why did you change the name casing here? LLVM style is UpperCamelCase.

186–188
416–418
llvm/lib/Object/GOFFObjectFile.cpp
78
191–193

No need for braces for single-line if.

350–351

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).

llvm/unittests/Object/GOFFObjectFileTest.cpp
262

Nit: this could be EXPECT_THAT_EXPECTED. The test doesn't need to abort, if this check fails.

312

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.

455

As above.

496

As above.

Addressed comments and cleaned up code to adhere to LLVM style

yusra.syeda marked 10 inline comments as done.Wed, May 12, 10:50 AM

Moved non trivial function out of header file and into llvm/lib/Object/GOFFObjectFile.cpp

jhenderson added inline comments.Thu, May 13, 12:18 AM
llvm/include/llvm/Object/GOFF.h
36

Ping this comment?

40

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).

126

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.

156

Ditto.

384

Ditto.

llvm/lib/Object/GOFFObjectFile.cpp
191

Have you built this code? This error looks like it would break the build...

438

Fixed function naming style

yusra.syeda marked 7 inline comments as done.Fri, May 14, 11:20 AM
yusra.syeda added inline comments.
llvm/include/llvm/Object/GOFF.h
36

Using \0 causes a build failure.

llvm/lib/Object/GOFFObjectFile.cpp
191

This is fixed with the new changes.

yusra.syeda marked an inline comment as done.Fri, May 14, 11:20 AM
jhenderson added inline comments.Mon, May 17, 12:46 AM
llvm/include/llvm/Object/GOFF.h
36

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?

llvm/lib/Object/GOFFObjectFile.cpp
84

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.

350–351

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.

443–444

Same as my other comments.

Removed setters from llvm/include/llvm/Object/GOFF.h.
I will add more details to the error messages in the next update.

yusra.syeda marked 3 inline comments as done.Mon, May 17, 12:12 PM
yusra.syeda added inline comments.
llvm/include/llvm/Object/GOFF.h
36

I removed the setters from this file as they are not required for this patch. The need for this change no longer applies.

126

I'm not sure which include I'm missing. Shouldn't a missing include cause a build failure?

llvm/lib/Object/GOFFObjectFile.cpp
84

Thanks, I will add more details to this error message and the others you pointed out.

yusra.syeda marked an inline comment as done.Mon, May 17, 12:12 PM
jhenderson requested changes to this revision.Tue, May 18, 12:39 AM

The pre-merge checks show build failures for Windows and Darwin. Please review them and fix as appropriate.

llvm/include/llvm/Object/GOFF.h
126

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.

This revision now requires changes to proceed.Tue, May 18, 12:39 AM