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

[Peter Maydell]

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.

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

thanks
-- PMM