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;
>          }
>