[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.

Some minor comments first, I would want to discuss the design offline to avoid spamming the mailing.



================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:23
 	StringRef Name;
-	uint8_t   Type;
+        int16_t TypeOrSmc; ///< For Elf, it stores the symbol type.
+                           ///< for XCOFF, it store storage mapping class of
----------------
The indentation looks off here.  


================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:34
 
         friend bool operator<(const SymbolInfoTy &P1, const SymbolInfoTy &P2) {
+          return std::tie(P1.Addr, P1.Name, P1.TypeOrSmc) <
----------------
should be a "hidden" friend, i.e put this function into private. 


================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:35
         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);
+          return std::tie(P1.Addr, P1.Name, P1.TypeOrSmc) <
+                 std::tie(P2.Addr, P2.Name, P2.TypeOrSmc);
----------------
Should we add member "Index" here?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:149
   support::ubig16_t StabSectNum;
+  uint16_t getAlignmentLog2() const {
+    return (SymbolAlignmentAndType & SymbolAlignmentMask) >>
----------------
Have a new line here to separate data member and member function. 


================
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.");
----------------
jasonliu wrote:
> 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.
> It's getXCOFF a few lines down for the other similarly-named function. Let's keep this consistent.

This comment haven't been addressed yet. 


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1213
         continue;
-    }
+    } else if (Obj->isXCOFF())
+      SymbolType = getXCoffSymbolCsectSMC(Obj, Symbol);
----------------
Add "{" and "}" to make the coding style consistent within this if block.


================
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)
----------------
DiggerLin wrote:
> jasonliu wrote:
> > const?
> > 
> why need a const  here ? it is not reference .
const is a contract communicates between the source reader and author. It tells source reader that this variable is intended not to get modified. Source reader would know that by looking at the const, no need to scope around to come to that conclusion. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973