| Message ID | 20210721064155.645508-4-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tcg: gdb singlestep reorg | expand |
+Michael/Alex/Pavel On 7/21/21 8:41 AM, Richard Henderson wrote: > GDB single-stepping is now handled generically. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/avr/translate.c | 19 ++++--------------- > 1 file changed, 4 insertions(+), 15 deletions(-) > > diff --git a/target/avr/translate.c b/target/avr/translate.c > index 1111e08b83..0403470dd8 100644 > --- a/target/avr/translate.c > +++ b/target/avr/translate.c > @@ -1089,11 +1089,7 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest) > tcg_gen_exit_tb(tb, n); > } else { > tcg_gen_movi_i32(cpu_pc, dest); > - if (ctx->base.singlestep_enabled) { > - gen_helper_debug(cpu_env); > - } else { > - tcg_gen_lookup_and_goto_ptr(); > - } > + tcg_gen_lookup_and_goto_ptr(); > } > ctx->base.is_jmp = DISAS_NORETURN; > } > @@ -3011,17 +3007,10 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs) > tcg_gen_movi_tl(cpu_pc, ctx->npc); > /* fall through */ > case DISAS_LOOKUP: > - if (!ctx->base.singlestep_enabled) { > - tcg_gen_lookup_and_goto_ptr(); > - break; > - } > - /* fall through */ > + tcg_gen_lookup_and_goto_ptr(); > + break; > case DISAS_EXIT: > - if (ctx->base.singlestep_enabled) { > - gen_helper_debug(cpu_env); > - } else { > - tcg_gen_exit_tb(NULL, 0); > - } > + tcg_gen_exit_tb(NULL, 0); > break; > default: > g_assert_not_reached(); > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Not related to this patch, but looking at the last gen_helper_debug() use: /* * The BREAK instruction is used by the On-chip Debug system, and is * normally not used in the application software. When the BREAK instruction is * executed, the AVR CPU is set in the Stopped Mode. This gives the On-chip * Debugger access to internal resources. If any Lock bits are set, or either * the JTAGEN or OCDEN Fuses are unprogrammed, the CPU will treat the BREAK * instruction as a NOP and will not enter the Stopped mode. This instruction * is not available in all devices. Refer to the device specific instruction * set summary. */ static bool trans_BREAK(DisasContext *ctx, arg_BREAK *a) { if (!avr_have_feature(ctx, AVR_FEATURE_BREAK)) { return true; } #ifdef BREAKPOINT_ON_BREAK tcg_gen_movi_tl(cpu_pc, ctx->npc - 1); gen_helper_debug(cpu_env); ctx->base.is_jmp = DISAS_EXIT; #else /* NOP */ #endif return true; } Shouldn't we have a generic 'bool gdbstub_is_attached()' in "exec/gdbstub.h", then use it in replay_gdb_attached() and trans_BREAK() instead of this BREAKPOINT_ON_BREAK build-time definitions?
Reviewed-by: Michael Rolnik <mrolnik@gmail.com> Tested-by: Michael Rolnik <mrolnik@gmail.com> On Wed, Jul 21, 2021 at 9:00 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > +Michael/Alex/Pavel > > On 7/21/21 8:41 AM, Richard Henderson wrote: > > GDB single-stepping is now handled generically. > > > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > > --- > > target/avr/translate.c | 19 ++++--------------- > > 1 file changed, 4 insertions(+), 15 deletions(-) > > > > diff --git a/target/avr/translate.c b/target/avr/translate.c > > index 1111e08b83..0403470dd8 100644 > > --- a/target/avr/translate.c > > +++ b/target/avr/translate.c > > @@ -1089,11 +1089,7 @@ static void gen_goto_tb(DisasContext *ctx, int n, > target_ulong dest) > > tcg_gen_exit_tb(tb, n); > > } else { > > tcg_gen_movi_i32(cpu_pc, dest); > > - if (ctx->base.singlestep_enabled) { > > - gen_helper_debug(cpu_env); > > - } else { > > - tcg_gen_lookup_and_goto_ptr(); > > - } > > + tcg_gen_lookup_and_goto_ptr(); > > } > > ctx->base.is_jmp = DISAS_NORETURN; > > } > > @@ -3011,17 +3007,10 @@ static void avr_tr_tb_stop(DisasContextBase > *dcbase, CPUState *cs) > > tcg_gen_movi_tl(cpu_pc, ctx->npc); > > /* fall through */ > > case DISAS_LOOKUP: > > - if (!ctx->base.singlestep_enabled) { > > - tcg_gen_lookup_and_goto_ptr(); > > - break; > > - } > > - /* fall through */ > > + tcg_gen_lookup_and_goto_ptr(); > > + break; > > case DISAS_EXIT: > > - if (ctx->base.singlestep_enabled) { > > - gen_helper_debug(cpu_env); > > - } else { > > - tcg_gen_exit_tb(NULL, 0); > > - } > > + tcg_gen_exit_tb(NULL, 0); > > break; > > default: > > g_assert_not_reached(); > > > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Not related to this patch, but looking at the last > gen_helper_debug() use: > > /* > * The BREAK instruction is used by the On-chip Debug system, and is > * normally not used in the application software. When the BREAK > instruction is > * executed, the AVR CPU is set in the Stopped Mode. This gives the > On-chip > * Debugger access to internal resources. If any Lock bits are set, or > either > * the JTAGEN or OCDEN Fuses are unprogrammed, the CPU will treat the > BREAK > * instruction as a NOP and will not enter the Stopped mode. This > instruction > * is not available in all devices. Refer to the device specific > instruction > * set summary. > */ > static bool trans_BREAK(DisasContext *ctx, arg_BREAK *a) > { > if (!avr_have_feature(ctx, AVR_FEATURE_BREAK)) { > return true; > } > > #ifdef BREAKPOINT_ON_BREAK > tcg_gen_movi_tl(cpu_pc, ctx->npc - 1); > gen_helper_debug(cpu_env); > ctx->base.is_jmp = DISAS_EXIT; > #else > /* NOP */ > #endif > > return true; > } > > Shouldn't we have a generic 'bool gdbstub_is_attached()' in > "exec/gdbstub.h", then use it in replay_gdb_attached() and > trans_BREAK() instead of this BREAKPOINT_ON_BREAK build-time > definitions? > -- Best Regards, Michael Rolnik <div dir="ltr">Reviewed-by: Michael Rolnik <<a href="mailto:mrolnik@gmail.com">mrolnik@gmail.com</a>><div>Tested-by: Michael Rolnik <<a href="mailto:mrolnik@gmail.com">mrolnik@gmail.com</a>></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 21, 2021 at 9:00 PM Philippe Mathieu-Daudé <<a href="mailto:f4bug@amsat.org">f4bug@amsat.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+Michael/Alex/Pavel<br> <br> On 7/21/21 8:41 AM, Richard Henderson wrote:<br> > GDB single-stepping is now handled generically.<br> > <br> > Signed-off-by: Richard Henderson <<a href="mailto:richard.henderson@linaro.org" target="_blank">richard.henderson@linaro.org</a>><br> > ---<br> > target/avr/translate.c | 19 ++++---------------<br> > 1 file changed, 4 insertions(+), 15 deletions(-)<br> > <br> > diff --git a/target/avr/translate.c b/target/avr/translate.c<br> > index 1111e08b83..0403470dd8 100644<br> > --- a/target/avr/translate.c<br> > +++ b/target/avr/translate.c<br> > @@ -1089,11 +1089,7 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest)<br> > tcg_gen_exit_tb(tb, n);<br> > } else {<br> > tcg_gen_movi_i32(cpu_pc, dest);<br> > - if (ctx->base.singlestep_enabled) {<br> > - gen_helper_debug(cpu_env);<br> > - } else {<br> > - tcg_gen_lookup_and_goto_ptr();<br> > - }<br> > + tcg_gen_lookup_and_goto_ptr();<br> > }<br> > ctx->base.is_jmp = DISAS_NORETURN;<br> > }<br> > @@ -3011,17 +3007,10 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs)<br> > tcg_gen_movi_tl(cpu_pc, ctx->npc);<br> > /* fall through */<br> > case DISAS_LOOKUP:<br> > - if (!ctx->base.singlestep_enabled) {<br> > - tcg_gen_lookup_and_goto_ptr();<br> > - break;<br> > - }<br> > - /* fall through */<br> > + tcg_gen_lookup_and_goto_ptr();<br> > + break;<br> > case DISAS_EXIT:<br> > - if (ctx->base.singlestep_enabled) {<br> > - gen_helper_debug(cpu_env);<br> > - } else {<br> > - tcg_gen_exit_tb(NULL, 0);<br> > - }<br> > + tcg_gen_exit_tb(NULL, 0);<br> > break;<br> > default:<br> > g_assert_not_reached();<br> > <br> <br> Reviewed-by: Philippe Mathieu-Daudé <<a href="mailto:f4bug@amsat.org" target="_blank">f4bug@amsat.org</a>><br> <br> Not related to this patch, but looking at the last<br> gen_helper_debug() use:<br> <br> /*<br> * The BREAK instruction is used by the On-chip Debug system, and is<br> * normally not used in the application software. When the BREAK<br> instruction is<br> * executed, the AVR CPU is set in the Stopped Mode. This gives the On-chip<br> * Debugger access to internal resources. If any Lock bits are set, or<br> either<br> * the JTAGEN or OCDEN Fuses are unprogrammed, the CPU will treat the BREAK<br> * instruction as a NOP and will not enter the Stopped mode. This<br> instruction<br> * is not available in all devices. Refer to the device specific<br> instruction<br> * set summary.<br> */<br> static bool trans_BREAK(DisasContext *ctx, arg_BREAK *a)<br> {<br> if (!avr_have_feature(ctx, AVR_FEATURE_BREAK)) {<br> return true;<br> }<br> <br> #ifdef BREAKPOINT_ON_BREAK<br> tcg_gen_movi_tl(cpu_pc, ctx->npc - 1);<br> gen_helper_debug(cpu_env);<br> ctx->base.is_jmp = DISAS_EXIT;<br> #else<br> /* NOP */<br> #endif<br> <br> return true;<br> }<br> <br> Shouldn't we have a generic 'bool gdbstub_is_attached()' in<br> "exec/gdbstub.h", then use it in replay_gdb_attached() and<br> trans_BREAK() instead of this BREAKPOINT_ON_BREAK build-time<br> definitions?<br> </blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature">Best Regards,<br>Michael Rolnik</div>
diff --git a/target/avr/translate.c b/target/avr/translate.c index 1111e08b83..0403470dd8 100644 --- a/target/avr/translate.c +++ b/target/avr/translate.c @@ -1089,11 +1089,7 @@ static void gen_goto_tb(DisasContext *ctx, int n, target_ulong dest) tcg_gen_exit_tb(tb, n); } else { tcg_gen_movi_i32(cpu_pc, dest); - if (ctx->base.singlestep_enabled) { - gen_helper_debug(cpu_env); - } else { - tcg_gen_lookup_and_goto_ptr(); - } + tcg_gen_lookup_and_goto_ptr(); } ctx->base.is_jmp = DISAS_NORETURN; } @@ -3011,17 +3007,10 @@ static void avr_tr_tb_stop(DisasContextBase *dcbase, CPUState *cs) tcg_gen_movi_tl(cpu_pc, ctx->npc); /* fall through */ case DISAS_LOOKUP: - if (!ctx->base.singlestep_enabled) { - tcg_gen_lookup_and_goto_ptr(); - break; - } - /* fall through */ + tcg_gen_lookup_and_goto_ptr(); + break; case DISAS_EXIT: - if (ctx->base.singlestep_enabled) { - gen_helper_debug(cpu_env); - } else { - tcg_gen_exit_tb(NULL, 0); - } + tcg_gen_exit_tb(NULL, 0); break; default: g_assert_not_reached();
GDB single-stepping is now handled generically. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/avr/translate.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) -- 2.25.1