Re: [PATCH for-6.2 03/23] target/avr: Drop checks for singlestep_enabled

From: Michael Rolnik <mrolnik@gmail.com>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: "Alex Bennée" <alex.bennee@linaro.org>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Pavel Dovgalyuk" <pavel.dovgaluk@ispras.ru>
Subject: Re: [PATCH for-6.2 03/23] target/avr: Drop checks for singlestep_enabled
Date: Thu, 22 Jul 2021 14:18:29 +0300	[thread overview]
Message-ID: <CAK4993gxuaRGo7StD1YNq3=5kmPV552mi4dZnV6qy3eSNT=quA@mail.gmail.com> (raw)
In-Reply-To: <f54d0645-5dad-4f7e-e804-9b926524ffa1@amsat.org>

[-- Attachment #1: Type: text/plain, Size: 3339 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 4445 bytes --]