| Message ID | 1362499399-19475-1-git-send-email-chouteau@adacore.com |
|---|---|
| State | New |
| Headers | show |
On 2013-03-05 17:03, Fabien Chouteau wrote: > With this patch GDB will issue a "detach" command at the end of a > debugging session instead of a "kill". This behavior can be inverted > with the new option -gdb-not-attached. > > This patch implements the requirement described in Jan Kiszka's patch: > "gdbstub: Do not kill target in system emulation mode". The patch can > therefore be reverted. > > Signed-off-by: Fabien Chouteau <chouteau@adacore.com> > --- > gdbstub.c | 13 ++++++++++++- > include/exec/gdbstub.h | 2 ++ > qemu-options.hx | 7 +++++++ > vl.c | 3 +++ > 4 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/gdbstub.c b/gdbstub.c > index e414ad9..c0686a9 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -32,7 +32,6 @@ > #include "monitor/monitor.h" > #include "char/char.h" > #include "sysemu/sysemu.h" > -#include "exec/gdbstub.h" > #endif > > #define MAX_PACKET_LENGTH 4096 > @@ -41,6 +40,9 @@ > #include "qemu/sockets.h" > #include "sysemu/kvm.h" > #include "qemu/bitops.h" > +#include "exec/gdbstub.h" > + > +static bool gdb_attached = true; > > #ifndef TARGET_CPU_MEMORY_RW_DEBUG > static inline int target_memory_rw_debug(CPUArchState *env, target_ulong addr, > @@ -2491,6 +2493,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > break; > } > #endif > + if (strncmp(p, "Attached", 8) == 0) { > + put_packet(s, gdb_attached ? "1" : "0"); > + break; > + } This works as expected for system mode, but now inverts the behaviour for user mode - that's unexpected and not ok. > /* Unrecognised 'q' command. */ > goto unknown_command; > > @@ -3055,3 +3061,8 @@ int gdbserver_start(const char *device) > return 0; > } > #endif > + > +void gdb_set_attached(bool attached) > +{ > + gdb_attached = attached; > +} > diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h > index ba20afa..b2d3b13 100644 > --- a/include/exec/gdbstub.h > +++ b/include/exec/gdbstub.h > @@ -50,4 +50,6 @@ int gdbserver_start(const char *port); > /* in gdbstub-xml.c, generated by scripts/feature_to_c.sh */ > extern const char *const xml_builtin[][2]; > > +void gdb_set_attached(bool attached); > + > #endif > diff --git a/qemu-options.hx b/qemu-options.hx > index 6f9334a..026d3eb 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -2988,6 +2988,13 @@ property must be set. These objects are placed in the > '/objects' path. > ETEXI > > +DEF("gdb-not-attached", 0, QEMU_OPTION_gdb_not_attached, > + "-gdb-not-attached\n" > + " Do not set Gdb remote server in attached mode.\n" > + " When exiting debugging session, Gdb will send a 'kill'\n" > + " command instead of a 'detach'.\n", > + QEMU_ARCH_ALL) > + First of all, why do we need this configurable? In which use cases do you want attached mode for user emulation or kill mode for system emulation/virtualization? Then, if you can convince us that such a switch is useful, a new top-level command line option is still a no-go. We already have -gdb, so this would have to become an option to it. In any case, I would suggest to add static attached mode first (enabled for system, disable for user), revert my patch and then, optionally, propose an attached mode control. Jan
On 03/10/2013 09:06 AM, Jan Kiszka wrote: >> @@ -2491,6 +2493,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) >> break; >> } >> #endif >> + if (strncmp(p, "Attached", 8) == 0) { >> + put_packet(s, gdb_attached ? "1" : "0"); >> + break; >> + } > > This works as expected for system mode, but now inverts the behaviour > for user mode - that's unexpected and not ok. > OK, I can change the default value for user mode. >> diff --git a/qemu-options.hx b/qemu-options.hx >> index 6f9334a..026d3eb 100644 >> --- a/qemu-options.hx >> +++ b/qemu-options.hx >> @@ -2988,6 +2988,13 @@ property must be set. These objects are placed in the >> '/objects' path. >> ETEXI >> >> +DEF("gdb-not-attached", 0, QEMU_OPTION_gdb_not_attached, >> + "-gdb-not-attached\n" >> + " Do not set Gdb remote server in attached mode.\n" >> + " When exiting debugging session, Gdb will send a 'kill'\n" >> + " command instead of a 'detach'.\n", >> + QEMU_ARCH_ALL) >> + > > First of all, why do we need this configurable? In which use cases do > you want attached mode for user emulation or kill mode for system > emulation/virtualization? > It's more convenient for us, we expect QEMU to terminate at the end of debugging session because we do not run big systems/kernels but short test programs. It used to be the default behavior of QEMU, and our test-suites, IDE, developers and users are expecting this. > Then, if you can convince us that such a switch is useful, a new > top-level command line option is still a no-go. We already have -gdb, so > this would have to become an option to it. > Something like: -gdb dev[,attached=on|off] wait for gdb connection on 'dev' This will not fit nicely in the current option because 'dev' is a chardev descriptor. I will have to add 'attached' in the list of chardev opts, which is not correct. Or do some nasty strings manipulation. Maybe this: -gdb-opts attached=on|off We can extend it latter if more options are needed. > In any case, I would suggest to add static attached mode first (enabled > for system, disable for user), revert my patch and then, optionally, > propose an attached mode control. > Mode control is not optional for me. Regards,
On 2013-03-11 12:22, Fabien Chouteau wrote: > On 03/10/2013 09:06 AM, Jan Kiszka wrote: >>> @@ -2491,6 +2493,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) >>> break; >>> } >>> #endif >>> + if (strncmp(p, "Attached", 8) == 0) { >>> + put_packet(s, gdb_attached ? "1" : "0"); >>> + break; >>> + } >> >> This works as expected for system mode, but now inverts the behaviour >> for user mode - that's unexpected and not ok. >> > > OK, I can change the default value for user mode. > >>> diff --git a/qemu-options.hx b/qemu-options.hx >>> index 6f9334a..026d3eb 100644 >>> --- a/qemu-options.hx >>> +++ b/qemu-options.hx >>> @@ -2988,6 +2988,13 @@ property must be set. These objects are placed in the >>> '/objects' path. >>> ETEXI >>> >>> +DEF("gdb-not-attached", 0, QEMU_OPTION_gdb_not_attached, >>> + "-gdb-not-attached\n" >>> + " Do not set Gdb remote server in attached mode.\n" >>> + " When exiting debugging session, Gdb will send a 'kill'\n" >>> + " command instead of a 'detach'.\n", >>> + QEMU_ARCH_ALL) >>> + >> >> First of all, why do we need this configurable? In which use cases do >> you want attached mode for user emulation or kill mode for system >> emulation/virtualization? >> > > It's more convenient for us, we expect QEMU to terminate at the end of > debugging session because we do not run big systems/kernels but short > test programs. It used to be the default behavior of QEMU, and our > test-suites, IDE, developers and users are expecting this. And it's not possible to replace 'q' with 'k' in your gdb control scripts? That gives you a well-defined behaviour, and we don't need to tweak QEMU, specifically its command line, for this special case. Jan
On 03/11/2013 12:36 PM, Jan Kiszka wrote: > On 2013-03-11 12:22, Fabien Chouteau wrote: >> On 03/10/2013 09:06 AM, Jan Kiszka wrote: >>>> diff --git a/qemu-options.hx b/qemu-options.hx >>>> index 6f9334a..026d3eb 100644 >>>> --- a/qemu-options.hx >>>> +++ b/qemu-options.hx >>>> @@ -2988,6 +2988,13 @@ property must be set. These objects are placed in the >>>> '/objects' path. >>>> ETEXI >>>> >>>> +DEF("gdb-not-attached", 0, QEMU_OPTION_gdb_not_attached, >>>> + "-gdb-not-attached\n" >>>> + " Do not set Gdb remote server in attached mode.\n" >>>> + " When exiting debugging session, Gdb will send a 'kill'\n" >>>> + " command instead of a 'detach'.\n", >>>> + QEMU_ARCH_ALL) >>>> + >>> >>> First of all, why do we need this configurable? In which use cases do >>> you want attached mode for user emulation or kill mode for system >>> emulation/virtualization? >>> >> >> It's more convenient for us, we expect QEMU to terminate at the end of >> debugging session because we do not run big systems/kernels but short >> test programs. It used to be the default behavior of QEMU, and our >> test-suites, IDE, developers and users are expecting this. > > And it's not possible to replace 'q' with 'k' in your gdb control > scripts? It's not just in a script or two, as I said it's test-suites, IDE, developers and users. > That gives you a well-defined behaviour, and we don't need to > tweak QEMU, specifically its command line, for this special case. > I don't understand the issue with this new option. Regards,
diff --git a/gdbstub.c b/gdbstub.c index e414ad9..c0686a9 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -32,7 +32,6 @@ #include "monitor/monitor.h" #include "char/char.h" #include "sysemu/sysemu.h" -#include "exec/gdbstub.h" #endif #define MAX_PACKET_LENGTH 4096 @@ -41,6 +40,9 @@ #include "qemu/sockets.h" #include "sysemu/kvm.h" #include "qemu/bitops.h" +#include "exec/gdbstub.h" + +static bool gdb_attached = true; #ifndef TARGET_CPU_MEMORY_RW_DEBUG static inline int target_memory_rw_debug(CPUArchState *env, target_ulong addr, @@ -2491,6 +2493,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) break; } #endif + if (strncmp(p, "Attached", 8) == 0) { + put_packet(s, gdb_attached ? "1" : "0"); + break; + } /* Unrecognised 'q' command. */ goto unknown_command; @@ -3055,3 +3061,8 @@ int gdbserver_start(const char *device) return 0; } #endif + +void gdb_set_attached(bool attached) +{ + gdb_attached = attached; +} diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h index ba20afa..b2d3b13 100644 --- a/include/exec/gdbstub.h +++ b/include/exec/gdbstub.h @@ -50,4 +50,6 @@ int gdbserver_start(const char *port); /* in gdbstub-xml.c, generated by scripts/feature_to_c.sh */ extern const char *const xml_builtin[][2]; +void gdb_set_attached(bool attached); + #endif diff --git a/qemu-options.hx b/qemu-options.hx index 6f9334a..026d3eb 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -2988,6 +2988,13 @@ property must be set. These objects are placed in the '/objects' path. ETEXI +DEF("gdb-not-attached", 0, QEMU_OPTION_gdb_not_attached, + "-gdb-not-attached\n" + " Do not set Gdb remote server in attached mode.\n" + " When exiting debugging session, Gdb will send a 'kill'\n" + " command instead of a 'detach'.\n", + QEMU_ARCH_ALL) + HXCOMM This is the last statement. Insert new options before this line! STEXI @end table diff --git a/vl.c b/vl.c index c03edf1..ccd7405 100644 --- a/vl.c +++ b/vl.c @@ -3815,6 +3815,9 @@ int main(int argc, char **argv, char **envp) exit(1); } break; + case QEMU_OPTION_gdb_not_attached: + gdb_set_attached(false); + break; default: os_parse_cmd_args(popt->index, optarg); }
With this patch GDB will issue a "detach" command at the end of a debugging session instead of a "kill". This behavior can be inverted with the new option -gdb-not-attached. This patch implements the requirement described in Jan Kiszka's patch: "gdbstub: Do not kill target in system emulation mode". The patch can therefore be reverted. Signed-off-by: Fabien Chouteau <chouteau@adacore.com> --- gdbstub.c | 13 ++++++++++++- include/exec/gdbstub.h | 2 ++ qemu-options.hx | 7 +++++++ vl.c | 3 +++ 4 files changed, 24 insertions(+), 1 deletion(-)