[PATCH] D72027: [XCOFF][AIX] Support basic relocation type on AIX

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:414
+  if (Type == XCOFF::RelocationType::R_POS)
+    FixedValue = SectionMap[SymASec]->Address +
+                 (SymA.isDefined() ? Layout.getSymbolOffset(SymA) : 0);
----------------
hubert.reinterpretcast wrote:
> Consider:
> ```
> int x = 2, arr[100] = { 1 };
> extern int *p = arr + 42;
> ```
> 
> The raw data with this patch does not reflect the offset from `arr` that `p` points to (and neither does the relocation itself):
> ```
> 00000004 arr:
>        4: 00 00 00 01                   <unknown>
>                 ...
> 
> 00000194 p:
>      194: 00 00 00 04                   <unknown>
> ```
> 
> The raw data without this patch was correct.
> 
As discussed off-list, the added test should cover this too.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:400
+  // If we could not find SymA directly in SymbolIndexMap, this symbol could
+  // either be a temporary symbol, or an undefined symbol. In this case, we
+  // would need to relocate its csect instead.
----------------
No comma before "or" here.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:401
+  // either be a temporary symbol, or an undefined symbol. In this case, we
+  // would need to relocate its csect instead.
+  uint32_t Index = SymbolIndexMap.find(&SymA) != SymbolIndexMap.end()
----------------
s/relocate its csect/have the relocation reference its csect/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:409
+    // plus any constant value that we might get.
+    FixedValue = SectionMap[SymASec]->Address +
+                 (SymA.isDefined() ? Layout.getSymbolOffset(SymA) : 0) +
----------------
Is `SectionMap[SymASec]->Address` a fancy way of saying `0` for undefined symbols? If so, can we move it into the ternary expression?


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:421
+  uint32_t FixupOffsetInCsect =
+      Layout.getFragmentOffset(Fragment) + Fixup.getOffset();
+
----------------
`assert(TargetObjectWriter->is64Bit() || Fixup.getOffset() <= UINT32_MAX - Layout.getFragmentOffset(Fragment));`


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:706
+               (UINT32_MAX / XCOFF::RelocationSerializationSize32) &&
+           "Relocation for the current section is overflowed.");
+    const uint32_t RelocationSizeInSec =
----------------
Size required for the relocation entries of the current section is too large for a 32-bit object file.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:709
+        Sec->RelocationCount * XCOFF::RelocationSerializationSize32;
+    RawPointer += RelocationSizeInSec;
+    assert(RawPointer <= UINT32_MAX &&
----------------
Before the increment:
`assert(RawPointer <= UINT32_MAX - RelocationSizeInSec && "Offset following the relocation data cannot be represented in 32 bits.");`


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:29
+                                                      const MCFixup &Fixup,
+                                                      bool IsPCRel) const;
 };
----------------
Missing `override`.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:46
+                          : Target.getSymA()->getKind();
+  uint8_t SignAndSize = 0;
+
----------------
This could be done (if indeed the current condition is correct) as:
```
const uint8_t EncodedSignednessIndicator = IsPCRel ? SignBitMask : 0u;
```



================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:52
+
+  // The magic number we use in SignAndSize has strong relationship with
+  // the corresponding MCFixupKind. In most cases, it's the MCFixupKind
----------------
Add "a" before "strong".


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:65
+      SignAndSize |= 15;
+      return {XCOFF::RelocationType::R_TOC, SignAndSize};
+    }
----------------
This could be done as `EncodedSignednessIndicator | 15`.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:71
+    // the instruction actually represents a 26 bit offset.
+    SignAndSize |= 25;
+    return {XCOFF::RelocationType::R_RBR, SignAndSize};
----------------
Same comment.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCXCOFFObjectWriter.cpp:74
+  case FK_Data_4:
+    SignAndSize |= 31;
+    return {XCOFF::RelocationType::R_POS, SignAndSize};
----------------
Same comment.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll:157
+; DIS:      00000040 globalA:
+; DIS-NEXT:       40: 00 00 00 06                   <unknown>
+; DIS:      00000044 globalB:
----------------
The column alignment is off here.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll:164
+; DIS-NEXT:      50: 00 00 00 00                   <unknown>
+; DIS:      00000054 globalA:
+; DIS-NEXT:      54: 00 00 00 40                   <unknown>
----------------
As discussed off-list, we should want the symbol table dump to match this virtual address with the TOC entry for `globalA`, etc.


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

https://reviews.llvm.org/D72027