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:address@hidden