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

[Richard Henderson <richard.henderson@linaro.org>]

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~