Re: [Qemu-devel] TCG sar UB (fwd)

Re: [Qemu-devel] TCG sar UB (fwd)

[Richard Henderson]

On 09/03/2011 03:47 PM, malc wrote:
> Doesn't make much sense to me, guest clearly asked for 0 and not -1,
> besides -1 violates TCG's sar constraints and PPC obliges by emiting
> illegal instruction in this case.

The shift that the guest asked for was completely folded away.

The -1 comes from gen_shift_rm_T1 in the computation of the new
flags value.  This could instead be moved inside the test for != 0,
which is the only place that value is actually used anyway.

Try this.  Lightly tested.


r~


diff --git a/target-i386/translate.c b/target-i386/translate.c
index ccef381..b966762 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -1406,70 +1406,84 @@ static void gen_shift_rm_T1(DisasContext *s, int ot, int op1,
 {
     target_ulong mask;
     int shift_label;
-    TCGv t0, t1;
+    TCGv t0, t1, t2;
 
-    if (ot == OT_QUAD)
+    if (ot == OT_QUAD) {
         mask = 0x3f;
-    else
+    } else {
         mask = 0x1f;
+    }
 
     /* load */
-    if (op1 == OR_TMP0)
+    if (op1 == OR_TMP0) {
         gen_op_ld_T0_A0(ot + s->mem_index);
-    else
+    } else {
         gen_op_mov_TN_reg(ot, 0, op1);
+    }
 
-    tcg_gen_andi_tl(cpu_T[1], cpu_T[1], mask);
+    t0 = tcg_temp_local_new();
+    t1 = tcg_temp_local_new();
+    t2 = tcg_temp_local_new();
 
-    tcg_gen_addi_tl(cpu_tmp5, cpu_T[1], -1);
+    tcg_gen_andi_tl(t2, cpu_T[1], mask);
 
     if (is_right) {
         if (is_arith) {
             gen_exts(ot, cpu_T[0]);
-            tcg_gen_sar_tl(cpu_T3, cpu_T[0], cpu_tmp5);
-            tcg_gen_sar_tl(cpu_T[0], cpu_T[0], cpu_T[1]);
+            tcg_gen_mov_tl(t0, cpu_T[0]);
+            tcg_gen_sar_tl(cpu_T[0], cpu_T[0], t2);
         } else {
             gen_extu(ot, cpu_T[0]);
-            tcg_gen_shr_tl(cpu_T3, cpu_T[0], cpu_tmp5);
-            tcg_gen_shr_tl(cpu_T[0], cpu_T[0], cpu_T[1]);
+            tcg_gen_mov_tl(t0, cpu_T[0]);
+            tcg_gen_shr_tl(cpu_T[0], cpu_T[0], t2);
         }
     } else {
-        tcg_gen_shl_tl(cpu_T3, cpu_T[0], cpu_tmp5);
-        tcg_gen_shl_tl(cpu_T[0], cpu_T[0], cpu_T[1]);
+        tcg_gen_mov_tl(t0, cpu_T[0]);
+        tcg_gen_shl_tl(cpu_T[0], cpu_T[0], t2);
     }
 
     /* store */
-    if (op1 == OR_TMP0)
+    if (op1 == OR_TMP0) {
         gen_op_st_T0_A0(ot + s->mem_index);
-    else
+    } else {
         gen_op_mov_reg_T0(ot, op1);
-        
+    }
+
     /* update eflags if non zero shift */
-    if (s->cc_op != CC_OP_DYNAMIC)
+    if (s->cc_op != CC_OP_DYNAMIC) {
         gen_op_set_cc_op(s->cc_op);
+    }
 
-    /* XXX: inefficient */
-    t0 = tcg_temp_local_new();
-    t1 = tcg_temp_local_new();
-
-    tcg_gen_mov_tl(t0, cpu_T[0]);
-    tcg_gen_mov_tl(t1, cpu_T3);
+    tcg_gen_mov_tl(t1, cpu_T[0]);
 
     shift_label = gen_new_label();
-    tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_T[1], 0, shift_label);
+    tcg_gen_brcondi_tl(TCG_COND_EQ, t2, 0, shift_label);
 
-    tcg_gen_mov_tl(cpu_cc_src, t1);
-    tcg_gen_mov_tl(cpu_cc_dst, t0);
-    if (is_right)
+    tcg_gen_addi_tl(t2, t2, -1);
+    tcg_gen_mov_tl(cpu_cc_dst, t1);
+
+    if (is_right) {
+        if (is_arith) {
+            tcg_gen_sar_tl(cpu_cc_src, t0, t2);
+        } else {
+            tcg_gen_shr_tl(cpu_cc_src, t0, t2);
+        }
+    } else {
+        tcg_gen_shl_tl(cpu_cc_src, t0, t2);
+    }
+
+    if (is_right) {
         tcg_gen_movi_i32(cpu_cc_op, CC_OP_SARB + ot);
-    else
+    } else {
         tcg_gen_movi_i32(cpu_cc_op, CC_OP_SHLB + ot);
-        
+    }
+
     gen_set_label(shift_label);
     s->cc_op = CC_OP_DYNAMIC; /* cannot predict flags after */
 
     tcg_temp_free(t0);
     tcg_temp_free(t1);
+    tcg_temp_free(t2);
 }
 
 static void gen_shift_rm_im(DisasContext *s, int ot, int op1, int op2,

Re: [Qemu-devel] TCG sar UB (fwd)

[malc]

On Sun, 4 Sep 2011, Richard Henderson wrote:

> On 09/03/2011 03:47 PM, malc wrote:
> > Doesn't make much sense to me, guest clearly asked for 0 and not -1,
> > besides -1 violates TCG's sar constraints and PPC obliges by emiting
> > illegal instruction in this case.
> 
> The shift that the guest asked for was completely folded away.
> 
> The -1 comes from gen_shift_rm_T1 in the computation of the new
> flags value.  This could instead be moved inside the test for != 0,
> which is the only place that value is actually used anyway.
> 
> Try this.  Lightly tested.

Now i either get hosts illegal instruction or (with logging enabled) a
guest kenrnel panic.

> 
> 
> r~
> 
> 
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index ccef381..b966762 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -1406,70 +1406,84 @@ static void gen_shift_rm_T1(DisasContext *s, int ot, int op1,
>  {
>      target_ulong mask;
>      int shift_label;
> -    TCGv t0, t1;
> +    TCGv t0, t1, t2;
>  
> -    if (ot == OT_QUAD)
> +    if (ot == OT_QUAD) {
>          mask = 0x3f;
> -    else
> +    } else {
>          mask = 0x1f;
> +    }
>  
>      /* load */
> -    if (op1 == OR_TMP0)
> +    if (op1 == OR_TMP0) {
>          gen_op_ld_T0_A0(ot + s->mem_index);
> -    else
> +    } else {
>          gen_op_mov_TN_reg(ot, 0, op1);
> +    }
>  
> -    tcg_gen_andi_tl(cpu_T[1], cpu_T[1], mask);
> +    t0 = tcg_temp_local_new();
> +    t1 = tcg_temp_local_new();
> +    t2 = tcg_temp_local_new();
>  
> -    tcg_gen_addi_tl(cpu_tmp5, cpu_T[1], -1);
> +    tcg_gen_andi_tl(t2, cpu_T[1], mask);
>  
>      if (is_right) {
>          if (is_arith) {
>              gen_exts(ot, cpu_T[0]);
> -            tcg_gen_sar_tl(cpu_T3, cpu_T[0], cpu_tmp5);
> -            tcg_gen_sar_tl(cpu_T[0], cpu_T[0], cpu_T[1]);
> +            tcg_gen_mov_tl(t0, cpu_T[0]);
> +            tcg_gen_sar_tl(cpu_T[0], cpu_T[0], t2);
>          } else {
>              gen_extu(ot, cpu_T[0]);
> -            tcg_gen_shr_tl(cpu_T3, cpu_T[0], cpu_tmp5);
> -            tcg_gen_shr_tl(cpu_T[0], cpu_T[0], cpu_T[1]);
> +            tcg_gen_mov_tl(t0, cpu_T[0]);
> +            tcg_gen_shr_tl(cpu_T[0], cpu_T[0], t2);
>          }
>      } else {
> -        tcg_gen_shl_tl(cpu_T3, cpu_T[0], cpu_tmp5);
> -        tcg_gen_shl_tl(cpu_T[0], cpu_T[0], cpu_T[1]);
> +        tcg_gen_mov_tl(t0, cpu_T[0]);
> +        tcg_gen_shl_tl(cpu_T[0], cpu_T[0], t2);
>      }
>  
>      /* store */
> -    if (op1 == OR_TMP0)
> +    if (op1 == OR_TMP0) {
>          gen_op_st_T0_A0(ot + s->mem_index);
> -    else
> +    } else {
>          gen_op_mov_reg_T0(ot, op1);
> -        
> +    }
> +
>      /* update eflags if non zero shift */
> -    if (s->cc_op != CC_OP_DYNAMIC)
> +    if (s->cc_op != CC_OP_DYNAMIC) {
>          gen_op_set_cc_op(s->cc_op);
> +    }
>  
> -    /* XXX: inefficient */
> -    t0 = tcg_temp_local_new();
> -    t1 = tcg_temp_local_new();
> -
> -    tcg_gen_mov_tl(t0, cpu_T[0]);
> -    tcg_gen_mov_tl(t1, cpu_T3);
> +    tcg_gen_mov_tl(t1, cpu_T[0]);
>  
>      shift_label = gen_new_label();
> -    tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_T[1], 0, shift_label);
> +    tcg_gen_brcondi_tl(TCG_COND_EQ, t2, 0, shift_label);
>  
> -    tcg_gen_mov_tl(cpu_cc_src, t1);
> -    tcg_gen_mov_tl(cpu_cc_dst, t0);
> -    if (is_right)
> +    tcg_gen_addi_tl(t2, t2, -1);
> +    tcg_gen_mov_tl(cpu_cc_dst, t1);
> +
> +    if (is_right) {
> +        if (is_arith) {
> +            tcg_gen_sar_tl(cpu_cc_src, t0, t2);
> +        } else {
> +            tcg_gen_shr_tl(cpu_cc_src, t0, t2);
> +        }
> +    } else {
> +        tcg_gen_shl_tl(cpu_cc_src, t0, t2);
> +    }
> +
> +    if (is_right) {
>          tcg_gen_movi_i32(cpu_cc_op, CC_OP_SARB + ot);
> -    else
> +    } else {
>          tcg_gen_movi_i32(cpu_cc_op, CC_OP_SHLB + ot);
> -        
> +    }
> +
>      gen_set_label(shift_label);
>      s->cc_op = CC_OP_DYNAMIC; /* cannot predict flags after */
>  
>      tcg_temp_free(t0);
>      tcg_temp_free(t1);
> +    tcg_temp_free(t2);
>  }
>  
>  static void gen_shift_rm_im(DisasContext *s, int ot, int op1, int op2,
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] TCG sar UB (fwd)

[Richard Henderson]

On 09/04/2011 08:03 AM, malc wrote:
> On Sun, 4 Sep 2011, Richard Henderson wrote:
> 
>> On 09/03/2011 03:47 PM, malc wrote:
>>> Doesn't make much sense to me, guest clearly asked for 0 and not -1,
>>> besides -1 violates TCG's sar constraints and PPC obliges by emiting
>>> illegal instruction in this case.
>>
>> The shift that the guest asked for was completely folded away.
>>
>> The -1 comes from gen_shift_rm_T1 in the computation of the new
>> flags value.  This could instead be moved inside the test for != 0,
>> which is the only place that value is actually used anyway.
>>
>> Try this.  Lightly tested.
> 
> Now i either get hosts illegal instruction or (with logging enabled) a
> guest kenrnel panic.

Hum.  Well, it's possible I made some silly mistake in the patch.
You now know where to look for the problem.

That said, it seems easier for you in the PPC port to mask the 
immediate operand so that you don't produce an illegal instruction.
It doesn't really matter what value that insn produces, as the
result won't be used.


r~

Re: [Qemu-devel] TCG sar UB (fwd)

[malc]

On Tue, 6 Sep 2011, Richard Henderson wrote:

> On 09/04/2011 08:03 AM, malc wrote:
> > On Sun, 4 Sep 2011, Richard Henderson wrote:
> > 
> >> On 09/03/2011 03:47 PM, malc wrote:
> >>> Doesn't make much sense to me, guest clearly asked for 0 and not -1,
> >>> besides -1 violates TCG's sar constraints and PPC obliges by emiting
> >>> illegal instruction in this case.
> >>
> >> The shift that the guest asked for was completely folded away.
> >>
> >> The -1 comes from gen_shift_rm_T1 in the computation of the new
> >> flags value.  This could instead be moved inside the test for != 0,
> >> which is the only place that value is actually used anyway.
> >>
> >> Try this.  Lightly tested.
> > 
> > Now i either get hosts illegal instruction or (with logging enabled) a
> > guest kenrnel panic.
> 
> Hum.  Well, it's possible I made some silly mistake in the patch.
> You now know where to look for the problem.

Correct me if i'm wrong, previously the code worked like this:

mov tmp, 0
sub tmp, 1
sar r, r, tmp

Still UB as far as TCG is concerned but since no immediates are involved
things worked, now, with constant folding, we are asked to sar by -1 
directly.

> 
> That said, it seems easier for you in the PPC port to mask the 
> immediate operand so that you don't produce an illegal instruction.
> It doesn't really matter what value that insn produces, as the
> result won't be used.
> 

I did that when first hit this problem, but decided not to push it.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] TCG sar UB (fwd)

[Richard Henderson]

On 09/06/2011 08:50 PM, malc wrote:
> Correct me if i'm wrong, previously the code worked like this:
> 
> mov tmp, 0
> sub tmp, 1
> sar r, r, tmp
> 
> Still UB as far as TCG is concerned but since no immediates are involved
> things worked, now, with constant folding, we are asked to sar by -1 
> directly.

You are exactly correct.

That's why I thought my patch to re-arrange the order of operations
and only perform the subtraction inside the %cl != 0 test was a good
idea.  No point in performing the shift if we're not going to use the
result.

> I did that when first hit this problem, but decided not to push it.

I think pushing it is a good idea.  Just because the result is not
defined, according to tcg/README, is no reason to SIGILL.


r~

Re: [Qemu-devel] TCG sar UB (fwd)

[malc]

On Sun, 4 Sep 2011, malc wrote:

> On Sun, 4 Sep 2011, Richard Henderson wrote:
> 
> > On 09/03/2011 03:47 PM, malc wrote:
> > > Doesn't make much sense to me, guest clearly asked for 0 and not -1,
> > > besides -1 violates TCG's sar constraints and PPC obliges by emiting
> > > illegal instruction in this case.
> > 
> > The shift that the guest asked for was completely folded away.
> > 
> > The -1 comes from gen_shift_rm_T1 in the computation of the new
> > flags value.  This could instead be moved inside the test for != 0,
> > which is the only place that value is actually used anyway.
> > 
> > Try this.  Lightly tested.
> 
> Now i either get hosts illegal instruction or (with logging enabled) a
> guest kenrnel panic.

Actually i was habitually testing i386-softmmu/qemu.. And after trying
the "properly" named binary things do work.. Want to provide a comment
so i can push that?

[..snip..]

-- 
mailto:av1474@comtv.ru

[Qemu-devel] [PATCH] target-i386: Compute all flag data inside %cl != 0 test.

[Richard Henderson]

The (x << (cl - 1)) quantity is only used if CL != 0.  Move the
computation of that quantity nearer its use.

This avoids the creation of undefined TCG operations when the
constant propagation optimization proves that CL == 0, and thus
CL-1 is outside the range [0-wordsize).

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-i386/translate.c |   72 ++++++++++++++++++++++++++++-------------------
 1 files changed, 43 insertions(+), 29 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index ccef381..b966762 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -1406,70 +1406,84 @@ static void gen_shift_rm_T1(DisasContext *s, int ot, int op1,
 {
     target_ulong mask;
     int shift_label;
-    TCGv t0, t1;
+    TCGv t0, t1, t2;
 
-    if (ot == OT_QUAD)
+    if (ot == OT_QUAD) {
         mask = 0x3f;
-    else
+    } else {
         mask = 0x1f;
+    }
 
     /* load */
-    if (op1 == OR_TMP0)
+    if (op1 == OR_TMP0) {
         gen_op_ld_T0_A0(ot + s->mem_index);
-    else
+    } else {
         gen_op_mov_TN_reg(ot, 0, op1);
+    }
 
-    tcg_gen_andi_tl(cpu_T[1], cpu_T[1], mask);
+    t0 = tcg_temp_local_new();
+    t1 = tcg_temp_local_new();
+    t2 = tcg_temp_local_new();
 
-    tcg_gen_addi_tl(cpu_tmp5, cpu_T[1], -1);
+    tcg_gen_andi_tl(t2, cpu_T[1], mask);
 
     if (is_right) {
         if (is_arith) {
             gen_exts(ot, cpu_T[0]);
-            tcg_gen_sar_tl(cpu_T3, cpu_T[0], cpu_tmp5);
-            tcg_gen_sar_tl(cpu_T[0], cpu_T[0], cpu_T[1]);
+            tcg_gen_mov_tl(t0, cpu_T[0]);
+            tcg_gen_sar_tl(cpu_T[0], cpu_T[0], t2);
         } else {
             gen_extu(ot, cpu_T[0]);
-            tcg_gen_shr_tl(cpu_T3, cpu_T[0], cpu_tmp5);
-            tcg_gen_shr_tl(cpu_T[0], cpu_T[0], cpu_T[1]);
+            tcg_gen_mov_tl(t0, cpu_T[0]);
+            tcg_gen_shr_tl(cpu_T[0], cpu_T[0], t2);
         }
     } else {
-        tcg_gen_shl_tl(cpu_T3, cpu_T[0], cpu_tmp5);
-        tcg_gen_shl_tl(cpu_T[0], cpu_T[0], cpu_T[1]);
+        tcg_gen_mov_tl(t0, cpu_T[0]);
+        tcg_gen_shl_tl(cpu_T[0], cpu_T[0], t2);
     }
 
     /* store */
-    if (op1 == OR_TMP0)
+    if (op1 == OR_TMP0) {
         gen_op_st_T0_A0(ot + s->mem_index);
-    else
+    } else {
         gen_op_mov_reg_T0(ot, op1);
-        
+    }
+
     /* update eflags if non zero shift */
-    if (s->cc_op != CC_OP_DYNAMIC)
+    if (s->cc_op != CC_OP_DYNAMIC) {
         gen_op_set_cc_op(s->cc_op);
+    }
 
-    /* XXX: inefficient */
-    t0 = tcg_temp_local_new();
-    t1 = tcg_temp_local_new();
-
-    tcg_gen_mov_tl(t0, cpu_T[0]);
-    tcg_gen_mov_tl(t1, cpu_T3);
+    tcg_gen_mov_tl(t1, cpu_T[0]);
 
     shift_label = gen_new_label();
-    tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_T[1], 0, shift_label);
+    tcg_gen_brcondi_tl(TCG_COND_EQ, t2, 0, shift_label);
 
-    tcg_gen_mov_tl(cpu_cc_src, t1);
-    tcg_gen_mov_tl(cpu_cc_dst, t0);
-    if (is_right)
+    tcg_gen_addi_tl(t2, t2, -1);
+    tcg_gen_mov_tl(cpu_cc_dst, t1);
+
+    if (is_right) {
+        if (is_arith) {
+            tcg_gen_sar_tl(cpu_cc_src, t0, t2);
+        } else {
+            tcg_gen_shr_tl(cpu_cc_src, t0, t2);
+        }
+    } else {
+        tcg_gen_shl_tl(cpu_cc_src, t0, t2);
+    }
+
+    if (is_right) {
         tcg_gen_movi_i32(cpu_cc_op, CC_OP_SARB + ot);
-    else
+    } else {
         tcg_gen_movi_i32(cpu_cc_op, CC_OP_SHLB + ot);
-        
+    }
+
     gen_set_label(shift_label);
     s->cc_op = CC_OP_DYNAMIC; /* cannot predict flags after */
 
     tcg_temp_free(t0);
     tcg_temp_free(t1);
+    tcg_temp_free(t2);
 }
 
 static void gen_shift_rm_im(DisasContext *s, int ot, int op1, int op2,
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH] target-i386: Compute all flag data inside %cl != 0 test.

[malc]

On Thu, 8 Sep 2011, Richard Henderson wrote:

> The (x << (cl - 1)) quantity is only used if CL != 0.  Move the
> computation of that quantity nearer its use.
> 
> This avoids the creation of undefined TCG operations when the
> constant propagation optimization proves that CL == 0, and thus
> CL-1 is outside the range [0-wordsize).
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>

Applied, thanks.

[..snip..]

-- 
mailto:av1474@comtv.ru