On Tue, 30 Jul 2019 at 01:39, Richard Henderson <richard.hender...@linaro.org> wrote: > > On 7/29/19 7:05 AM, Peter Maydell wrote: > > On Fri, 26 Jul 2019 at 18:50, Richard Henderson > > <richard.hender...@linaro.org> 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 <richard.hender...@linaro.org> > >> --- > >> 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