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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Lots of stylistic comments from me. Someone else will need to review functionality.



================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:263
 
+struct XCOFFRelocation64 {
+  // Masks for packing/unpacking the r_rsize field of relocations.
----------------
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?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:918-922
+    if (TargetObjectWriter->is64Bit()) {
+      RelocationSizeInSec = Sec->RelocationCount * XCOFF::RelocationSerializationSize64;
+    } else {
+      RelocationSizeInSec = Sec->RelocationCount * XCOFF::RelocationSerializationSize32;
+    }
----------------
Don't use braces for single-statement if/else clauses (see the style guide).


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:1018-1024
+  if (TargetObjectWriter->is64Bit()) {
+  RawPointer = sizeof(XCOFF::FileHeader64) + auxiliaryHeaderSize() +
+                        SectionCount * sizeof(XCOFF::SectionHeader64);
+  } else {
+  RawPointer = sizeof(XCOFF::FileHeader32) + auxiliaryHeaderSize() +
                         SectionCount * sizeof(XCOFF::SectionHeader32);
+  }
----------------
Don't use braces (as above). Also, please run clang-format on all your changed code before uploading for review.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:347-348
                                                  const MCAsmLayout &Layout) {
+  // TODO 
+  // Add ".file" in the StringTableBuilder implicitly for XCOFF64 
   if (TargetObjectWriter->is64Bit())
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:603
+  if (TargetObjectWriter->is64Bit()) {
+    // Symbole value 64 bits
+    W.write<uint64_t>(CSectionRef.Address + SymbolOffset);
----------------
I'm not sure this comment gives us anything, since the `is64Bit` check is right before it. Same below for the 32-bit version.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:606-607
+  }
+  // Name or Zeros and string table offset on 32 bits
+  // String table offset on 64 bits
   writeSymbolName(SymbolRef.getSymbolTableName());
----------------
Please ensure comments are complete sentences, as per the style guide. Additionally, why is `Zeros` capitalised and plural? In fact, I'm not sure you need to go into this much detail - that belongs in the underlying function. Here, I'd just specify "Name or string table offset", if you feel you really need to, although really the function name for `writeSymbolName` should be descriptive enough that the comment is superfluous.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:630
+  if (TargetObjectWriter->is64Bit()) {
+    // Low 4 bytes lenght only on 64 bits
+    W.write<uint32_t>( (uint32_t) (CSectionRef.SymbolTableIndex & 0xFFFFFFFF) );
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:631
+    // Low 4 bytes lenght only on 64 bits
+    W.write<uint32_t>( (uint32_t) (CSectionRef.SymbolTableIndex & 0xFFFFFFFF) );
+  } else {
----------------
This doesn't look to be clang-formatted to me. Additionally, do you actually need the mask and cast? Does the `write` function not already do that conversion implicitly?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:644
+  if (TargetObjectWriter->is64Bit()) {
+    // High 4 bytes lenght only on 64 bits
+    W.write<uint32_t>( (uint32_t) ((CSectionRef.SymbolTableIndex & 0xFFFFFFFF00000000) >> 32) );
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:645
+    // High 4 bytes lenght only on 64 bits
+    W.write<uint32_t>( (uint32_t) ((CSectionRef.SymbolTableIndex & 0xFFFFFFFF00000000) >> 32) );
+    // Reserved (pad)
----------------
As above - clang-format and do you need the cast?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:646
+    W.write<uint32_t>( (uint32_t) ((CSectionRef.SymbolTableIndex & 0xFFFFFFFF00000000) >> 32) );
+    // Reserved (pad)
+    W.write<uint8_t>(0);
----------------
Please end all your comments with `.` as per the style guide and the existing code.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:648
+    W.write<uint8_t>(0);
+    // Auxiliary type entry
+    W.write<uint8_t>(XCOFF::SymbolAuxType::AUX_CSECT);
----------------



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:689-690
+  if (TargetObjectWriter->is64Bit()) {
+    // Low 4 bytes lenght only on 64 bits
+   W.write<uint32_t>((uint32_t) (CSectionRef.Size & 0xFFFFFFFF));
+  } else {
----------------
Same comments as above re. typos/grammar/clang-format/casting.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:703-708
+    // High 4 bytes lenght only on 64 bits
+    W.write<uint32_t>((uint32_t) ((CSectionRef.Size & 0xFFFFFFFF00000000) >> 32) );
+    // Reserved (pad)
+    W.write<uint8_t>(0);
+    // Auxiliary type entry
+    W.write<uint8_t>(XCOFF::SymbolAuxType::AUX_CSECT);
----------------
Ditto.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:383
+    const uint16_t NumberOfSections = getNumberOfSections();
+    for (uint16_t i = 0; i < NumberOfSections; ++i) {
+      // Find which section this relocation is belonging to, and get the
----------------
Use upper-case variable names.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:741
+    const XCOFFSectionHeader64 &Sec) const {
+
+  uint16_t SectionIndex = &Sec - sectionHeaderTable64() + 1;
----------------
Don't have blank lines at the start of functions.


================
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 &&
----------------



================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:170-172
+    if (Sec.Flags != XCOFF::STYP_TEXT && Sec.Flags != XCOFF::STYP_DATA &&
+        Sec.Flags != XCOFF::STYP_TDATA && Sec.Flags != XCOFF::STYP_DWARF)
+      continue;
----------------
Is there a particular need for this? Whilst the spec may say that only these section types have relocations, if I follow things correctly, there's nothing actually stopping XCOFF emitters from generating files with relocations for other section types.


================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:173
+      continue;
+    auto Relocations = unwrapOrError(Obj.getFileName(), Obj.relocations64(Sec));
     if (Relocations.empty())
----------------
Prefer to report warnings rather than errors, so that llvm-readobj doesn't bail out on malformed inputs. This helps diagnose more issues.

See `reportUniqueWarning` for the preferred method.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103696