[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/include/llvm/MC/MCDisassembler/MCDisassembler.h:52
+  friend bool operator<(const SymbolInfoTy &P1, const SymbolInfoTy &P2) {
+    assert(P1.IsXCOFF == P2.IsXCOFF && "P1.IsXCOFF should be equal to P2.IsXCOFF.");
+    if (P1.IsXCOFF)
----------------
Please clang-format as the linter indicated for all cases identified.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:133
+  enum {
+    SymbolTypeMask = 0x07,
+    SymbolAlignmentMask = 0xF8,
----------------
This can now be
```
static constexpr uint8_t SymbolTypeMask = 0x07u;
```


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:134
+    SymbolTypeMask = 0x07,
+    SymbolAlignmentMask = 0xF8,
+  };
----------------
This is unused now.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:798
+  if (Ret)
+        assert(getNumberOfAuxEntries() >=1 &&
+           "No CSECT Auxiliary Entry is found.");
----------------
Sorry for the churn. Seeing it in this context makes me realize that this probably can't be an `assert` in the first place unless if know that the checking was already done. We will need a function without the `assert` to query whether or not we should expect to find a CSECT Auxiliary Entry (`hasCsectAuxExt` is not named well for that purpose). When we request a CSECT Auxiliary Entry, we can assert that we had a reason to expect that there is one and we can also pass an error back to the caller if there isn't one. Whether or not we can propagate said error without too much difficulty will depend on where the caller is.


================
Comment at: llvm/test/tools/llvm-objdump/XCOFF/disassemble-symbol-description.test:20
+
+; REQUIRES: powerpc-registered-target
+
----------------
Please address take the comments identified on the original test file in D75131 into account. For example, the `REQUIRES` should be moved and made consistent with the use of `#` on the other directive lines, the comment lines should use `##`, etc.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:14
 
-#include "llvm-objdump.h"
-#include "llvm/Object/XCOFFObjectFile.h"
+#include "XCOFFdump.h"
 
----------------
Please name the header `XCOFFDump.h` since the non-header source file is `XCOFFDump.cpp`.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:43
+  if (SymRef.hasCsectAuxEnt()) {
+    const XCOFFCsectAuxEnt32 *AuxEntPtr = SymRef.getXCOFFCsectAuxEnt32();
+    return AuxEntPtr->StorageMappingClass;
----------------
As mentioned on the prior review, this variable is now not particularly helpful to have. I do note that this block of code is likely to change again anyway if `getXCOFFCsectAuxEnt32` is changed such that it can indicate an error.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:63
+                                 StringRef SymbolName) {
+  // Dummy symbol do not have symbol index.
+  if (SymbolInfo.XCOFFSymInfo.Index)
----------------
Suggestion
Dummy symbols have no symbol index.


================
Comment at: llvm/tools/llvm-objdump/XCOFFdump.h:1
+//===-- XCOFFdump.h ----------------------------------------*- C++ -*-===//
+//
----------------
In addition to the naming fix requested, please adjust the length of this line.


================
Comment at: llvm/tools/llvm-objdump/llvm-xcoffdump.h:26
+
+bool isLabel(const object::XCOFFObjectFile *Obj, const object::SymbolRef &Sym);
+
----------------
DiggerLin wrote:
> jhenderson wrote:
> > hubert.reinterpretcast wrote:
> > > I am having trouble accepting that `llvm-objdump` has any business injecting symbols directly into the `llvm` namespace. I understand that `llvm-objdump.h` does do so. @jhenderson, would you object if we introduced an `llvm::objdump_impl` namespace?
> > Seems like a reasonable idea, although I'd probably just call it `llvm::objdump`.
> do this in a new patch ?
Please do the XCOFF part in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973