[BOLT][RISCV] Implement R_RISCV_ADD32/SUB32

[jobnoorman]

Summary

Thispatch implements the R_RISCV_ADD32 and R_RISCV_SUB32 relocations for
RISC-V.

[jobnoorman]

Add tests.

Note that the reloc-jt.s test is a real-world use case for label subtraction
on RISC-V (jump tables). However, I haven't found a way to implement this test
without having to hardcode the label difference. I have tried to minimize the
fragility of this test by hardcoding the section addresses but ideas to improve
this test would be appreciated.

[rafauler]

bolt/lib/Rewrite/RewriteInstance.cpp

2989

Aren't these data-to-code relocs? (BOLT doesn't need to update data-to-data relocs).

bolt/test/RISCV/reloc-jt.s

14–15

I'm not sure I get that. Aren't the two relocs supposed to point to the same symbol (.LJTI0_0)?

[jobnoorman]

bolt/lib/Rewrite/RewriteInstance.cpp

2989

Aren't these data-to-code relocs?

The relevant relocations in the reloc-jt.s test case look like this:

Relocation section '.rela.data' at offset 0x2068 contains 2 entries:
  Offset          Info           Type           Sym. Value    Sym. Name + Addend
000000002000  000300000023 R_RISCV_ADD32     000000000000100e .LBB0_1 + 0
000000002000  000200000027 R_RISCV_SUB32     0000000000002000 .LJTI0_0 + 0

.LJTI0_0 is a label in .data so the R_RISCV_SUB32 is a data-to-data relocation.

BOLT doesn't need to update data-to-data relocs

iiuc, BOLT usually doesn't need to update data-to-data relocs because the .data section is not relocated, correct?

In this case, however, the data-to-data reloc gets composed (see D146546) with a data-to-code reloc (R_RISCV_ADD32) so BOLT needs to process it to ensure the linker can properly update the value.

The condition here might be a bit coarse-grained, though, as all data-to-data relocs on RISC-V will now be processed even though it might not always be necessary. I suppose this doesn't really matter as the linker will just overwrite data contents with its original value. Or am I missing something here?

bolt/test/RISCV/reloc-jt.s

14–15

Aren't the two relocs supposed to point to the same symbol (.LJTI0_0)?

No, they aren't, but this is something that also used to confuse me :)

Creating a 32-bit PC-relative address on RISC-V works as follows:

1. auipc ("add upper immediate to PC") with a R_RISCV_PCREL_HI20 reloc pointing to the target symbol (.LJTI0_0 in this case). The result only has the upper 20-bits filled, the lower 12 are set to 0.
2. addi (or another instruction with a 12-bit immediate, like lw) with a R_RISCV_PCREL_LO12_I pointing to the corresponding auipc (.LBB0_0 in this case) to fill in the lower 12-bits.

The psABI has a good explanation for why it works this way (an important reason is that the auipc and addi are not necessarily consecutive).

[rafauler]

bolt/lib/Rewrite/RewriteInstance.cpp

2989

I see, it makes sense. You're correct in your assessment and it should be harmless. it does increase the chance that BOLT might incorrectly update some value that didn't even need updating in the first place, although that would be a hidden bug in BOLT that would need to be solved anyway.

bolt/test/RISCV/reloc-jt.s

14–15

Thanks for explaining

bolt/lib/Core/Relocation.cpp

static bool isSupportedRISCV(uint64_t Type) {
   case ELF::R_RISCV_CALL_PLT:
   case ELF::R_RISCV_BRANCH:
   case ELF::R_RISCV_RELAX:
   case ELF::R_RISCV_GOT_HI20:
   case ELF::R_RISCV_PCREL_HI20:
   case ELF::R_RISCV_PCREL_LO12_I:
   case ELF::R_RISCV_RVC_JUMP:
   case ELF::R_RISCV_RVC_BRANCH:
+  case ELF::R_RISCV_ADD32:
+  case ELF::R_RISCV_SUB32:
     return true;
   }
 }
 
 static size_t getSizeForTypeX86(uint64_t Type) {
   switch (Type) {
   default:
     errs() << object::getELFRelocationTypeName(ELF::EM_X86_64, Type) << '\n';
case ELF::R_RISCV_RVC_BRANCH:
     return 2;
   case ELF::R_RISCV_JAL:
   case ELF::R_RISCV_BRANCH:
   case ELF::R_RISCV_PCREL_HI20:
   case ELF::R_RISCV_PCREL_LO12_I:
   case ELF::R_RISCV_32_PCREL:
   case ELF::R_RISCV_CALL:
   case ELF::R_RISCV_CALL_PLT:
+  case ELF::R_RISCV_ADD32:
+  case ELF::R_RISCV_SUB32:
     return 4;
   case ELF::R_RISCV_GOT_HI20:
     // See extractValueRISCV for why this is necessary.
     return 8;
   }
 }
 
 static bool skipRelocationTypeX86(uint64_t Type) {
static uint64_t extractValueRISCV(uint64_t Type, uint64_t Contents,
   case ELF::R_RISCV_PCREL_HI20:
     return extractUImmRISCV(Contents);
   case ELF::R_RISCV_PCREL_LO12_I:
     return extractIImmRISCV(Contents);
   case ELF::R_RISCV_RVC_JUMP:
     return SignExtend64<11>(Contents >> 2);
   case ELF::R_RISCV_RVC_BRANCH:
     return SignExtend64<8>(((Contents >> 2) & 0x1f) | ((Contents >> 5) & 0xe0));
+  case ELF::R_RISCV_ADD32:
+  case ELF::R_RISCV_SUB32:
+    return Contents;
   }
 }
 
 static bool isGOTX86(uint64_t Type) {
   switch (Type) {
   default:
     return false;
   case ELF::R_X86_64_GOT32:
case ELF::R_AARCH64_PREL64:
     return true;
   }
 }
 
 static bool isPCRelativeRISCV(uint64_t Type) {
   switch (Type) {
   default:
     llvm_unreachable("Unknown relocation type");
+  case ELF::R_RISCV_ADD32:
+  case ELF::R_RISCV_SUB32:
+    return false;
   case ELF::R_RISCV_JAL:
   case ELF::R_RISCV_CALL:
   case ELF::R_RISCV_CALL_PLT:
   case ELF::R_RISCV_BRANCH:
   case ELF::R_RISCV_GOT_HI20:
   case ELF::R_RISCV_PCREL_HI20:
   case ELF::R_RISCV_PCREL_LO12_I:
   case ELF::R_RISCV_RVC_JUMP:
if (RetainedValue) {
     Value = MCBinaryExpr::create(getComposeOpcodeFor(Type), RetainedValue,
                                  Value, Streamer->getContext());
   }
 
   return Value;
 }
 
 MCBinaryExpr::Opcode Relocation::getComposeOpcodeFor(uint64_t Type) {
+  assert(Arch == Triple::riscv64 && "only implemented for RISC-V");
+
+  switch (Type) {
+  default:
     llvm_unreachable("not implemented");
+  case ELF::R_RISCV_ADD32:
+    return MCBinaryExpr::Add;
+  case ELF::R_RISCV_SUB32:
+    return MCBinaryExpr::Sub;
+  }
 }
 
 #define ELF_RELOC(name, value) #name,
 
 void Relocation::print(raw_ostream &OS) const {
   static const char *X86RelocNames[] = {
 #include "llvm/BinaryFormat/ELFRelocs/x86_64.def"
   };

bolt/lib/Rewrite/RewriteInstance.cpp

LLVM_DEBUG(if (opts::MaxDataRelocations &&
                           Addend, ExtractedValue);
     });
 
     return (!opts::MaxDataRelocations ||
             NumDataRelocations < opts::MaxDataRelocations);
   };
 
   if ((ReferencedSection && refersToReorderedSection(ReferencedSection)) ||
-      (opts::ForceToDataRelocations && checkMaxDataRelocations()))
+      (opts::ForceToDataRelocations && checkMaxDataRelocations()) ||
+      // RISC-V has ADD/SUB data-to-data relocations
+      BC->isRISCV())
     ForceRelocation = true;
 
   if (IsFromCode) {
     ContainingBF->addRelocation(Rel.getOffset(), ReferencedSymbol, RType,
                                 Addend, ExtractedValue);
   } else if (IsToCode || ForceRelocation) {
     BC->addRelocation(Rel.getOffset(), ReferencedSymbol, RType, Addend,
                       ExtractedValue);

bolt/test/RISCV/reloc-jt.s

+/// NOTE: assign section addresses explicitly to make the symbol difference
+/// calculation below less fragile.
+// RUN: %clang %cflags -Wl,--section-start=.text=0x1000,--section-start=.data=0x2000 -o %t %s
+// RUN: llvm-bolt -o %t.bolt %t
+// RUN: llvm-readelf -x .data %t.bolt | FileCheck %s
+
+  .text
+
+  .globl _start
+  .p2align 1
+_start:
+.LBB0_0:
+  auipc a1, %pcrel_hi(.LJTI0_0)
+  addi a1, a1, %pcrel_lo(.LBB0_0)
+  lw a0, (a1)
+  add a0, a0, a1
+  jr a0
+.LBB0_1:
+  ret
+  .size _start, .-_start
+
+  .data
+/// .LJTI0_0 = 0x2000
+/// .LBB0_1 = 0x40000e
+// CHECK: Hex dump of section '.data':
+// CHECK-NEXT: 0x00002000 0ee03f00
+.LJTI0_0:
+  .word .LBB0_1 - .LJTI0_0

bolt/test/RISCV/reloc-label-diff.s

+// RUN: %clang %cflags -o %t %s
+// RUN: llvm-bolt -o %t.bolt %t
+// RUN: llvm-readelf -x .data %t.bolt | FileCheck %s
+
+  .text
+  .option norvc
+  .globl _start
+  .p2align 1
+_start:
+  // Force BOLT into relocation mode
+  .reloc 0, R_RISCV_NONE
+  // BOLT removes this nop so the label difference is initially 8 but should be
+  // 4 after BOLT processes it.
+  nop
+  beq x0, x0, _test_end
+_test_end:
+  ret
+  .size _start, .-_start
+
+  .data
+// CHECK: Hex dump of section '.data':
+// CHECK: 0x{{.*}} 04000000
+  .word _test_end - _start