qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Michael Rolnik
Subject: Re: [PATCH for-6.2 03/23] target/avr: Drop checks for singlestep_enabled
Date: Thu, 22 Jul 2021 14:18:29 +0300

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]