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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 26 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1205
                                 uint64_t /*Offset*/, uint64_t /*InstSize*/) {
-  using SymbolInfoTy = std::tuple<uint64_t, StringRef, uint8_t>;
+  using SymbolInfoTy = std::tuple<uint64_t, StringRef, uint8_t, int64_t>;
   using SectionSymbolsTy = std::vector<SymbolInfoTy>;
----------------
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > Is this a stray change that got picked up?
> If the `SectionSymbolsTy` below needs to match the definition elsewhere, it is concerning that there is no common definition in an interface file that can be included from both places.
agree with you , but not on the  patch .  we maybe need to create NFC for  common definition in an interface file that can be included from both places.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:4
+
+namespace llvm {
+
----------------
hubert.reinterpretcast wrote:
> I suggest to remove this and qualify the function definition explicitly.
I am not sure whether we will add new function in the file later for the new functionality of the llvm-objdump, if we remove the namespace llvm , we have to add the llvm qualify for very function and parameter. I am refer to keep this way.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:21
+  }
+  return XCOFF::XMC_MAX_SMC;
+}
----------------
hubert.reinterpretcast wrote:
> Should we just return an `int` and use `-1` as a magic number? `XMC_MAX_SMC` does not really capture the semantics. It's the largest value that fits, but it is not currently valid. It is also not guaranteed not to become a valid value in the future. The `-1` magic number is more idiomatic here.
the getXCoffSymbolCsectSMC() will be put in the third element of  SymbolInfoTy = std::tuple<uint64_t, StringRef, uint8_t, int64_t>; , it is unsigned 8bits


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


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1128
 
+static void printIndexNameSmcAsLable(SectionSymbolsTy &Symbols,
+                                     std::string SymbolName, unsigned int SI) {
----------------
jasonliu wrote:
> hubert.reinterpretcast wrote:
> > jasonliu wrote:
> > > 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.
> > > 
> > `printXCOFFDecoratedSymbolDescription` or `printXCOFFSymbolDescription`?
> I'm good with either. 
    if (Demangle)
        SymbolName = demangle(SymbolName);

SymbolName maybe demangle  , we need to pass SymbolName here. 


================
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)
----------------
jasonliu wrote:
> const?
> 
why need a const  here ? it is not reference .


================
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]) << ") ";
----------------
jasonliu wrote:
> std::get<3>(Symbols[SI]) used two times, let's name it and make it a local variable. 
done


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1178
     uint8_t SymbolType = ELF::STT_NOTYPE;
+    int64_t SymbolIndex = getSymbolIndex(Obj, Symbol);
+
----------------
jasonliu wrote:
> jasonliu wrote:
> > const?
> We are not using this SymbolIndex anywhere other than the emplace_back(), we don't really need this variable.
> And also for consistency, we might want to consider calling getSymbolIndex() in every emplace_back that needs a symbol table index. 
there are several  emplace_back(), I think we can use a local variable? SymbolType is local variable too. keep the same style as SymbolType


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973