Re: [PATCH] gdbstub: Fix client Ctrl-C handling

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

On 8/1/23 11:40, Matheus Tavares Bernardino wrote:
> Hi, Nick.
> 
>> Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> On Tue Jul 11, 2023 at 9:03 PM AEST, Matheus Tavares Bernardino wrote:
>>>> Nicholas Piggin <npiggin@gmail.com> wrote:
>>>>
>>>> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
>>>> index 6911b73c07..ce8b42eb15 100644
>>>> --- a/gdbstub/gdbstub.c
>>>> +++ b/gdbstub/gdbstub.c
>>>> @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
>>>>               return;
>>>>       }
>>>>       if (runstate_is_running()) {
>>>> -        /* when the CPU is running, we cannot do anything except stop
>>>> -           it when receiving a char */
>>>> +        /*
>>>> +         * When the CPU is running, we cannot do anything except stop
>>>> +         * it when receiving a char. This is expected on a Ctrl-C in the
>>>> +         * gdb client. Because we are in all-stop mode, gdb sends a
>>>> +         * 0x03 byte which is not a usual packet, so we handle it specially
>>>> +         * here, but it does expect a stop reply.
>>>> +         */
>>>> +        if (ch != 0x03) {
>>>> +            warn_report("gdbstub: client sent packet while target running\n");
>>>> +        }
>>>> +        gdbserver_state.allow_stop_reply = true;
>>>>           vm_stop(RUN_STATE_PAUSED);
>>>>       } else
>>>>   #endif
>>>
>>> Makes sense to me, but shouldn't we send the stop-reply packet only for
>>> Ctrl+C/0x03?
>>
>> Good question.
>>
>> I think if we get a character here that's not a 3, we're already in
>> trouble, and we eat it so even worse. Since we only send a stop packet
>> back when the vm stops, then if we don't send one now we might never
>> send it. At least if we send one then the client might have some chance
>> to get back to a sane state.
> 
> I just noticed now (as I was integrating the latest upstream patches
> with our downstream qemu-system-hexagon) that this causes the
> gdbstub-untimely-packet tcg test to fail.
> 
> My first thought was that, if 0x3 is the only valid case where we will
> read a char when the cpu is running, perhaps not issuing the stop-reply
> isn't that bad as GDB would ignore it anyways. E.g. from a `set debug
> remote 1` output:
> 
>    Sending packet: $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;
>                     fork-events+;vfork-events+;exec-events+;vContSupported+;
> 		   QThreadEvents+;no-resumed+;
> 		   xmlRegisters=i386#6a...
>    Packet instead of Ack, ignoring it
> 
> So, perhaps, we could do:
> 
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index f123b40ce7..8af066301a 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -2055,8 +2055,9 @@ void gdb_read_byte(uint8_t ch)
>            */
>           if (ch != 0x03) {
>               warn_report("gdbstub: client sent packet while target running\n");
> +        } else {
> +            gdbserver_state.allow_stop_reply = true;
>           }
> -        gdbserver_state.allow_stop_reply = true;
>           vm_stop(RUN_STATE_PAUSED);
>       } else
>   #endif
> -- >8 --
> 
> Alternatively, since GDB ignores the packet anyways, should we just let
> this be and refactor/remove the test?

Ping, Alex and Nick.

I can confirm that Matheus' patch fixes the problem I am seeing.
But I'm also open to removing the test.


r~