| Message ID | 1521217393-8162-1-git-send-email-frederic.konrad@adacore.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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(); > >
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(); >> >>
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>
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/
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.
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/ >
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();
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(+)