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

[Lucien Anti-Spam]

 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 
<address@hidden> 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 <address@hidden>
>>> ---
>>>  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