Message ID | 1548334069-9158-1-git-send-email-frederic.konrad@adacore.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | gdbstub: shutdown guest when the target is killed | expand |
On Thu, 24 Jan 2019 at 12:47, KONRAD Frederic <frederic.konrad@adacore.com> wrote: > > Under MinGW when the target is killed no "W00" packet are received by GDB > because gdbstub takes the "exit(0)" path. Why is this a problem? The gdb protocol documentation for the 'k' packet says: # If the target system immediately closes the connection in response to 'k', # GDB does not consider the lack of packet acknowledgment to be an error, # and assumes the kill was successful. So it shouldn't matter whether we send the W00 packet or not. (In fact it's not clear to me that it's right to send the W00 packet unconditionally -- W00 is a "stop reply packet" so it's only correct to send it on QEMU exit if the stub was currently executing one of the continue/run commands, not if the guest CPU was already stopped.) This also isn't specific to mingw as far as I can see -- we behave the same on all hosts for system emulation. > So replace the "exit(0)" call by > a normal guest shutdown so the "W00" packet has a chance to be sent in > "gdb_cleanup". On the other hand I think that triggering a clean shutdown in system emulation mode is arguably a more reasonable choice than just bailing out immediately. I'm just not clear on what the problem is that you're trying to solve with the change... > Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com> > --- > gdbstub.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/gdbstub.c b/gdbstub.c > index bfc7afb..c91a909 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -1389,7 +1389,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > case 'k': > /* Kill the target */ > error_report("QEMU: Terminated via GDBstub"); I think we could usefully expand the comment. /* * Kill the target. The protocol does not specify the exact effect * of this packet, but notes that usually for a single-process target * it will kill the process, and for a bare-metal target it may * power-cycle or reset the system, so we follow that suggestion. * Note that this packet has no reply. */ > +#ifdef CONFIG_USER_ONLY > exit(0); > +#else > + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN); > +#endif > + break; > case 'D': > /* Detach packet */ > pid = 1; thanks -- PMM
Le 1/24/19 à 2:04 PM, Peter Maydell a écrit : > On Thu, 24 Jan 2019 at 12:47, KONRAD Frederic > <frederic.konrad@adacore.com> wrote: >> >> Under MinGW when the target is killed no "W00" packet are received by GDB >> because gdbstub takes the "exit(0)" path. > > Why is this a problem? The gdb protocol documentation for the 'k' > packet says: > > # If the target system immediately closes the connection in response to 'k', > # GDB does not consider the lack of packet acknowledgment to be an error, > # and assumes the kill was successful. > > So it shouldn't matter whether we send the W00 packet or not. > (In fact it's not clear to me that it's right to send the W00 > packet unconditionally -- W00 is a "stop reply packet" so it's > only correct to send it on QEMU exit if the stub was currently > executing one of the continue/run commands, not if the guest CPU > was already stopped.) Ah yes good point.. I didn't found this doc when I wrote this patch.. So maybe there is something odd with my GDB... > > This also isn't specific to mingw as far as I can see -- we behave > the same on all hosts for system emulation. Yes I wondered the same.. I'm not sure how the CharBackend behave when calling exit(..) under Windows though and I think I already had issue with some buffer not beeing flushed correctly. > >> So replace the "exit(0)" call by >> a normal guest shutdown so the "W00" packet has a chance to be sent in >> "gdb_cleanup". > > On the other hand I think that triggering a clean shutdown in > system emulation mode is arguably a more reasonable choice than > just bailing out immediately. I'm just not clear on what the > problem is that you're trying to solve with the change... True.. I can just make this commit has a cleanup commit if you like.. Thanks, Fred > >> Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com> >> --- >> gdbstub.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/gdbstub.c b/gdbstub.c >> index bfc7afb..c91a909 100644 >> --- a/gdbstub.c >> +++ b/gdbstub.c >> @@ -1389,7 +1389,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) >> case 'k': >> /* Kill the target */ >> error_report("QEMU: Terminated via GDBstub"); > > I think we could usefully expand the comment. > /* > * Kill the target. The protocol does not specify the exact effect > * of this packet, but notes that usually for a single-process target > * it will kill the process, and for a bare-metal target it may > * power-cycle or reset the system, so we follow that suggestion. > * Note that this packet has no reply. > */ > >> +#ifdef CONFIG_USER_ONLY >> exit(0); >> +#else >> + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN); >> +#endif >> + break; >> case 'D': >> /* Detach packet */ >> pid = 1; > > thanks > -- PMM >
diff --git a/gdbstub.c b/gdbstub.c index bfc7afb..c91a909 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -1389,7 +1389,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) case 'k': /* Kill the target */ error_report("QEMU: Terminated via GDBstub"); +#ifdef CONFIG_USER_ONLY exit(0); +#else + qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN); +#endif + break; case 'D': /* Detach packet */ pid = 1;
Under MinGW when the target is killed no "W00" packet are received by GDB because gdbstub takes the "exit(0)" path. So replace the "exit(0)" call by a normal guest shutdown so the "W00" packet has a chance to be sent in "gdb_cleanup". Signed-off-by: KONRAD Frederic <frederic.konrad@adacore.com> --- gdbstub.c | 5 +++++ 1 file changed, 5 insertions(+)