Re: [Qemu-devel] [PATCH 06/67] target/arm: Introduce pc_read

[Peter Maydell]

On Tue, 30 Jul 2019 at 01:39, Richard Henderson
<address@hidden> wrote:
>
> On 7/29/19 7:05 AM, Peter Maydell wrote:
> > On Fri, 26 Jul 2019 at 18:50, 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.
> >>
> >> Let s->pc_read hold the architectural value of PC for all cases.
> >>
> >> Signed-off-by: Richard Henderson <address@hidden>
> >> ---
> >>  target/arm/translate.h | 10 ++++++++
> >>  target/arm/translate.c | 53 ++++++++++++++++++------------------------
> >>  2 files changed, 32 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/target/arm/translate.h b/target/arm/translate.h
> >> index a20f6e2056..2dfdd8ca66 100644
> >> --- a/target/arm/translate.h
> >> +++ b/target/arm/translate.h
> >> @@ -9,7 +9,17 @@ typedef struct DisasContext {
> >>      DisasContextBase base;
> >>      const ARMISARegisters *isar;
> >>
> >> +    /*
> >> +     * Summary of the various values for "PC":
> >> +     * base.pc_next -- the start of the current insn
> >> +     * pc           -- the start of the next insn
> >
> > These are confusingly named -- logically "pc_next" ought to
> > be the PC of the next instruction and "pc" ought to be
> > that of the current one...
>
> Yes, well.  I don't quite remember why "pc_next" was chosen for this field.  
> It
> is the "next" upon entry to tr_foo_disas_insn().  Often the target will
> increment s->base.pc_next immediately, so it will also be the "next" insn
> throughout translation.  Though that isn't currently the case for ARM.
>
> Once most of the uses of s->pc get moved to s->pc_read, it might be reasonable
> to rename the remaining "s->base.pc_next" -> "s->pc_orig" and "s->pc" ->
> "s->base.pc_next".  Perhaps that would be clearer, I'm not sure.

Renaming pc_next would be a cross-target change, so let's put that
on the shelf for the moment. Maybe just put a TODO comment to the
effect that we could consider renaming in future ?

Or we could just copy s->base.pc_next into s->some_field_we_choose_the_name_of,
eating 8 unnecessary bytes in the DisasContext struct but giving us a more
uniform s->something for all of the different PC variants rather than
having one of them be in s->base.something.

thanks
-- PMM