Regression for m68k causing Single-Step via GDB/RSP to not single step

[Qemu-devel] [PATCH] Regression for m68k causing Single-Step via GDB/RSP to not single step

Posted by Lucien Murray-Pitts via Qemu-devel 5 years, 5 months ago

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

This patch resolves that issue.

Signed-off-by: Lucien Murray-Pitts <lucienmp_antispam@yahoo.com>
---
 target/m68k/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index f0534a4ba0..2922ea79c3 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -6130,6 +6130,8 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
         return;
     }
     if (dc->base.singlestep_enabled) {
+        update_cc_op(dc);
+        tcg_gen_movi_i32(QREG_PC, dc->pc);
         gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
         return;
     }
-- 
2.21.0

Re: [Qemu-devel] [PATCH] Regression for m68k causing Single-Step via GDB/RSP to not single step

Posted by no-reply@patchew.org 5 years, 5 months ago

Patchew URL: https://patchew.org/QEMU/20190526075056.33865-1-lucienmp_antispam@yahoo.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190526075056.33865-1-lucienmp_antispam@yahoo.com
Type: series
Subject: [Qemu-devel] [PATCH] Regression for m68k causing Single-Step via GDB/RSP to not single step

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190526075056.33865-1-lucienmp_antispam@yahoo.com -> patchew/20190526075056.33865-1-lucienmp_antispam@yahoo.com
Switched to a new branch 'test'
eab81805cc Regression for m68k causing Single-Step via GDB/RSP to not single step

=== OUTPUT BEGIN ===
ERROR: Author email address is mangled by the mailing list
#2: 
Author: Lucien Murray-Pitts via Qemu-devel <qemu-devel@nongnu.org>

total: 1 errors, 0 warnings, 8 lines checked

Commit eab81805ccc1 (Regression for m68k causing Single-Step via GDB/RSP to not single step) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190526075056.33865-1-lucienmp_antispam@yahoo.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] Regression for m68k causing Single-Step via GDB/RSP to not single step

Posted by Laurent Vivier 5 years, 5 months ago

On 26/05/2019 09:50, Lucien Murray-Pitts wrote:
> A regression that was introduced, with the refactor to TranslatorOps,
> drops two lines that update the PC when single-stepping is being performed.
> ( short commit 11ab74b )
> 
> This patch resolves that issue.
> 
> Signed-off-by: Lucien Murray-Pitts <lucienmp_antispam@yahoo.com>
> ---
>   target/m68k/translate.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index f0534a4ba0..2922ea79c3 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -6130,6 +6130,8 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>           return;
>       }
>       if (dc->base.singlestep_enabled) {
> +        update_cc_op(dc);
> +        tcg_gen_movi_i32(QREG_PC, dc->pc);
>           gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
>           return;
>       }
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH] Regression for m68k causing Single-Step via GDB/RSP to not single step

Posted by Laurent Vivier 5 years, 4 months ago

Le 26/05/2019 à 09:50, Lucien Murray-Pitts a écrit :
> A regression that was introduced, with the refactor to TranslatorOps,
> drops two lines that update the PC when single-stepping is being performed.
> ( short commit 11ab74b )
> 
> This patch resolves that issue.

Fixes: 11ab74b01e0a ("target/m68k: Convert to TranslatorOps")

> Signed-off-by: Lucien Murray-Pitts <lucienmp_antispam@yahoo.com>
> ---
>  target/m68k/translate.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index f0534a4ba0..2922ea79c3 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -6130,6 +6130,8 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>          return;
>      }
>      if (dc->base.singlestep_enabled) {
> +        update_cc_op(dc);
> +        tcg_gen_movi_i32(QREG_PC, dc->pc);
>          gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
>          return;
>      }
> 

I've tested this fix single-stepping on a kernel, these two lines are 
not enough to fix the problem. In fact four lines have been dropped and 
we must re-add them all:

iff --git a/target/m68k/translate.c b/target/m68k/translate.c
index d0f6d1f5cc..6c78001501 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -6200,6 +6200,10 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
         return;
     }
     if (dc->base.singlestep_enabled) {
+        if (dc->base.is_jmp != DISAS_JUMP) {
+            update_cc_op(dc);
+            tcg_gen_movi_i32(QREG_PC, dc->pc);
+        }
         gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
         return;
     }

The problem happens when we single-step over an "rts" instruction, 
instead of returning to the caller the PC points to the following 
instruction (PC + 2):

0x00002e26 in arch_cpu_idle ()
1: x/i $pc  0x2e26 <arch_cpu_idle+4>:	rts
(gdb) si
0x00002e28 in machine_restart ()
1: x/i $pc  0x2e28 <machine_restart>:	moveal 0x438ae4 <mach_reset>,%a0

The good instruction stream is:

0x00002e26 in arch_cpu_idle ()
1: x/i $pc  0x2e26 <arch_cpu_idle+4>:	rts
(gdb) si
0x0002be6a in do_idle ()
1: x/i $pc  0x2be6a <do_idle+114>:	movew %sr,%d0

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] Regression for m68k causing Single-Step via GDB/RSP to not single step

Posted by Richard Henderson 5 years, 4 months ago

On 6/18/19 11:44 AM, Laurent Vivier wrote:
> Le 26/05/2019 à 09:50, Lucien Murray-Pitts a écrit :
>> A regression that was introduced, with the refactor to TranslatorOps,
>> drops two lines that update the PC when single-stepping is being performed.
>> ( short commit 11ab74b )
>>
>> This patch resolves that issue.
> 
> Fixes: 11ab74b01e0a ("target/m68k: Convert to TranslatorOps")
> 
>> Signed-off-by: Lucien Murray-Pitts <lucienmp_antispam@yahoo.com>
>> ---
>>  target/m68k/translate.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>> index f0534a4ba0..2922ea79c3 100644
>> --- a/target/m68k/translate.c
>> +++ b/target/m68k/translate.c
>> @@ -6130,6 +6130,8 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>>          return;
>>      }
>>      if (dc->base.singlestep_enabled) {
>> +        update_cc_op(dc);
>> +        tcg_gen_movi_i32(QREG_PC, dc->pc);
>>          gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
>>          return;
>>      }
>>
> 
> I've tested this fix single-stepping on a kernel, these two lines are 
> not enough to fix the problem. In fact four lines have been dropped and 
> we must re-add them all:
> 
> iff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index d0f6d1f5cc..6c78001501 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -6200,6 +6200,10 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>          return;
>      }
>      if (dc->base.singlestep_enabled) {
> +        if (dc->base.is_jmp != DISAS_JUMP) {
> +            update_cc_op(dc);
> +            tcg_gen_movi_i32(QREG_PC, dc->pc);
> +        }
>          gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
>          return;
>      }

Even this isn't quite right, according to the comments in the switch that
follows.  I think it'd be best written like so.


r~


diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 2ae537461f..b61c7ea0f1 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -6124,27 +6124,34 @@ 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();

Re: [Qemu-devel] [PATCH] Regression for m68k causing Single-Step via GDB/RSP to not single step

Posted by Laurent Vivier 5 years, 4 months ago

Le 18/06/2019 à 21:39, Richard Henderson a écrit :
> On 6/18/19 11:44 AM, Laurent Vivier wrote:
>> Le 26/05/2019 à 09:50, Lucien Murray-Pitts a écrit :
>>> A regression that was introduced, with the refactor to TranslatorOps,
>>> drops two lines that update the PC when single-stepping is being performed.
>>> ( short commit 11ab74b )
>>>
>>> This patch resolves that issue.
>>
>> Fixes: 11ab74b01e0a ("target/m68k: Convert to TranslatorOps")
>>
>>> Signed-off-by: Lucien Murray-Pitts <lucienmp_antispam@yahoo.com>
>>> ---
>>>  target/m68k/translate.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>> index f0534a4ba0..2922ea79c3 100644
>>> --- a/target/m68k/translate.c
>>> +++ b/target/m68k/translate.c
>>> @@ -6130,6 +6130,8 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>>>          return;
>>>      }
>>>      if (dc->base.singlestep_enabled) {
>>> +        update_cc_op(dc);
>>> +        tcg_gen_movi_i32(QREG_PC, dc->pc);
>>>          gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
>>>          return;
>>>      }
>>>
>>
>> I've tested this fix single-stepping on a kernel, these two lines are 
>> not enough to fix the problem. In fact four lines have been dropped and 
>> we must re-add them all:
>>
>> iff --git a/target/m68k/translate.c b/target/m68k/translate.c
>> index d0f6d1f5cc..6c78001501 100644
>> --- a/target/m68k/translate.c
>> +++ b/target/m68k/translate.c
>> @@ -6200,6 +6200,10 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>>          return;
>>      }
>>      if (dc->base.singlestep_enabled) {
>> +        if (dc->base.is_jmp != DISAS_JUMP) {
>> +            update_cc_op(dc);
>> +            tcg_gen_movi_i32(QREG_PC, dc->pc);
>> +        }
>>          gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
>>          return;
>>      }
> 
> Even this isn't quite right, according to the comments in the switch that
> follows.  I think it'd be best written like so.
> 
> 
> r~
> 
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index 2ae537461f..b61c7ea0f1 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -6124,27 +6124,34 @@ 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();
> 

Yes, it works too.

Could you formally send a patch?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] Regression for m68k causing Single-Step via GDB/RSP to not single step

Posted by Lucien Anti-Spam via Qemu-devel 5 years, 4 months ago

 Hi Richard/Laurent,
Great catch, I also just stumbled on this problem as well which I didnt see with my other application code.
But I have another problem after applying the changes from your email, "RTE" and breakpoints around a MOV/BusException/RTE behave oddly.
I would like to test with the same software you are using, could you tell me what M68K machine setup, and images you use as well as your debugger please?
Cheers,Luc
    On Wednesday, June 19, 2019, 04:59:12 AM GMT+9, Laurent Vivier <laurent@vivier.eu> wrote:  
 
 Le 18/06/2019 à 21:39, Richard Henderson a écrit :
> On 6/18/19 11:44 AM, Laurent Vivier wrote:
>> Le 26/05/2019 à 09:50, Lucien Murray-Pitts a écrit :
>>> A regression that was introduced, with the refactor to TranslatorOps,
>>> drops two lines that update the PC when single-stepping is being performed.
>>> ( short commit 11ab74b )
>>>
>>> This patch resolves that issue.
>>
>> Fixes: 11ab74b01e0a ("target/m68k: Convert to TranslatorOps")
>>
>>> Signed-off-by: Lucien Murray-Pitts <lucienmp_antispam@yahoo.com>
>>> ---
>>>  target/m68k/translate.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>> index f0534a4ba0..2922ea79c3 100644
>>> --- a/target/m68k/translate.c
>>> +++ b/target/m68k/translate.c
>>> @@ -6130,6 +6130,8 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>>>          return;
>>>      }
>>>      if (dc->base.singlestep_enabled) {
>>> +        update_cc_op(dc);
>>> +        tcg_gen_movi_i32(QREG_PC, dc->pc);
>>>          gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
>>>          return;
>>>      }
>>>
>>
>> I've tested this fix single-stepping on a kernel, these two lines are 
>> not enough to fix the problem. In fact four lines have been dropped and 
>> we must re-add them all:
>>
>> iff --git a/target/m68k/translate.c b/target/m68k/translate.c
>> index d0f6d1f5cc..6c78001501 100644
>> --- a/target/m68k/translate.c
>> +++ b/target/m68k/translate.c
>> @@ -6200,6 +6200,10 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase, CPUState *cpu)
>>          return;
>>      }
>>      if (dc->base.singlestep_enabled) {
>> +        if (dc->base.is_jmp != DISAS_JUMP) {
>> +            update_cc_op(dc);
>> +            tcg_gen_movi_i32(QREG_PC, dc->pc);
>> +        }
>>          gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
>>          return;
>>      }
> 
> Even this isn't quite right, according to the comments in the switch that
> follows.  I think it'd be best written like so.
> 
> 
> r~
> 
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index 2ae537461f..b61c7ea0f1 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -6124,27 +6124,34 @@ 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();
> 

Yes, it works too.

Could you formally send a patch?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] Regression for m68k causing Single-Step via GDB/RSP to not single step

Posted by Laurent Vivier 5 years, 4 months ago

Le 26/06/2019 à 13:24, Lucien Anti-Spam a écrit :
> Hi Richard/Laurent,
> 
> Great catch, I also just stumbled on this problem as well which I didnt
> see with my other application code.
> 
> But I have another problem after applying the changes from your email,
> "RTE" and breakpoints around a MOV/BusException/RTE behave oddly.
> 
> I would like to test with the same software you are using, could you
> tell me what M68K machine setup, and images you use as well as your
> debugger please?

Hi,

I use the Q800 machine from the series:

https://patchew.org/QEMU/20190619221933.1981-1-laurent@vivier.eu/

and a kernel I cross-build from the git repo.

The debugger is the one from a debian/etch-m68k installation (a real
m68k machine or a container with qemu-m68k and configured with
binfmt_misc). I use it remotely (with command "target remote
localhost:1234" and I start qemu with "-s") . More recent gdb (native or
cross built) have a bug: they don't manage correctly the size of FP
registers (96bit), they use 64bit FP registers that is only valid with
coldfire

I pass the kernel and cmdline to qemu with "-kernel" and "-append"
parameters.

Thanks,
Laurent

> Cheers,
> Luc
> 
> On Wednesday, June 19, 2019, 04:59:12 AM GMT+9, Laurent Vivier
> <laurent@vivier.eu> wrote:
> 
> 
> Le 18/06/2019 à 21:39, Richard Henderson a écrit :
>> On 6/18/19 11:44 AM, Laurent Vivier wrote:
>>> Le 26/05/2019 à 09:50, Lucien Murray-Pitts a écrit :
>>>> A regression that was introduced, with the refactor to TranslatorOps,
>>>> drops two lines that update the PC when single-stepping is being
> performed.
>>>> ( short commit 11ab74b )
>>>>
>>>> This patch resolves that issue.
>>>
>>> Fixes: 11ab74b01e0a ("target/m68k: Convert to TranslatorOps")
>>>
>>>> Signed-off-by: Lucien Murray-Pitts <lucienmp_antispam@yahoo.com
> <mailto:lucienmp_antispam@yahoo.com>>
>>>> ---
>>>>  target/m68k/translate.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>>> index f0534a4ba0..2922ea79c3 100644
>>>> --- a/target/m68k/translate.c
>>>> +++ b/target/m68k/translate.c
>>>> @@ -6130,6 +6130,8 @@ static void m68k_tr_tb_stop(DisasContextBase
> *dcbase, CPUState *cpu)
>>>>          return;
>>>>      }
>>>>      if (dc->base.singlestep_enabled) {
>>>> +        update_cc_op(dc);
>>>> +        tcg_gen_movi_i32(QREG_PC, dc->pc);
>>>>          gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
>>>>          return;
>>>>      }
>>>>
>>>
>>> I've tested this fix single-stepping on a kernel, these two lines are
>>> not enough to fix the problem. In fact four lines have been dropped and
>>> we must re-add them all:
>>>
>>> iff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>> index d0f6d1f5cc..6c78001501 100644
>>> --- a/target/m68k/translate.c
>>> +++ b/target/m68k/translate.c
>>> @@ -6200,6 +6200,10 @@ static void m68k_tr_tb_stop(DisasContextBase
> *dcbase, CPUState *cpu)
>>>          return;
>>>      }
>>>      if (dc->base.singlestep_enabled) {
>>> +        if (dc->base.is_jmp != DISAS_JUMP) {
>>> +            update_cc_op(dc);
>>> +            tcg_gen_movi_i32(QREG_PC, dc->pc);
>>> +        }
>>>          gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
>>>          return;
>>>      }
>>
>> Even this isn't quite right, according to the comments in the switch that
>> follows.  I think it'd be best written like so.
>>
>>
>> r~
>>
>>
>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>> index 2ae537461f..b61c7ea0f1 100644
>> --- a/target/m68k/translate.c
>> +++ b/target/m68k/translate.c
>> @@ -6124,27 +6124,34 @@ 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();
>>
> 
> Yes, it works too.
> 
> Could you formally send a patch?
> 
> Thanks,
> 
> Laurent