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

[Jason Liu via Phabricator via llvm-commits]

jasonliu marked 4 inline comments as done.
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;
----------------
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.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:537
+
+  uintptr_t getAddress() const {
+    if (Entry32)
----------------
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.


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

https://reviews.llvm.org/D85774