Re: [Qemu-devel] [PATCH v2 2/2] target-arm: Fix CPU breakpoint handling

[Peter Maydell]

On 28 September 2015 at 11:07, Sergey Fedorov <address@hidden> wrote:
> A QEMU breakpoint match is not definitely an architectural breakpoint
> match. If an exception is generated unconditionally during translation,
> it is hardly possible to ignore it in the debug exception handler.
>
> Generate a call to a helper to check CPU breakpoints and raise an
> exception only if any breakpoint matches architecturally.
>
> Signed-off-by: Sergey Fedorov <address@hidden>

> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index ec0936c..426229f 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -11082,11 +11082,14 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>          if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>              QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>                  if (bp->pc == dc->pc) {
> -                    gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> -                    /* Advance PC so that clearing the breakpoint will
> -                       invalidate this TB.  */
> -                    dc->pc += 2;
> -                    goto done_generating;
> +                    if (bp->flags & BP_CPU) {
> +                        gen_helper_check_breakpoints(cpu_env);
> +                    } else {
> +                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);

We shouldn't just continue here, because now we'll try to generate the
code for the instruction even in the "we know this will always be a bp"
case. Also, you've dropped the "advance PC" part which we need so this
TB is not zero length.

> +                    }
> +                    /* End the TB early; it's likely not going to be 
> executed */
> +                    dc->is_jmp = DISAS_UPDATE;

gen_exception_internal_insn sets is_jmp to DISAS_EXC, and then this
overrides it; so this line should go inside the "is this a BP_CPU bp?"
if clause. (I think the effect is just that we pointlessly generate some
unreachable code after the exception generating call.)

> +                    break;
>                  }
>              }
>          }
> @@ -11209,7 +11212,6 @@ void gen_intermediate_code_internal_a64(ARMCPU *cpu,
>          }
>      }
>
> -done_generating:
>      gen_tb_end(tb, num_insns);
>
>  #ifdef DEBUG_DISAS
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 84a21ac..405d6d0 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -11328,11 +11328,14 @@ static inline void 
> gen_intermediate_code_internal(ARMCPU *cpu,
>          if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>              QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
>                  if (bp->pc == dc->pc) {
> -                    gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> -                    /* Advance PC so that clearing the breakpoint will
> -                       invalidate this TB.  */
> -                    dc->pc += 2;
> -                    goto done_generating;
> +                    if (bp->flags & BP_CPU) {
> +                        gen_helper_check_breakpoints(cpu_env);
> +                    } else {
> +                        gen_exception_internal_insn(dc, 0, EXCP_DEBUG);
> +                    }
> +                    /* End the TB early; it's likely not going to be 
> executed */
> +                    dc->is_jmp = DISAS_UPDATE;

Similar comments here.

Annoying corner case which I don't think we need to handle necessarily:
if you set a breakpoint on a 32-bit Thumb instruction which spans a page
boundary, and the second page is not present, we will end up taking the
page fault when I think we should take the breakpoint. I can't think
of a way to get that right, so just commenting that it isn't handled
right would do.

> +                    break;
>                  }
>              }
>          }
> --
> 1.9.1
>

Otherwise I think this is the right approach.

thanks
-- PMM