[XCOFF] adjust the Fixedvalue for R_RBR relocations.

[Esme]

Summary

Currently we get a wrong fixed value for R_RBR relocations when -ffunction-sections enabled. This patch fixes this.

      4c: 4b ff ff f5  	bl 0x40 <.bar>
			0000004c:  R_RBR	.foo
      50: 60 00 00 00  	nop
      54: 4b ff ff ed  	bl 0x40 <.bar>
			00000054:  R_RBR	.static_overalign_foo
      58: 60 00 00 00  	nop
      5c: 4b ff ff e5  	bl 0x40 <.bar>
			0000005c:  R_RBR	.alias_foo
      60: 60 00 00 00  	nop
      64: 4b ff ff dd  	bl 0x40 <.bar>
			00000064:  R_RBR	.extern_foo
      68: 60 00 00 00  	nop
      6c: 4b ff ff d5  	bl 0x40 <.bar>
			0000006c:  R_RBR	.hidden_foo

[DiggerLin]

llvm/lib/MC/XCOFFObjectWriter.cpp

648

according to
https://www.ibm.com/docs/en/aix/7.2?topic=formats-xcoff-object-file-format#XCOFF__sua3i125jbau

R_RBR
Specifies (relative) branch relocation. Provides a displacement value between the address of the symbol specified by the r_symndx field and the address of the csect containing the branch instruction to be modified. The instruction can be modified to an absolute branch instruction if the target address is not relocatable.

[DiggerLin]

llvm/lib/MC/XCOFFObjectWriter.cpp

648

but the output looks correct. the document is error? or I misunderstand the doc ?

[Esme]

Hi @DiggerLin, thank you for your review. I'm also confused by the description from the doc, if my understanding is correct, according to the doc, the FixedValue will ignore the value of Fixup.getOffset(), because the address of the csect containing the branch instruction to be modified equals to SectionMap[ParentSec]->Address + Layout.getFragmentOffset(Fragment). This is obviously incorrect. I have tested LNT and bootstrap for this patch under the options of -fintegrated-as -ffunction-sections, and both are clean.

[DiggerLin]

In D138982#3974293, @Esme wrote:

Hi @DiggerLin, thank you for your review. I'm also confused by the description from the doc, if my understanding is correct, according to the doc, the FixedValue will ignore the value of Fixup.getOffset(), because the address of the csect containing the branch instruction to be modified equals to SectionMap[ParentSec]->Address + Layout.getFragmentOffset(Fragment). This is obviously incorrect. I have tested LNT and bootstrap for this patch under the options of -fintegrated-as -ffunction-sections, and both are clean.

In D138982#3974293, @Esme wrote:

Hi @DiggerLin, thank you for your review. I'm also confused by the description from the doc, if my understanding is correct, according to the doc, the FixedValue will ignore the value of Fixup.getOffset(), because the address of the csect containing the branch instruction to be modified equals to SectionMap[ParentSec]->Address + Layout.getFragmentOffset(Fragment). This is obviously incorrect. I have tested LNT and bootstrap for this patch under the options of -fintegrated-as -ffunction-sections, and both are clean.

I agree with you, I asked the problem in our slack channel and wait for confirm, if it is doc error, we should the ask doc to be modified.

[DiggerLin]

llvm/test/CodeGen/PowerPC/aix-xcoff-funcsect.ll

13

add --symbol-description here for llvm-objdump

20

add --symbol-description here for llvm-objdump

[shchenz]

I also thinks the doc is wrong about the relocation type R_RBR, it should be the diff between the relocatable symbol and the branch instruction, not between the symbol and the csect which contains the branch instruction.

llvm/lib/MC/XCOFFObjectWriter.cpp

639

Is it possible that the XCOFF::RelocationType::R_RBR is generated for a non XMC_PR type symbol? Maybe we should make it as an assertion here for both SymASec and ParentSec?

[Esme]

Addressed comments. Thanks @DiggerLin and @shchenz

[stephenpeckham]

llvm/lib/MC/XCOFFObjectWriter.cpp

646–647

// The FixedValue is the difference between the SymA address and BRInstrAddress plus any constant value.

The documentation for R_RBR is easy to misunderstand. The documentation doesn't describe what should be computed for the FixedValue. It describes how the FixedValue should be adjusted at link time. This adjustment is based both on the change to the address of SymA (pre-link vs. post-link) and the change to the address of the branch instruction (or its csect--they're equivalent)

[DiggerLin]

LGTM , but wait for several days to see whether other reviewer has comment or not.

[shchenz]

LGTM too with one nit. Thanks for fixing this.

llvm/lib/MC/XCOFFObjectWriter.cpp

650

For the other else(relocation types), maybe we should add an assertion here to avoid any other unhandled relocation type that may introduce in PPCXCOFFObjectWriter::getRelocTypeAndSignSize() in future. The assertion will help us address the root cause early like the one for XCOFF::RelocationType::R_RBR.

[Esme]

llvm/lib/MC/XCOFFObjectWriter.cpp

646–647

@stephenpeckham Thank you for your explanation.

650

Good point!

[Esme]

llvm/lib/MC/XCOFFObjectWriter.cpp

650

Oh, it's incorrect to add the assertion/unreachable here, because what we are doing in the function is just to adjust the fixedvalue for some particular cases, and the fixedvalue for general cases are already calculated before this function.

[shchenz]

llvm/lib/MC/XCOFFObjectWriter.cpp

650

Thanks. My initial thought was like:

else {
  assert(Type == {list all other relocation types in `PPCXCOFFObjectWriter::getRelocTypeAndSignSize()` that we know are OK with current FixedValue handling})
}

But this assertion may report false alarm if we add a new relocation type in PPCXCOFFObjectWriter::getRelocTypeAndSignSize() in future but that relocation type does not need special handling for the FixedValue.

If we know what relocation types need special handling for FixedValue and assert here that all of them are handled or known to be unhandled for now, that would be the most reasonable solution. But I guess it must need non trivial effort to understand each relocation type. So let's just back to previous version for now.

Thanks again for the try.

[Esme]

llvm/lib/MC/XCOFFObjectWriter.cpp

650

Oh, I see. Thank you for your explanation. I can post another patch for this after I look through other relocation types. And I'll land this patch before that.

llvm/lib/MC/XCOFFObjectWriter.cpp

else if (Type == XCOFF::RelocationType::R_TOC ||
     // constant offset value.
     const int64_t TOCEntryOffset = SectionMap[SymASec]->Address -
                                    TOCCsects.front().Address +
                                    Target.getConstant();
     if (Type == XCOFF::RelocationType::R_TOC && !isInt<16>(TOCEntryOffset))
       report_fatal_error("TOCEntryOffset overflows in small code model mode");
 
     FixedValue = TOCEntryOffset;
+  } else if (Type == XCOFF::RelocationType::R_RBR &&
+             SymASec->getMappingClass() == XCOFF::XMC_PR) {
+    MCSectionXCOFF *ParentSec = cast<MCSectionXCOFF>(Fragment->getParent());
+    // The address of the branch instruction should be the sum of section
+    // address, fragment offset and Fixup offset.
+    uint64_t BRInstrAddress = SectionMap[ParentSec]->Address +
+                              Layout.getFragmentOffset(Fragment) +
+                              Fixup.getOffset();
+    // The FixedValue should be the difference between SymA csect address and BR
+    // instr address plus any constant value.
+    FixedValue =
+        SectionMap[SymASec]->Address - BRInstrAddress + Target.getConstant();
   }
 
   assert((Fixup.getOffset() <=
           MaxRawDataSize - Layout.getFragmentOffset(Fragment)) &&
          "Fragment offset + fixup offset is overflowed.");
   uint32_t FixupOffsetInCsect =
       Layout.getFragmentOffset(Fragment) + Fixup.getOffset();
 
   XCOFFRelocation Reloc = {Index, FixupOffsetInCsect, SignAndSize, Type};

llvm/test/CodeGen/PowerPC/aix-xcoff-funcsect.ll

 ; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
 ; RUN:     -mattr=-altivec -function-sections < %s | \
 ; RUN:   FileCheck --check-prefix=ASM %s
 ; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \
 ; RUN:     -mattr=-altivec -function-sections < %s | \
 ; RUN:   FileCheck --check-prefix=ASM %s
 
 ; RUN: llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 \
 ; RUN:     -mattr=-altivec -function-sections -xcoff-traceback-table=true \
 ; RUN:     -filetype=obj -o %t32.o < %s
 ; RUN: llvm-objdump --syms --reloc --symbol-description %t32.o | \
 ; RUN:   FileCheck --check-prefix=XCOFF32 %s
+; RUN: llvm-objdump -dr %t32.o | FileCheck --check-prefix=DIS32 %s
 
 ; RUN: llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr4 \
 ; RUN:     -mattr=-altivec -function-sections -xcoff-traceback-table=true \
 ; RUN:     -filetype=obj -o %t64.o < %s
 ; RUN: llvm-objdump --syms --reloc --symbol-description %t64.o | \
 ; RUN:   FileCheck --check-prefix=XCOFF64 %s
+; RUN: llvm-objdump -dr %t64.o | FileCheck --check-prefix=DIS64 %s
 
 @alias_foo = alias void (...), bitcast (void ()* @foo to void (...)*)
 
 define void @foo() {
 entry:
   ret void
 }
 

 ; XCOFF64-NEXT: 0000000000000000 R_POS                    (idx: 7) .foo[PR]
 ; XCOFF64-NEXT: 0000000000000008 R_POS                    (idx: 27) TOC[TC0]
 ; XCOFF64-NEXT: 0000000000000018 R_POS                    (idx: 11) .hidden_foo[PR]
 ; XCOFF64-NEXT: 0000000000000020 R_POS                    (idx: 27) TOC[TC0]
 ; XCOFF64-NEXT: 0000000000000030 R_POS                    (idx: 13) .bar[PR]
 ; XCOFF64-NEXT: 0000000000000038 R_POS                    (idx: 27) TOC[TC0]
 ; XCOFF64-NEXT: 0000000000000048 R_POS                    (idx: 15) .static_overalign_foo[PR]
 ; XCOFF64-NEXT: 0000000000000050 R_POS                    (idx: 27) TOC[TC0]
+
+; DIS32:      Disassembly of section .text:
+; DIS32:      00000000 <.alias_foo>:
+; DIS32-NEXT:        0: 4e 80 00 20   blr
+; DIS32:      00000020 <.hidden_foo>:
+; DIS32-NEXT:       20: 4e 80 00 20   blr
+; DIS32:      00000040 <.bar>:
+; DIS32-NEXT:       40: 7c 08 02 a6   mflr 0
+; DIS32-NEXT:       44: 94 21 ff c0   stwu 1, -64(1)
+; DIS32-NEXT:       48: 90 01 00 48   stw 0, 72(1)
+; DIS32-NEXT:       4c: 4b ff ff b5   bl 0x0 <.alias_foo>
+; DIS32-NEXT:                         0000004c:  R_RBR        .foo
+; DIS32-NEXT:       50: 60 00 00 00   nop
+; DIS32-NEXT:       54: 48 00 00 6d   bl 0xc0 <.static_overalign_foo>
+; DIS32-NEXT:                         00000054:  R_RBR        .static_overalign_foo
+; DIS32-NEXT:       58: 60 00 00 00   nop
+; DIS32-NEXT:       5c: 4b ff ff a5   bl 0x0 <.alias_foo>
+; DIS32-NEXT:                         0000005c:  R_RBR        .alias_foo
+; DIS32-NEXT:       60: 60 00 00 00   nop
+; DIS32-NEXT:       64: 4b ff ff 9d   bl 0x0 <.alias_foo>
+; DIS32-NEXT:                         00000064:  R_RBR        .extern_foo
+; DIS32-NEXT:       68: 60 00 00 00   nop
+; DIS32-NEXT:       6c: 4b ff ff b5   bl 0x20 <.hidden_foo>
+; DIS32-NEXT:                         0000006c:  R_RBR        .hidden_foo
+; DIS32:      000000c0 <.static_overalign_foo>:
+; DIS32-NEXT:       c0: 4e 80 00 20   blr
+
+; DIS64:      Disassembly of section .text:
+; DIS64:      0000000000000000 <.alias_foo>:
+; DIS64-NEXT:        0: 4e 80 00 20   blr
+; DIS64:      0000000000000020 <.hidden_foo>:
+; DIS64-NEXT:       20: 4e 80 00 20   blr
+; DIS64:      0000000000000040 <.bar>:
+; DIS64-NEXT:       40: 7c 08 02 a6   mflr 0
+; DIS64-NEXT:       44: f8 21 ff 91   stdu 1, -112(1)
+; DIS64-NEXT:       48: f8 01 00 80   std 0, 128(1)
+; DIS64-NEXT:       4c: 4b ff ff b5   bl 0x0 <.alias_foo>
+; DIS64-NEXT:     000000000000004c:  R_RBR  .foo
+; DIS64-NEXT:       50: 60 00 00 00   nop
+; DIS64-NEXT:       54: 48 00 00 6d   bl 0xc0 <.static_overalign_foo>
+; DIS64-NEXT:     0000000000000054:  R_RBR  .static_overalign_foo
+; DIS64-NEXT:       58: 60 00 00 00   nop
+; DIS64-NEXT:       5c: 4b ff ff a5   bl 0x0 <.alias_foo>
+; DIS64-NEXT:     000000000000005c:  R_RBR  .alias_foo
+; DIS64-NEXT:       60: 60 00 00 00   nop
+; DIS64-NEXT:       64: 4b ff ff 9d   bl 0x0 <.alias_foo>
+; DIS64-NEXT:     0000000000000064:  R_RBR  .extern_foo
+; DIS64-NEXT:       68: 60 00 00 00   nop
+; DIS64-NEXT:       6c: 4b ff ff b5   bl 0x20 <.hidden_foo>
+; DIS64-NEXT:     000000000000006c:  R_RBR  .hidden_foo
+; DIS64-NEXT:       70: 60 00 00 00   nop
+; DIS64:      00000000000000c0 <.static_overalign_foo>:
+; DIS64-NEXT:       c0: 4e 80 00 20   blr