[PATCH] D107328: [JITLink] Add fixup value range check

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

This was clearly not done whilst referencing LLD's implementation as it's buggy. Please fix.



================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:204
+        return makeTargetOutOfRangeError(G, B, E);
       int32_t Hi = (Value + 0x800) & 0xFFFFF000;
       uint32_t RawInstr = *(little32_t *)FixupPtr;
----------------
You need to check that `Value + 0x800` is a 32-bit signed integer, not that `Value` is a 32-bit unsigned integer. LUI sign-extends, hence checking it as a //signed// integer, and the + 0x800 needs to be added to account for the slight asymmetry resulting from the ADDI/etc's sign-extension of the LO12.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:212
+      if (LLVM_UNLIKELY(!isInRangeForImmU32(Value)))
+        return makeTargetOutOfRangeError(G, B, E);
       int32_t Lo = Value & 0xFFF;
----------------
LO12 should never be range checked, it's a waste of time as the HI20 half will also range check.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:234
       int64_t Value = E.getTarget().getAddress() + E.getAddend() - FixupAddress;
+      if (LLVM_UNLIKELY(!isInRangeForImmS32(Value)))
+        return makeTargetOutOfRangeError(G, B, E);
----------------
As with HI20, this needs to check Value + 0x800, not Value. But this at least already checks it as a signed integer rather than an unsigned integer.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:248
+      if (LLVM_UNLIKELY(!isInRangeForImmS32(Value)))
+        return makeTargetOutOfRangeError(G, B, E);
       int64_t Lo = Value & 0xFFF;
----------------
As with LO12, these are a waste of time


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107328/new/

https://reviews.llvm.org/D107328