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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:47
+  friend bool operator<(const SymbolInfoTy &P1, const SymbolInfoTy &P2) {
+    return std::tie(P1.Addr, P1.Name, P1.Type) <
+           std::tie(P2.Addr, P2.Name, P2.Type);
----------------
jasonliu wrote:
> What if you are comparing a SymbolInfoTy which have XCOFFSymbolInfo instead of Type?
This has been marked as "Done" but I don't see where?


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:17
 StringRef XCOFF::getMappingClassString(XCOFF::StorageMappingClass SMC) {
   switch (SMC) {
+    SMC_CASE(PR)
----------------
If I'm not mistaken, this function could end up not hitting a return if the `SMC` is not a recognised enum. It needs handling in some way (e.g. returning `StringRef()` or a string representing an unknown value such as `to_string(SMC)`).


================
Comment at: llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test:5
+# RUN: llvm-objdump -D  %p/Inputs/xcoff-section-headers.o | \
+# RUN: FileCheck --check-prefix=CHECK-GNU %s
+
----------------
Indent continuation lines by a couple of spaces for readability.

```
# RUN: llvm-objdump -D  %p/Inputs/xcoff-section-headers.o | \
# RUN:   FileCheck --check-prefix=CHECK-GNU %s
```

Also, I think `CHECK` should be the default without `--symbol-description` and `CHECK-DESC` or something with the option, or better yet, add it to a new test file. Also, why are you checking `--symbol-description` with `--disassemble-all/-D` instead of plain disassembly (i.e. `--disassemble`).

Finally, you probably need a test case + maybe a warning for using `--symbol-description` with something that isn't XCOFF.


================
Comment at: llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test:59-61
+
+
+
----------------
Why so many blank lines?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:35
+
+int64_t getSymbolIndex(const ObjectFile *Obj, const SymbolRef &Sym) {
+  if (!Obj->isXCOFF())
----------------
Same comment as @jasonliu's above. Only take in `XCOFFObjectFile` here.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:44
+
+bool isSymbolDescriptionDisplay(const ObjectFile *Obj, const SymbolRef &Sym) {
+  if (!Obj->isXCOFF())
----------------
Ditto.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:64
+void printXCOFFSymbolDescription(SymbolInfoTy &SymbolInfo,
+                                 std::string &SymbolName) {
+
----------------
`SymbolName` isn't modified, so make this a `StringRef`.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:65
+                                 std::string &SymbolName) {
+
+  const int64_t SymIndex = SymbolInfo.XCOFFSymInfo.Index;
----------------
Delete this blank line.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:67
+  const int64_t SymIndex = SymbolInfo.XCOFFSymInfo.Index;
+  if (SymIndex >= 0)
+    outs() << "(idx: " << SymIndex << ") ";
----------------
When can this be less than zero?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:77
+      SymbolInfo.XCOFFSymInfo.StorageMappingClass.getValue();
+  if (SymIndex >= 0 && SymbolInfo.XCOFFSymInfo.ShowSymDesc)
+    outs() << "[" << XCOFF::getMappingClassString(Smc).str() << "]";
----------------
Seems like `ShowSymDesc` should be an argument of this function rather than a property of the symbol?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:149
+    SymbolDescription("symbol-description",
+                      cl::desc("Add symbol description for dissembly"),
+                      cl::init(false), cl::cat(ObjdumpCat));
----------------
jhenderson wrote:
> hubert.reinterpretcast wrote:
> > s/dissembly/disassembly/;
> Please add the new switch to the CommandGuide documentation. It probably also needs an example in that doc.
> 
> Also, if this opton is XCOFF-specific, please say so in the description.
> Please add the new switch to the CommandGuide documentation. It probably also needs an example in that doc.

This hasn't been addressed.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:149-150
+    SymbolDescription("symbol-description",
+                      cl::desc("Add symbol description for disassembly, This "
+                               "option is for XCOFF-only."),
+                      cl::init(false), cl::cat(ObjdumpCat));
----------------
"Add symbol description for disassembly. This option is for XCOFF files only."


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1148
 
+SymbolInfoTy CreateSymbolInfo(const ObjectFile *Obj, const SymbolRef &Symbol) {
+  const StringRef FileName = Obj->getFileName();
----------------
`CreateSymbolInfo` -> `createSymbolInfo`

(use lower-camel case for function names).


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1169
+
+SymbolInfoTy CreateDummySymbolInfo(const ObjectFile *Obj, const uint64_t Addr,
+                                   StringRef &Name, uint8_t Type) {
----------------
`createDummySymbolInfo`




================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1173
+    return SymbolInfoTy(Addr, Name, Optional<XCOFF::StorageMappingClass>(),
+                        No_Sym_Index, false);
+  else
----------------
How about making the symbol index an `Optional`? That way you don't need a magic number/named constant.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:150-157
+getXCOFFSymbolCsectSMC(const object::XCOFFObjectFile *Obj,
+                       const object::SymbolRef &Sym);
+bool isSymbolDescriptionDisplay(const object::ObjectFile *Obj,
+                                const object::SymbolRef &Sym);
+int64_t getSymbolIndex(const object::ObjectFile *Obj,
+                       const object::SymbolRef &Sym);
+void printXCOFFSymbolDescription(SymbolInfoTy &SymbolInfo,
----------------
These are all defined in XCOFFDump.cpp, so put the declarations in XCOFFDump.h.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973