Re: [PATCH v2 08/10] accel/tcg: Adjust interface of TranslatorOps.breakpoint_check

[Richard Henderson]

On 7/17/21 10:52 AM, Peter Maydell wrote:
> On Mon, 12 Jul 2021 at 16:48, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > We don't need the whole CPUBreakpoint structure in the check,
> > only the flags.  Return the instruction length to consolidate
> > the adjustment of db->pc_next.
> >
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
>
> > diff --git a/target/avr/translate.c b/target/avr/translate.c
> > index 8237a03c23..73ff467926 100644
> > --- a/target/avr/translate.c
> > +++ b/target/avr/translate.c
> > @@ -2944,13 +2944,13 @@ static void avr_tr_insn_start(DisasContextBase *dcbase, 
> > CPUState *cs)
> >       tcg_gen_insn_start(ctx->npc);
> >   }
> >
> > -static bool avr_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
> > -                                    const CPUBreakpoint *bp)
> > +static int avr_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
> > +                                   int bp_flags)
> >   {
> >       DisasContext *ctx = container_of(dcbase, DisasContext, base);
> >
> >       gen_breakpoint(ctx);
> > -    return true;
> > +    return 2; /* minimum instruction length */
>
> Here we weren't advancing pc_next at all, and now we do.
>
> > diff --git a/target/mips/tcg/translate.c b/target/mips/tcg/translate.c
> > index 47c967acbf..c7b9d813c2 100644
> > --- a/target/mips/tcg/translate.c
> > +++ b/target/mips/tcg/translate.c
> > @@ -16190,22 +16190,16 @@ static void mips_tr_insn_start(DisasContextBase 
> > *dcbase, CPUState *cs)
> >                          ctx->btarget);
> >   }
> >
> > -static bool mips_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
> > -                                     const CPUBreakpoint *bp)
> > +static int mips_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
> > +                                    int bp_flags)
> >   {
> >       DisasContext *ctx = container_of(dcbase, DisasContext, base);
> >
> >       save_cpu_state(ctx, 1);
> >       ctx->base.is_jmp = DISAS_NORETURN;
> >       gen_helper_raise_exception_debug(cpu_env);
> > -    /*
> > -     * The address covered by the breakpoint must be included in
> > -     * [tb->pc, tb->pc + tb->size) in order to for it to be
> > -     * properly cleared -- thus we increment the PC here so that
> > -     * the logic setting tb->size below does the right thing.
> > -     */
> > -    ctx->base.pc_next += 4;
> > -    return true;
> > +
> > +    return 2; /* minimum instruction length */
> >   }
>
> Here we were advancing by 4 and now advance by 2.
>
> > diff --git a/target/riscv/translate.c b/target/riscv/translate.c
> > index deda0c8a44..8a6bc58572 100644
> > --- a/target/riscv/translate.c
> > +++ b/target/riscv/translate.c
> > @@ -961,20 +961,15 @@ static void riscv_tr_insn_start(DisasContextBase *dcbase, 
> > CPUState *cpu)
> >       tcg_gen_insn_start(ctx->base.pc_next);
> >   }
> >
> > -static bool riscv_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
> > -                                      const CPUBreakpoint *bp)
> > +static int riscv_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
> > +                                     int bp_flags)
> >   {
> >       DisasContext *ctx = container_of(dcbase, DisasContext, base);
> >
> >       tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next);
> >       ctx->base.is_jmp = DISAS_NORETURN;
> >       gen_exception_debug();
> > -    /* The address covered by the breakpoint must be included in
> > -       [tb->pc, tb->pc + tb->size) in order to for it to be
> > -       properly cleared -- thus we increment the PC here so that
> > -       the logic setting tb->size below does the right thing.  */
> > -    ctx->base.pc_next += 4;
> > -    return true;
> > +    return 2; /* minimum instruction length */
> >   }
>
> Ditto.
>
> If these are intentional changes (are they bugfixes?) they should be in a
> separate patch.

Yes, they are bug fixes.

r~

> Otherwise
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> thanks
> -- PMM