Page MenuHomePhabricator

[AIX][XCOFF] parsing xcoff object file auxiliary header
ClosedPublic

Authored by DiggerLin on Jun 25 2020, 7:24 AM.

Details

Summary

the patch supports parsing the xcoff object file auxiliary header with llvm-readobj with option "auxiliary-headers"

the format of auxiliary header as
https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/filesreference/XCOFF.html#XCOFF__fyovh386shar

the object file
llvm/test/tools/llvm-readobj/XCOFF/Inputs/xcoff-32-xlc-exec
llvm/test/tools/llvm-readobj/XCOFF/Inputs/xcoff-32-xlc-obj.o
llvm/test/tools/llvm-readobj/XCOFF/Inputs/xcoff-64-xlc-exec

are build from the source code with xlc
struct Point {
double x;
double y;
double z;
};

Point add_points(Point a, Point b) {
Point p;
p.x = a.x + b.x;
p.y = a.y + b.y;
p.z = a.z + b.z;
return p;
}

int main() {
Point a = {1.0, 3.0, 4.0};
Point b = {2.0, 8.0, 5.0};
Point c = add_points(a, b);
return 0;
}

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jhenderson added inline comments.Oct 4 2021, 2:18 AM
llvm/docs/CommandGuide/llvm-readobj.rst
315

The underline here needs to match the title length or I think you'll get an error when building the doc.

llvm/test/tools/llvm-readobj/XCOFF/xcoff-auxiliary-header.test
2

Even though it's not currently required, I'd suggest using ## for comments and # for lit & FileCheck directives in this file, as likely at some point it'll contain YAML, and then you'd have to change every line in the test.

4

I don't think that should prevent it being added at this point (although you wouldn't be able to test it). That being said, do a check of other format-specific options to see if --all dumps them.

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

Are you sure you want to print something? I think for other formats, we often just don't do anything, or print an empty dictionary/set-like thing. E.g. what happens if you do --segments with an ELF object file?

599

I think AuxSize would be a more normal abbreviation, and would therefore be less likely to trip people up when typing.

601

I recommend explicitly initialising this to nullptr to avoid any risk of using an uninitialized variable.

605

Could this not just be a lambda? That would play friendlier with IDEs if nothing else.

633

CPU is an acronym, so should be all caps.

652

Delete the blank line after this comment. Also, I'd suggest you change it to something like "Handle unknown/unsupported data." or something like that.

653

Testing should be in the same patch as the code it is testing. Please don't split them into separate patches. If the functionality can be added standalone, and there's a sensible behaviour (even if that behaviour is "do nothing") without it, then that functionality and testing should be in a separate patch.

655–670

This is a mess, in my opinion. Please use reportWarning/reportUniqueWarning to report the warning as per other warnings in llvm-readobj. Printing the other raw data is fine, but should be prefixed with simply something like "Raw data from offset 0x1234:" rather than the ugly "!!!Warning".

678–681

See my above comment for the 32-bit case.

If possible, it would be good though if this check (whether it prints or not) was in some higher-level function that then calls this and the 32-bit function, rather than duplicating it.

682

As above.

684

= nullptr; as suggested above.

686

This is common between the two functions: consider moving it into a higher-level calling function, along with the check above.

688

Same as before: make this a lambda.

735

Same as before: use reportWarning/reportUniqueWarning to report warnings.

llvm/tools/llvm-readobj/llvm-readobj.cpp
100

File-format specific options are grouped separately to the generic options (see below for examples).

275

Don't forget to clang-format your modified code.

Esme added a comment.EditedOct 10 2021, 10:03 PM

Tests required for this patch can now be generated with D111487:

  1. dumping full contents of the aux file header for XCOFF32 and XCOFF64.
  2. reporting the warning message for partial field by specifying the value of AuxiliaryHeaderSize in the FileHeader, ex.
--- !XCOFF
FileHeader:
  MagicNumber:         0x1DF
  AuxiliaryHeaderSize: 71
AuxiliaryHeader:
  Magic: 0x10B
...
  1. reporting the warning message for extra data by set the value of AuxiliaryHeaderSize bigger than AuxFileHeaderSize32/64, ex.
--- !XCOFF
FileHeader:
  MagicNumber:         0x1DF
  AuxiliaryHeaderSize: 75
AuxiliaryHeader:
  Magic: 0x10B
  1. dumping empty AuxiliaryHeader {} if AuxiliaryHeader is not set.

However, tests of this patch and tests of D111487 are interdependent. We just need to mark a TODO in this patch, and replace the tests after D111487 has landed too.

Esme added inline comments.Oct 10 2021, 10:15 PM
llvm/lib/Object/XCOFFObjectFile.cpp
1045

See D110320, it is better to report a detailed error message, i.g
auxiliary header with offset ** and size ** goes past the end of the file.

Esme added inline comments.Oct 11 2021, 2:02 AM
llvm/tools/llvm-readobj/Opts.td
29

To be consistent with the spec llvm/docs/CommandGuide/llvm-readobj.rst, please move it to XCOFF specific options with the title OPTIONS (XCOFF specific).

DiggerLin marked 19 inline comments as done.Oct 12 2021, 2:48 PM

Tests required for this patch can now be generated with D111487:

  1. dumping full contents of the aux file header for XCOFF32 and XCOFF64.
  2. reporting the warning message for partial field by specifying the value of AuxiliaryHeaderSize in the FileHeader, ex.
--- !XCOFF
FileHeader:
  MagicNumber:         0x1DF
  AuxiliaryHeaderSize: 71
AuxiliaryHeader:
  Magic: 0x10B
...
  1. reporting the warning message for extra data by set the value of AuxiliaryHeaderSize bigger than AuxFileHeaderSize32/64, ex.
--- !XCOFF
FileHeader:
  MagicNumber:         0x1DF
  AuxiliaryHeaderSize: 75
AuxiliaryHeader:
  Magic: 0x10B
  1. dumping empty AuxiliaryHeader {} if AuxiliaryHeader is not set.

However, tests of this patch and tests of D111487 are interdependent. We just need to mark a TODO in this patch, and replace the tests after D111487 has landed too.

thanks for your work.

llvm/docs/CommandGuide/llvm-readobj.rst
315

thanks

llvm/lib/Object/XCOFFObjectFile.cpp
1045

when we parses the FileHeader (line 894~897) , Section header, symbol table etc in the function. they use the same mechanism to deal with error as AuxiliaryHeader . If you strong suggestion that we need to report the detail error message , we can create a separate patch to deal with all the parse error at the same time after the patch landed.

llvm/test/tools/llvm-readobj/XCOFF/xcoff-auxiliary-header.test
2

thanks

4

if add --all option in the test, it will invoke the function

if (opts::UnwindInfo)
   Dumper->printUnwindInfo();

and for XCOFF. XCOFFDumper::printUnwindInfo() not implement

void XCOFFDumper::printUnwindInfo() {
  llvm_unreachable("Unimplemented functionality for XCOFFDumper");
}

It will be crash.

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

I think print out some info will let user know what happen on the xcoff object file.

599

Ok, thanks

605

the T is a member of class XCOFFDumper.cpp , I do not think C++11 support a template lambda.

678–681

I think it is difficult . we call the function here

void XCOFFDumper::printAuxiliaryHeader() {
  if (Obj.is64Bit())
    printAuxiliaryHeader(Obj.auxiliaryHeader64());
  else
    printAuxiliaryHeader(Obj.auxiliaryHeader32());
}
686

I do not think I can not

DictScope DS(W, "AuxiliaryHeader");

in higher level,
if I put in the higher level as

void XCOFFDumper::printAuxiliaryHeader() {
  DictScope DS(W, "AuxiliaryHeader");
  if (Obj.is64Bit())
    printAuxiliaryHeader(Obj.auxiliaryHeader64());
  else
    printAuxiliaryHeader(Obj.auxiliaryHeader32());
}

it will always print out the "AuxiliaryHeader" even the (AuxHeader == nullptr)

688

same comment as before.

DiggerLin updated this revision to Diff 379204.Oct 12 2021, 3:23 PM
DiggerLin marked 16 inline comments as done.
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
653

added a new test for this.

DiggerLin updated this revision to Diff 379864.Oct 14 2021, 3:22 PM
DiggerLin edited the summary of this revision. (Show Details)
DiggerLin removed a reviewer: jasonliu.
DiggerLin added a reviewer: jasonliu.
Esme added inline comments.Oct 18 2021, 12:48 AM
llvm/tools/llvm-readobj/Opts.td
87
llvm/tools/llvm-readobj/XCOFFDumper.cpp
54

I noticed that the the two functions are very similar, can we make these common to avoid code duplication? As far as I know, almost all members are the same for 32-bit and 64-bit except for "Additional flags 64-bit XCOFF", XCOFF64Flag.
I.e. use template <typename AuxT> void printAuxiliaryHeader(AuxT *AuxHeader); to replace them.

DiggerLin updated this revision to Diff 380387.Oct 18 2021, 7:23 AM
DiggerLin marked 2 inline comments as done.
DiggerLin added inline comments.Oct 21 2021, 6:26 AM
llvm/tools/llvm-readobj/Opts.td
87

thanks

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

I have think it over before, But I do not think it is possible to use a template to do it. because the member (and the order of the members) of XCOFFAuxiliaryHeader64 and XCOFFAuxiliaryHeader32 are different. and we need to print out the info based on the order which defined in the https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__fyovh386shar

Esme added a comment.Oct 21 2021, 11:50 PM

I have no more comments. But please wait and see if James @jhenderson has other comments.

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

Thanks.
In fact, I wouldn't require members to be printed strictly in the order of their offsets. But, of course, printing in order also makes sense to me.

DiggerLin marked an inline comment as done.Oct 22 2021, 6:03 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
54

thanks

DiggerLin marked an inline comment as done.Oct 22 2021, 10:46 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
54

I keep to print out the member strictly in the order of their offsets. it is more easy to read and figure out the error if there is errors when we encode of the xcoff object

DiggerLin added inline comments.Oct 22 2021, 11:00 AM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
686

sorry. it should be "I do not think I can" in above message

@jhenderson , @Esme , is there any more comments on the patch, if not ,I am very appreciated that anyone of you can approve it?

Esme accepted this revision.Oct 26 2021, 2:05 AM

LGTM with small nits.

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

Remove braces since it's a single-line if.

683

Ditto.

llvm/tools/llvm-readobj/llvm-readobj.cpp
354

Ditto.

This revision is now accepted and ready to land.Oct 26 2021, 2:05 AM
DiggerLin marked 3 inline comments as done.Oct 26 2021, 6:59 AM
This revision was landed with ongoing or failed builds.Oct 26 2021, 7:41 AM
This revision was automatically updated to reflect the committed changes.

This breaks tests on Windows: http://45.33.8.238/win/47662/step_11.txt

Please take a look and revert for now if it takes a while to fix.

I'm sorry I didn't come back to this - I've been struggling under an excessive review burden, and then had last week as PTO.

However, given my comments, I'd have preferred you had held off committing this a little longer. I don't believe this code is within acceptable LLVM quality at this time, and given the problem mentioned by @thakis, I think this should be reverted until the comments are addressed and the test fixed properly. (As far as I can tell, there is no reason why the test should not work on Windows, apart from some bug in this code - rGc2d2fb509306618b982bff94e1ad9acff6a41bcf appears to only be a temporary fix to get the build bot green again).

llvm/test/tools/llvm-readobj/XCOFF/xcoff-auxiliary-header.test
4

llvm_unreachable shouldn't be used where code is actually reachable. In such a case, it should just be a standard error message or similar (there are examples in LLVM/GNU style ELF output where one is implemented but not the other - look at those for the precedent). Not having a hard-error/crash would allow --all to be used.

125

Delete trailing blank line.

The final character at EOF should be \n, but not a character sequence of more than one \n.

llvm/tools/llvm-readobj/Opts.td
88

Existing help text style in llvm-readobj appears to be to have a leading capital (unlike error messages), so this should be "Display the auxiliary header".

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

Not sure I quite follow, but note that LLVM supports C++14 and auto can be used to much the same effect, if I'm not mistaken. I believe something like the following might work (note: I haven't tested, and am not particularly familiar with auto lambdas).

enum PrintStyle { Hex, Number };
auto PrintAuxMember32 = [&](PrintStyle P, StringRef S, auto *T) {
  ptrdiff_t Offset = T - AuxHeader;
  if (Offset + sizeof(*T) <= AuxSize) {
    switch(P) {
    case Hex:
      W.printHex(S, *T);
      break;
    case Number:
      W.printNumber(S, *T);
      break;
    }
  } else if (Offset < AuxSize) {
    PartialFieldOffset = Offset;
    PartialFieldName = S;
  }
}; 

PrintAuxMember32(Hex, "Magic", &AuxHeader->AuxMagic);
PrintAuxMember32(Hex, "Version", &AuxHeader->Version);
// etc...

You might be able to avoid the enum using some sort of std::bind call or similar, which would be nicer, but not essential.

Even without that, I think you should factor out the repeated offsetof calls into a variable, and use more meaningful variable names rather than H, S and T, just like you would for any function.

652

You ignored the second part of this comment...

654–655

Please refer to the LLVM coding standards for proper formatting of warning and error messages: such messages should start with a lower-case letter and not end with a full stop.

You don't need a string stream here, either. Just do:

std::string ErrInfo = (Twine("only partial field for") + PartialFieldName + " at offset (" + PartialFieldOffset)").str();
657–659
  1. Use createError, not make_error. See existing usage throughout llvm-readobj. Same applies in all other usages of make_error.
  2. Why are you using reportWarning(..., "-"); That reports a warning as if the error was in a file read in from stdin, which in many cases will be incorrect. a) We encourage using reportUniqueWarning in new llvm-readobj code, so that the same warning isn't reported multiple times; b) use the file name for the last argument, like it should be. Same below.
666
686

I think that's fine. We do similar things in other cases in ELF, see e.g. ELFDumper<ELFT>::printHashTable. It shows there is no content relevant to the AuxHeader printing, even though you asked for it.

735

This block looks very similar to the equivalent 32-bit version. Could you move it into a function?

@DiggerLin, have you seen my post-commit review?

@DiggerLin, have you seen my post-commit review?

I saw it , I will address the comment.

As far as I can tell, there is no reason why the test should not work on Windows, apart from some bug in this code - rGc2d2fb509306618b982bff94e1ad9acff6a41bcf appears to only be a temporary fix to get the build bot green again)

I think the fail in the window is related to
llvm-readobj --auxiliary-header %p/Inputs/xcoff-64-xlc-exec 2>&1

C:\src\llvm-project\llvm\test\tools\llvm-readobj\XCOFF\xcoff-auxiliary-header.test:91:11: error: WARN64: expected string not found in input
\# WARN64: {{.*}}llvm-readobj: warning: '<stdin>': There are extra data beyond auxiliary header

the window can not deal with 2>&1 correctly as linux (or aix) .
so I add a guardian
\# REQUIRES: system-aix
is OK. do you have any good solution ? @jhenderson

As far as I can tell, there is no reason why the test should not work on Windows, apart from some bug in this code - rGc2d2fb509306618b982bff94e1ad9acff6a41bcf appears to only be a temporary fix to get the build bot green again)

I think the fail in the window is related to
llvm-readobj --auxiliary-header %p/Inputs/xcoff-64-xlc-exec 2>&1

C:\src\llvm-project\llvm\test\tools\llvm-readobj\XCOFF\xcoff-auxiliary-header.test:91:11: error: WARN64: expected string not found in input
\# WARN64: {{.*}}llvm-readobj: warning: '<stdin>': There are extra data beyond auxiliary header

the window can not deal with 2>&1 correctly as linux (or aix) .
so I add a guardian
\# REQUIRES: system-aix
is OK. do you have any good solution ? @jhenderson

2>&1 works fine in all other tests that use it (it is commonly used for testing errors), so it won't ever be that. I ran this test locally, and it was clear what the problem was: your warning check starts with {{.*}}llvm-readobj: but on Windows, the executable name is llvm-readobj.exe, so the ".exe" doesn't match and the test fails. Adding (.exe)? would fix this, although I believe we usually don't bother checking the tool name as part of the warning/error message (take a look at other test cases to see what we typically check for llvm-readobj errors).

llvm/test/tools/llvm-readobj/XCOFF/xcoff-auxiliary-header.test
1

You can remove xcoff from the test name, as it's in the XCOFF folder. The xcoff bit of the name is superfluous.

Same goes for the input files - no need for a leading xcoff.

DiggerLin added a comment.EditedFri, Nov 19, 11:58 AM

@jhenderson , I am so sorry that delay so long to address comment, when I tried to revert the patch, but I found patch https://reviews.llvm.org/D111487 "[XCOFF][yaml2obj] support for the auxiliary file header." is depend on the patch and it commit. if I revert the patch , I have to revert the patch D111487 too.
Base on current situation , it maybe better to create a NFC patch to address your comments (after commit) for the patch ? what is your suggestion ?

@jhenderson , I am so sorry that delay so long to address comment, when I tried to revert the patch, but I found patch https://reviews.llvm.org/D111487 "[XCOFF][yaml2obj] support for the auxiliary file header." is depend on the patch and it commit. if I revert the patch , I have to revert the patch D111487 too.
Base on current situation , it maybe better to create a NFC patch to address your comments (after commit) for the patch ? what is your suggestion ?

Creating a new patch would be acceptable, yes. I'd suggest that, instead of additional reverts.