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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:26
+  uint64_t Index;
+  bool ShowSymDesc;
+  XCOFFSymbolInfo(Optional<XCOFF::StorageMappingClass> Smc, uint64_t Idx,
----------------
I wonder if this bool is really necessary. Could you instead check if it is label in getXCOFFSymbolCsectSMC? If it is a label, then return the empty Optional. 
In other words, is it necessary to get the SMC from a label at the first place?


================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:37
+    uint8_t Type;
+    XCOFFSymbolInfo XCoffSymInfo;
+  };
----------------
We should keep the spelling consistent, use XCOFF instead of XCoff.


================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:40
+  SymbolInfoTy(uint64_t Addr, StringRef Name,
+               Optional<XCOFF::StorageMappingClass> Smc, int64_t Idx, bool Show)
+      : Addr(Addr), Name(Name), XCoffSymInfo(Smc, Idx, Show){};
----------------
struct XCOFFSymbolInfo have member Index as uint64_t, and you are passing in int64_t here.


================
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);
----------------
What if you are comparing a SymbolInfoTy which have XCOFFSymbolInfo instead of Type?


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:21
+Optional<XCOFF::StorageMappingClass>
+getXCOFFSymbolCsectSMC(const ObjectFile *Obj, const SymbolRef &Sym) {
+  assert(Obj->isXCOFF() && "Not XCOFF object file.");
----------------
For functions in this file, we should use XCOFFObjectFile instead of ObjectFile as parameter.
And the assert, and if checks for XCOFF is not really necessary for the function in this file.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:147
 
+cl::opt<bool>
+    SymbolDescription("symbol-description",
----------------
Could we list this as XCOFF-only option? 


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1159
+
+  if (Obj->isXCOFF()) {
+    const int64_t SymbolIndex = getSymbolIndex(Obj, Symbol);
----------------
You only need the XCOFF specific version when the option is enabled. 


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1338
+            Symbols.begin(),
+            SymbolInfoTy(SectionAddr, SectionName,
+                         Section.isText() ? ELF::STT_FUNC : ELF::STT_OBJECT));
----------------
I don't like the inconsistency of this. One is calling a factory function to create SymbolInfo, the other one calls the constructor directly. And the factory function is a not normal one, which takes three parameters instead of two. 

I think we should remove the 3 parameter version of CreateSymbolInfo, and just have another function called CreateDummySymbolInfo, which handle the object file differences inside, instead of having a "if (Obj->isXCOFF())" here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973