[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:263
 
+struct XCOFFRelocation64 {
+  // Masks for packing/unpacking the r_rsize field of relocations.
----------------
MaryamBen wrote:
> sfertile wrote:
> > 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. 
> Yeah, it would be great if I were able to do it, especially that the only difference is the size of VirtualAddress (32-bit/64-bit).
> 
> I have done this because in function relocation32 they read relocations from memory and save them into an array of XCOFFRelocation32, so the structure must match the content of memory.   
> 
> ```
> static_assert(
>       sizeof(XCOFFRelocation32) == XCOFF::RelocationSerializationSize32, "");
>   auto RelocationOrErr =
>       getObject<XCOFFRelocation32>(Data, reinterpret_cast<void *>(RelocAddr),
>                                    NumRelocEntries * sizeof(XCOFFRelocation32));
> ```
> 
Yep, I believe the only difference between the 32-bit and 64-bit relocation structures is the VirtualAddress field. Cribing from the section header implementation, something like:

```
template <typename VirtualAddress_t>
struct XCOFFReloc {
  static constexpr uint8_t XR_SIGN_INDICATOR_MASK = 0x80;
  static constexpr uint8_t XR_FIXUP_MASK = 0x40;
  static constexpr uint8_t XR_BIASED_LENGTH_MASK = 0x3f;

  VirtualAddress_t         VirtualAddress;
  llvm::support::ubig32_t  SymbolIndex;
  uint8_t                  Info;
  uint8_t                  Type;

  bool isRelocationSigned() const;
  bool isFixupIndicated() const;

  uint8_t getRelocatedLength() const;
};

struct XCOFFReloc32;
struct XCOFFReloc64;

extern template struct XCOFFReloc<llvm::support::ubig32_t>;
extern template struct XCOFFReloc<llvm::support::ubig64_t>;

struct XCOFFReloc32 : XCOFFReloc<llvm::support::ubig32_t> {};
struct XCOFFReloc64 : XCOFFReloc<llvm::support::ubig64_t> {};
```

allows us to implement all the shared functionality in the base class, while having the correct size and layout for each of the derived types with minimal boiler plate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103696