[PATCH] D113825: [llvm-readobj][XCOFF] dump auxiliary symbols.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test:6
+
+# CASE1: llvm-readobj: warning: '[[FILE]]': the C_EXT/C_WEAKEXT/C_HIDEXT symbol, which is not for a function, should have only 1 auxiliary entry, i.e. the CSECT auxiliary entry 
+
----------------
Esme wrote:
> jhenderson wrote:
> > How about "a non-function C_EXT/C_WEAKEXT/C_HIDEXT symbol should have only..."?
> > 
> > You probably should have this test case for each of the three different types. If viable, I'd also dynamically change the warning to explicitly state the type of the symbol (i.e. something like "a non-function C_EXT symbol ..." and "a non-function C_WEAKEXT symbol ..." etc).
> > 
> > Finally, ideally you'd incldue the symbol's name if possible in this warning, since it's not necessarily clear out-of-context which symbol this warning refers to. Applies to each of the other warnings too.
> Thanks!
It looks like you haven't addressed the first part of this comment?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:503
+      reportUniqueWarning(
+          "the " + SymbolClassName + " symbol with index " + Twine(SymbolIdx) +
+          ", which is not for a function, "
----------------
Should have spotted this earlier, but it should be "at index N" not "with index N". Same goes for the other message(s).


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:587
+      std::string SymbolClassName =
+          SymbolClass == XCOFF::C_BLOCK ? "C_BLOCK" : "C_FCN";
+      reportUniqueWarning("the " + SymbolClassName + " symbol with index " +
----------------
I wonder if a simple function that converts enum values to strings would be useful here and elsewhere that you do similar things? I believe there are similar things in ELF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113825