[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 a comment.
Herald added a subscriber: wuzish.

I'm not seeing any changes to the output of llvm-readobj's relocation symbol info (We still only print the symbol name). Are we planning to do it in a follow up patch instead?



================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:13
 
+#define ECASE(A)                                                               \
+  case XCOFF::XMC_##A:                                                         \
----------------
I believe you want an "undef ECASE" at the end of this function to close it. 
Also ECASE sounds a bit general, do we want to choose a name to reflect what this switch is? For example, SMC_CASE? 


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:345
 
-typedef std::vector<std::tuple<uint64_t, StringRef, uint8_t>> SectionSymbolsTy;
+typedef std::vector<std::tuple<uint64_t, StringRef, uint8_t, int64_t>>
+    SectionSymbolsTy;
----------------
I probably need some time to think over this interface change. But is there a reason we choose a signed value to represent a symbol index?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:910
+static uint8_t getXCoffSymbolCsectSMC(const ObjectFile *Obj,
+                                      const SymbolRef &Sym) {
+  assert(Obj->isXCOFF() && "Not XCOFF object file.");
----------------
I think We should create an XCOFFDump.cpp file to contain this function and the function below, as they are very XCOFF-centric. 


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1387
+      if (Obj->isXCOFF()) {
+        uint8_t SymbolType = std::get<2>(Symbols[SI]);
+        if (std::get<3>(Symbols[SI]) >= 0)
----------------
Does it make sense to put this into a function? If we are going to print out the relocation symbol the same way, then we would call the same function. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973