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

[Digger via Phabricator via llvm-commits]

DiggerLin added a comment.

I just wonder whether we can implement two separate structure XCOFFSymbolEntry32 and XCOFFSymbolEntry64 without so much union be used on currently implement.



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:116
+    support::ubig32_t Value; // Symbol value; storage class-dependent.
+  } Entry32Type;
+
----------------
the type name Entry32Type is not easy to understand.
change to the SymNameAndValue32  ? it means the first 12 bytes are related to symbol name and symbol value .


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:153
+  // For SectionOrLength32 in 32-bit, SectionOrLengthLowByte64 and
+  // SectionOrLengthHighByte64 in 64-bit:
+  // If the symbol type is XTY_SD or XTY_CM, the csect length.
----------------
there only SectionOrLengthLowByte64 in the union, the SectionOrLengthHighByte64 can deleted in the comment


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:744
+  CurOffset = Obj->is64Bit() ? Obj->getSymbolTableOffset64()
+                             : Obj->getSymbolTableOffset32();
+  uint64_t SymbolTableSize = static_cast<uint64_t>(sizeof(XCOFFSymbolEntry)) *
----------------
can we a member function as  
uint64_t XCOFFObjectFile::getSymbolTableOffset() const {
  return is64Bit() ? fileHeader64()->SymbolTableOffset
                   : fileHeader32()->SymbolTableOffset;
}
add
CurOffset = getSymbolTableOffset() here;




================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:338
   W.printHex(GetSymbolValueName(SymbolEntPtr->StorageClass),
-             SymbolEntPtr->Value);
+             Obj.is64Bit() ? SymbolEntPtr->Obj64.Value
+                           : SymbolEntPtr->Obj32.Value);
----------------
can we a new member function in the XCOFFSymbolRef 
getValue() 
and we will not see any Obj.is64Bit() here.
XCOFFSymRef->getValue()


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

https://reviews.llvm.org/D85774