This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Check missed Expected<T> result
ClosedPublic

Authored by dkurt on Nov 28 2022, 1:47 AM.

Diff Detail

Event Timeline

dkurt created this revision.Nov 28 2022, 1:47 AM
dkurt requested review of this revision.Nov 28 2022, 1:47 AM

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

dkurt updated this revision to Diff 478489.Nov 29 2022, 2:24 AM

Add test for R_RISCV_PCREL_LO12_S

dkurt added a comment.Dec 1 2022, 12:21 AM

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 accepted this revision.Dec 1 2022, 1:15 PM

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!

This revision is now accepted and ready to land.Dec 1 2022, 1:15 PM
dkurt added a comment.Dec 2 2022, 12:05 AM

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 added inline comments.Dec 2 2022, 12:14 AM
llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s
49 ↗(On Diff #478489)

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.

dkurt updated this revision to Diff 479534.Dec 2 2022, 12:28 AM

Add full context diff

dkurt added inline comments.Dec 2 2022, 12:32 AM
llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s
49 ↗(On Diff #478489)

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 accepted this revision.Dec 2 2022, 12:36 AM
jrtc27 added inline comments.
llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s
49 ↗(On Diff #478489)

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

dkurt added inline comments.Dec 2 2022, 12:39 AM
llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s
49 ↗(On Diff #478489)

Yeah, sorry about that. Thanks for review!

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 accepted this revision.Dec 4 2022, 3:41 PM

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 added a comment.Dec 4 2022, 8:10 PM

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 "dmitry.kurtaev@gmail.com".

dkurt added a comment.Dec 7 2022, 1:47 AM

@lhames, friendly reminder

This revision was landed with ongoing or failed builds.Dec 7 2022, 8:27 AM
This revision was automatically updated to reflect the committed changes.