[RISCV] Check missed Expected<T> result

[dkurt]

Summary

related issue: https://github.com/llvm/llvm-project/issues/59139

[dblaikie]

any chance of portable test coverage? (like a JITLink unit test or llvm-jitlink lit test)

[dkurt]

Add test for R_RISCV_PCREL_LO12_S

[dkurt]

In D138781#3955477, @dblaikie wrote:

any chance of portable test coverage? (like a JITLink unit test or llvm-jitlink lit test)

Hi, test was added. I see that x64 Debian is failed (probably, an error is not related to this fix?).

[dblaikie]

Is it possible to test the actual error case too? (show that the error is propagated/reported?)

Looks good though - if you happen to be able to conjure up the failing test too, that'd be great!

[dkurt]

In D138781#3964747, @dblaikie wrote:

Is it possible to test the actual error case too? (show that the error is propagated/reported?)

Looks good though - if you happen to be able to conjure up the failing test too, that'd be great!

Not sure I could handle it. The added test is similar to one for R_RISCV_PCREL_LO12_I (success path) but I have not found any existing checks for fail results of getRISCVPCRelHi20.

[jrtc27]

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

49

Why is this a test for the case you're fixing? This is valid input so shouldn't be hitting an error path in the first place.

[jrtc27]

Please upload diffs with full context; see https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[dkurt]

Add full context diff

[dkurt]

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

49

Actually, no error is expected. Without changes in ELF_riscv.cpp LLVM throws

Expected<T> must be checked before access or destruction.
Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).

because result RelHI20 wrapped to Expected<T> is correct but not checked for the potential error.

[jrtc27]

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

49

Oh ok, the context makes this clearer, I thought it was code shared with the _I form but no. Thanks for the clarification.

[dkurt]

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

49

Yeah, sorry about that. Thanks for review!

[dblaikie]

In D138781#3965828, @dkurt wrote:

In D138781#3964747, @dblaikie wrote:

Is it possible to test the actual error case too? (show that the error is propagated/reported?)

Looks good though - if you happen to be able to conjure up the failing test too, that'd be great!

Not sure I could handle it. The added test is similar to one for R_RISCV_PCREL_LO12_I (success path) but I have not found any existing checks for fail results of getRISCVPCRelHi20.

Ah, yeah, I tried and I guess it'd require hand-crafted yaml to feed to yaml2obj, probably - because the assembler won't let you create this situation:

/usr/local/google/home/blaikie/dev/llvm/src/llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s:37:9: error: could not find corresponding %pcrel_hi
        lw  a0, %pcrel_lo(test_pcrel32)(a0)
        ^

(if I comment out the prior line with the pcrel_hi on it, to produce this ^)

SGTM - will leave it to @lhames for final sign-off.

[lhames]

LGTM.

@dkurt Do you have commit access already? If not please just let me know the name and email that you'd like me to use for attribution and I'll commit on your behalf.

[dkurt]

In D138781#3969557, @lhames wrote:

LGTM.

@dkurt Do you have commit access already? If not please just let me know the name and email that you'd like me to use for attribution and I'll commit on your behalf.

Thank you!
Not yet, I believe. Name is "Dmitry Kurtaev" and email "[e-mail]".

[dkurt]

@lhames, friendly reminder

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp

case R_RISCV_PCREL_LO12_I: {
           (RawInstr & 0xFFFFF) | (static_cast<uint32_t>(Lo & 0xFFF) << 20);
       break;
     }
     case R_RISCV_PCREL_LO12_S: {
       // FIXME: We assume that R_RISCV_PCREL_HI20 is present in object code and
       // pairs with current relocation R_RISCV_PCREL_LO12_S. So here may need a
       // check.
       auto RelHI20 = getRISCVPCRelHi20(E);
+      if (!RelHI20)
+        return RelHI20.takeError();
       int64_t Value = RelHI20->getTarget().getAddress() +
                       RelHI20->getAddend() - E.getTarget().getAddress();
       int64_t Lo = Value & 0xFFF;
       uint32_t Imm31_25 = extractBits(Lo, 5, 7) << 25;
       uint32_t Imm11_7 = extractBits(Lo, 0, 5) << 7;
       uint32_t RawInstr = *(little32_t *)FixupPtr;
 
       *(little32_t *)FixupPtr = (RawInstr & 0x1FFF07F) | Imm31_25 | Imm11_7;

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

# jitlink-check: decode_operand(test_pcrel32+4, 2)[11:0] = (external_data - test_pcrel32)[11:0]
         .p2align  1
         .type   test_pcrel32,@function
 test_pcrel32:
         auipc a0, %pcrel_hi(external_data)
         lw  a0, %pcrel_lo(test_pcrel32)(a0)
 
         .size   test_pcrel32, .-test_pcrel32
 
+# Test R_RISCV_PCREL_HI20 and R_RISCV_PCREL_LO12_S
+# jitlink-check: decode_operand(test_pcrel32_s, 1) = ((external_data - test_pcrel32_s) + 0x800)[31:12]
+# jitlink-check: decode_operand(test_pcrel32_s+4, 2)[11:0] = (external_data - test_pcrel32_s)[11:0]
+        .globl  test_pcrel32_s
+        .p2align  1
+        .type   test_pcrel32_s,@function
+test_pcrel32_s:
+        auipc a0, %pcrel_hi(external_data)
+        sw  a0, %pcrel_lo(test_pcrel32_s)(a0)
+
+        .size   test_pcrel32_s, .-test_pcrel32_s
+
 # Test R_RISCV_CALL
 # jitlink-check: decode_operand(test_call, 1) = ((external_func - test_call) + 0x800)[31:12]
 # jitlink-check: decode_operand(test_call+4, 2)[11:0] = (external_func - test_call)[11:0]
         .globl test_call, external_func
         .p2align  1
         .type  test_call,@function
 test_call:
         .reloc ., R_RISCV_CALL, external_func
         auipc ra, 0
         jalr ra
         ret
         .size test_call, .-test_call