On Thu, 28 Nov 2024 at 12:59, Richard Henderson
<richard.henderson@linaro.org> wrote:
On 11/28/24 04:38, Peter Maydell wrote:
In riscv_cpu_do_interrupt() we use the 'cause' value we got out of
cs->exception as a shift value. However this value can be larger
than 31, which means that "1 << cause" is undefined behaviour,
because we do the shift on an 'int' type.
This causes the undefined behaviour sanitizer to complain
on one of the check-tcg tests:
$ UBSAN_OPTIONS=print_stacktrace=1:abort_on_error=1:halt_on_error=1
./build/clang/qemu-system-riscv64 -M virt -semihosting -display none -device
loader,file=build/clang/tests/tcg/riscv64-softmmu/issue1060
../../target/riscv/cpu_helper.c:1805:38: runtime error: shift exponent 63 is
too large for 32-bit type 'int'
#0 0x55f2dc026703 in riscv_cpu_do_interrupt
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../target/riscv/cpu_helper.c:1805:38
#1 0x55f2dc3d170e in cpu_handle_exception
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/clang/../../accel/tcg/cpu-exec.c:752:9
In this case cause is RISCV_EXCP_SEMIHOST, which is 0x3f.
Semihosting is completely artificial and should never be injected.
The maximum "real" cause is
RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT = 0x17,
We ought to hoist the handling of RISCV_EXCP_SEMIHOST higher in the function,
before these
calculations.
Perhaps so, but looking at
https://github.com/riscv/riscv-aia/releases/download/1.0/riscv-interrupts-1.0.pdf
it says that mvien, mie, etc are 64-bit registers and the
cause value can be validly greater than 31. So we need to
use the ULL suffix here. And if we're doing that, then it's
harmless to also calculate these booleans even in the
semihosting case, because we don't look at them then.
So I think we definitely need this patch, and whether
to refactor the code to move the bool initializers around
is a separate question.