TCG sar UB (fwd)

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~

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,
>

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~

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.

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~

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..]