Page MenuHomePhabricator

[AIX][XCOFF] address post-commit review comments of patch https://reviews.llvm.org/D82549
ClosedPublic

Authored by DiggerLin on Dec 23 2021, 7:45 AM.

Details

Summary

Address post-commit review comments in the https://reviews.llvm.org/D82549, including

  1. changed file name from llvm/test/tools/llvm-readobj/XCOFF/xcoff-auxiliary-header.test --> llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test
  2. replaced macro define by using lambda function.
  3. added a helper function to reduce the duplicated check and print error code.

Diff Detail

Event Timeline

DiggerLin created this revision.Dec 23 2021, 7:45 AM
DiggerLin requested review of this revision.Dec 23 2021, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2021, 7:45 AM
DiggerLin retitled this revision from [AIX][XCOFF][NFC] add some helper function for the XCOFFDumper.cpp to [AIX][XCOFF][NFC] add helper functions for the XCOFFDumper.cpp.Dec 23 2021, 7:46 AM
DiggerLin added a reviewer: Restricted Project.Dec 23 2021, 10:04 AM

Your patch summary seems incorrect to me, compared to what it should be. Perhaps simply saying "address post-commit review comments" in the summary is fine. This is also not NFC, as some output has changed slightly (see the "extra data" warning for example). There should be testing for the changed output.

Some of the items I've highlighted (e.g. spurious blank lines, C++ style casts, function case naming) are things that I've asked you to fix many times in other reviews. Please could you take more care when making changes, and consider reviewing your own code first for these sort of things, before putting up patches for wider community review, so that we don't have to waste time pointing them out to you again and again.

llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test
2 ↗(On Diff #396027)

I previously suggested test input files should be renamed too, i.e. 64-xlc-exec since it's already in an XCOFF subdirectory.

llvm/tools/llvm-readobj/XCOFFDumper.cpp
19–20

This header shouldn't have been added in your previous patch. Please delete it here.

743

My original suggestion was that this should be a lambda witihin the relevant methods, so that appropriate members could be captured. However, you've pulled it out of those functions into a standalone variable. I don't really mind having it separate, but please explain to me why this is still a lambda now and not a template function?

744

Don't abbreviate unnecessarily. It harms readability.

I previously suggested you looked at std::bind to allow you to pass in the print* function, rather than having to have the PrintStyle enum. Did you look at doing that?

747

Don't use C-style casts. Use static_cast and reinterpret_cast.

758
762

Don't start functions with a blank line.

765

No need to provide llvm:: namespace qualifier.

However, I previously pointed out you don't want a string stream here at all. From my comment in D82549 (with a couple of minor fixes):

std::string ErrInfo = (Twine("only partial field for ") + PartialFieldName + " at offset (" + PartialFieldOffset + ")").str();

Alternatively, you could just do:

Dumper->reportUniqueWarning(Twine("only partial field for ") + PartialFieldName + " at offset (" + PartialFieldOffset + ")");
772

Use reinterpret_cast/static_cast, not C-style casts.

779

C++ style cast needed.

789–790

You seem to have ignored my comment chain in D82549 about putting this scope into a higher-level function. Yes, this means that there's empty output, if you don't have an auxiliary header, but that's fine, since you explicitly asked for the auxiliary header output. Additionally, that's what is already being done in LLVM-style ELF output.

DiggerLin retitled this revision from [AIX][XCOFF][NFC] add helper functions for the XCOFFDumper.cpp to [AIX][XCOFF] add helper functions for the XCOFFDumper.cpp.Jan 11 2022, 12:32 PM
DiggerLin edited the summary of this revision. (Show Details)
DiggerLin retitled this revision from [AIX][XCOFF] add helper functions for the XCOFFDumper.cpp to [AIX][XCOFF] address post-commit review comments of patch https://reviews.llvm.org/D82549.Mar 2 2022, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 7:51 AM
DiggerLin marked 10 inline comments as done.Mar 2 2022, 10:23 AM

Your patch summary seems incorrect to me, compared to what it should be. Perhaps simply saying "address post-commit review comments" in the summary is fine. This is also not NFC, as some output has changed slightly (see the "extra data" warning for example). There should be testing for the changed output.

Some of the items I've highlighted (e.g. spurious blank lines, C++ style casts, function case naming) are things that I've asked you to fix many times in other reviews. Please could you take more care when making changes, and consider reviewing your own code first for these sort of things, before putting up patches for wider community review, so that we don't have to waste time pointing them out to you again and again.

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

change to template function, thanks

744

sorry that std::bind do not work for a template function. it will compile error

DiggerLin updated this revision to Diff 412498.Mar 2 2022, 11:23 AM
DiggerLin marked 2 inline comments as done.

address comment

DiggerLin added inline comments.Mar 2 2022, 12:00 PM
llvm/test/tools/yaml2obj/XCOFF/aux-hdr-defaults.yaml
107

can you check why the yaml2obj generate the flag 0x80 here ,not flag 0? @Esme

llvm/test/tools/yaml2obj/XCOFF/basic-doc.yaml
49

according to comment https://reviews.llvm.org/D116220#inline-1115640, there will be empty AuxiliaryHeader { } be printed out even if there no Auxiliary Header in an object file when we use "llvm-readobj --headers".
what is your opinion ? @hubert.reinterpretcast .@Esme

jhenderson added inline comments.Mar 8 2022, 12:25 AM
llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test
3 ↗(On Diff #412498)
15 ↗(On Diff #412498)
17 ↗(On Diff #412498)
54 ↗(On Diff #412498)

Delete extra blank line.

91 ↗(On Diff #412498)

There's no need to show the tool name is printed here.

Why do you have a warning in a "normal" case?

122 ↗(On Diff #412498)
llvm/tools/llvm-readobj/XCOFFDumper.cpp
19–20

Ping. The header is still there. Please delete it, and please stop marking things as Done when they haven't been addressed.

744

You've not addressed my comment about the parameter name. Also, the function name shouldn't start with a capital letter.

sorry that std::bind do not work for a template function. it will compile error

Are you sure? (see e.g. https://stackoverflow.com/questions/26981608/how-to-use-template-function-parameter-in-stdbind).

DiggerLin marked 9 inline comments as done.Mar 8 2022, 10:02 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test
91 ↗(On Diff #412498)

please see the comment in
"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."
https://reviews.llvm.org/D82549#inline-770356

llvm/tools/llvm-readobj/XCOFFDumper.cpp
19–20

thanks

744

if you look into the link info, when using std::bind, it need to explicit instantiate the template PrintAuxMemberHelper function.

there is info as
"Execute_For is a function template, therefore you need to either supply it with type template arguments"

and

https://stackoverflow.com/questions/14727313/c-how-to-reference-templated-functions-using-stdbind-stdfunction

https://en.cppreference.com/w/cpp/utility/functional/bind

"f - Callable object (function object, pointer to function, reference to function, pointer to member function, or pointer to data member) that will be bound to some arguments"

if we need to initiate the template , we need to define several helper function witch std::bind, what is the benefit of using std::bind ?

I need to define something as

using namespace std::placeholders;

auto PrintAuxMember16 = std::bind(PrintAuxMemberHelper<support::ubig16_t,XCOFFFileHeader32>, _1, _2, _3, AuxHeader,AuxSize,PartialFieldOffset, PartialFieldName, W);

auto PrintAuxMember32 = std::bind(PrintAuxMemberHelper<support::ubig32_t,XCOFFFileHeader32>, _1, _2, _3, AuxHeader,AuxSize,PartialFieldOffset, PartialFieldName, W);
DiggerLin updated this revision to Diff 413878.Mar 8 2022, 10:56 AM
DiggerLin marked 3 inline comments as done.
jhenderson added inline comments.Mar 8 2022, 11:20 PM
llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test
17 ↗(On Diff #413878)

You might be able to use [[FILE]] here too, instead of the wildcard regex.

90 ↗(On Diff #413878)

Please delete the wildcard regex, as per my original suggestion.

Yes, my question isn't whether we should be printing the warning, but rather whether the normal case should have the warning. I don't think it should, and instead the warning case should be tested explicitly in a separate case. This is a common testing pattern used throughout: handle the normal case, and then have additional cases for errors and warnings.

Aside: if the xlc tool is explicitly generating data that llvm-readobj can't understand, then either llvm-readobj is missing some functionality, or there's a bug in xlc. I'm guessing it's llvm-readobj...

121 ↗(On Diff #413878)

Ditto, please remove the wildcard regex.

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

Also, the function name shouldn't start with a capital letter.

Still not addressed. Please use lowerCameCase for function names.

DiggerLin marked 4 inline comments as done.Mar 9 2022, 7:09 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-readobj/XCOFF/auxiliary-header.test
17 ↗(On Diff #413878)

thanks

90 ↗(On Diff #413878)

Aside: if the xlc tool is explicitly generating data that llvm-readobj can't understand, then either llvm-readobj is missing some functionality, or there's a bug in xlc. I'm guessing it's llvm-readobj...

The llvm-readobj is implement is based on the https://www.ibm.com/support/knowledgecenter/en/ssw_aix_72/com.ibm.aix.files/XCOFF.htm

I think the extra raw data is align padding data.

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

thanks

DiggerLin updated this revision to Diff 414090.Mar 9 2022, 7:13 AM
DiggerLin marked 3 inline comments as done.
jhenderson accepted this revision.Mar 9 2022, 11:00 PM

LGTM, but you probably want someone else with XCOFF experience to discuss the warning. As noted earlier inline, I don't think a warning in "regular" usage makes sense, since this warning will always be emitted and there's nothing the user can do about it. However, it's possible it means there's extra data that llvm-readobj might be intended to interpret that it isn't, I don't know.

This revision is now accepted and ready to land.Mar 9 2022, 11:00 PM