Re: [Qemu-devel] [PATCH 66/67] target/arm: Move singlestep check from gen_jmp to gen_goto_tb

[Peter Maydell]

On Fri, 26 Jul 2019 at 18:51, Richard Henderson
<address@hidden> wrote:
>
> We miss quite a number of single-step events by having
> the check in the wrong place.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/translate.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index c2b8b86fd2..9ae9b23823 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -2740,7 +2740,10 @@ static void gen_goto_ptr(void)
>   */
>  static void gen_goto_tb(DisasContext *s, int n, target_ulong dest)
>  {
> -    if (use_goto_tb(s, dest)) {
> +    if (unlikely(is_singlestepping(s))) {
> +        gen_set_pc_im(s, dest);
> +        gen_singlestep_exception(s);
> +    } else if (use_goto_tb(s, dest)) {
>          tcg_gen_goto_tb(n);
>          gen_set_pc_im(s, dest);
>          tcg_gen_exit_tb(s->base.tb, n);
> @@ -2751,16 +2754,9 @@ static void gen_goto_tb(DisasContext *s, int n, 
> target_ulong dest)
>      s->base.is_jmp = DISAS_NORETURN;
>  }
>
> -static inline void gen_jmp (DisasContext *s, uint32_t dest)
> +static inline void gen_jmp(DisasContext *s, uint32_t dest)
>  {
> -    if (unlikely(is_singlestepping(s))) {
> -        /* An indirect jump so that we still trigger the debug exception.  */
> -        if (s->thumb)
> -            dest |= 1;
> -        gen_bx_im(s, dest);
> -    } else {
> -        gen_goto_tb(s, 0, dest);
> -    }
> +    gen_goto_tb(s, 0, dest);
>  }
>
>  static inline void gen_mulxy(TCGv_i32 t0, TCGv_i32 t1, int x, int y)

I haven't tested but I'm not sure this change is right.
The idea of the current code is that we handle generating
the actual singlestep exception in arm_tr_tb_stop() at the
end, rather than in the process of generating code for the
insn. This change means we now do a gen_singlestep_exception()
in the gen_jmp()/gen_goto_tb() part of the code, but we haven't
removed the handling of it in arm_tr_tb_stop(), so we're now
doing this in two places. Why doesn't the current design work?
And if we need to do something different for the route to
"change the PC via gen_jmp()" why don't we need to do it
also when we change the PC via eg gen_bx_im() ?

(Incidentally, the only places other than gen_jmp()
which call gen_goto_tb() are:
 * the end-of-TB handling of DISAS_NEXT/DISAS_TOO_MANY
 * four places for barrier insns where we use a
   gen_goto_tb to end the TB -- this isn't consistent
   with how we end the TB for other situations...)

thanks
-- PMM