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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:330
+  Add symbol description for disassembly. This option is used with option:
+  `--disassemble` , `--disassemble-all` or `-d` , `-D`.
+
----------------
I don't think you really need the second sentence. I think it's implied by the first saying "disassembly". However, if you still want it, see, e.g. `--x86-asm-syntax` for an example of how to reference options in rst, and remove the references to the short options.


================
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);
----------------
DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > DiggerLin wrote:
> > > > > jasonliu wrote:
> > > > > > What if you are comparing a SymbolInfoTy which have XCOFFSymbolInfo instead of Type?
> > > > > 1. for the XCOFF , as we discuss before, for symbols which has the same address , we do not care about the which symbol we pick up for the llvm-objdump -D .
> > > > > 
> > > > > 2. we do not want to change the elf related test case .so we use use Addr, Name,  Type to compare. 
> > > > > 
> > > > > 3. when you look into the Optional class data structure, I think we compare storage mapping class actually.
> > > > > 
> > > > This has been marked as "Done" but I don't see where?
> > > I have explain why I use P1.Addr, P1.Name, P1.Type to  compare in my comment, So I marked it done. 
> > > we do not want to change the elf related test case .so we use use Addr, Name, Type to compare.
> > As noted above, two ELF symbols can be identical, but be distinct symbols. Do you care about this situation?
> > 
> > > when you look into the Optional class data structure, I think we compare storage mapping class actually.
> > But you don't compare using the XCOFFSymInfo member. I don't think you can assume anything about the layout of the union, so cannot assume that comparing using `Type` will work if it has been initialised the other way.
> please  checked the patch.  https://reviews.llvm.org/D74240
> 
> it compare the std::tuple<uint64_t, StringRef, uint8_t> and sort the symbol in the SectionSymbolsTy
> 
> the default less function of std:tuple is tie uint64_t, StringRef, uint8_t together and compare. we do not want to change the no xcoff test case for llvm-objdum -D, we keep to compare P1.Addr, P1.Name, P1.Type
Right, this is fine for non-XCOFF, but in an XCOFF object, you CANNOT reference `P1.Type` as it is not initialised.


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:41
   }
+  return "Unknown";
 }
----------------
Test-case using this? More generally, you should have a test case for all the supported SMC values probably (i.e. showing how they're printed), rather than a subset. You could use a unit test instead of a lit test if it's not possible to hit all cases at this point, or consider a yaml2obj extension.


================
Comment at: llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test:1-5
 # RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
-# RUN: FileCheck %s
+# RUN: FileCheck --check-prefixes=COMMON,PLAIN %s
+
+# RUN: llvm-objdump -D --symbol-description %p/Inputs/xcoff-section-headers.o | \
+# RUN: FileCheck --check-prefixes=COMMON,DESC %s
----------------
Indentation comment STILL hasn't been addressed:

```
# RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
# RUN:   FileCheck --check-prefixes=COMMON,PLAIN %s

# RUN: llvm-objdump -D --symbol-description %p/Inputs/xcoff-section-headers.o | \
# RUN:   FileCheck --check-prefixes=COMMON,DESC %s
```


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:52
+void printXCOFFSymbolDescription(const SymbolInfoTy &SymbolInfo,
+                                 const StringRef &SymbolName) {
+  if (SymbolInfo.XCOFFSymInfo.Index)
----------------
No need to take `StringRef` as `const &`. Just use `StringRef SymbolName`.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:149-150
+    SymbolDescription("symbol-description",
+                      cl::desc("Add symbol description for disassembly, This "
+                               "option is for XCOFF-only."),
+                      cl::init(false), cl::cat(ObjdumpCat));
----------------
jhenderson wrote:
> jhenderson wrote:
> > "Add symbol description for disassembly. This option is for XCOFF files only."
> This hasn't been properly addressed. ',' -> '.' before the "This".
> 
> Also, please remove the trailing full stop in the help description, to match other options.
> Also, please remove the trailing full stop in the help description, to match other options.

This hasn't been addressed. Please remember to address ALL points in inline comments.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:150-157
+getXCOFFSymbolCsectSMC(const object::XCOFFObjectFile *Obj,
+                       const object::SymbolRef &Sym);
+bool isSymbolDescriptionDisplay(const object::ObjectFile *Obj,
+                                const object::SymbolRef &Sym);
+int64_t getSymbolIndex(const object::ObjectFile *Obj,
+                       const object::SymbolRef &Sym);
+void printXCOFFSymbolDescription(SymbolInfoTy &SymbolInfo,
----------------
DiggerLin wrote:
> jhenderson wrote:
> > DiggerLin wrote:
> > > jhenderson wrote:
> > > > These are all defined in XCOFFDump.cpp, so put the declarations in XCOFFDump.h.
> > > There are something as following in this header file.
> > > void parseInputMachO(object::MachOUniversalBinary *UB);
> > > void printCOFFUnwindInfo(const object::COFFObjectFile *O);
> > > void printMachOUnwindInfo(const object::MachOObjectFile *O);
> > > void printMachOExportsTrie(const object::MachOObjectFile *O);
> > > void printMachORebaseTable(object::MachOObjectFile *O);
> > > void printMachOBindTable(object::MachOObjectFile *O);
> > > void printMachOLazyBindTable(object::MachOObjectFile *O);
> > > void printMachOWeakBindTable(object::MachOObjectFile *O);
> > > 
> > > I do not think I need to put the new declare  function into a  new XCOFFDump.h
> > Those should be refactored into separate files too. Let's not compound bad form by blindly following what was done before.
> I agree with you. But I think we maybe need to create separate NFC patch to address the problem.
We should do XCOFF one in this patch I think, as it is new code. Others can be addressed later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973