Re: [Qemu-devel] [PATCH v2 04/22] target-*: Introduce and use cpu_breakpoint_test

[Richard Henderson <rth@twiddle.net>]

On 09/18/2015 03:32 AM, Peter Maydell wrote:
>> +/* Return true if PC matches an installed breakpoint.  */
>> +static inline bool cpu_breakpoint_test(CPUState *cpu, vaddr pc, int mask)
>> +{
>> +    CPUBreakpoint *bp;
>> +
>> +    if (unlikely(!QTAILQ_EMPTY(&cpu->breakpoints))) {
>> +        QTAILQ_FOREACH(bp, &cpu->breakpoints, entry) {
>> +            if (bp->pc == pc && (bp->flags & mask)) {
>> +                return true;
>> +            }
>> +        }
>> +    }
>> +    return false;
>> +}
> 
> This won't work with the fix for ARM breakpoints Sergey currently has
> on list: http://patchwork.ozlabs.org/patch/517359/
> where we need to behave differently for "there's a GDB breakpoint
> here" and "there's a CPU breakpoint here" (because the complex
> conditions on the latter require us to call a helper function to
> see if we need to actually generate an EXCP_DEBUG exception).

Hmm.  Ok, well, I suppose it might do for all but one target then...

>> +        if (unlikely(cpu_breakpoint_test(cs, dc->pc, BP_ANY))) {
>> +            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 (num_insns == max_insns && (tb->cflags & CF_LAST_IO)) {
>>              gen_io_start();
>>          }
> 
> Do you know why some but not all targets do this "advance PC"
> thing if there's a breakpoint?

No.  I don't believe I've seen it before yesterday.

My suspicion is that if we have a TB that would span two pages, and the
breakpoint is exactly at the page boundary, then we must advance the pc like
this so that it's clear that the TB utilizes the second page.

If so, it means that there are some targets that are broken based on this (e.g.
i386), and there are a few for which this situation is impossible, and this
fixup is cargo culting (e.g. aarch64).


r~