[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 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:
> > > > > > 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.
> > 
> >  
> I'm pretty sure referencing an unitialised part of a union (i.e. the `Type` member) is undefined behaviour in C++. See https://stackoverflow.com/a/11996970. Also "It's undefined behavior to read from the member of the union that wasn't most recently written" (see https://en.cppreference.com/w/cpp/language/union).
> 
> It doesn't matter if you have set the other member of the union or whether you care about that part of the comparison etc. So, if this code is touched by XCOFF, what you are doing is illegal.
I agree with what James said.
And add to that, even if you are XCOFF, this comparison could still happen. i.e. P1 and P2 have the same Address and Name. So You have to compare XCOFFSymbolInfo in some way, even if you don't really care about the result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72973