[PATCH] target/arm: Avoid writing to constant TCGv in trans_CSEL()

[PATCH] target/arm: Avoid writing to constant TCGv in trans_CSEL()

[Peter Maydell]

In commit 0b188ea05acb5 we changed the implementation of
trans_CSEL() to use tcg_constant_i32(). However, this change
was incorrect, because the implementation of the function
sets up the TCGv_i32 rn and rm to be either zero or else
a TCG temp created in load_reg(), and these TCG temps are
then in both cases written to by the emitted TCG ops.
The result is that we hit a TCG assertion:

qemu-system-arm: ../../tcg/tcg.c:4455: tcg_reg_alloc_mov: Assertion `!temp_readonly(ots)' failed.

(or on a non-debug build, just produce a garbage result)

Adjust the code so that rn and rm are always writeable
temporaries whether the instruction is using the special
case "0" or a normal register as input.

Cc: qemu-stable@nongnu.org
Fixes: 0b188ea05acb5 ("target/arm: Use tcg_constant in trans_CSEL")
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
These insns are v8.1M-only, which is why this bug has
lingered for so long.
---
 target/arm/tcg/translate.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
index 13c88ba1b9f..b71ac2d0d53 100644
--- a/target/arm/tcg/translate.c
+++ b/target/arm/tcg/translate.c
@@ -8799,7 +8799,7 @@ static bool trans_IT(DisasContext *s, arg_IT *a)
 /* v8.1M CSEL/CSINC/CSNEG/CSINV */
 static bool trans_CSEL(DisasContext *s, arg_CSEL *a)
 {
-    TCGv_i32 rn, rm, zero;
+    TCGv_i32 rn, rm;
     DisasCompare c;
 
     if (!arm_dc_feature(s, ARM_FEATURE_V8_1M)) {
@@ -8817,16 +8817,17 @@ static bool trans_CSEL(DisasContext *s, arg_CSEL *a)
     }
 
     /* In this insn input reg fields of 0b1111 mean "zero", not "PC" */
-    zero = tcg_constant_i32(0);
+    rn = tcg_temp_new_i32();
+    rm = tcg_temp_new_i32();
     if (a->rn == 15) {
-        rn = zero;
+        tcg_gen_movi_i32(rn, 0);
     } else {
-        rn = load_reg(s, a->rn);
+        load_reg_var(s, rn, a->rn);
     }
     if (a->rm == 15) {
-        rm = zero;
+        tcg_gen_movi_i32(rm, 0);
     } else {
-        rm = load_reg(s, a->rm);
+        load_reg_var(s, rm, a->rm);
     }
 
     switch (a->op) {
@@ -8846,7 +8847,7 @@ static bool trans_CSEL(DisasContext *s, arg_CSEL *a)
     }
 
     arm_test_cc(&c, a->fcond);
-    tcg_gen_movcond_i32(c.cond, rn, c.value, zero, rn, rm);
+    tcg_gen_movcond_i32(c.cond, rn, c.value, tcg_constant_i32(0), rn, rm);
 
     store_reg(s, a->rd, rn);
     return true;
-- 
2.34.1

Re: [PATCH] target/arm: Avoid writing to constant TCGv in trans_CSEL()

[Richard Henderson]

On 7/27/23 03:39, Peter Maydell wrote:
> In commit 0b188ea05acb5 we changed the implementation of
> trans_CSEL() to use tcg_constant_i32(). However, this change
> was incorrect, because the implementation of the function
> sets up the TCGv_i32 rn and rm to be either zero or else
> a TCG temp created in load_reg(), and these TCG temps are
> then in both cases written to by the emitted TCG ops.
> The result is that we hit a TCG assertion:
> 
> qemu-system-arm: ../../tcg/tcg.c:4455: tcg_reg_alloc_mov: Assertion `!temp_readonly(ots)' failed.
> 
> (or on a non-debug build, just produce a garbage result)
> 
> Adjust the code so that rn and rm are always writeable
> temporaries whether the instruction is using the special
> case "0" or a normal register as input.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: 0b188ea05acb5 ("target/arm: Use tcg_constant in trans_CSEL")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> These insns are v8.1M-only, which is why this bug has
> lingered for so long.
> ---
>   target/arm/tcg/translate.c | 15 ++++++++-------
>   1 file changed, 8 insertions(+), 7 deletions(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~