| Message ID | 20220331150858.96348-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-7.0] target/arm: Don't use DISAS_NORETURN in STXP !HAVE_CMPXCHG128 codegen | expand |
On 3/31/22 09:08, Peter Maydell wrote: > In gen_store_exclusive(), if the host does not have a cmpxchg128 > primitive then we generate bad code for STXP for storing two 64-bit > values. We generate a call to the exit_atomic helper, which never > returns, and set is_jmp to DISAS_NORETURN. However, this is > forgetting that we have already emitted a brcond that jumps over this > call for the case where we don't hold the exclusive. The effect is > that we don't generate any code to end the TB for the > exclusive-not-held execution path, which falls into the "exit with > TB_EXIT_REQUESTED" code that gen_tb_end() emits. This then causes an > assert at runtime when cpu_loop_exec_tb() sees an EXIT_REQUESTED TB > return that wasn't for an interrupt or icount. > > In particular, you can hit this case when using the clang sanitizers > and trying to run the xlnx-versal-virt acceptance test in 'make > check-acceptance'. This bug was masked until commit 848126d11e93ff > ("meson: move int128 checks from configure") because we used to set > CONFIG_CMPXCHG128=1 and avoid the buggy codepath, but after that we > do not. > > Fix the bug by not setting is_jmp. The code after the exit_atomic > call up to the fail_label is dead, but TCG is smart enough to > eliminate it. We do need to set 'tmp' to some valid value, though > (in the same way the exit_atomic-using code in tcg/tcg-op.c does). > > Resolves:https://gitlab.com/qemu-project/qemu/-/issues/953 > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > --- > target/arm/translate-a64.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index d1a59fad9c2..9333d7be41a 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -2470,7 +2470,12 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2, } else if (tb_cflags(s->base.tb) & CF_PARALLEL) { if (!HAVE_CMPXCHG128) { gen_helper_exit_atomic(cpu_env); - s->base.is_jmp = DISAS_NORETURN; + /* + * Produce a result so we have a well-formed opcode + * stream when the following (dead) code uses 'tmp'. + * TCG will remove the dead ops for us. + */ + tcg_gen_movi_i64(tmp, 0); } else if (s->be_data == MO_LE) { gen_helper_paired_cmpxchg64_le_parallel(tmp, cpu_env, cpu_exclusive_addr,
In gen_store_exclusive(), if the host does not have a cmpxchg128 primitive then we generate bad code for STXP for storing two 64-bit values. We generate a call to the exit_atomic helper, which never returns, and set is_jmp to DISAS_NORETURN. However, this is forgetting that we have already emitted a brcond that jumps over this call for the case where we don't hold the exclusive. The effect is that we don't generate any code to end the TB for the exclusive-not-held execution path, which falls into the "exit with TB_EXIT_REQUESTED" code that gen_tb_end() emits. This then causes an assert at runtime when cpu_loop_exec_tb() sees an EXIT_REQUESTED TB return that wasn't for an interrupt or icount. In particular, you can hit this case when using the clang sanitizers and trying to run the xlnx-versal-virt acceptance test in 'make check-acceptance'. This bug was masked until commit 848126d11e93ff ("meson: move int128 checks from configure") because we used to set CONFIG_CMPXCHG128=1 and avoid the buggy codepath, but after that we do not. Fix the bug by not setting is_jmp. The code after the exit_atomic call up to the fail_label is dead, but TCG is smart enough to eliminate it. We do need to set 'tmp' to some valid value, though (in the same way the exit_atomic-using code in tcg/tcg-op.c does). Resolves: https://gitlab.com/qemu-project/qemu/-/issues/953 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/translate-a64.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)