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

[Digger via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:23
 	StringRef Name;
-	uint8_t   Type;
+        int16_t TypeOrSmc;
+        int64_t Index;
----------------
jhenderson wrote:
> DiggerLin wrote:
> > daltenty wrote:
> > > I think using a union or inheritance is probably better than dual purposing this field.
> > I am prefer to keep current  implement , in the binaryformat/ELF.h , there is no  enum type for  // Symbol types.
> > enum {
> >   STT_NOTYPE = 0,     // Symbol's type is not specified
> >   STT_OBJECT = 1,     // Symbol is a data object (variable, array, etc.)
> >   STT_FUNC = 2,       // Symbol is executable code (function, etc.)
> >   STT_SECTION = 3,    // Symbol refers to a section
> >   STT_FILE = 4,       // Local, absolute symbol that refers to a file
> >   STT_COMMON = 5,     // An uninitialized common block
> >   STT_TLS = 6,        // Thread local data object
> >   STT_GNU_IFUNC = 10, // GNU indirect function
> >   STT_LOOS = 10,      // Lowest operating system-specific symbol type
> >   STT_HIOS = 12,      // Highest operating system-specific symbol type
> >   STT_LOPROC = 13,    // Lowest processor-specific symbol type
> >   STT_HIPROC = 15,    // Highest processor-specific symbol type
> > 
> >   // AMDGPU symbol types
> >   STT_AMDGPU_HSA_KERNEL = 10
> > };
> > 
> > as we discuss offline , if we use the union for the this field, we need to modify the 
> > ELF.h and add enum name first. I do not want this patch to go further.
> > 
> > I can add comment here in the source code.
> >         int16_t TypeOrSmc;  ///< For Elf, it stores the symbol type,
> >                            ///< for XCOFF, it store the storage mapping class if there is, other wise it store -1. 
> > 
> > even if I use the union here for example.
> > union {
> >  ELFType Type;
> >  StorageMappingClass Smc;
> >  }
> > 
> > how can I use the union , for example 
> > AllSymbols[*SecI].emplace_back(  .....) ;
> > 
> > or I need the code like 
> > if(obj->isXCOFF())
> >    AllSymbols[*SecI].emplace_back( .... )
> > else if (obj->isELF())
> >    AllSymbols[*SecI].emplace_back(..... )
> > 
> > 
> > 
> I'm really not happy with `Type` being changed to something else entirely for ELF (and other non-ELF targets for that matter, if any are using it). Not only does the name imply something that is irrelevant for other formats, but you've also changed the type of `Type` to accommodate your desired interface (i.e. -1 being special).
> 
> Rather than -1 being special, have you considered using llvm::Optional? If unset, it is -1. Furthermore, why not use inheritance here, if it's viable, or simply adding an extra field? You could even use the union as originally suggested, by simply having two different constructors, one for XCOFF and one for ELF. This would resolve the emplace_back etc issues.
@jhenderson ,

we will try to refactor it as following first 

struct GenericSymbolInfoTy {
  uint64_t  Addr;
  StringRef Name;
};


struct ELFSymbolInfoTy {
  uint64_t  Addr;
  StringRef Name;
  uint8_t   Type;
};

union SymbolInfoTy {
GenericSymbolInfoTy  GenericSymbolInfo;
ELFSymbolInfoTy   ELFSymbolInfoTy;
};

And for the XCOFF , we will implement it as 

struct XCOFFSymbolInfoTy {
  uint64_t  Addr;
  StringRef Name;
  uint64_t index;
  optional<XCOFF::StorageMappingClass>  Smc;
};

union SymbolInfoTy {
GenericSymbolInfoTy  GenericSymbolInfo;
ELFSymbolInfoTy   ELFSymbolInfoTy;
XCOFFSymbolInfoTy   XCOFFSymbolInfoTy;
};
what do you think about it ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973