[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/tools/llvm-objdump/XCOFFDump.cpp:6
+
+#define SymbolTypeMask 0x07
+
----------------
hubert.reinterpretcast wrote:
> I suggest to use an enum defined inside the function body.
Could we make  "if ((AuxEntPtr->SymbolAlignmentAndType & SymbolTypeMask) != XCOFF::XTY_LD)" into a query function in Object/XCOFFObjectFile.cpp, and have SymbolTypeMask defined as enum inside the function body as Hubert suggested?
I think querying if a certain symbol is a label or not could happen fairly often when we are consuming the XCOFF object file. 

If we do not want to make it a function, then I would want to put SymbolTypeMask into XCOFF.h as enum, since this mask could get used somewhere else as well. 


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:8
+
+uint8_t getXCoffSymbolCsectSMC(const ObjectFile *Obj, const SymbolRef &Sym) {
+  assert(Obj->isXCOFF() && "Not XCOFF object file.");
----------------
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > It's getXCOFF a few lines down for the other similarly-named function. Let's keep this consistent.
> Use `llvm::` here to help ensure that this function signature matches the one from the header at compile time.
Since this is so obviously a XCOFF specific function, could we use "const XCOFFObjectFile *Obj" as parameter directly instead of consuming a general ObjectFile?
Then we could skip the assert(Obj->isXCOFF()) and the static_cast below.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:11
+  DataRefImpl SymbolDRI = Sym.getRawDataRefImpl();
+  XCOFFSymbolRef XCOFFSymRef(SymbolDRI,
+                             static_cast<const XCOFFObjectFile *>(Obj));
----------------
nit: XCOFFSymRef -> SymRef
as the additional XCOFF prefix is not needed in this XCOFF specific function. 


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:911
+
+  auto *XCOFFObj = dyn_cast<XCOFFObjectFile>(Obj);
+  DataRefImpl SymbolDRI = Sym.getRawDataRefImpl();
----------------
const qualifier?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1128
 
+static void printIndexNameSmcAsLable(SectionSymbolsTy &Symbols,
+                                     std::string SymbolName, unsigned int SI) {
----------------
1. Label mis-spelled in the function name. And we are not necessarily printing the "label", so the name is misleading. I would suggest to name it as printFullyQualifiedSymbolName. Or, printXCOFFSymbolName.
2. This function looks very XCOFF centric, do we want to put it in XCOFFDump.cpp?
3. We only want one symbol inside of SectionSymbolsTy, so it does not make much sense to pass in a pointer to all of the symbols inside of the section, and a index to just get one symbol. I would try to typedef std::tuple<uint64_t, StringRef, uint8_t, int64_t> somewhere in the header and use that type here. And that should be the only one parameter you need. We don't need to pass in the SymbolName as well.



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1130
+                                     std::string SymbolName, unsigned int SI) {
+  uint8_t SymbolType = std::get<2>(Symbols[SI]);
+  if (std::get<3>(Symbols[SI]) >= 0)
----------------
const?



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1131
+  uint8_t SymbolType = std::get<2>(Symbols[SI]);
+  if (std::get<3>(Symbols[SI]) >= 0)
+    outs() << "(idx: " << std::get<3>(Symbols[SI]) << ") ";
----------------
std::get<3>(Symbols[SI]) used two times, let's name it and make it a local variable. 


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1142
+           << "]";
+  outs() << ":\n";
+}
----------------
I don't think this line belongs to this function. For example, when printing the symbol name in relocation, we would want to call this function but not print this ":" there. 


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1178
     uint8_t SymbolType = ELF::STT_NOTYPE;
+    int64_t SymbolIndex = getSymbolIndex(Obj, Symbol);
+
----------------
const?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1241
         --Sec;
-        AllSymbols[Sec->second].emplace_back(VA, Name, ELF::STT_NOTYPE);
+        AllSymbols[Sec->second].emplace_back(VA, Name, ELF::STT_NOTYPE, 0);
       } else
----------------
Why are we passing in '0' when there is a macro defined NO_SYMINDEX and it is actually a '-1'?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973