[PATCH] D112450: support xcoff for llvm-nm

[James Henderson via Phabricator via llvm-commits]

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