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.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.

Yes, they are bug fixes.

r~

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

thanks
-- PMM