Re: [PATCH v5 9/9] target/arm: Enable TARGET_TB_PCREL

[Richard Henderson]

On 10/4/22 12:27, Richard Henderson wrote:
> On 10/4/22 09:23, Peter Maydell wrote:
> > >   void arm_cpu_synchronize_from_tb(CPUState *cs,
> > >                                    const TranslationBlock *tb)
> > >   {
> > > -    ARMCPU *cpu = ARM_CPU(cs);
> > > -    CPUARMState *env = &cpu->env;
> > > -
> > > -    /*
> > > -     * It's OK to look at env for the current mode here, because it's
> > > -     * never possible for an AArch64 TB to chain to an AArch32 TB.
> > > -     */
> > > -    if (is_a64(env)) {
> > > -        env->pc = tb_pc(tb);
> > > -    } else {
> > > -        env->regs[15] = tb_pc(tb);
> > > +    /* The program counter is always up to date with TARGET_TB_PCREL. */
> >
> > I was confused for a bit about this, but it works because
> > although the synchronize_from_tb hook has a name that implies
> > it's comparatively general purpose, in fact we use it only
> > in the special case of "we abandoned execution at the start of
> > this TB without executing any of it".
>
> Correct.
>
> > > @@ -347,16 +354,22 @@ static void gen_exception_internal(int excp)
> > >
> > >   static void gen_exception_internal_insn(DisasContext *s, int excp)
> > >   {
> > > +    target_ulong pc_save = s->pc_save;
> > > +
> > >       gen_a64_update_pc(s, 0);
> > >       gen_exception_internal(excp);
> > >       s->base.is_jmp = DISAS_NORETURN;
> > > +    s->pc_save = pc_save;
> >
> > What is trashing s->pc_save that we have to work around like this,
> > here and in the other similar changes ?
>
> gen_a64_update_pc trashes pc_save.
>
> Off of the top of my head, I can't remember what conditionally uses exceptions (single 
> step?).

Oh, duh, any conditional a32 instruction.

To some extent this instance duplicates s->pc_cond_save, but the usage pattern 
there is

    brcond(..., s->condlabel);
    s->pc_cond_save = s->pc_save;

    gen_update_pc(s, 0);  /* pc_save = pc_curr */
    raise_exception;

    if (s->pc_cond_save != s->pc_save) {
        gen_update_pc(s->pc_save - s->pc_cond_save);
    }
    /* s->pc_save now matches the state at brcond */

condlabel:


So, we have exited the TB via exception, and the second gen_update_pc would be deleted as 
dead code, it's just as easy to keep s->pc_save unchanged so that the second gen_update_pc 
is not emitted.  We certainly *must* update s->pc_save around indirect branches, so that 
we don't wind up with an assert on s->pc_save != -1.


r~