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

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:158
+      return Entry32->ParameterHashIndex;
+    return Entry64->ParameterHashIndex;
+  }
----------------
change to 
return Entry32 ? Entry32->ParameterHashIndex : Entry64->ParameterHashIndex

and change in the following functions too ?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:176
+      return reinterpret_cast<uintptr_t>(Entry32);
+    return reinterpret_cast<uintptr_t>(Entry64);
+  }
----------------
change to return reinterpret_cast<uintptr_t>(Entry32 ? Entry32 : Entry64)


================
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;
----------------
this means for getSymbolEntryAddressByIndex(uint32_t SymbolTableIndex) const ?     


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:486
+    assert(SymEntDataRef.p != 0 && "Symbol table pointer can not be nullptr!");
+    if (OwningObjectPtr->is64Bit())
+      Entry64 = reinterpret_cast<const XCOFFSymbolEntry64 *>(SymEntDataRef.p);
----------------
 assert(OwningObjectPtr != nullptr) here ?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:497
 
-  bool hasCsectAuxEnt() const;
+  int16_t getSectionNumber() const {
+    if (Entry32)
----------------
maybe we can use a macro here.
#define GETVALUE(X) Entry32 ?  Entry32->X :  Entry64 ->X

int16_t getSectionNumber() const {
   return GETVALUE(SectionNumber);
}

uint16_t getSymbolType() const {
     return GETVALUE(SymbolType);
  }

and so on


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:537
+
+  uintptr_t getAddress() const {
+    if (Entry32)
----------------
getAddress() may confuse with getting the address of the symbol.  maybe good to rename to getEntryAddress() ?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:572
+      is64Bit() ? getNumberOfSymbolTableEntries64()
+                : getLogicalNumberOfSymbolTableEntries32();
 
----------------
several place use above NumberOfSymTableEntries , maybe good to provide a helper function.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:730
+                             : Obj->getSymbolTableOffset32();
+  uint64_t SymbolTableSize =
+      static_cast<uint64_t>(XCOFF::SymbolTableEntrySize) *
----------------
change to  const uint64_t SymbolTableSize   ?


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:792
 
-bool XCOFFSymbolRef::isFunction() const {
-  if (OwningObjectPtr->is64Bit())
-    report_fatal_error("64-bit support is unimplemented yet.");
+Expected<XCOFFCsectAuxRef> XCOFFSymbolRef::getXCOFFCsectAuxRef() const {
+  if (!isCsectSymbol())
----------------
not all the symbol has Csect entry.
what about to return 
Optional<XCOFFCsectAuxRef>XCOFFSymbolRef::getXCOFFCsectAuxRef() 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:796
+        object_error::parse_failed,
+        "this symbol entry type could not have a csect auxiliary entry");
 
----------------
I think assert(isCsectSymbol()) myabe better.
sometime maybe our developer call getXCOFFCsectAuxRef()  at a no CsectSymbol . it is not a object file parse failed.


================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:49
                                 const SymbolRef &Sym) {
-  XCOFFSymbolRef SymRef(Sym.getRawDataRefImpl(), Obj);
+  const XCOFFSymbolRef SymRef = Obj->toSymbolRef(Sym.getRawDataRefImpl());
 
----------------
I can not see benefit to change 
from  
XCOFFSymbolRef SymRef(Sym.getRawDataRefImpl(), Obj); 
to 
XCOFFSymbolRef SymRef = Obj->toSymbolRef(Sym.getRawDataRefImpl());


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

https://reviews.llvm.org/D85774