[Qemu-devel] [PATCH] gdbstub: send a terminaison packet instead of crashing gdb

[Qemu-devel] [PATCH] gdbstub: send a terminaison packet instead of crashing gdb

[KONRAD Frederic]

Since the commit:
commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268
Author: Stefan Hajnoczi <stefanha@redhat.com>
Date:   Wed Mar 7 14:42:05 2018 +0000

    vl: introduce vm_shutdown()

GDB crash when qemu exits (at least on sparc-softmmu):
Remote communication error.  Target disconnected.: Connection reset by peer.
Quitting: putpkt: write failed: Broken pipe.

So send a packet to kill GDB before we exit QEMU:
[Inferior 1 (Thread 0) exited normally]

Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
---
 gdbstub.c              | 7 +++++++
 include/exec/gdbstub.h | 2 ++
 vl.c                   | 2 ++
 3 files changed, 11 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index f1d5148..a76b2fa 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2052,6 +2052,13 @@ int gdbserver_start(const char *device)
     return 0;
 }
 
+void gdbserver_cleanup(void)
+{
+    if (gdbserver_state) {
+        put_packet(gdbserver_state, "W00");
+    }
+}
+
 static void register_types(void)
 {
     type_register_static(&char_gdb_type_info);
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 9aa7756..2e8a4b8 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -103,6 +103,8 @@ int gdbserver_start(int);
 int gdbserver_start(const char *port);
 #endif
 
+void gdbserver_cleanup(void);
+
 /**
  * gdb_has_xml:
  * This is an ugly hack to cope with both new and old gdb.
diff --git a/vl.c b/vl.c
index 3ef04ce..0427b15 100644
--- a/vl.c
+++ b/vl.c
@@ -4723,6 +4723,8 @@ int main(int argc, char **argv, char **envp)
 
     main_loop();
 
+    gdbserver_cleanup();
+
     /* No more vcpu or device emulation activity beyond this point */
     vm_shutdown();
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] gdbstub: send a terminaison packet instead of crashing gdb

[Peter Maydell]

On 16 March 2018 at 16:23, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
> Since the commit:
> commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268
> Author: Stefan Hajnoczi <stefanha@redhat.com>
> Date:   Wed Mar 7 14:42:05 2018 +0000
>
>     vl: introduce vm_shutdown()
>
> GDB crash when qemu exits (at least on sparc-softmmu):
> Remote communication error.  Target disconnected.: Connection reset by peer.
> Quitting: putpkt: write failed: Broken pipe.
>
> So send a packet to kill GDB before we exit QEMU:
> [Inferior 1 (Thread 0) exited normally]
>
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> ---
>  gdbstub.c              | 7 +++++++
>  include/exec/gdbstub.h | 2 ++
>  vl.c                   | 2 ++
>  3 files changed, 11 insertions(+)

We didn't send an exiting packet before commit 4486e89c219c0,
so do you know why this worked before then? (Telling gdb we're
exiting seems like the right thing, though.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] gdbstub: send a terminaison packet instead of crashing gdb

[KONRAD Frederic]

On 03/16/2018 05:34 PM, Peter Maydell wrote:
> On 16 March 2018 at 16:23, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
>> Since the commit:
>> commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268
>> Author: Stefan Hajnoczi <stefanha@redhat.com>
>> Date:   Wed Mar 7 14:42:05 2018 +0000
>>
>>      vl: introduce vm_shutdown()
>>
>> GDB crash when qemu exits (at least on sparc-softmmu):
>> Remote communication error.  Target disconnected.: Connection reset by peer.
>> Quitting: putpkt: write failed: Broken pipe.
>>
>> So send a packet to kill GDB before we exit QEMU:
>> [Inferior 1 (Thread 0) exited normally]
>>
>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
>> ---
>>   gdbstub.c              | 7 +++++++
>>   include/exec/gdbstub.h | 2 ++
>>   vl.c                   | 2 ++
>>   3 files changed, 11 insertions(+)
> 
> We didn't send an exiting packet before commit 4486e89c219c0,
> so do you know why this worked before then? (Telling gdb we're
> exiting seems like the right thing, though.)
> 

Hmmm good question, I didn't had time to investigate in detail

Before 4486e89c219c0:

(gdb) tar rem :1234
Remote debugging using :1234
0x40000000 in trap_table ()
(gdb) c
Continuing.
Remote connection closed

After 4486e89c219c0:

(gdb) tar rem :1234
Remote debugging using :1234
0x40000000 in trap_table ()
(gdb) c
Continuing.
putpkt: write failed: Connection reset by peer.

With the patch:

(gdb) tar rem :1234
Remote debugging using :1234
0x40000000 in trap_table ()
(gdb) c
Continuing.
[Inferior 1 (Thread 0) exited normally]

We use to have this patch in our repository to avoid the remote
connection closed above.

Thanks,
Fred

> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] gdbstub: send a terminaison packet instead of crashing gdb

[Philippe Mathieu-Daudé]

On 03/16/2018 05:23 PM, KONRAD Frederic wrote:
> Since the commit:
> commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268
> Author: Stefan Hajnoczi <stefanha@redhat.com>
> Date:   Wed Mar 7 14:42:05 2018 +0000
> 
>     vl: introduce vm_shutdown()
> 
> GDB crash when qemu exits (at least on sparc-softmmu):
> Remote communication error.  Target disconnected.: Connection reset by peer.
> Quitting: putpkt: write failed: Broken pipe.
> 
> So send a packet to kill GDB before we exit QEMU:
> [Inferior 1 (Thread 0) exited normally]
> 
> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  gdbstub.c              | 7 +++++++
>  include/exec/gdbstub.h | 2 ++
>  vl.c                   | 2 ++
>  3 files changed, 11 insertions(+)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index f1d5148..a76b2fa 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -2052,6 +2052,13 @@ int gdbserver_start(const char *device)
>      return 0;
>  }
>  
> +void gdbserver_cleanup(void)
> +{
> +    if (gdbserver_state) {
> +        put_packet(gdbserver_state, "W00");
> +    }
> +}
> +
>  static void register_types(void)
>  {
>      type_register_static(&char_gdb_type_info);
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index 9aa7756..2e8a4b8 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -103,6 +103,8 @@ int gdbserver_start(int);
>  int gdbserver_start(const char *port);
>  #endif
>  
> +void gdbserver_cleanup(void);
> +
>  /**
>   * gdb_has_xml:
>   * This is an ugly hack to cope with both new and old gdb.
> diff --git a/vl.c b/vl.c
> index 3ef04ce..0427b15 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4723,6 +4723,8 @@ int main(int argc, char **argv, char **envp)
>  
>      main_loop();
>  
> +    gdbserver_cleanup();
> +
>      /* No more vcpu or device emulation activity beyond this point */
>      vm_shutdown();
>  
> 

Re: [Qemu-devel] [PATCH] gdbstub: send a terminaison packet instead of crashing gdb

[KONRAD Frederic]

Hi Philippe,

Thanks for the review!

BTW I forgot the for 2.12 tag can this be included in 2.12 or is
it too late?

Thanks,
Fred

On 03/19/2018 12:30 AM, Philippe Mathieu-Daudé wrote:
> On 03/16/2018 05:23 PM, KONRAD Frederic wrote:
>> Since the commit:
>> commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268
>> Author: Stefan Hajnoczi <stefanha@redhat.com>
>> Date:   Wed Mar 7 14:42:05 2018 +0000
>>
>>      vl: introduce vm_shutdown()
>>
>> GDB crash when qemu exits (at least on sparc-softmmu):
>> Remote communication error.  Target disconnected.: Connection reset by peer.
>> Quitting: putpkt: write failed: Broken pipe.
>>
>> So send a packet to kill GDB before we exit QEMU:
>> [Inferior 1 (Thread 0) exited normally]
>>
>> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 
>> ---
>>   gdbstub.c              | 7 +++++++
>>   include/exec/gdbstub.h | 2 ++
>>   vl.c                   | 2 ++
>>   3 files changed, 11 insertions(+)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index f1d5148..a76b2fa 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -2052,6 +2052,13 @@ int gdbserver_start(const char *device)
>>       return 0;
>>   }
>>   
>> +void gdbserver_cleanup(void)
>> +{
>> +    if (gdbserver_state) {
>> +        put_packet(gdbserver_state, "W00");
>> +    }
>> +}
>> +
>>   static void register_types(void)
>>   {
>>       type_register_static(&char_gdb_type_info);
>> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
>> index 9aa7756..2e8a4b8 100644
>> --- a/include/exec/gdbstub.h
>> +++ b/include/exec/gdbstub.h
>> @@ -103,6 +103,8 @@ int gdbserver_start(int);
>>   int gdbserver_start(const char *port);
>>   #endif
>>   
>> +void gdbserver_cleanup(void);
>> +
>>   /**
>>    * gdb_has_xml:
>>    * This is an ugly hack to cope with both new and old gdb.
>> diff --git a/vl.c b/vl.c
>> index 3ef04ce..0427b15 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4723,6 +4723,8 @@ int main(int argc, char **argv, char **envp)
>>   
>>       main_loop();
>>   
>> +    gdbserver_cleanup();
>> +
>>       /* No more vcpu or device emulation activity beyond this point */
>>       vm_shutdown();
>>   
>>

Re: [Qemu-devel] [PATCH] gdbstub: send a terminaison packet instead of crashing gdb

[Stefan Hajnoczi]

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

On Fri, Mar 16, 2018 at 07:45:18PM +0100, KONRAD Frederic wrote:
> On 03/16/2018 05:34 PM, Peter Maydell wrote:
> > On 16 March 2018 at 16:23, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
> > > Since the commit:
> > > commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268
> > > Author: Stefan Hajnoczi <stefanha@redhat.com>
> > > Date:   Wed Mar 7 14:42:05 2018 +0000
> > > 
> > >      vl: introduce vm_shutdown()
> > > 
> > > GDB crash when qemu exits (at least on sparc-softmmu):
> > > Remote communication error.  Target disconnected.: Connection reset by peer.
> > > Quitting: putpkt: write failed: Broken pipe.
> > > 
> > > So send a packet to kill GDB before we exit QEMU:
> > > [Inferior 1 (Thread 0) exited normally]
> > > 
> > > Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com>
> > > ---
> > >   gdbstub.c              | 7 +++++++
> > >   include/exec/gdbstub.h | 2 ++
> > >   vl.c                   | 2 ++
> > >   3 files changed, 11 insertions(+)
> > 
> > We didn't send an exiting packet before commit 4486e89c219c0,
> > so do you know why this worked before then? (Telling gdb we're
> > exiting seems like the right thing, though.)
> > 
> 
> Hmmm good question, I didn't had time to investigate in detail
> 
> Before 4486e89c219c0:
> 
> (gdb) tar rem :1234
> Remote debugging using :1234
> 0x40000000 in trap_table ()
> (gdb) c
> Continuing.
> Remote connection closed
> 
> After 4486e89c219c0:
> 
> (gdb) tar rem :1234
> Remote debugging using :1234
> 0x40000000 in trap_table ()
> (gdb) c
> Continuing.
> putpkt: write failed: Connection reset by peer.
> 
> With the patch:
> 
> (gdb) tar rem :1234
> Remote debugging using :1234
> 0x40000000 in trap_table ()
> (gdb) c
> Continuing.
> [Inferior 1 (Thread 0) exited normally]
> 
> We use to have this patch in our repository to avoid the remote
> connection closed above.

Previously pause_vcpus() didn't invoke the vm change state handler.
Therefore the gdbstub didn't tell GDB that the vcpu was being stopped
from gdb_vm_state_change().

Now vm_shutdown() invokes vm change state handlers so the gdbstub tells
GDB that the vcpu is stopping.  As a result GDB sends a packet to query
the state of the vcpu - but QEMU terminates and closes the connection
partway through.

Here is the strace:
31365 poll([{fd=3, events=POLLIN}, {fd=7, events=POLLIN}, {fd=11, events=POLLIN}, {fd=13, events=POLLIN}], 4, -1) = 1 ([{fd=13, revents=POLLIN}])
31365 recvfrom(13, "$T03thread:01;#05", 8192, 0, NULL, NULL) = 17
31365 poll([{fd=13, events=POLLIN}], 1, 0) = 1 ([{fd=13, revents=POLLIN}])
31365 sendto(13, "+", 1, 0, NULL, 0)    = 1
31365 sendto(13, "$g#67", 5, 0, NULL, 0) = -1 EPIPE (Broken pipe)
31365 --- SIGPIPE {si_signo=SIGPIPE, si_code=SI_USER, si_pid=31365, si_uid=1000} ---

The ugly error message is because QEMU doesn't respond to GDB's "$g#67"
message.

Your patch is making GDB exit gracefully.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] gdbstub: send a terminaison packet instead of crashing gdb

[Eric Blake]

On 03/16/2018 11:23 AM, KONRAD Frederic wrote:

In the subject: s/terminaison/termination/

> Since the commit:
> commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268
> Author: Stefan Hajnoczi <stefanha@redhat.com>
> Date:   Wed Mar 7 14:42:05 2018 +0000
> 
>      vl: introduce vm_shutdown()
> 
> GDB crash when qemu exits (at least on sparc-softmmu):

s/crash/crashes/

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-2.12] gdbstub: send a terminaison packet instead of crashing gdb

[Eric Blake]

On 03/19/2018 04:52 AM, KONRAD Frederic wrote:
> Hi Philippe,
> 
> Thanks for the review!
> 
> BTW I forgot the for 2.12 tag can this be included in 2.12 or is
> it too late?

It's a bug-fix, so it can be included during softfreeze.  I've amended 
the subject line if it helps the right maintainer notice; or you can 
resend a v2 with the typos fixed, R-b comments added, and the 2.12 tag 
in your resend.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] gdbstub: send a terminaison packet instead of crashing gdb

[KONRAD Frederic]

oops sorry for that, I'll resend.

Thanks,
Fred

On 03/19/2018 06:43 PM, Eric Blake wrote:
> On 03/16/2018 11:23 AM, KONRAD Frederic wrote:
> 
> In the subject: s/terminaison/termination/
> 
>> Since the commit:
>> commit 4486e89c219c0d1b9bd8dfa0b1dd5b0d51ff2268
>> Author: Stefan Hajnoczi <stefanha@redhat.com>
>> Date:   Wed Mar 7 14:42:05 2018 +0000
>>
>>      vl: introduce vm_shutdown()
>>
>> GDB crash when qemu exits (at least on sparc-softmmu):
> 
> s/crash/crashes/
>