Re: [Qemu-devel] [PATCH v2 56/60] target-i386: Tidy gen_add_A0_im

[Richard Henderson <rth@twiddle.net>]

On 12/26/2013 10:58 AM, Peter Maydell wrote:
> On 29 November 2013 03:00, Richard Henderson <rth@twiddle.net> wrote:
>> Merge gen_op_addl_A0_im and gen_op_addq_A0_im into gen_add_A0_im
>> and clean up the ifdef.
>>
>> Replace the one remaining user of gen_op_addl_A0_im with gen_add_A0_im.
>>
>> Signed-off-by: Richard Henderson <rth@twiddle.net>
>> ---
>>  target-i386/translate.c | 27 +++++----------------------
>>  1 file changed, 5 insertions(+), 22 deletions(-)
>>
>> diff --git a/target-i386/translate.c b/target-i386/translate.c
>> index 19cabf6..ee9d586 100644
>> --- a/target-i386/translate.c
>> +++ b/target-i386/translate.c
>> @@ -376,29 +376,12 @@ static inline void gen_op_mov_v_reg(TCGMemOp ot, TCGv t0, int reg)
>>      }
>>  }
>>
>> -static inline void gen_op_addl_A0_im(int32_t val)
>> -{
>> -    tcg_gen_addi_tl(cpu_A0, cpu_A0, val);
>> -#ifdef TARGET_X86_64
>> -    tcg_gen_andi_tl(cpu_A0, cpu_A0, 0xffffffff);
>> -#endif
>> -}
>> -
>> -#ifdef TARGET_X86_64
>> -static inline void gen_op_addq_A0_im(int64_t val)
>> -{
>> -    tcg_gen_addi_tl(cpu_A0, cpu_A0, val);
>> -}
>> -#endif
>> -
>>  static void gen_add_A0_im(DisasContext *s, int val)
>>  {
>> -#ifdef TARGET_X86_64
>> -    if (CODE64(s))
>> -        gen_op_addq_A0_im(val);
>> -    else
>> -#endif
>> -        gen_op_addl_A0_im(val);
>> +    tcg_gen_addi_tl(cpu_A0, cpu_A0, val);
>> +    if (!CODE64(s)) {
>> +        tcg_gen_ext32u_tl(cpu_A0, cpu_A0);
>> +    }
>>  }
>>
>>  static inline void gen_op_jmp_T0(void)
>> @@ -6231,7 +6214,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>>                 exception */
>>              gen_op_jmp_T0();
>>              /* pop selector */
>> -            gen_op_addl_A0_im(1 << dflag);
>> +            gen_add_A0_im(s, 1 << dflag);
> 
> Why is it OK that we no longer zero extend the result of
> the add from 32 to 64 bits if CODE64(s) ? Previously we
> did the extend unconditionally.

I can only imagine that's a bug, to have suddenly zapped the high 32-bits of
the address from which we're loading.  Indeed, even this is probably not 100%
correct wrt stack segment wraparound.

Probably better to generate both addresses from ESP and ESP+C from scratch,
using gen_lea_v_seg.


r~

r~