[PATCH] D103696: [XCOFF][AIX] Add support for XCOFF 64 bit Object files

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:450
   Expected<uint32_t>
-  getLogicalNumberOfRelocationEntries(const XCOFFSectionHeader32 &Sec) const;
+  getLogicalNumberOfRelocationEntries32(const XCOFFSectionHeader32 &Sec) const;
+
----------------
No need to disambiguate by adding '32' on this since this interface only makes sense with 32-bit XCOFF files. But we might want to rethink the naming here. It would be useful to have the same function name, overloaded by the section argument type for both 64-bit and 32-bit that returns the 'real' count of relocations. @hubert.reinterpretcast Do you have any. thoughts on this?


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:456
   Expected<ArrayRef<XCOFFRelocation32>>
-  relocations(const XCOFFSectionHeader32 &) const;
+  relocations32(const XCOFFSectionHeader32 &) const;
 
----------------
Do we need the '64' vs '32' the disambiguate for relocations? We have it for the section header tables but that's because we don't have an argument to use for overloading with those.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:263
 
+struct XCOFFRelocation64 {
+  // Masks for packing/unpacking the r_rsize field of relocations.
----------------
jhenderson wrote:
> It would be preferable if we didn't duplicate nearly the entire contents of the 32-bit version of this struct. Could you have a single generic version which stores the virtual address in the larger field, and then just write it out to the right size at the appropriate time?
As an alternate, why don't we take the same approach as the section headers. For Relocations we would have a base class that can implement everything, and simply have a template parameter for the type of the VirtualAddress field. That will simplify the llvm-readobj XCOFF dumper changes in this patch nicely, and keeps the relocation implementation consistent with the other parts of this class. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103696