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

[Sergey Fedorov <serge.fdrv@gmail.com>]

On 09.10.2015 17:00, Peter Maydell wrote:
> On 9 October 2015 at 14:53, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 08.10.2015 21:40, Peter Maydell wrote:
>>> On 28 September 2015 at 11:07, Sergey Fedorov <serge.fdrv@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.fdrv@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.
>> 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.
>

I think we could try figuring out the actual instruction length in case
of Thumb instruction, i.e. to do partial instruction decoding.