[1/2] Add GDB qAttached support

Submitted by Fabien Chouteau on March 5, 2013, 4:03 p.m.

Details

Message ID 1362499399-19475-1-git-send-email-chouteau@adacore.com
State New
Headers show

Commit Message

Fabien Chouteau March 5, 2013, 4:03 p.m.
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(-)

Comments

Jan Kiszka March 10, 2013, 8:06 a.m.
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
Fabien Chouteau March 11, 2013, 11:22 a.m.
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,
Jan Kiszka March 11, 2013, 11:36 a.m.
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
Fabien Chouteau March 11, 2013, 11:59 a.m.
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,

Patch hide | download patch | download mbox

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);
             }