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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/XCOFF.h:280
 
+struct FileHeader64 {
+  uint16_t Magic;
----------------
Let's keep the 32 bit and 64 bit versions of each of these structs next to each other, so the order will be: `FileHeader32`, `FileHeader64`, `SectionHeader32`, `SectionHeader64`.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:231
 
-struct XCOFFRelocation32 {
+template <typename VirtualAddress_t>
+struct XCOFFRelocation {
----------------
Drop the `_t`. Optionally, change this to `VirtualAddressType` or probably more simply just `AddressType` or similar. We don't name our types and variables with prefix/suffixes like this typically (exceptions are some enums - see the Coding Standards for details).


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:431
   Expected<ArrayRef<XCOFFRelocation32>>
-  relocations(const XCOFFSectionHeader32 &) const;
+  relocations32(const XCOFFSectionHeader32 &) const;
 
----------------
Name your inptu parameter.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:450
   Expected<uint32_t>
-  getLogicalNumberOfRelocationEntries(const XCOFFSectionHeader32 &Sec) const;
+  getLogicalNumberOfRelocationEntries32(const XCOFFSectionHeader32 &Sec) const;
+
----------------
sfertile wrote:
> 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?
+1 to something like this. For `ELF` we often use templates for these functions, and then just explicitly instantiate them in the .cpp. Take a look throughout the LLD source code for example. You'd end up with something like:

```
template<typename XCOFFT> Expected<uint32_t>
  getNumberOfRelocationEntries(const XOCFFT::XCOFFSectionHeader &Sec) const;
```

For this to work, you'd create an equivalent of the ElfTraits structs, which contains typedefs for the 32-bit and 64-bit types.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:609
+           "Symbol address overflows.");
+    // Symbole value 32 bits.
+    W.write<uint32_t>(CSectionRef.Address + SymbolOffset);
----------------
As previously stated, this comment doesn't really give us anything. At least drop the "32 bits", since you're inside the`!is64Bit` check. (Also "Symbole" -> "Symbol").


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:707-711
+  if (TargetObjectWriter->is64Bit()) {
+    W.write<uint16_t>(0x01f7);
+  } else {
+    W.write<uint16_t>(0x01df);
+  }
----------------
Drop the braces for simple if-else statements, as per the Coding Standards.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:722
+    W.write<uint32_t>(SymbolTableOffset);
+    // Number of entries in the symbol table (32 bits).
+    W.write<int32_t>(SymbolTableEntryCount);
----------------
You don't need to cahnge this comment. It's clear from the context that this is for 32 bits.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:729
   W.write<uint16_t>(0);
+  // Number of entries in the symbol table (64 bits).
+  if (TargetObjectWriter->is64Bit())
----------------
Put this comment inside the if and drop the "64 bits" bit of it.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1011-1012
+  if (TargetObjectWriter->is64Bit())
+    RawPointer = sizeof(XCOFF::FileHeader64) + auxiliaryHeaderSize() +
+                 SectionCount * sizeof(XCOFF::SectionHeader64);
+  else
----------------
If you were to switch to the ElfTraits model suggested above, you could avoid this if and just do something like:
```
RawPointer = sizeof(XCOFFTraits::FileHeader) + auxiliaryHeaderSize() +
             SectionCount * sizeof(XCOFFTraits::SectionHeader);
```


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:73
 
-bool XCOFFRelocation32::isRelocationSigned() const {
+template <typename VirtualAddress_t>
+bool XCOFFRelocation<VirtualAddress_t>::isRelocationSigned() const {
----------------
Same comment as above re `_t`. Also goes for the following functions.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:371
+    for (uint16_t I = 0; I < NumberOfSections; ++I) {
+      // Find which section this relocation is belonging to, and get the
+      // relocation offset relative to the start of the section.
----------------



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:515-516
 size_t XCOFFObjectFile::getSectionHeaderSize() const {
-  return is64Bit() ? sizeof(XCOFFSectionHeader64) :
-                     sizeof(XCOFFSectionHeader32);
+  return is64Bit() ? sizeof(XCOFFSectionHeader64)
+                   : sizeof(XCOFFSectionHeader32);
 }
----------------
Unrelated formatting changes should be applied in a different commit.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:61
     report_fatal_error("Unimplemented fixup kind.");
+  case PPC::fixup_ppc_half16ds:
   case PPC::fixup_ppc_half16: {
----------------
The changes in this file doesn't seem related to the rest of the patch. Should they be a separate patch?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-extern-weak.ll:335
+
+
+; CHECKSYM64: Symbols [
----------------
Delete second blank line.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:137
       continue;
-    auto Relocations = unwrapOrError(Obj.getFileName(), Obj.relocations(Sec));
+    auto Relocations = unwrapOrError(Obj.getFileName(), Obj.relocations32(Sec));
+    if (Relocations.empty())
----------------
Don't use `auto` where the type isn't obvious (except in certain cases to do with iterators). See the Coding Standards.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:143
+                  << " {\n";
+    for (auto Reloc : Relocations) {
+      StringRef SymbolName = unwrapOrError(
----------------
Ditto. Don't use `auto` here. Also should this be `const &`?


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:162
+void XCOFFDumper::printRelocations64(ArrayRef<XCOFFSectionHeader64> Sections) {
+  if (!opts::ExpandRelocs)
+    report_fatal_error("Unexpanded relocation output not implemented.");
----------------
Do you plan to add support for this? I think it would make more sense to just ignore the option, if there's no strong motivation to crash in this case... (even if you do want a warning/error, use the reportUniqueWarning/reportError functions).


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:167
+  uint16_t Index = 0;
+  for (const auto &Sec : Sections) {
+    ++Index;
----------------
Don't use `auto`.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:169
+    ++Index;
+    // Only the .text, .data, .tdata, and STYP_DWARF sections have relocation.
+    if (Sec.Flags != XCOFF::STYP_TEXT && Sec.Flags != XCOFF::STYP_DATA &&
----------------
jhenderson wrote:
> 
This and the following comments haven't been addressed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103696