m68k: Fix regression causing Single-Step via GDB/RSP to not single step

Commit Message

Laurent Vivier Jan. 15, 2020, 11:03 a.m. UTC

A regression that was introduced, with the refactor to TranslatorOps,
drops two lines that update the PC when single-stepping is being performed.

Fixes: 11ab74b01e0a ("target/m68k: Convert to TranslatorOps")
Reported-by: Lucien Murray-Pitts <[e-mail]>
Suggested-by: Lucien Murray-Pitts <[e-mail]>
Suggested-by: Richard Henderson <[e-mail]>
Signed-off-by: Laurent Vivier <[e-mail]>
---

Notes:
    v2: update patch from Lucien with changes from Richard
        update subject to prefix it with "m68k:"
        rebase

 target/m68k/translate.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

Comments

Richard Henderson Jan. 15, 2020, 9:54 p.m. UTC | #1

On 1/15/20 1:03 AM, Laurent Vivier wrote:
> +        if (dc->base.singlestep_enabled) {
> +            tcg_gen_movi_i32(QREG_PC, dc->pc);
> +            gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));

This leaks the temporary, and so not quite ideal.  It would be of more concern
if the rest of the m68k port was audited, so that you could turn on leak detection.

I would suggest routing all calls to gen_helper_raise_exception through a
function (gen_raise_exception, usually), which takes an int argument, and does
all of the TCGv_i32 management internally.

But otherwise,
Reviewed-by: Richard Henderson <[e-mail]>


r~

Patch

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index fcdb7bc8e4..a400c2295f 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -6198,29 +6198,36 @@  static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
 
-    if (dc->base.is_jmp == DISAS_NORETURN) {
-        return;
-    }
-    if (dc->base.singlestep_enabled) {
-        gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
-        return;
-    }
-
     switch (dc->base.is_jmp) {
+    case DISAS_NORETURN:
+        break;
     case DISAS_TOO_MANY:
         update_cc_op(dc);
-        gen_jmp_tb(dc, 0, dc->pc);
+        if (dc->base.singlestep_enabled) {
+            tcg_gen_movi_i32(QREG_PC, dc->pc);
+            gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
+        } else {
+            gen_jmp_tb(dc, 0, dc->pc);
+        }
         break;
     case DISAS_JUMP:
         /* We updated CC_OP and PC in gen_jmp/gen_jmp_im.  */
-        tcg_gen_lookup_and_goto_ptr();
+        if (dc->base.singlestep_enabled) {
+            gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
+        } else {
+            tcg_gen_lookup_and_goto_ptr();
+        }
         break;
     case DISAS_EXIT:
         /*
          * We updated CC_OP and PC in gen_exit_tb, but also modified
          * other state that may require returning to the main loop.
          */
-        tcg_gen_exit_tb(NULL, 0);
+        if (dc->base.singlestep_enabled) {
+            gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
+        } else {
+            tcg_gen_exit_tb(NULL, 0);
+        }
         break;
     default:
         g_assert_not_reached();