[BOLT] Fix .relr section addend patching

[yota9]

Summary

The new relocation offset in .relr section patching was calculated
wrong previously. Pass the new file offset to lambda instead of
re-calculating it in it. Test removes relocation from mytext section,
so in case of wrong offset calculation we won't emit right addend value
in expected place, i.e. on the new relocation offset.

[maksfb]

bolt/lib/Core/BinaryEmitter.cpp

322–328 ↗

(On Diff #557247)

The code in AlignerPass tries to address the same issue: https://github.com/llvm/llvm-project/blob/b7961f2cb97556bfc50f7828d5f869d96ab9352e/bolt/lib/Passes/Aligner.cpp#L166-L171

That code doesn't work because having 64-byte alignment with max 8-byte alignment will often do nothing.

What if the function is not empty, would we have a similar issue with alignment issued before the symbol?

[yota9]

bolt/lib/Core/BinaryEmitter.cpp

322–328 ↗

(On Diff #557247)

If the function is non-empty we would emit the function first, then align the CI emitting NOPs to the end of the current function and then emit CI symbols, so I don't expect any problems here since we're not addressing CI by function symbols/offset. On the other hand this condition is used for pure data in code, and currently emitting its symbols first and then aligning the data results in the problems, that is why we need this condition to be here

[maksfb]

bolt/lib/Core/BinaryEmitter.cpp

322–328 ↗

(On Diff #557247)

I understand the need for the fix, but I don't follow why we have the new code in BinaryEmitter fixing the broken functionality in AlignerPass. Could you also point to the code that works for non-empty functions and why we can't make it work for empty functions?

[yota9]

bolt/lib/Core/BinaryEmitter.cpp

322–328 ↗

(On Diff #557247)

Now I see your point and you're totally right, thanks! Let me update the patch. As for non-empty functions we're just emitting CI alignment at the beginning of emitConstantIslands

[yota9]

Update aligner pass

[maksfb]

The alignment change looks good to me (with a nit). The answer to my second question lies in the way we attach a label to a constant island. For. a pure CI, i.e. the one not attached to a function, it's the function label that defines the island. Even though the island will have another FunctionConstantIslandLabel, similar to regular CIs attached to a function, this label will not be used.

Can you commit this fix separately since it's not directly related to .relr patching?

bolt/lib/Passes/Aligner.cpp

165

[yota9]

Thanks @maksfb ! Yes, that is what I mean in my previous answer that first we emit the function and it's symbol and then CI and it's symbol. Also the case where we would search not by symbol directly, rather by getBinaryFunctionAtAddress, we would end up by finding CI aligned with NOP and the beginning. I would separate it, thanks!

[yota9]

Separate changes

[yota9]

Separate changes

[maksfb]

LGTM. Thanks!

bolt/lib/Passes/Aligner.cpp

void AlignerPass::runOnFunctions(BinaryContext &BC) {
 
   AlignHistogram.resize(opts::BlockAlignment);
 
   ParallelUtilities::WorkFuncTy WorkFun = [&](BinaryFunction &BF) {
     // Create a separate MCCodeEmitter to allow lock free execution
     BinaryContext::IndependentCodeEmitter Emitter =
         BC.createIndependentMCCodeEmitter();
 
-    if (opts::UseCompactAligner)
-      alignCompact(BF, Emitter.MCE.get());
-    else
-      alignMaxBytes(BF);
-
     // Align objects that contains constant islands and no code
     // to at least 8 bytes.
     if (!BF.size() && BF.hasIslandsInfo()) {
-      const uint16_t Alignment = BF.getConstantIslandAlignment();
-      if (BF.getAlignment() < Alignment)
-        BF.setAlignment(Alignment);
+      uint16_t Alignment = BF.getConstantIslandAlignment();
+      // Check if we're forcing output alignment and it is greater then minimal
+      // CI required one
+      if (!opts::UseCompactAligner && Alignment < opts::AlignFunctions &&
+          opts::AlignFunctions <= opts::AlignFunctionsMaxBytes)
+        Alignment = opts::AlignFunctions;
 
-      if (BF.getMaxAlignmentBytes() < Alignment)
-        BF.setMaxAlignmentBytes(Alignment);
-
-      if (BF.getMaxColdAlignmentBytes() < Alignment)
-        BF.setMaxColdAlignmentBytes(Alignment);
+      BF.setAlignment(Alignment);
+      return;
     }
 
+    if (opts::UseCompactAligner)
+      alignCompact(BF, Emitter.MCE.get());
+    else
+      alignMaxBytes(BF);
+
     if (opts::AlignBlocks && !opts::PreserveBlocksAlignment)
       alignBlocks(BF, Emitter.MCE.get());
   };
 
   ParallelUtilities::runOnEachFunction(
       BC, ParallelUtilities::SchedulingPolicy::SP_TRIVIAL, WorkFun,
       ParallelUtilities::PredicateTy(nullptr), "AlignerPass");
 

bolt/lib/Rewrite/RewriteInstance.cpp

void RewriteInstance::patchELFAllocatableRelrSection(
     ELFObjectFile<ELFT> *File) {
   if (!DynamicRelrAddress)
     return;
 
   raw_fd_ostream &OS = Out->os();
   const uint8_t PSize = BC->AsmInfo->getCodePointerSize();
   const uint64_t MaxDelta = ((CHAR_BIT * DynamicRelrEntrySize) - 1) * PSize;
 
-  auto FixAddend = [&](const BinarySection &Section, const Relocation &Rel) {
+  auto FixAddend = [&](const BinarySection &Section, const Relocation &Rel,
+                       uint64_t FileOffset) {
     // Fix relocation symbol value in place if no static relocation found
-    // on the same address
+    // on the same address. We won't check the BF relocations here since it
+    // is rare case and no optimization is required.
     if (Section.getRelocationAt(Rel.Offset))
       return;
 
     // No fixup needed if symbol address was not changed
     const uint64_t Addend = getNewFunctionOrDataAddress(Rel.Addend);
     if (!Addend)
       return;
 
-    uint64_t FileOffset = Section.getOutputFileOffset();
-    if (!FileOffset)
-      FileOffset = Section.getInputFileOffset();
-
-    FileOffset += Rel.Offset;
     OS.pwrite(reinterpret_cast<const char *>(&Addend), PSize, FileOffset);
   };
 
   // Fill new relative relocation offsets set
   std::set<uint64_t> RelOffsets;
   for (const BinarySection &Section : BC->allocatableSections()) {
     const uint64_t SectionInputAddress = Section.getAddress();
     uint64_t SectionAddress = Section.getOutputAddress();
     if (!SectionAddress)
       SectionAddress = SectionInputAddress;
 
     for (const Relocation &Rel : Section.dynamicRelocations()) {
       if (!Rel.isRelative())
         continue;
 
       uint64_t RelOffset =
           getNewFunctionOrDataAddress(SectionInputAddress + Rel.Offset);
 
       RelOffset = RelOffset == 0 ? SectionAddress + Rel.Offset : RelOffset;
       assert((RelOffset & 1) == 0 && "Wrong relocation offset");
       RelOffsets.emplace(RelOffset);
-      FixAddend(Section, Rel);
+      FixAddend(Section, Rel, RelOffset);
     }
   }
 
   ErrorOr<BinarySection &> Section =
       BC->getSectionForAddress(*DynamicRelrAddress);
   assert(Section && "cannot get .relr.dyn section");
   assert(Section->isRelr() && "Expected section to be SHT_RELR type");
   uint64_t RelrDynOffset = Section->getInputFileOffset();

bolt/test/AArch64/constant_island_pie_update.s

 // This test checks that the constant island offset and value is updated if
 // it has dynamic relocation. The tests checks both .rela.dyn and relr.dyn
 // dynamic relocations.
 // Also check that we don't duplicate CI if it has dynamic relocations.
 
 # RUN: llvm-mc -filetype=obj -triple aarch64-unknown-unknown %s -o %t.o
 // .rela.dyn
 # RUN: %clang %cflags -fPIC -pie %t.o -o %t.rela.exe -nostdlib \
 # RUN:   -Wl,-q -Wl,-z,notext
 # RUN: llvm-bolt %t.rela.exe -o %t.rela.bolt --use-old-text=0 --lite=0
 # RUN: llvm-objdump -j .text -d --show-all-symbols %t.rela.bolt | FileCheck %s
 # RUN: llvm-readelf -rsW %t.rela.bolt | FileCheck --check-prefix=ELFCHECK %s
 // .relr.dyn
 # RUN: %clang %cflags -fPIC -pie %t.o -o %t.relr.exe -nostdlib \
 # RUN:   -Wl,-q -Wl,-z,notext -Wl,--pack-dyn-relocs=relr
+# RUN: llvm-objcopy --remove-section .rela.mytext %t.relr.exe
 # RUN: llvm-bolt %t.relr.exe -o %t.relr.bolt --use-old-text=0 --lite=0
 # RUN: llvm-objdump -j .text -d --show-all-symbols %t.relr.bolt | FileCheck %s
+# RUN: llvm-objdump -j .text -d %t.relr.bolt | \
+# RUN:   FileCheck %s --check-prefix=ADDENDCHECK
 # RUN: llvm-readelf -rsW %t.relr.bolt | FileCheck --check-prefix=ELFCHECK %s
 # RUN: llvm-readelf -SW %t.relr.bolt | FileCheck --check-prefix=RELRSZCHECK %s
 
 // Check that the CI value was updated
 # CHECK: [[#%x,ADDR:]] <exitLocal>:
 # CHECK: {{.*}} <$d>:
 # CHECK-NEXT: {{.*}} .word 0x{{[0]+}}[[#ADDR]]
 # CHECK-NEXT: {{.*}} .word 0x00000000
 # CHECK-NEXT: {{.*}} .word 0x{{[0]+}}[[#ADDR]]
 # CHECK-NEXT: {{.*}} .word 0x00000000
 # CHECK-NEXT: {{.*}} .word 0x00000000
 # CHECK-NEXT: {{.*}} .word 0x00000000
 # CHECK-NEXT: {{.*}} .word 0x{{[0]+}}[[#ADDR]]
 # CHECK-NEXT: {{.*}} .word 0x00000000
 
+// Check that addend was properly patched in mytextP with stripped relocations
+# ADDENDCHECK: [[#%x,ADDR:]] <exitLocal>:
+# ADDENDCHECK: {{.*}} <mytextP>:
+# ADDENDCHECK-NEXT: {{.*}} .word 0x{{[0]+}}[[#ADDR]]
+# ADDENDCHECK-NEXT: {{.*}} .word 0x00000000
 
 // Check that we've relaxed adr to adrp + add to refer external CI
 # CHECK: <addressDynCi>:
 # CHECK-NEXT: adrp
 # CHECK-NEXT: add
 
 // Check that relocation offsets were updated
 # ELFCHECK: [[#%x,OFF:]] [[#%x,INFO_DYN:]] R_AARCH64_RELATIVE
 # ELFCHECK-NEXT: [[#OFF + 8]] {{0*}}[[#INFO_DYN]] R_AARCH64_RELATIVE
 # ELFCHECK-NEXT: [[#OFF + 24]] {{0*}}[[#INFO_DYN]] R_AARCH64_RELATIVE
+# ELFCHECK-NEXT: {{.*}} R_AARCH64_RELATIVE
 # ELFCHECK: {{.*}}[[#OFF]] {{.*}} $d
 
-// Check that .relr.dyn size is 2 bytes to ensure that last 2 relocations were
+// Check that .relr.dyn size is 2 bytes to ensure that last 3 relocations were
 // encoded as a bitmap so the total section size for 3 relocations is 2 bytes.
 # RELRSZCHECK: .relr.dyn RELR [[#%x,ADDR:]] [[#%x,OFF:]] {{0*}}10
 
   .text
   .align 4
   .local exitLocal
   .type exitLocal, %function
 exitLocal:
.Lci:
   .size _start, .-_start
 
   .global addressDynCi
   .type addressDynCi, %function
 addressDynCi:
   adr x1, .Lci
   bl _start
 .size addressDynCi, .-addressDynCi
+
+  .section ".mytext", "ax"
+  .balign 8
+  .global dummy
+  .type dummy, %function
+dummy:
+  nop
+  .word 0
+  .size dummy, .-dummy
+
+  .global mytextP
+mytextP:
+  .xword exitLocal
+  .size mytextP, .-mytextP