add the xcoff symbol type functionality for llvm-nm.
Details
- Reviewers
hubert.reinterpretcast sfertile Esme jhenderson - Group Reviewers
Restricted Project - Commits
- rG6bf20aa59030: [AIX] support xcoff for llvm-nm
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 | ||
943–946 | 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. |
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 | ||
943–946 | 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. |
llvm/test/tools/llvm-nm/XCOFF/xcoff_nm_print_symbol.test | ||
---|---|---|
2 ↗ | (On Diff #381975) | |
llvm/tools/llvm-nm/llvm-nm.cpp | ||
943–946 | 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. 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. |
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.
llvm/tools/llvm-nm/llvm-nm.cpp | ||
---|---|---|
943–946 | 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). |
llvm/test/tools/llvm-nm/XCOFF/basic.test | ||
---|---|---|
2 | ||
llvm/tools/llvm-nm/llvm-nm.cpp | ||
160 |
| |
165–166 | I think this comment is better (please reflow with clang-format as appropriate). | |
928–929 | 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. | |
931 | 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. | |
940 | Delete this blank line. | |
941–942 | As above - you need to return '?' or similar, to prevent trying to dereference an Expected in a bad state. | |
1697 | This change is unrelated - please revert it. |
llvm/tools/llvm-nm/llvm-nm.cpp | ||
---|---|---|
160 | if using WithColor::defaultWarningHandler(Error Warning) directly , we can not tell user which object file has the warning. |
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. | |
160 | 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. | |
929 | Test case? | |
943 | 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": ..." Same goes above. |
llvm/tools/llvm-nm/llvm-nm.cpp | ||
---|---|---|
160 | the defininiton of void WithColor::defaultWarningHandler(Error Warning) { handleAllErrors(std::move(Warning), [](ErrorInfoBase &Info) { WithColor::warning() << Info.message() << '\n'; }); } if I write something like
the ToolName will be lost. I think we need ToolName in the warning info, | |
929 | in the invalid-section-index.test
| |
943 | get symbol name maybe error again. in order to display more info on a warning, we have to deal with another error or warning, it do not worth it. |
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 | ||
160 | Right, fair enough about the tool name (you said file name before, which confused me). | |
943 | 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: 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: ..."). |
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:
|
llvm/test/tools/llvm-nm/XCOFF/invalid-section-index.test | ||
---|---|---|
4–7 | thanks |
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:
Also, as a reminder, modern LLVM style doesn't have full stops at ends of errors and warnings. | |
942–943 | Some suggested improvements. Please reflow too. |
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). | |
24–25 | 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. |
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? |
the parent patch of the patch is https://reviews.llvm.org/D113552
- using yaml2obj for test case
- 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:
## Comment # CHECK: symbol check line ## Comment # CHECK: symbol check line
| |
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 |
| |
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. | |
266–267 | 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. | |
944–945 | I'm not sure I see a test case for this particular line? | |
959 | Ditto. |
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. | |
266–267 | add a test case to the 8 byte address are print out in 64bits object file. | |
944–945 | add a test case on N_DEBUG section to test it. | |
959 | add symbol in .except to test. |