[PATCH] D109452: using symbol index and qualname for --sym --symbol--description for llvm-objdump for xcoff

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'd like other XCOFF people to look at this and decide if the output is a good style for them.



================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test:1
+; RUN: llc -mtriple powerpc-ibm-aix -verify-machineinstrs -mcpu=pwr4  \
+; RUN: -filetype=obj -o %t.o < %s
----------------
Please add a brief comment explaining what this test is intended to test.

Also, this test won't succeed if the appropriate target is not configured for the person running the test. You need a REQUIRES statement.

I don't know much about llc, but I suspect it's likely that, based purely on the name, the `-verify-machineinstrs` option isn't needed?


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test:2
+; RUN: llc -mtriple powerpc-ibm-aix -verify-machineinstrs -mcpu=pwr4  \
+; RUN: -filetype=obj -o %t.o < %s
+; RUN: llvm-objdump --syms %t.o | FileCheck --check-prefix=SYM %s
----------------
When continuing run lines onto the next one, add spacing after the RUN: on the subsequent lines to indent the arguments a bit. This helps readability by tying it to the previous line.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/symbol-table.test:27-28
+; SYM-NEXT: 00000000 l       .text .text
+; SYM-NEXT: 00000000 l       .text(Csect: .text)  ._Z3bari
+; SYM-NEXT: 00000030 l       .text(Csect: .text)  ._Z3fooi
+; SYM-NEXT: 00000060 l       .data _Z3bari
----------------
Should there be a space after the first `.text` on each of these lines, before the parenthesis?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:69
+
+  auto CsectAuxEntOrErr = SymRef.getXCOFFCsectAuxRef();
+  if (!CsectAuxEntOrErr || !CsectAuxEntOrErr.get().isLabel())
----------------
Don't use `auto` if the type isn't clear (i.e. it isn't mentioned on the line already, in e.g. a cast or constructor). See the style guide.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:72
+    return None;
+  uint32_t Idx = (uint32_t)CsectAuxEntOrErr.get().getSectionOrLength();
+  DataRefImpl DRI;
----------------
It seems that if a case is necessary here, there's a problem with the function you're calling. At a guess, it should just be two functions, one returning a section, the other an index, with errors or assertions reported if using the wrong one for the type.

If a case is really necessary, use `static_cast`.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.h:25
+getXCOFFSymbolContainingSymbolRef(const object::XCOFFObjectFile *Obj,
+                                  const object::SymbolRef &Sym);
+
----------------
This is a problem throughout this header file (and its cpp) already, so please make a small commit to fix it: `SymbolRef` is designed to be a lightweight struct and passed by value, not by reference (just like `StringRef`).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2058
+        if (Demangle)
+          SymName = demangle(std::string(CName));
+
----------------



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2109
   if (Demangle)
-    outs() << ' ' << demangle(std::string(Name)) << '\n';
-  else
-    outs() << ' ' << Name << '\n';
+    SymName = demangle(std::string(Name));
+
----------------



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2112
+  if (O->isXCOFF() && SymbolDescription)
+    SymName = getXCOFFSymbolDescription(createSymbolInfo(O, Symbol), SymName);
+
----------------
Seems to me like you're changing the name to add the description. But then the variable is no longer just the name, which could cause issues in the future.

How about you just print the name before this check, then print the description (inside the if), and finally the new line?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109452