This is an archive of the discontinued LLVM Phabricator instance.

[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
jasonliu added inline comments.Jul 8 2020, 8:42 AM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
500

Why do you need to pass in AuxSize to the macro function when all inputs are the same?

593

We should print this as hex.

llvm/tools/llvm-readobj/llvm-readobj.cpp
178

I'm assuming we need to add it somewhere in the llvm docs about this new option.

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

This strikes me as extremely hazardous. What if we get a length value that is reflective of a partial field?

500

AuxSize is modified by each macro(!) invocation...

DiggerLin marked 20 inline comments as done.Jul 9 2020, 1:15 PM
DiggerLin added inline comments.
llvm/include/llvm/Object/XCOFFObjectFile.h
90

thanks

llvm/lib/Object/XCOFFObjectFile.cpp
139

thanks

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

thanks

490

thanks

493

we print out the information based on the binary sequence of auxiliary header.
the same field is on different offset between the 32bit and 64 bits. it is difficult to implement in template.

for example.
o_tsize : Offset at 4 in 32bits , but Offset at 56 at 64bits.

500

for AuxSize is modified , I just make it looks like a function.

llvm/tools/llvm-readobj/llvm-readobj.cpp
178

thanks

DiggerLin updated this revision to Diff 276827.Jul 9 2020, 1:34 PM
DiggerLin marked 7 inline comments as done.

address comment

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

This macro does not operate within the confines of what a function can do with respect to its caller (it can cause the caller to return early). I do not believe that using a function-like naming style is appropriate. I also do not believe that using such a macro for control flow is desirable.

You can encode a table (yes, a macro is okay for that) with much the same information:
(format, description, pointer-to-member, offset in the table past-the-end of the member)

and use that table in the place where this macro is being invoked.

490

We still have to build with C++14 compilers for the time being. Assigning a large 64-bit value to a 32-bit signed type is verboten. In any case, checking the table size against the last field of the table I described above would avoid this issue.

DiggerLin updated this revision to Diff 277057.Jul 10 2020, 8:38 AM
DiggerLin marked 4 inline comments as done.

address comment

DiggerLin added inline comments.Jul 10 2020, 8:39 AM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
488

for the function printNumber is a overload function. using a macro, the complie will determine which version of printNumber will be used when compile. if using a table, I think how to make the code call the correct overload version of printNumber based in the parameter type when running may be complicated.

490

thanks.

hubert.reinterpretcast marked an inline comment as done.Jul 10 2020, 12:44 PM
hubert.reinterpretcast added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
488

It can be solved using pointers-to-member in the table (I guess we don't really want to use those for selecting functions), but the macro is in better shape anyway now; thanks.

489

You can use XCOFFAuxiliaryHeader32::T in the sizeof.

499

Since the macro refers to it, move the macro definition into the scope of AuxiSize.

527

For symmetry, move the undef into the function body.

DiggerLin marked 6 inline comments as done and an inline comment as not done.Jul 13 2020, 6:48 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
489

thanks

499

thanks

527

thanks

DiggerLin updated this revision to Diff 277416.Jul 13 2020, 6:56 AM
DiggerLin marked 3 inline comments as done.

address comment

llvm/include/llvm/Object/XCOFFObjectFile.h
142

I believe that the name @jasonliu suggested is better:
s/XCOFF64bitsFlag/XCOFF64Flags/;

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

The use of capitalization is odd. Even for title case, "of" is not capitalized in such a position.
To avoid this issue, please use lowercase following the first word (except for abbreviations or proper nouns). Please apply for all of the strings.

528

We interpreted the packed field for the symbol alignment and type in the similar case of csect auxiliary symbol table entries. For consistency within the tool, I believe we should do it here as well.

530

We should produce some warning if the auxiliary table size indicates additional content past the last field that the code understands. This is another case where I think we should also output the raw data.

548

Please indent the body of the if. Also, if AuxiSize is greater than the offset of the field but less than the offset past-the-end of the field, then we have a partial field and should probably produce some warning about it (in case the producer of the input file has behaviour we did not expect). I would also suggest to output the raw data in that case.

579

The adjective should be "64-bit". Also, these are only the additional XCOFF64 flags (and not all of the flags for XCOFF64).

Suggestion:
Additional flags for 64-bit XCOFF

jasonliu added inline comments.Jul 13 2020, 8:42 AM
llvm/docs/CommandGuide/llvm-readobj.rst
309

nit: to match the naming convention in this file.
XCOFF SPECIFIC OPTIONS

llvm/include/llvm/Object/XCOFFObjectFile.h
142

nit: If we want to keep the bit part. Then XCOFF64BitFlag.

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

Should llvm-readobj --all print out the auxiliary headers?

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

nit: Address clang-format comment.

579

nit:
64 bits -> 64-bit

DiggerLin marked 6 inline comments as done.Jul 14 2020, 7:50 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
548

I agree your suggestion that "we have a partial field and should probably produce some warning about it " . If I implement the suggestion in the the patch, I also need to add test case to test it. It maybe cause the patch too big, I think it maybe better to put the suggestion in other patch.

DiggerLin updated this revision to Diff 277833.Jul 14 2020, 7:53 AM
DiggerLin marked an inline comment as done.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
548

I would suggest implementing it in this patch and to post another patch (while this review is still up) with the specific testing for the error cases of having partial fields or trailing unrecognized fields.

DiggerLin marked 2 inline comments as done.Jul 14 2020, 9:56 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
548

without the test case , I do not think we can 100% guarantee the code is correct.
if when we write a test case for it. we find an error on the code, we would put the fix and specific test case in the some patch?

hubert.reinterpretcast marked an inline comment as done.Jul 14 2020, 9:59 AM
hubert.reinterpretcast added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
548

Well, if the original patch has not already landed, I do not see why we wouldn't apply the fix in the original patch. As for the testing, I think it should be fine to have it in a separate patch that is intended to land at around the same time.

DiggerLin updated this revision to Diff 278168.Jul 15 2020, 6:52 AM
DiggerLin marked an inline comment as done.

deal with partial field data

DiggerLin marked 2 inline comments as done.Jul 15 2020, 8:55 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-readobj/XCOFF/xcoff-auxiliary-header.test
4

there are several other options are not implemented, -all will be crashed.

jasonliu added inline comments.Jul 16 2020, 11:24 AM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
530

This is marked as Done. But I don't think we are able to print additional content under current revision. But anyway, I would expect this scenario would get added in the test case (which belong to another patch).

540

This field does not appear to have partial field treatment.

jasonliu added inline comments.Jul 16 2020, 11:38 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
65

This block currently cut in between XCOFFSectionHeader code block.
I would suggest to move the new block to the current line 51.
Also, is it worth to consider taking the XCOFFSectionHeader template approach now? Just like XCOFFSectionHeader, we have same member functions and share the same mask between 32 and 64 bit.

DiggerLin marked 3 inline comments as done.Jul 16 2020, 12:10 PM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
540

only more one bytes has partial field treatment, the flag only one byte.

DiggerLin updated this revision to Diff 278597.Jul 16 2020, 2:07 PM
DiggerLin marked an inline comment as done.

address comment

DiggerLin marked 2 inline comments as done.Sep 14 2020, 12:46 PM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
530

I think the code , print out the additional content.

if f (sizeof(XCOFFAuxiliaryHeader32) < AuxiSize) {

W.printBinary(
    "!!!Warning",
    "There are extra data beyond XCOFFAuxiliaryHeader32. Extra raw data",
    ArrayRef<uint8_t>((const uint8_t *)(AuxHeader) +
                          sizeof(XCOFFAuxiliaryHeader32),
                      AuxiSize - sizeof(XCOFFAuxiliaryHeader32)));

Yes, I will test additional content in another patch.

DiggerLin marked an inline comment as done.Sep 14 2020, 12:50 PM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
530

there are already has additional content auxiliary data in the test case
xcoff-auxiliary-header.test

XLC64EXEC-NEXT: !!!Warning: There are extra data beyond XCOFFAuxiliaryHeader64. Extra raw data (00 00 00 00 00 00 00 00 00 00)

Using crtp to implement XCOFFAuxiliaryHeader32 and XCOFFAuxiliaryHeader64

sfertile added inline comments.
llvm/docs/CommandGuide/llvm-readobj.rst
309

Can you update this to reflect Jasons comment please.

jsji added reviewers: Esme, Restricted Project.Sep 29 2021, 7:47 PM
Esme added a comment.Sep 30 2021, 2:22 AM

FYI. I am trying to add yaml2obj support for auxFileHeader to avoid using binary files as input in the tests. However, I am about to take a vacation and if I can't get it done before this patch is submitted, I will modify the tests in follow-up works.

DiggerLin updated this revision to Diff 376211.Sep 30 2021, 7:28 AM

rebase the option --auxiliary-header option on the new option mechanism. and address comment.

FYI. I am trying to add yaml2obj support for auxFileHeader to avoid using binary files as input in the tests. However, I am about to take a vacation and if I can't get it done before this patch is submitted, I will modify the tests in follow-up works.

thanks, Esme.

jhenderson added inline comments.Oct 4 2021, 2:18 AM
llvm/docs/CommandGuide/llvm-readobj.rst
310

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
491

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?

494

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

496

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

500

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

528

CPU is an acronym, so should be all caps.

547

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.

548

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.

llvm/tools/llvm-readobj/llvm-readobj.cpp
409

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

jhenderson added inline comments.Oct 4 2021, 2:18 AM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
550–565

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".

573–576

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.

577

As above.

579

= nullptr; as suggested above.

581

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

583

Same as before: make this a lambda.

630

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

llvm/tools/llvm-readobj/llvm-readobj.cpp
48

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

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
729

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 ↗(On Diff #376211)

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
310

thanks

llvm/lib/Object/XCOFFObjectFile.cpp
729

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
491

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

494

Ok, thanks

500

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

573–576

I think it is difficult . we call the function here

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

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)

583

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
548

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 ↗(On Diff #379864)
llvm/tools/llvm-readobj/XCOFFDumper.cpp
52

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 ↗(On Diff #379864)

thanks

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

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
52

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
52

thanks

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

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
581

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
492

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

578

Ditto.

llvm/tools/llvm-readobj/llvm-readobj.cpp
474

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.
thakis added a subscriber: thakis.Oct 26 2021, 8:54 AM

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.

126

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 ↗(On Diff #382313)

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
500

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.

547

You ignored the second part of this comment...

549–550

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();
552–554
  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.
561
581

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.

630

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
2

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.EditedNov 19 2021, 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.

DiggerLin marked an inline comment as done.Dec 13 2021, 7:11 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
500

there is an article about the auto for c++14. https://solarianprogrammer.com/2014/08/21/cpp-14-auto-tutorial/

G++ 4.9.1 also supports unconstrained generic functions, basically you can use auto in a function parameter list, so the above code can be further simplified as :

1 auto& add_one(auto& v) {
2 for(auto& it : v) {
3 it += 1;
4 }
5 return v;
6 }
7
8 void multiply_by_two(auto& v) {
9 for(auto& it : v) {
10 it *= 2;
11 }
12 }
unfortunately, this didn’t entered in the final C++14 standard. It is expected to be added later to the standard as a technical report.

I test the code in the clang,
the 'auto' not allowed in function prototype

@jhenderson

jhenderson added inline comments.Dec 14 2021, 1:47 AM
llvm/tools/llvm-readobj/XCOFFDumper.cpp
500

You've missed where I've said specifically about lambdas. Please see https://en.cppreference.com/w/cpp/language/lambda. Specifically, look for "generic lambdas". Generic lambdas can take auto as type parameters, and are essentially equivalent to a templated function with the type specified as a template parameter (templated lambdas aren't in the C++14 standard, but generic lambdas mean they often aren't needed). I am not talking about auto in a function parameter list.

DiggerLin marked 12 inline comments as done.Dec 23 2021, 7:34 AM
DiggerLin added inline comments.
llvm/tools/llvm-readobj/XCOFFDumper.cpp
500

thanks

630

create a help function.

Esme added inline comments.Jan 10 2022, 1:00 AM
llvm/include/llvm/Object/XCOFFObjectFile.h
114

The typename should be XCOFFAuxiliaryHeader64.
Sorry for not catching this error before.