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/[email protected]/ 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 > <[email protected]> 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 <[email protected] > <mailto:[email protected]>> >>>> --- >>>> 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
