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

[Job Noorman via Phabricator via llvm-commits]

jobnoorman created this revision.
jobnoorman added reviewers: lhames, StephenFan.
Herald added subscribers: asb, luke, pmatos, VincentWu, vkmr, frasercrmck, evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, shiva0217, kito-cheng, niosHD, sabuasal, simoncook, johnrusso, rbar, hiraditya, arichardson.
Herald added a project: All.
jobnoorman requested review of this revision.
Herald added subscribers: llvm-commits, pcwang-thead, eopXD.
Herald added a project: LLVM.

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.

Note that I'm not quite sure how to write tests for this as it doesn't
seem possible to tell llvm-jitlink which addresses it should use.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147693

Files:
  llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp


Index: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp
===================================================================
--- llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp
+++ llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp
@@ -322,63 +322,52 @@
     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())) -
-          E.getTarget().getAddress().getValue() - E.getAddend();
+      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;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D147693.511344.patch
Type: text/x-patch
Size: 3546 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230406/d56465d6/attachment.bin>