[PATCH] D72973: [llvm-objdump] Use symbol index+symbol name + storage mapping class as label for -D

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:53
+                                 StringRef SymbolName) {
+  if (SymbolInfo.XCOFFSymInfo.Index)
+    outs() << "(idx: " << SymbolInfo.XCOFFSymInfo.Index.getValue() << ") ";
----------------
Please add a comment above this block explaining the circumstances when there is no index (say, in the case of a generated "dummy" value). Be exact if possible.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:62
+        SymbolInfo.XCOFFSymInfo.StorageMappingClass.getValue();
+    outs() << "[" << XCOFF::getMappingClassString(Smc).str() << "]";
+  }
----------------
Please don't call `.str()` here.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1157
+  if (Obj->isELF()) {
+    SymbolType = getElfSymbolType(Obj, Symbol);
+  }
----------------
`SymbolType` is unused on the descriptive-XCOFF path. Move it into the else (perhaps avoiding it entirely with `Obj->isELF() ? getElfSymbolType(Obj, Symbol) : ELF::STT_NOTYPE`.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1161
+  if (Obj->isXCOFF() && SymbolDescription) {
+    const auto *XCOFFObj = dyn_cast<XCOFFObjectFile>(Obj);
+    DataRefImpl SymbolDRI = Symbol.getRawDataRefImpl();
----------------
Given that we checked `Obj->isXCOFF()`, can we use `cast` instead of `dyn_cast`?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1164
+
+    const Optional<uint32_t> SymbolIndex =
+        XCOFFObj->getSymbolIndex(SymbolDRI.p);
----------------
Can't this just be `uint32_t`? I believe that the call to the `SymbolInfoTy` constructor would do implicit conversion.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:12
 #include "llvm/DebugInfo/DIContext.h"
+#include "llvm/Object/Archive.h"
 #include "llvm/Support/CommandLine.h"
----------------
Moving the include directive is the only change in this file from the patch. We don't need to touch this file at all.


================
Comment at: llvm/tools/llvm-objdump/llvm-xcoffdump.h:1
+//===-- llvm-xcoffdump.h ----------------------------------------*- C++ -*-===//
+//
----------------
The original suggestion was for `XCOFFDump.h` as the header name. Is there a reason to invent `llvm-xcoffdump.h`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973