[Qemu-devel] [PATCH] gdbstub: allow killing QEMU via vKill command

[Qemu-devel] [PATCH] gdbstub: allow killing QEMU via vKill command

[Max Filippov]

With multiprocess extensions gdb uses 'vKill' packet instead of 'k' to
kill the inferior. Handle 'vKill' the same way 'k' was handled in the
presence of single process.

Fixes: 7cf48f6752e5 ("gdbstub: add multiprocess support to
(f|s)ThreadInfo and ThreadExtraInfo")

Cc: Luc Michel <luc.michel@greensocs.com>
Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 gdbstub.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index bfc7afb50968..1ef31240c055 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1383,6 +1383,28 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
 
             put_packet(s, buf);
             break;
+        } else if (strncmp(p, "Kill;", 5) == 0) {
+            unsigned long pid;
+
+            p += 5;
+
+            if (qemu_strtoul(p, &p, 16, &pid)) {
+                put_packet(s, "E22");
+                break;
+            }
+            process = gdb_get_process(s, pid);
+
+            if (process == NULL) {
+                put_packet(s, "E22");
+                break;
+            }
+            if (s->process_num <= 1) {
+                /* Kill the target */
+                error_report("QEMU: Terminated via GDBstub");
+                exit(0);
+            }
+            /* TODO: handle multiprocess case */
+            goto unknown_command;
         } else {
             goto unknown_command;
         }
-- 
2.11.0

Re: [Qemu-devel] [PATCH] gdbstub: allow killing QEMU via vKill command

[KONRAD Frederic]

Hi,

Le 1/29/19 à 9:32 PM, Max Filippov a écrit :
> With multiprocess extensions gdb uses 'vKill' packet instead of 'k' to
> kill the inferior. Handle 'vKill' the same way 'k' was handled in the
> presence of single process.
> 
> Fixes: 7cf48f6752e5 ("gdbstub: add multiprocess support to
> (f|s)ThreadInfo and ThreadExtraInfo")
> 
> Cc: Luc Michel <luc.michel@greensocs.com>
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>   gdbstub.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index bfc7afb50968..1ef31240c055 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1383,6 +1383,28 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>   
>               put_packet(s, buf);
>               break;
> +        } else if (strncmp(p, "Kill;", 5) == 0) {
> +            unsigned long pid;
> +
> +            p += 5;
> +
> +            if (qemu_strtoul(p, &p, 16, &pid)) {
> +                put_packet(s, "E22");
> +                break;
> +            }
> +            process = gdb_get_process(s, pid);
> +
> +            if (process == NULL) {
> +                put_packet(s, "E22");
> +                break;
> +            }
> +            if (s->process_num <= 1) {
> +                /* Kill the target */
> +                error_report("QEMU: Terminated via GDBstub");
> +                exit(0);

I suggest:

#ifdef CONFIG_USER_ONLY
         exit(0);
#else
         qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
#endif

instead of exit(0);

> +            }
> +            /* TODO: handle multiprocess case */
> +            goto unknown_command;
>           } else {
>               goto unknown_command;
>           }
> 

Re: [Qemu-devel] [PATCH] gdbstub: allow killing QEMU via vKill command

[Luc Michel]

[-- Attachment #1: Type: text/plain, Size: 3844 bytes --]

Hi,

On 1/30/19 2:37 PM, KONRAD Frederic wrote:
> Hi,
> 
> Le 1/29/19 à 9:32 PM, Max Filippov a écrit :
>> With multiprocess extensions gdb uses 'vKill' packet instead of 'k' to
>> kill the inferior. Handle 'vKill' the same way 'k' was handled in the
>> presence of single process.
>>
>> Fixes: 7cf48f6752e5 ("gdbstub: add multiprocess support to
>> (f|s)ThreadInfo and ThreadExtraInfo")
Thanks for the patch.

>>
>> Cc: Luc Michel <luc.michel@greensocs.com>
>> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
>> ---
>>   gdbstub.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index bfc7afb50968..1ef31240c055 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1383,6 +1383,28 @@ static int gdb_handle_packet(GDBState *s, const
>> char *line_buf)
>>                 put_packet(s, buf);
>>               break;
>> +        } else if (strncmp(p, "Kill;", 5) == 0) {
>> +            unsigned long pid;
>> +
>> +            p += 5;
>> +
>> +            if (qemu_strtoul(p, &p, 16, &pid)) {
>> +                put_packet(s, "E22");
>> +                break;
>> +            }
>> +            process = gdb_get_process(s, pid);
>> +
>> +            if (process == NULL) {
>> +                put_packet(s, "E22");
>> +                break;
>> +            }
>> +            if (s->process_num <= 1) {
>> +                /* Kill the target */
>> +                error_report("QEMU: Terminated via GDBstub");
>> +                exit(0);
> 
> I suggest:
> 
> #ifdef CONFIG_USER_ONLY
>         exit(0);
> #else
>         qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
You are missing a `break;' here.

Regarding the cause, I think it's more an 'host' cause than a 'guest' one.
> #endif
> 
> instead of exit(0);
I just tested, this is not ideal in the current shape. Not quitting
immediately makes the stub sends a "W00" packet. It's better if we send
nothing because GDB interprets that as the connection was closed (which
is effectively the case). Otherwise in multiprocess mode, GDB thinks the
other processes are still alive (see below).

with exit(0):
(gdb) k
Kill the program being debugged? (y or n) y
Sending packet: $vKill;1#6e...Ack
Remote connection closed

with qemu_system_shutdown_request:
(gdb) k
Kill the program being debugged? (y or n) y
Sending packet: $vKill;1#6e...Ack
Packet received: W00
Packet vKill (kill) is supported
[Inferior 1 (process 1) killed]

So maybe a solution (for another patch) would be to send a 'W' packet
for each attached process (using the multiprocess format). Another is to
prevent 'W' packets to be sent at all... they are not required in this case.

> 
>> +            }
>> +            /* TODO: handle multiprocess case */
About the multiprocess case, I would vote to adopt the same behaviour
(i.e. terminate QEMU), whatever the PID is or how many processes are
attached. Otherwise we would have different behaviour for the GDB `kill`
command, whether the simulated board exposes multiple processes or not.
Moreover, I think considering just one PID for a kill command does not
make much sense in our case.

I just tested, GDB is fine with QEMU closing the connection when having
multiple processes attached if we do exit(0) directly (i.e. do not send
a 'W00' packet).

Thanks.

Luc
>> +            goto unknown_command;
>>           } else {
>>               goto unknown_command;
>>           }
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH] gdbstub: allow killing QEMU via vKill command

[KONRAD Frederic]

Le 1/30/19 à 3:31 PM, Luc Michel a écrit :
> Hi,
> 
> On 1/30/19 2:37 PM, KONRAD Frederic wrote:
>> Hi,
>>
>> Le 1/29/19 à 9:32 PM, Max Filippov a écrit :
>>> With multiprocess extensions gdb uses 'vKill' packet instead of 'k' to
>>> kill the inferior. Handle 'vKill' the same way 'k' was handled in the
>>> presence of single process.
>>>
>>> Fixes: 7cf48f6752e5 ("gdbstub: add multiprocess support to
>>> (f|s)ThreadInfo and ThreadExtraInfo")
> Thanks for the patch.
> 
>>>
>>> Cc: Luc Michel <luc.michel@greensocs.com>
>>> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
>>> ---
>>>    gdbstub.c | 22 ++++++++++++++++++++++
>>>    1 file changed, 22 insertions(+)
>>>
>>> diff --git a/gdbstub.c b/gdbstub.c
>>> index bfc7afb50968..1ef31240c055 100644
>>> --- a/gdbstub.c
>>> +++ b/gdbstub.c
>>> @@ -1383,6 +1383,28 @@ static int gdb_handle_packet(GDBState *s, const
>>> char *line_buf)
>>>                  put_packet(s, buf);
>>>                break;
>>> +        } else if (strncmp(p, "Kill;", 5) == 0) {
>>> +            unsigned long pid;
>>> +
>>> +            p += 5;
>>> +
>>> +            if (qemu_strtoul(p, &p, 16, &pid)) {
>>> +                put_packet(s, "E22");
>>> +                break;
>>> +            }
>>> +            process = gdb_get_process(s, pid);
>>> +
>>> +            if (process == NULL) {
>>> +                put_packet(s, "E22");
>>> +                break;
>>> +            }
>>> +            if (s->process_num <= 1) {
>>> +                /* Kill the target */
>>> +                error_report("QEMU: Terminated via GDBstub");
>>> +                exit(0);
>>
>> I suggest:
>>
>> #ifdef CONFIG_USER_ONLY
>>          exit(0);
>> #else
>>          qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> You are missing a `break;' here.
> 
> Regarding the cause, I think it's more an 'host' cause than a 'guest' one.
>> #endif
>>
>> instead of exit(0);
> I just tested, this is not ideal in the current shape. Not quitting
> immediately makes the stub sends a "W00" packet. It's better if we send
> nothing because GDB interprets that as the connection was closed (which
> is effectively the case). Otherwise in multiprocess mode, GDB thinks the
> other processes are still alive (see below).
> 
> with exit(0):
> (gdb) k
> Kill the program being debugged? (y or n) y
> Sending packet: $vKill;1#6e...Ack
> Remote connection closed
> 
> with qemu_system_shutdown_request:
> (gdb) k
> Kill the program being debugged? (y or n) y
> Sending packet: $vKill;1#6e...Ack
> Packet received: W00
> Packet vKill (kill) is supported
> [Inferior 1 (process 1) killed]

Ok makes sense. I didn't think of that.

> 
> So maybe a solution (for another patch) would be to send a 'W' packet
> for each attached process (using the multiprocess format). Another is to
> prevent 'W' packets to be sent at all... they are not required in this case.
> 
>>
>>> +            }
>>> +            /* TODO: handle multiprocess case */
> About the multiprocess case, I would vote to adopt the same behaviour
> (i.e. terminate QEMU), whatever the PID is or how many processes are
> attached. Otherwise we would have different behaviour for the GDB `kill`
> command, whether the simulated board exposes multiple processes or not.
> Moreover, I think considering just one PID for a kill command does not
> make much sense in our case.
> 
> I just tested, GDB is fine with QEMU closing the connection when having
> multiple processes attached if we do exit(0) directly (i.e. do not send
> a 'W00' packet).

Yes I see the same behavior with Linux but it makes GDB crashes with MinGW
at least before this patch and the multiprocess one (eg: v3.0.0).

I'll wait for this patch to land and send some trace if I can still reproduce.

Cheers,
Fred

> 
> Thanks.
> 
> Luc
>>> +            goto unknown_command;
>>>            } else {
>>>                goto unknown_command;
>>>            }
>>>
>