Reviewed-by: Michael Rolnik <mrol...@gmail.com>
Tested-by: Michael Rolnik <mrol...@gmail.com>
On Wed, Jul 21, 2021 at 9:00 PM Philippe Mathieu-Daudé <f4...@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.hender...@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é <f4...@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
