This is an archive of the discontinued LLVM Phabricator instance.

[AIX][XCOFF] 64-bit relocation reading support
ClosedPublic

Authored by MaryamBen on Jun 21 2021, 7:50 AM.

Details

Summary

Support XCOFFDumper relocation reading support
This patch is part of D103696 partition

Diff Detail

Event Timeline

MaryamBen requested review of this revision.Jun 21 2021, 7:50 AM
MaryamBen created this revision.
MaryamBen edited the summary of this revision. (Show Details)Jun 22 2021, 12:31 AM

For a patch advertised as llvm-readobj relocation dumping support, I wouldn't expect to see test changes in the CodeGen directory.

If you need the llvm-readobj changes to test some other functionality in other locations, you should make the llvm-readobj patch first, and then add testing for the other part in a separate patch.

I've said this elsewhere on XCOFF patches, but I really don't like the sort of duplication that the 32-bit/64-bit implementations in the ObjectFile code have. It seems to me like you should be able to avoid it by some additional abstraction. Take a look at the equivalent ELF code - there we use templates to avoid needing to duplicate most code, and/or common objects that hide the details from the user.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
136

These changes to use warnings are good, but should be their own separate patch, along with appropriate testing in tools/llvm-readobj.

141

It's not clear to me what the type of Relocations is here, and similarly the ErrOrRelocations above. Don't use auto unless the type is obvious from the RHS (or for some iterator cases). See the Coding Standards for details.

147–148

Ditto.

MaryamBen updated this revision to Diff 354483.Jun 25 2021, 6:35 AM

I kept one test from llvm-objdump. Is it okey, or I should move it to another patch?

MaryamBen marked 2 inline comments as done.Jun 25 2021, 6:36 AM
MaryamBen updated this revision to Diff 354501.Jun 25 2021, 7:54 AM
jhenderson added inline comments.Jun 29 2021, 1:15 AM
llvm/lib/Object/XCOFFObjectFile.cpp
353

I believe this will cause an assertion in some configurations because you don't do anything with a failing error. You should either report it, or more likely in this case, use consumeError() with a TODO saying the error should be propagated up the stack, to be reported elsewhere.

You also need to make sure you've got a test case that triggers this error. Whether it is a lit test or a gtest unit test, I don't mind, although I suspect the latter may be easier.

716–717

Perhaps name these types more descriptively, e.g. Shdr and Reloc.

727

It would be nice if this static assert included a message that the compiler can properly report.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
124–125

As elsewhere, consider renaming these types to be more descriptive.

I kept one test from llvm-objdump. Is it okey, or I should move it to another patch?

The problem is that you label this patch as "llvm-readobj 64-bit relocation reading support", which suggests this is specific to llvm-readobj. You shouldn't conflate llvm-readobj and llvm-objdump. If the changes in the patch mean that llvm-objdump starts to just work, then consider updating the patch summary.

MaryamBen updated this revision to Diff 356626.Jul 6 2021, 12:58 AM
MaryamBen retitled this revision from [AIX][XCOFF] llvm-readobj 64-bit relocation reading support to [AIX][XCOFF] 64-bit relocation reading support.
MaryamBen marked 2 inline comments as done.Jul 6 2021, 12:59 AM
jhenderson added inline comments.Jul 6 2021, 1:10 AM
llvm/lib/Object/XCOFFObjectFile.cpp
334

I'd add a TODO to say to report the error up the stack. Same below.

In general, consumeError calls should be accompanied with a comment saying why it's okay to throw away the error. In this case, it's just because the interface doesn't currently support reporting the error, hence the TODO to improve the situation.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
147–150

Or something similar - "Rel" could easily be confused with the variable name. I'd probably change the other instanecs in a similar manner too.

MaryamBen updated this revision to Diff 356815.Jul 6 2021, 1:45 PM
MaryamBen marked 3 inline comments as done.Jul 6 2021, 1:50 PM

Thank you for your valuable comments

MaryamBen marked an inline comment as done.Jul 6 2021, 1:53 PM
jhenderson added inline comments.Jul 7 2021, 12:32 AM
llvm/lib/Object/XCOFFObjectFile.cpp
334

Nit: The common pattern is // TODO: <some text>. (note the colon, also it's a comment, so should have a trailing '.').

Same below.

MaryamBen updated this revision to Diff 356884.Jul 7 2021, 12:37 AM
MaryamBen marked an inline comment as done.

Any news regarding the patch?

Apologies, this patch somehow dropped off my radar. I've still got one outstanding comment (move the error to warning switch to a prerequisite patch). Beyond that, this should probably get another pair of eyes on it, ideally from someone with XCOFF experience. I don't feel comfortable approving it myself.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
136

It looks like this comment has been ignored? Or does this patch need rebasing on a different patch that addresses this point?

MaryamBen marked 2 inline comments as done.Jul 26 2021, 2:59 AM

I created another patch to replace unwrapOrError with reportUniqueWarning.
Could you suggest someone who can take a look on it ?
Thank you

I created another patch to replace unwrapOrError with reportUniqueWarning.
Could you suggest someone who can take a look on it ?

I assume you mean this patch (I'll take a look at the unwrapOrError one)? Take a look at who has made changes to the XCOFF code up to now, and add them as reviewers.

MaryamBen updated this revision to Diff 362297.Jul 28 2021, 1:06 AM

Spotted one more case that needs a test.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
107

This needs a test.

MaryamBen marked an inline comment as done.Aug 6 2021, 12:31 AM
MaryamBen added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
107

It's checked in D104913, since you have asked to put the tests from the CodeGen directory in a different patch.

Helflym accepted this revision.Aug 6 2021, 1:00 AM
Helflym added a subscriber: Helflym.

Ok for the XCOFF part

This revision is now accepted and ready to land.Aug 6 2021, 1:00 AM
daltenty added a comment.EditedAug 17 2021, 9:18 AM

nit: the existing relocation test llvm/test/tools/llvm-objdump/XCOFF/print-reloc.test could probably have the 64-bit cases added now, but that can easily be handled in a follow on patch if required

daltenty accepted this revision as: daltenty.Aug 18 2021, 9:02 AM

LGTM, apart from minor nits (and possibly follow on tests). I can assist with committing this once you're ready.

llvm/lib/Object/XCOFFObjectFile.cpp
702

nit: maybe we'd benefit from an assert or some other way to make sure the type T and the bitmode agree?

sfertile added inline comments.Aug 19 2021, 11:19 AM
llvm/lib/Object/XCOFFObjectFile.cpp
702

This is a good suggestion. As an alternative (and it can come as a post commit NFC patch if need be) would it make sense to specialize this for both valid section header types since the implementation is not the same for 32-bit vs 64-bit?

We'll follow up with the rest of the minor suggestions here in a NFC follow-on, since the author will be unavailable after today and these are minor cleanups.

This revision was landed with ongoing or failed builds.Aug 19 2021, 6:57 PM
This revision was automatically updated to reflect the committed changes.
MaryamBen marked an inline comment as done.Aug 20 2021, 12:21 AM

LGTM, apart from minor nits (and possibly follow on tests). I can assist with committing this once you're ready.

I'm sorry, I just saw your comment.

We'll follow up with the rest of the minor suggestions here in a NFC follow-on, since the author will be unavailable after today and these are minor cleanups.

Thank you