Re: [Qemu-devel] [PATCH 03/11] target/arm: Introduce read_pc

[Richard Henderson <richard.henderson@linaro.org>]

On 8/7/19 10:27 AM, Peter Maydell wrote:
>> +/* The architectural value of PC.  */
>> +static uint32_t read_pc(DisasContext *s)
>> +{
>> +    return s->pc_curr + (s->thumb ? 4 : 8);
>> +}
>> +
>>  /* Set a variable to the value of a CPU register.  */
>>  static void load_reg_var(DisasContext *s, TCGv_i32 var, int reg)
>>  {
>>      if (reg == 15) {
>> -        uint32_t addr;
>> -        /* normally, since we updated PC, we need only to add one insn */
>> -        if (s->thumb)
>> -            addr = (long)s->pc + 2;
>> -        else
>> -            addr = (long)s->pc + 4;
>> -        tcg_gen_movi_i32(var, addr);
>> +        tcg_gen_movi_i32(var, read_pc(s));
> 
> So previously:
>  * for A32 we would return s->pc + 4, which is the same as s->pc_curr + 8
>  * for T16 we would return s->pc + 2, which is the same as s->pc_curr + 4
>  * for T32 we would return s->pc + 2 -- but that's not the same as
>    s->pc_curr + 4, it's s->pc_curr + 6...
> 
> Since s->pc_curr + 4 is the right architectural answer, are we
> fixing a bug here? Or are all the places where T32 code calls
> this function UNPREDICTABLE for the reg == 15 case ?

I believe that this is UNPREDICTABLE.

The T32 cases that reference the PC that are not UNPREDICTABLE, literal memory
references and adr, are all of the form (s->pc & ~3) and do not come through
load_reg_var().  Those will be unified by add_reg_for_lit() in the next patch.


r~