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

[Jason Liu via Phabricator via llvm-commits]

jasonliu 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.

Agree that current implementation have many union, and it's hard for people to parse what exactly is inside for the structure.
But separating them into two structures, namely, XCOFFSymbolEntry32 and XCOFFSymbolEntry64, would mean a lot more `if (Obj->is64Bit())` check across all tooling, which sacrifice a lot in the usability department. A lot more logic would look duplicated.
One potential solution I thought about is for every data member, we introduce a getter to retrieve the data, and mark the data members private.  So that most of the time, user of the structure do not need to look inside of the structure to figure out how to retrieve certain data. But the downside is we are going to introduce a lot of getters for that, and not sure if it would be worth the effort.



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:116
+    support::ubig32_t Value; // Symbol value; storage class-dependent.
+  } Entry32Type;
+
----------------
DiggerLin wrote:
> 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 .
I don't think the name of Entry32Type matters much though. It just tells user these are 32 bit only entries. It's what's inside matters.
Changing it to SymNameAndValue32 means an extra long name to retrieve its actual entry, and the name would have overlaps with its members as well.
For example:
SymEntry.SymNameAndValue32.SymbolName
vs
SymEntry.Entry32Type.SymbolName



================
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.
----------------
DiggerLin wrote:
> there only SectionOrLengthLowByte64 in the union, the SectionOrLengthHighByte64 can deleted in the comment
But for 64-bit, SectionOrLength is represented by SectionOrLengthLowByte64 and SectionOrLengthHighByte64 combined. Deleting the high byte from the comment would make people think the high byte doesn't matter here. 


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:744
+  CurOffset = Obj->is64Bit() ? Obj->getSymbolTableOffset64()
+                             : Obj->getSymbolTableOffset32();
+  uint64_t SymbolTableSize = static_cast<uint64_t>(sizeof(XCOFFSymbolEntry)) *
----------------
DiggerLin wrote:
> can we a member function as  
> uint64_t XCOFFObjectFile::getSymbolTableOffset() const {
>   return is64Bit() ? fileHeader64()->SymbolTableOffset
>                    : fileHeader32()->SymbolTableOffset;
> }
> add
> CurOffset = getSymbolTableOffset() here;
> 
> 
getSymbolTableOffset64 and getSymbolTableOffset32 returns different types. 
Combine them to return the same type is likely to introduce error when caller use them. 


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:338
   W.printHex(GetSymbolValueName(SymbolEntPtr->StorageClass),
-             SymbolEntPtr->Value);
+             Obj.is64Bit() ? SymbolEntPtr->Obj64.Value
+                           : SymbolEntPtr->Obj32.Value);
----------------
DiggerLin wrote:
> can we a new member function in the XCOFFSymbolRef 
> getValue() 
> and we will not see any Obj.is64Bit() here.
> XCOFFSymRef->getValue()
Same reason why we don't want to combine getSymbolTableOffset()
They are returning different values. Caller have to be aware of that.


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

https://reviews.llvm.org/D85774