Page MenuHomePhabricator

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

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
jhenderson added inline comments.May 12 2021, 12:55 AM
llvm/include/llvm/Object/GOFF.h
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.May 12 2021, 10:50 AM

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

jhenderson added inline comments.May 13 2021, 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.May 14 2021, 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.May 14 2021, 11:20 AM
jhenderson added inline comments.May 17 2021, 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.May 17 2021, 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.May 17 2021, 12:12 PM
jhenderson requested changes to this revision.May 18 2021, 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.May 18 2021, 12:39 AM

Added more detail to error messages
Added missing include

yusra.syeda marked 6 inline comments as done.May 19 2021, 2:11 PM
yusra.syeda added inline comments.
llvm/include/llvm/Object/GOFF.h
126

Thanks for the explanation, I've added this include.

llvm/lib/Object/GOFFObjectFile.cpp
350–351

This error message has been updated.

yusra.syeda marked 2 inline comments as done.May 19 2021, 2:11 PM
jhenderson added inline comments.May 20 2021, 12:34 AM
llvm/lib/Object/GOFFObjectFile.cpp
97–98

Is there an index/ID/offset you could use in these messages? It will help users find the invalid record (since it may not be easy to identify which record has the specified type, if things are broken). Take a look at how llvm-readobj reports errors in ELF objects for some good examples.

Same goes for your other error messages.

370

Please ensure you build and test your code before uploading a patch - this will cause a build failure.

yusra.syeda marked an inline comment as done.May 25 2021, 6:41 AM
jhenderson added inline comments.May 26 2021, 12:57 AM
llvm/include/llvm/Object/GOFF.h
85

There are a number of these clang-tidy complaints, similar to how there were those SmallString complaints before. Would it make sense to move the body of this and similar functions into a cpp? That would avoid needing to include Error.h in this file directly.

Added record number to the error messages

yusra.syeda marked an inline comment as done.May 26 2021, 3:28 PM
yusra.syeda added inline comments.
llvm/lib/Object/GOFFObjectFile.cpp
97–98

I've added the record numbers to the error messages to make them more descriptive. Section indices which is used by ELF is not available in GOFF.

yusra.syeda marked an inline comment as done.

Moved function body of the getData function to GOFFObjectFile.cpp to avoid clang tidy warning

yusra.syeda marked an inline comment as done.May 26 2021, 8:02 PM
yusra.syeda added inline comments.
llvm/include/llvm/Object/GOFF.h
85

Done.

yusra.syeda marked an inline comment as done.May 26 2021, 8:03 PM

Nearly there. Not approving since there's a build error though...

llvm/lib/Object/GOFFObjectFile.cpp
77

Shouldn't this be a size_t or similar? It's an offset, and can't be negative.

86

Please make sure you at least build your changes before uploading them...

Changed variable type from int to size_t for RecordNum variable, fixed build error, and added size to enums

yusra.syeda marked 2 inline comments as done.May 27 2021, 7:08 AM
jhenderson accepted this revision.May 28 2021, 12:39 AM

LGTM, with one spelling error. Thanks!

llvm/lib/Object/GOFFObjectFile.cpp
86
This revision is now accepted and ready to land.May 28 2021, 12:39 AM
yusra.syeda marked an inline comment as done.May 28 2021, 6:42 AM
MaskRay added inline comments.Jul 15 2021, 11:48 PM
llvm/include/llvm/BinaryFormat/GOFF.h
26

inconsistent constexpr/const. You can use constexpr everywhere.

llvm/include/llvm/Object/GOFF.h
26

the prevailing code style doesn't insert a blank line between nested namespaces

83

delete user-defined constructor which does nothing. ditto elsewhere in the file

llvm/lib/Object/GOFFObjectFile.cpp
26

llvm::object

339

!= is more common

Removed unnecessary white space, empty constructors, and addressed minor comments

This needs to be rebased on latest main.