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

[Peter Maydell]

On Wed, 7 Aug 2019 at 05:53, Richard Henderson
<address@hidden> wrote:
>
> We currently have 3 different ways of computing the architectural
> value of "PC" as seen in the ARM ARM.
>
> The value of s->pc has been incremented past the current insn,
> but that is all.  Thus for a32, PC = s->pc + 4; for t32, PC = s->pc;
> for t16, PC = s->pc + 2.  These differing computations make it
> impossible at present to unify the various code paths.
>
> With the newly introduced s->pc_curr, we can compute the correct
> value for all cases, using the formula given in the ARM ARM.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/translate.c | 59 ++++++++++++++++--------------------------
>  1 file changed, 23 insertions(+), 36 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 59e35aafbf..61933865d5 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -196,17 +196,17 @@ static inline void store_cpu_offset(TCGv_i32 var, int 
> offset)
>  #define store_cpu_field(var, name) \
>      store_cpu_offset(var, offsetof(CPUARMState, name))
>
> +/* 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 ?

thanks
-- PMM