| Message ID | 20190129203219.6473-1-jcmvbkbc@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gdbstub: allow killing QEMU via vKill command | expand |
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; > } >
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; >> } >>
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; >>> } >>> >
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; }
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(+)