Support XCOFFDumper relocation reading support
This patch is part of D103696 partition
Details
- Reviewers
jhenderson nemanjai jasonliu sfertile Esme daltenty hubert.reinterpretcast MaskRay Helflym - Group Reviewers
Restricted Project - Commits
- rG2cdfd0b25976: [AIX][XCOFF] 64-bit relocation reading support
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
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.
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. |
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. |
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? |
I created another patch to replace unwrapOrError with reportUniqueWarning.
Could you suggest someone who can take a look on it ?
Thank you
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.
Spotted one more case that needs a test.
llvm/tools/llvm-readobj/XCOFFDumper.cpp | ||
---|---|---|
107 | This needs a test. |
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
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? |
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.
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
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.