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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I recommend separating the migration to YAML from binaries into a separate successor patch, to avoid confusion.



================
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 
+
----------------
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.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test:20
+# CASE2: llvm-readobj: warning: '[[FILE]]': C_STAT symbol should not have more than 1 auxiliary entry.
+
+
----------------
Nit: double blank line.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test:30
+
+# CASE4: llvm-readobj: warning: '[[FILE]]': C_BLOCK or C_FCN symbol should not have more than 1 auxiliary entry
+
----------------
Same as above: test both cases, adn consider being explicity about which particular type of symbol this is.


================
Comment at: llvm/test/tools/llvm-readobj/XCOFF/symbols-invalid.test:32
+
+## This case tests the raw data output ability when a file auxiliary entry does
+## not have the matching auxiliary type.
----------------
Why specifically the file auxiliary entry and not others?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:289
 
+void XCOFFDumper::printExcpetionAuxEnt(const XCOFFExcpetionAuxEnt *AuxEntPtr) {
+  assert(Obj.is64Bit() && "64-bit interface called on 32-bit object file.");
----------------



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:427
+#ifndef NDEBUG
+  Obj.checkSymbolEntryPointer(reinterpret_cast<uintptr_t>(AuxEntPtr));
+#endif
----------------
Why do we only check this in the debug case? It seems like it's something that's applicable for all cases?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:432
+
+static void printfUnexpectedRawAuxEnt(raw_ostream &OS, uintptr_t AuxAddress) {
+  OS << "!Unexpected raw auxiliary entry data:\n";
----------------
Unless you're explicitly calling `printf` (which you aren't) don't call a function based on that name.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:502-505
+    // For 32-bit object, print the function auxiliary symbol table entry. The
+    // last one must be a CSECT auxiliary entry.
+    // For 64-bit object, both a function auxiliary entry and an exeption
+    // auxiliary entry may appear, print them and skips if SymbolAuxType is
----------------
Skip doing what?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113825