[PATCH] D109452: implement the --syms and 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.

Some mostly stylistic comments. Beyond that, I've got not much else to add. @hubert.reinterpretcast will need to review the actual XCOFF implementation and corresponding code coverage.



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:280
+  if (SymNameOrError) {
+    // The "TOC" symbol is considerated as SymbolRef::ST_Other.
+    if (SymNameOrError.get() == "TOC")
----------------
"considerated" isn't a word. I assume you mean "considered", but I think "treated" would be better. Same applies before.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:284
+
+    // The symbol of section name is considerated as SymbolRef::ST_Other.
+    StringRef SecName;
----------------



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:803
+    if (!CsectAuxRefOrError)
+      consumeError(CsectAuxRefOrError.takeError());
+    else {
----------------
`consumeError` usage needs explanation/TODO etc, as before.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/print-linenumber.test:21
 # LINES32:       00000000 <.text>:
-# LINES32:       ; main():
+# LINES32:       ; .main():
 # LINES32-NEXT:  ; /basic.c:1
----------------
Mildly surprising there's a test change here. Why is there this change?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2042
+    if (O->isXCOFF()) {
+      XCOFFSymbolRef XCoffSymb =
+          dyn_cast<const XCOFFObjectFile>(O)->toSymbolRef(
----------------
1) Be consistent in your capitalization.
2) If an abbreviation is needed, "Sym" is the usual abbreviation, not "Symb".

Consequently, I'd rename this variable "XCOFFSym" or, if possible "XCOFFSymbol", depending on whether the latter clashes with a type somewhere or not.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2063-2064
+
+        StringRef CName = unwrapOrError(SymRef.getValue().getName(), FileName,
+                                        ArchiveName, ArchitectureName);
+        std::string SymName(CName);
----------------
This will result in a hard error, and stopping of any further dumping of the file, if there's a problem getting the name. It seems to me like this should actually be reported as a warning, and just keep going, using some placeholder string instead (e.g. "<?>") for the invalid name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109452