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

[Peter Maydell]

On 9 October 2015 at 14:53, Sergey Fedorov <address@hidden> wrote:
> On 08.10.2015 21:40, Peter Maydell wrote:
>> 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.
>
> Actually, I was going to do the same way as some architectures (e.g.
> alpha) did: always translate one instruction so that TB size is
> determined by actual instruction decoding. At least, it makes sense for
> AArch32 where we can have 16/32-bit insns. If we advance PC incorrectly,
> we will get "Disassembler disagrees with translator over instruction
> decoding" warning messages when in_asm log enabled. I can rewrite it
> with PC advancement, but at least, I would like to change the
> advancement to 4 bytes for A64 translation.

Hmm, I see. I'm not sure it makes sense to do a bunch of extra
work at codegen just to avoid a debug message. It would be nicer
to suppress that warning some other way.

thanks
-- PMM