This is an archive of the discontinued LLVM Phabricator instance.

support xcoff for llvm-nm
ClosedPublic

Authored by DiggerLin on Oct 25 2021, 7:00 AM.

Details

Summary

add the xcoff symbol type functionality for llvm-nm.

Diff Detail

Event Timeline

DiggerLin created this revision.Oct 25 2021, 7:00 AM
DiggerLin requested review of this revision.Oct 25 2021, 7:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 25 2021, 7:00 AM

Let's keep this bare-bones for now, and split off the size extraction and name demangling into separate patches.

llvm/test/tools/llvm-nm/XCOFF/xcoff_nm_print_symbol.test
1 ↗(On Diff #381975)

;; (actually probably ## and # for comments for YAML support). Also, should it be "XCOFF" in this comment?

Also, rename the test file to remove the "xcoff" and "nm" from the name. Both are implied by the file path. Given my out-of-line comment suggesting this patch be stripped back to just regular printing, with no options etc, I'd suggest naming it basic.test.

2 ↗(On Diff #381975)

Can't we use yaml2obj here now? That would allow for much better targeted testing.

llvm/tools/llvm-nm/llvm-nm.cpp
941–944

I know that other formats throw away errors, but I would suggest that's a mistake - how is a user supposed to know why a given symbol results in ?? I'd recommend reporting the Error as a warning, rather than just throwing it away.

DiggerLin marked 2 inline comments as done.Nov 2 2021, 7:00 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-nm/XCOFF/xcoff_nm_print_symbol.test
2 ↗(On Diff #381975)

I do not think I can use the yaml2obj now, the yaml2obj do not support "csect Auxiliary Entry for C_EXT, C_WEAKEXT, and C_HIDEXT Symbols" . in the xcoff , some important symbol attributes are stored in the csect Auxiliary Entry" , @esms, if you have time , would you like to consider to implement the functionality into yaml2obj ?

llvm/tools/llvm-nm/llvm-nm.cpp
941–944

I agree with you, But I am prefer to keep the same code style and error handling with other object file format in the patch. and fixing the error handling for all the other object file format in a separate patch.

jhenderson added inline comments.Nov 2 2021, 7:15 AM
llvm/test/tools/llvm-nm/XCOFF/xcoff_nm_print_symbol.test
2 ↗(On Diff #381975)
llvm/tools/llvm-nm/llvm-nm.cpp
941–944

You're suggesting the following path:

XCOFF with bad error handling -> Fix handling for all formats (including XCOFF)

I think it would be better to avoid ever having a state where XCOFF has bad handling, so I'd prefer one of the two following:

Land XCOFF with good error handling -> Fix handling of other formats.
Fix handling of other formats -> Land XCOFF with good error handling.

I don't mind if you want to change the other formats up-front, but beware that there is a risk this will be more complicated.

DiggerLin marked 3 inline comments as done.Nov 2 2021, 1:55 PM

Let's keep this bare-bones for now, and split off the size extraction and name demangling into separate patches.

I agree with you that it may be better to split the patch into three at begin . Since I have implemented all these functionality together and there is not a lot of code change in the "size extraction" and "name demangling" , I do not think we can get a lot of benefit from putting effort to split the patch into three now.

DiggerLin updated this revision to Diff 384219.Nov 2 2021, 1:57 PM

change consumeError to error handle.

DiggerLin marked an inline comment as done.Nov 2 2021, 1:58 PM

Let's keep this bare-bones for now, and split off the size extraction and name demangling into separate patches.

I agree with you that it may be better to split the patch into three at begin . Since I have implemented all these functionality together and there is not a lot of code change in the "size extraction" and "name demangling" , I do not think we can get a lot of benefit from putting effort to split the patch into three now.

Please split it up. It will allow me to review each part in isolation. It will also allow for finer grained control of the patches, e.g. because there is an issue with the size one and it needs reverting.

jhenderson added inline comments.Nov 3 2021, 1:07 AM
llvm/tools/llvm-nm/llvm-nm.cpp
941–944

As suggested in my original comment, this should be a warning rather than an error: in general, dumping tools should warn and then do what they can to continue. The warning will describe why the symbols are ? labelled, but you'll still be able to see the list of symbols (which may be of some use).

DiggerLin updated this revision to Diff 384422.Nov 3 2021, 7:09 AM

split the patch

jhenderson added inline comments.Nov 4 2021, 2:19 AM
llvm/test/tools/llvm-nm/XCOFF/basic.test
2
llvm/tools/llvm-nm/llvm-nm.cpp
158
  1. Move this function so that it isn't sandwiched by the error functions.
  2. I think you can just use WithColor::defaultWarningHandler to replace most of the logic around handleAllErrors below.
  3. For consistency with the error functions in this file, I'd just call this function warn rather than reportWarning.
163–164

I think this comment is better (please reflow with clang-format as appropriate).

926–927

You need to handle the case where TypeOrErr is in an error state after this function. Returning '?' is probably reasonable.

You need a test case that shows this warning is printed too.

929

This is the more common way of getting the value from an Expected, at least in places I usually read code. I think it's a little clearer too.

938

Delete this blank line.

939–940

As above - you need to return '?' or similar, to prevent trying to dereference an Expected in a bad state.

1696

This change is unrelated - please revert it.

DiggerLin marked 7 inline comments as done.Nov 4 2021, 8:15 AM
DiggerLin added inline comments.
llvm/tools/llvm-nm/llvm-nm.cpp
158

if using WithColor::defaultWarningHandler(Error Warning) directly , we can not tell user which object file has the warning.

DiggerLin updated this revision to Diff 384759.Nov 4 2021, 8:18 AM
DiggerLin marked an inline comment as done.

address comment and add a new test cases

jhenderson added inline comments.Nov 4 2021, 8:41 AM
llvm/tools/llvm-nm/llvm-nm.cpp
131

warn not warning - warn is a verb, warning is a noun, and functions should be verbs or verb phrases.

158

That's not true: just pass in an error created via createFileError, right? The implementation of the defaultWarningHandler is almost identical to your existing code.

927

Test case?

941

If possible, add additional context to this error: which symbol has the invalid section?

Something like the following message would be ideal:

"warning: "a.o": unable to load section for symbol "foo": ..."
or
"warning: "a.o": unable to load section for symbol with index 3: ..."

Same goes above.

DiggerLin marked 5 inline comments as done.Nov 5 2021, 7:39 AM
DiggerLin added inline comments.
llvm/tools/llvm-nm/llvm-nm.cpp
158

the defininiton of

void WithColor::defaultWarningHandler(Error Warning) {
   handleAllErrors(std::move(Warning), [](ErrorInfoBase &Info) {
     WithColor::warning() << Info.message() << '\n';
   });
 }

if I write something like

static void warn(Error Err, StringRef Input) {

assert(Err);
if (Input == "-")
  Input = "<stdin>";

// Flush the standard output so that the warning isn't interleaved with other
// output if stdout and stderr are writing to the same place.
outs().flush();
WithColor::defaultWarningHandler(createFileError(Input, std::move(Err)));

}

the ToolName will be lost.

I think we need ToolName in the warning info,
we have ToolName in the error info in llvm-nm

927

in the invalid-section-index.test

  1. NM-NEXT: 00000000 ? .text
941

get symbol name maybe error again.
Expected<StringRef> XCOFFObjectFile::getSymbolName(DataRefImpl Symb) const

in order to display more info on a warning, we have to deal with another error or warning, it do not worth it.

DiggerLin updated this revision to Diff 385069.Nov 5 2021, 7:40 AM
DiggerLin marked 2 inline comments as done.
jhenderson added inline comments.Nov 9 2021, 12:57 AM
llvm/test/tools/llvm-nm/XCOFF/invalid-section-index.test
4–7

You can just call this %t.o. No need to show that it comes from YAML.

When running FileCheck, you should pass in the file name as a variable name, as per the inline edit, and then use the variable in the name check. Also, you need to add an optional .exe suffix for the llvm-nm tool for Windows support.

llvm/tools/llvm-nm/llvm-nm.cpp
158

Right, fair enough about the tool name (you said file name before, which confused me).

941

llvm-readobj's ELF port at least, has a number of examples of how to unwrap an error to add additional context. Typically it might end up as something like:
"warning: 'a.o': unable to load section for symbol 'x' (index: 1): <error message from lower-level library>"

The symbol name will be handled and reported elsewhere, since it needs printing as part of the output. As such, you could just use consumeError and then use a placeholder string like <?> for the name. Alternatively, don't bother with the symbol name entirely and just print the index ("unable to load section for symbol with index 1: ...").

jhenderson added inline comments.Nov 9 2021, 1:00 AM
llvm/test/tools/llvm-nm/XCOFF/basic.test
42

Coming back to this list of symbols: there are too many, and I doubt you need all of them for your test cases. I'd suggest the following:

  1. Consider using assembly if you can as your input, as this will give you greater control over your symbol names and properties.
  2. Only have one symbol for each possible code path through the llvm-nm code. 90% of these symbols look like they're just adding noise, and no value.
DiggerLin updated this revision to Diff 388528.Nov 19 2021, 9:30 AM
DiggerLin marked 4 inline comments as done.

address comment

DiggerLin marked an inline comment as done.Nov 19 2021, 9:31 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-nm/XCOFF/invalid-section-index.test
4–7

thanks

jhenderson added inline comments.Nov 22 2021, 12:11 AM
llvm/tools/llvm-nm/llvm-nm.cpp
142–143

As a general note, please remember in English prose (such as warning messages) to have a space before opening parentheses. However, in this particular case, I'd suggest a slight restructuring of the message, which will work regardless of the output of EI.message(). See the inline edit.

This puts the symbol index before the rest of the message. In fact, I'd recommend making it even more generic, and change the signature to static void warn(Error Err, Twine Path, Twine Context = Twine()), with it becoming something like:

WithColor::warning(errs(), ToolName) << Path << ": " << (Context.empty() ? "" : Context + ": ")
    << EI.message() << "\n";

The reason is that this is a generic warning method, not "warnForSymbolError" or similar. If you'd like to add an additional wrapper function that converts a symbol index into a context string, before calling this, that would be fine too. The reasons for the suggested changes:

  1. The error functions take a Twine Path for the file name, so change this to be consistent with those.
  2. There may be occasions in the future of llvm-nm when we want to print a warning where no additional context is needed, so handle that case too.
  3. Use Twine rather than StringRef for the Context string, to allow it to be built up dynamically (note that the error function that takes a Message rather than an Error uses Twine for the Message).

Also, as a reminder, modern LLVM style doesn't have full stops at ends of errors and warnings.

940–941

Some suggested improvements. Please reflow too.

jhenderson added inline comments.Nov 22 2021, 12:42 AM
llvm/test/tools/llvm-nm/XCOFF/basic.test
25–26

Why are you not using check-next here? Are there symbols you are deliberately ignoring in this case?

32

Why's this line here?

DiggerLin marked 4 inline comments as done.Dec 7 2021, 8:06 AM
DiggerLin added inline comments.
llvm/test/tools/llvm-nm/XCOFF/basic.test
25–26

yes, for the convenience to review. there are some symbols are ignore in the case.

32

thanks . delete it.

llvm/tools/llvm-nm/llvm-nm.cpp
940–941

thanks

DiggerLin updated this revision to Diff 392406.Dec 7 2021, 8:07 AM
DiggerLin marked 3 inline comments as done.
DiggerLin updated this revision to Diff 392413.Dec 7 2021, 8:18 AM
jhenderson added inline comments.Dec 10 2021, 12:44 AM
llvm/test/tools/llvm-nm/XCOFF/basic.test
2

Add to this comment what exact behaviours for XCOFF objects and llvm-nm you are testing, e.g. showing that llvm-nm uses can identify the symbols in such an object file and provide appropriate letter codes for them.

18

I'd recommend adding --match-full-lines to FileCheck, to show there's no garbage after symbol names (or in this particular case, that there's no name at all).

25–26

This then raises the question of why are those symbols in the input?

I'm guessing it's so you can test other test cases without having multiple checked-in objects, but that just highlights one of the problems with canned binaries: you cannot tailor them to the individual test case, if you want to avoid having lots to them. I think this just shows you should focus on helping @Esme get the yaml2obj work finished, above getting wider support for XCOFF in the tools.

jhenderson added inline comments.Dec 10 2021, 12:47 AM
llvm/test/tools/llvm-nm/XCOFF/basic.test
26

I guess this is an XCOFF-ism, but this and the below symbol look like functions to me, from their name?

DiggerLin updated this revision to Diff 394953.EditedDec 16 2021, 12:15 PM
DiggerLin marked 3 inline comments as done.

the parent patch of the patch is https://reviews.llvm.org/D113552

  1. using yaml2obj for test case
  2. change

static void warn(Error Err, Twine FileName, Twine Context = Twine())
to
static void warn(Error Err, Twine FileName, Twine Context = Twine(), Twine Archive = Twine())

This is my last day working before Christmas. If I find time to respond to any updates/comments before the end of my work day, I'll do so, but otherwise, don't expect further responses from me until at least the 4th of January (likely a day or two after that).

llvm/test/tools/llvm-nm/XCOFF/basic.test
3–12

I think you could simplify these slightly by doing the following:

  1. Move each individual line down to the symbol you are checking (either in the YAML or the CHECK lines, so that you have something like
## Comment
# CHECK: symbol check line
## Comment
# CHECK: symbol check line
  1. No need for "Test:" at the start of the statement.
  2. End with a "." like other comments.
  3. You can drop the "show type char 'X'" part of each comment, because this is implicit by what the test actually does. The bit that's important is what properties of the symbol are significant.
14

No need for --docnum with only one YAML document in the file.

15

Use the default CHECK, since you only have one test case in this file.

llvm/test/tools/llvm-nm/XCOFF/invalid-section-index.test
4–8
  1. Don't use --docnum/--check-prefix unnecessarily.
  2. {{.*}} is unnecessary at the start of a CHECK line, if not using --match-full-lines.
18

Line this up with the other entries in the block.

llvm/tools/llvm-nm/llvm-nm.cpp
141

Please include a test case that uses an archive, and produces this warning, to show that the archive name is printed.

264–265

You need a test case that uses a 64-bit input, and one that uses a 32-bit input, and then demonstrate how this part of the patch impacts the behaviour.

942–943

I'm not sure I see a test case for this particular line?

957

Ditto.

This is my last day working before Christmas. If I find time to respond to any updates/comments before the end of my work day, I'll do so, but otherwise, don't expect further responses from me until at least the 4th of January (likely a day or two after that).

Merry Christmas ! Have a good holiday vacation. @jhenderson

DiggerLin marked 7 inline comments as done.Dec 20 2021, 9:19 AM
DiggerLin added inline comments.
llvm/tools/llvm-nm/llvm-nm.cpp
141

I rollback to previous version, it do not use archive here, I will add archive name for the warn function in "create export list" patch. and there will be a test case.

264–265

add a test case to the 8 byte address are print out in 64bits object file.

942–943

add a test case on N_DEBUG section to test it.

957

add symbol in .except to test.

DiggerLin updated this revision to Diff 395479.Dec 20 2021, 9:26 AM
DiggerLin marked 4 inline comments as done.
jhenderson added inline comments.Jan 5 2022, 2:37 AM
llvm/test/tools/llvm-nm/XCOFF/basic.test
94

As stated in my earlier comment, there's no need to state "show type char 'W'" or equivalent in the following comments. Please delete.

llvm/test/tools/llvm-nm/XCOFF/basic_64.test
2
DiggerLin updated this revision to Diff 397562.Jan 5 2022, 7:10 AM
DiggerLin marked 2 inline comments as done.
This revision is now accepted and ready to land.Jan 6 2022, 12:11 AM
This revision was landed with ongoing or failed builds.Jan 11 2022, 12:53 PM
This revision was automatically updated to reflect the committed changes.