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

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
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);
----------------
jhenderson wrote:
> 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.
in xcoff , we only sort the symbols base on the address and name, that means we do not care about the order of the symbols which has the same address in xcoff   the XCOFFSymbolInfo will cast to P1.Type here and compare.

 


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:41
   }
+  return "Unknown";
 }
----------------
jhenderson wrote:
> 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.
llvm yaml2obj do not support xcoff currently. We can add a test case for this when the llvm yaml2obj support xcoff.


================
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,
----------------
jhenderson wrote:
> 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.
assume that I added  a new header file llvm/tools/llvm-objdump/xcoffobjdump.h.
and then the header files will be included llvm/tools/llvm-objdump/llvm-objdump.cpp

as 

#include "llvm-objdump.h"
#include "coffobjdump.h"
#include "elfpbjdump.h"
#include "xcoffobjdump.h"

I do not think it is a good idea.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973