[PATCH] D112450: support xcoff for llvm-nm
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 9 00:57:48 PST 2021
jhenderson added inline comments.
================
Comment at: llvm/test/tools/llvm-nm/XCOFF/invalid-section-index.test:3-6
+# RUN: yaml2obj --docnum=1 %s -o %t_yamlgen.o
+# RUN: llvm-nm %t_yamlgen.o 2>&1 | FileCheck %s --check-prefix=NM
+
+# NM: {{.*}}llvm-nm: warning: {{.*}}_yamlgen.o': the section index (4) is invalid
----------------
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.
================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:941
+ if (!SymSecOrErr) {
+ warning(SymSecOrErr.takeError(), Obj.getFileName());
+ return '?';
----------------
DiggerLin wrote:
> jhenderson wrote:
> > 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.
> 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.
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: ...").
================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:144
+void reportWarning(Error Err, StringRef Input) {
+ assert(Err);
----------------
DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > # Move this function so that it isn't sandwiched by the `error` functions.
> > > > # I think you can just use `WithColor::defaultWarningHandler` to replace most of the logic around `handleAllErrors` below.
> > > > # For consistency with the `error` functions in this file, I'd just call this function `warn` rather than `reportWarning`.
> > >
> > > if using WithColor::defaultWarningHandler(Error Warning) directly , we can not tell user which object file has the warning.
> > >
> > >
> > 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.
> 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
Right, fair enough about the tool name (you said file name before, which confused me).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D112450/new/
https://reviews.llvm.org/D112450
More information about the llvm-commits
mailing list