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

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

[Luc Michel]

When single-stepping with a debugger attached to QEMU, and when an
exception is raised, the debugger misses the first instruction after the
exception:

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

$ aarch64-linux-gnu-gdb
GNU gdb (GDB) 9.2
[...]
(gdb) tar rem :1234
Remote debugging using :1234
warning: No executable has been specified and target does not support
determining executable automatically.  Try using the "file" command.
0x0000000000000000 in ?? ()
(gdb) # writing nop insns to 0x200 and 0x204
(gdb) set *0x200 = 0xd503201f
(gdb) set *0x204 = 0xd503201f
(gdb) # 0x0 address contains 0 which is an invalid opcode.
(gdb) # The CPU should raise an exception and jump to 0x200
(gdb) si
0x0000000000000204 in ?? ()

With this commit, the same run steps correctly on the first instruction
of the exception vector:

(gdb) si
0x0000000000000200 in ?? ()

Signed-off-by: Luc Michel <luc.michel@greensocs.com>
---

RFC because I'm really not sure this is the good place to do that since
EXCP_DEBUG are usually raised in each target translate.c. It could also
have implications with record/replay I'm not aware of.

---
 accel/tcg/cpu-exec.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

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);
+            }
+
         } else if (!replay_has_interrupt()) {
             /* give a chance to iothread in replay mode */
             *ret = EXCP_INTERRUPT;
             return true;
         }
-- 
2.27.0

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

[Richard Henderson]

On 7/16/20 3:04 AM, Luc Michel 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:
> 
> $ qemu-system-aarch64 -M virt -display none -cpu cortex-a53 -s -S
> 
> $ aarch64-linux-gnu-gdb
> GNU gdb (GDB) 9.2
> [...]
> (gdb) tar rem :1234
> Remote debugging using :1234
> warning: No executable has been specified and target does not support
> determining executable automatically.  Try using the "file" command.
> 0x0000000000000000 in ?? ()
> (gdb) # writing nop insns to 0x200 and 0x204
> (gdb) set *0x200 = 0xd503201f
> (gdb) set *0x204 = 0xd503201f
> (gdb) # 0x0 address contains 0 which is an invalid opcode.
> (gdb) # The CPU should raise an exception and jump to 0x200
> (gdb) si
> 0x0000000000000204 in ?? ()
> 
> With this commit, the same run steps correctly on the first instruction
> of the exception vector:
> 
> (gdb) si
> 0x0000000000000200 in ?? ()
> 
> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> ---
> 
> RFC because I'm really not sure this is the good place to do that since
> EXCP_DEBUG are usually raised in each target translate.c. It could also
> have implications with record/replay I'm not aware of.
> 
> ---
>  accel/tcg/cpu-exec.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> 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);

Plausible.  Although recursion on an inline function is going to defeat the
inline, in general.

We could expand the recursion by hand with

    if (unlikely(cpu->singlestep_enabled)) {
        *ret = EXCP_DEBUG;
        cpu_handle_debug_exception(cpu);
        return true;
    }

which might even be clearer.


r~

> +            }
> +
>          } else if (!replay_has_interrupt()) {
>              /* give a chance to iothread in replay mode */
>              *ret = EXCP_INTERRUPT;
>              return true;
>          }
> 

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

[Peter Maydell]

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().

thanks
-- PMM

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

[Richard Henderson]

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...

Probably best to keep this a separate patch anyway.


r~

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~
> 

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

[Richard Henderson]

On 7/17/20 4:01 AM, Luc Michel wrote:
> I wrote a small test case for the interrupt side that can be run on the
> virt board:
...
> This is with your fix. Without it, the second stepi stops on 0x284.

Awesome, thanks.

> 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?

I'll put it together.  You can then add your Reviewed/Tested-by.


r~