On Mon, 12 Jul 2021 at 16:48, Richard Henderson
<richard.hender...@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.hender...@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.mayd...@linaro.org>
thanks
-- PMM
