This is an archive of the discontinued LLVM Phabricator instance.

[XCOFF] llvm-readobj support decoding the loader section header field for XCOFF object file.
ClosedPublic

Authored by DiggerLin on Sep 29 2022, 8:20 AM.

Details

Summary

according to https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__vra3i31ejbau
we add a new option --loader-section-header for llvm-readobj to decode the decode the loader section header field of XCOFF object file.

Diff Detail

Event Timeline

DiggerLin created this revision.Sep 29 2022, 8:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 8:20 AM
DiggerLin requested review of this revision.Sep 29 2022, 8:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 8:20 AM

Please don't forget to update the documentation for new options.

I'm going to be on paid time off next week, and don't have the time to look at this today. I will look at it at some point after I get back.

Please don't forget to update the documentation for new options.

I'm going to be on paid time off next week, and don't have the time to look at this today. I will look at it at some point after I get back.

Enjoy your vacation!

jhenderson added inline comments.Oct 14 2022, 1:19 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
198

Reading the code, it seems like the CRTP (and therefore this base class) are unnecessary. In your lambda where you print the fields, you can use the common subclass memeber fields directly, because you've provided the input parameter type of the lamdba as auto.

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

Nit: don't start functions with a blank line.

169

Nit: Lambdas are function objects. The consensus is that their names should be written like variables, because they are passed around like them (i.e. PrintLoadSecHeaderCommon).

DiggerLin updated this revision to Diff 468553.Oct 18 2022, 8:22 AM
DiggerLin marked 2 inline comments as done.

address James' comment.

One more suggestion. I thought I made this in the previous version of the patch, but apparently not.

llvm/tools/llvm-readobj/llvm-readobj.cpp
514–515

This is a very odd way of getting it to print a specific part of the loader section. It would be much more normal to pass in booleans into printLoaderSection, rather than modifying the state of the dumper to ensure it does print it. Take a look at the printSymbols call for example.

DiggerLin marked an inline comment as done.Oct 19 2022, 8:34 AM
DiggerLin added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
198

thanks, good idea.

llvm/tools/llvm-readobj/llvm-readobj.cpp
514–515

thanks

DiggerLin updated this revision to Diff 468934.Oct 19 2022, 8:54 AM
DiggerLin marked an inline comment as done.

address comment.

jhenderson accepted this revision.Oct 20 2022, 2:28 AM

Looks good. Probably worth an XCOFF developer taking a quick look too, for more file-format specific aspects of this.

This revision is now accepted and ready to land.Oct 20 2022, 2:28 AM
Esme added a comment.Oct 20 2022, 8:19 PM
This comment was removed by Esme.
llvm/test/tools/llvm-readobj/XCOFF/loader-section-header.test
25

Nit: trailing whitespace in this line.

Esme accepted this revision.Oct 20 2022, 8:20 PM

LGTM. Thanks for your work.