Re: [RFC PATCH] tcg/cpu-exec: precise single-stepping after an exception

[Luc Michel]

On 7/16/20 11:08 PM, Richard Henderson wrote:
> On 7/16/20 1:12 PM, Peter Maydell wrote:
>> On Thu, 16 Jul 2020 at 11:08, Luc Michel <luc.michel@greensocs.com> wrote:
>>>
>>> When single-stepping with a debugger attached to QEMU, and when an
>>> exception is raised, the debugger misses the first instruction after the
>>> exception:
>>
>> This is a long-standing bug; thanks for looking at it.
>> (https://bugs.launchpad.net/qemu/+bug/757702)
>>
>>
>>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>>> index d95c4848a4..e85fab5d40 100644
>>> --- a/accel/tcg/cpu-exec.c
>>> +++ b/accel/tcg/cpu-exec.c
>>> @@ -502,10 +502,21 @@ static inline bool cpu_handle_exception(CPUState 
>>> *cpu, int *ret)
>>>              CPUClass *cc = CPU_GET_CLASS(cpu);
>>>              qemu_mutex_lock_iothread();
>>>              cc->do_interrupt(cpu);
>>>              qemu_mutex_unlock_iothread();
>>>              cpu->exception_index = -1;
>>> +
>>> +            if (unlikely(cpu->singlestep_enabled)) {
>>> +                /*
>>> +                 * After processing the exception, ensure an EXCP_DEBUG is
>>> +                 * raised when single-stepping so that GDB doesn't miss the
>>> +                 * next instruction.
>>> +                 */
>>> +                cpu->exception_index = EXCP_DEBUG;
>>> +                return cpu_handle_exception(cpu, ret);
>>> +            }
>>
>> I like the idea of being able to do this generically in
>> the main loop.
>>
>> How about interrupts? If we are single-stepping and we
>> take an interrupt I guess we want to stop before the first
>> insn of the interrupt handler rather than after it, which
>> would imply a similar change to cpu_handle_interrupt().
> 
> Fair.  I think something like this:
> 
>             if (cc->cpu_exec_interrupt(cpu, interrupt_request)) {
>                 replay_interrupt();
> -               cpu->exception_index = -1;
> +               cpu->exception_index =
> +                   (cpu->singlestep_enabled ? EXCP_DEBUG : -1);
>                 *last_tb = NULL;
>             }
> 
> I'm not quite sure how to test this though...
I wrote a small test case for the interrupt side that can be run on the
virt board:


8<-----------------------------------------------
    .global _start
    .text

#define GIC_DIST_BASE 0x08000000
#define GIC_CPU_BASE  0x08010000

#define ARCH_TIMER_NS_EL1_IRQ (16 + 14)

.org 0x280
_irq_handler:
    nop
    nop


.org 0x1000
_start:
    ldr x1, =GIC_DIST_BASE  /* GICD_CTLR */
    mov w0, #1              /* enable */
    str w0, [x1]

    ldr x1, =GIC_DIST_BASE + 0x100 /* GICD_ISENABLER0 */
    ldr w0, =(1 << ARCH_TIMER_NS_EL1_IRQ)
    str w0, [x1]

    ldr x1, =GIC_CPU_BASE   /* GICC_CTLR */
    mov w0, #1              /* enable */
    str w0, [x1]

    ldr x1, =GIC_CPU_BASE + 0x4  /* GICC_PMR */
    mov w0, #255              /* min priority */
    str w0, [x1]

    msr daifclr, 2          /* unmask IRQ line */

    mov x0, 10              /* timer will trigger in 10 ticks */
    msr cntp_tval_el0, x0

    mov x0, 1               /* enable timer */
enable_timer:
    msr cntp_ctl_el0, x0

1:
    b 1b

8<-----------------------------------------------------

$ aarch64-linux-gnu-gcc -g -nostdinc -nostdlib -Wl,-Ttext=0x0 -o foo foo.S

$ qemu-system-aarch64 -display none -M virt -cpu cortex-a53 -kernel foo
-s -S

$ aarch64-linux-gnu-gdb foo
(gdb) tar rem :1234
(gdb) maintenance packet Qqemu.sstep=0x1
(gdb) b enable_timer
(gdb) disp /i $pc
(gdb) c
Continuing.

Breakpoint 1, enable_timer () at foo.S:40
1: x/i $pc
=> 0x1040 <enable_timer>:       msr     cntp_ctl_el0, x0

(gdb) si
1: x/i $pc
=> 0x1044 <enable_timer+4>:     b       0x1044 <enable_timer+4>

(gdb)
1: x/i $pc
=> 0x280 <_irq_handler>:        nop

This is with your fix. Without it, the second stepi stops on 0x284.

> 
> Probably best to keep this a separate patch anyway.
Do you want me to send it? If yes, how should I give credit to you?
Should I put your Signed-off-by: in it?

thanks

Luc

> 
> 
> r~
>