[PATCH] D112450: support xcoff for llvm-nm

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 5 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:927
+  if (!TypeOrErr) {
+    warning(TypeOrErr.takeError(), Obj.getFileName());
+    return '?';
----------------
jhenderson wrote:
> Test case?
in the invalid-section-index.test
# NM-NEXT: 00000000 ? .text 


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:941
+  if (!SymSecOrErr) {
+    warning(SymSecOrErr.takeError(), Obj.getFileName());
+    return '?';
----------------
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. 


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:144
 
+void reportWarning(Error Err, StringRef Input) {
+  assert(Err);
----------------
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112450/new/

https://reviews.llvm.org/D112450