On 28 September 2015 at 11:07, Sergey Fedorov <serge.f...@gmail.com> 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 <serge.f...@gmail.com>
> 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