[PATCH] D85774: [XCOFF][AIX] Enable tooling support for 64 bit symbol table parsing

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:401
   uint32_t getSymbolIndex(uintptr_t SymEntPtr) const;
+  uintptr_t getSymbolAddressByIndex(uint32_t SymbolTableIndex) const;
   Expected<StringRef> getSymbolNameByIndex(uint32_t SymbolTableIndex) const;
----------------
jasonliu wrote:
> DiggerLin wrote:
> > jasonliu wrote:
> > > DiggerLin wrote:
> > > > this means for getSymbolEntryAddressByIndex(uint32_t SymbolTableIndex) const ?     
> > > Sorry, what's the difference between Symbol and SymbolEntry? 
> > > I'm also seeing `getSymbolIndex` and `getSymbolNameByIndex` around this function. Any reason they are not "SymbolEntry"?
> > the value of symbol maybe a symbol relocation address , I was confused  getSymbolAddress with getting the relocation address at my first glance of the code, getSymbolEntryAddress, that means we need the SymbolEntry address not relocation address of a symbol.
> I don't think we have a symbol relocation address IMO. The address related to relocation could be relocation entry's address, or the virtual address data member inside of a relocation entry. But those addresses are not related to symbol in any ways.
> Also, there is a function from base class which we overrides here called getSymbolAddress, which we don't want to change. So it would make sense to keep the same naming style here.
hmm... Just realized what you meant. 
getSymbolAddress actually returns toSymbolRef.getValue() which is a relocatable address. 
This function is suppose to return the address of the symbol table entry within the object file.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:537
+
+  uintptr_t getAddress() const {
+    if (Entry32)
----------------
jasonliu wrote:
> DiggerLin wrote:
> > getAddress() may confuse with getting the address of the symbol.  maybe good to rename to getEntryAddress() ?
> Do you still find it confusing after seeing my other comments about `Symbol` vs `SymbolEntry`?
> I would think it's fine since we don't really have other addresses we could get in here.
Will change.


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

https://reviews.llvm.org/D85774