[JITLink][RISCV] ADD/SUB relocs: read value from working memory

[jobnoorman]

Summary

The various ADD/SUB relocations work by reading the current value the
relocation points to, transforming it, and then writing it back to
memory. While the current implementation writes the value back to
working memory, it reads the current value from the execution address of
the relocation. This causes at least wrong results, but often crashes,
when the addresses of working memory are not equal to execution
addresses. This patch fixes this by reading the current value from
working memory.

The bug was noticed by applying the BOLT RISC-V port patch (D145687) on
top of the patch that moves BOLT to JITLink (D147544). In the case of
BOLT, working memory and execution addresses are not equal.

[lhames]

@jobnoorman Nice catch!

You can test this by adding -slab-allocate=1Mb -slab-address=0x1000 -slab-page-size=0x1000 to the RUN: llvm-jitlink lines in the existing llvm/test/ExecutionEngine/JITLink/RISCV/riscv_reloc_add.s test. This will force the JIT'd code to be laid out as-if allocated at 0x1000, which will trigger the bug.

[jobnoorman]

You can test this by adding -slab-allocate=1Mb -slab-address=0x1000 -slab-page-size=0x1000 to the RUN: llvm-jitlink lines in the existing llvm/test/ExecutionEngine/JITLink/RISCV/riscv_reloc_add.s test. This will force the JIT'd code to be laid out as-if allocated at 0x1000, which will trigger the bug.

Nice, thanks for this @lhames!

Diff update:

• Modify tests to use different working/execution addresses.
• Fix forgotten SUB6 reloc.

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp

case R_RISCV_LO12_S: {
       uint32_t Imm4_0 = extractBits(Lo, 0, 5) << 7;
       uint32_t RawInstr = *(little32_t *)FixupPtr;
       *(little32_t *)FixupPtr = (RawInstr & 0x1FFF07F) | Imm11_5 | Imm4_0;
       break;
     }
     case R_RISCV_ADD8: {
       int64_t Value =
           (E.getTarget().getAddress() +
-           *(reinterpret_cast<const uint8_t *>(FixupAddress.getValue())) +
-           E.getAddend())
+           *(reinterpret_cast<const uint8_t *>(FixupPtr)) + E.getAddend())
               .getValue();
       *FixupPtr = static_cast<uint8_t>(Value);
       break;
     }
     case R_RISCV_ADD16: {
       int64_t Value = (E.getTarget().getAddress() +
-                       support::endian::read16le(reinterpret_cast<const void *>(
-                           FixupAddress.getValue())) +
-                       E.getAddend())
+                       support::endian::read16le(FixupPtr) + E.getAddend())
                           .getValue();
       *(little16_t *)FixupPtr = static_cast<uint16_t>(Value);
       break;
     }
     case R_RISCV_ADD32: {
       int64_t Value = (E.getTarget().getAddress() +
-                       support::endian::read32le(reinterpret_cast<const void *>(
-                           FixupAddress.getValue())) +
-                       E.getAddend())
+                       support::endian::read32le(FixupPtr) + E.getAddend())
                           .getValue();
       *(little32_t *)FixupPtr = static_cast<uint32_t>(Value);
       break;
     }
     case R_RISCV_ADD64: {
       int64_t Value = (E.getTarget().getAddress() +
-                       support::endian::read64le(reinterpret_cast<const void *>(
-                           FixupAddress.getValue())) +
-                       E.getAddend())
+                       support::endian::read64le(FixupPtr) + E.getAddend())
                           .getValue();
       *(little64_t *)FixupPtr = static_cast<uint64_t>(Value);
       break;
     }
     case R_RISCV_SUB8: {
-      int64_t Value =
-          *(reinterpret_cast<const uint8_t *>(FixupAddress.getValue())) -
+      int64_t Value = *(reinterpret_cast<const uint8_t *>(FixupPtr)) -
                       E.getTarget().getAddress().getValue() - E.getAddend();
       *FixupPtr = static_cast<uint8_t>(Value);
       break;
     }
     case R_RISCV_SUB16: {
-      int64_t Value = support::endian::read16le(reinterpret_cast<const void *>(
-                          FixupAddress.getValue())) -
+      int64_t Value = support::endian::read16le(FixupPtr) -
                       E.getTarget().getAddress().getValue() - E.getAddend();
       *(little16_t *)FixupPtr = static_cast<uint32_t>(Value);
       break;
     }
     case R_RISCV_SUB32: {
-      int64_t Value = support::endian::read32le(reinterpret_cast<const void *>(
-                          FixupAddress.getValue())) -
+      int64_t Value = support::endian::read32le(FixupPtr) -
                       E.getTarget().getAddress().getValue() - E.getAddend();
       *(little32_t *)FixupPtr = static_cast<uint32_t>(Value);
       break;
     }
     case R_RISCV_SUB64: {
-      int64_t Value = support::endian::read64le(reinterpret_cast<const void *>(
-                          FixupAddress.getValue())) -
+      int64_t Value = support::endian::read64le(FixupPtr) -
                       E.getTarget().getAddress().getValue() - E.getAddend();
       *(little64_t *)FixupPtr = static_cast<uint64_t>(Value);
       break;
     }
     case R_RISCV_RVC_BRANCH: {
       int64_t Value = E.getTarget().getAddress() + E.getAddend() - FixupAddress;
       if (LLVM_UNLIKELY(!isInRangeForImm(Value >> 1, 8)))
         return makeTargetOutOfRangeError(G, B, E);
case R_RISCV_RVC_JUMP: {
       uint16_t Imm3_1 = extractBits(Value, 1, 3) << 3;
       uint16_t Imm5 = extractBits(Value, 5, 1) << 2;
       uint16_t RawInstr = *(little16_t *)FixupPtr;
       *(little16_t *)FixupPtr = (RawInstr & 0xE003) | Imm11 | Imm4 | Imm9_8 |
                                 Imm10 | Imm6 | Imm7 | Imm3_1 | Imm5;
       break;
     }
     case R_RISCV_SUB6: {
-      int64_t Value =
-          *(reinterpret_cast<const uint8_t *>(FixupAddress.getValue())) & 0x3f;
+      int64_t Value = *(reinterpret_cast<const uint8_t *>(FixupPtr)) & 0x3f;
       Value -= E.getTarget().getAddress().getValue() - E.getAddend();
       *FixupPtr = (*FixupPtr & 0xc0) | (static_cast<uint8_t>(Value) & 0x3f);
       break;
     }
     case R_RISCV_SET6: {
       int64_t Value = (E.getTarget().getAddress() + E.getAddend()).getValue();
       uint32_t RawData = *(little32_t *)FixupPtr;
       int64_t Word6 = Value & 0x3f;

llvm/test/ExecutionEngine/JITLink/RISCV/riscv_reloc_add.s

 # RUN: rm -rf %t && mkdir -p %t
 # RUN: llvm-mc -triple=riscv64 -filetype=obj -o %t/riscv64_reloc_add.o %s
 # RUN: llvm-mc -triple=riscv32 -filetype=obj -o %t/riscv32_reloc_add.o %s
-# RUN: llvm-jitlink -noexec -check %s %t/riscv64_reloc_add.o
-# RUN: llvm-jitlink -noexec -check %s %t/riscv32_reloc_add.o
+# RUN: llvm-jitlink -noexec -check %s %t/riscv64_reloc_add.o \
+# RUN:     -slab-allocate=1Mb -slab-address=0x1000 -slab-page-size=0x1000
+# RUN: llvm-jitlink -noexec -check %s %t/riscv32_reloc_add.o \
+# RUN:     -slab-allocate=1Mb -slab-address=0x1000 -slab-page-size=0x1000
 
 # jitlink-check: *{8}(named_data) = 0x8
 # jitlink-check: *{4}(named_data+8) = 0x8
 # jitlink-check: *{2}(named_data+12) = 0x8
 # jitlink-check: *{1}(named_data+14) = 0x8
 # jitlink-check: *{1}(named_data+15) = 0x8
 
 .global main