[Qemu-devel] [PATCH 1/2] Add GDB qAttached support

[Qemu-devel] [PATCH 1/2] Add GDB qAttached support

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

From: Jan Kiszka <jan.kiszka@siemens.com>

With this patch QEMU handles qAttached request from gdb. When QEMU
replies 1, GDB sends a "detach" command at the end of a debugging
session otherwise GDB sends "kill".

The default value for qAttached is 1 on system emulation and 0 on user
emulation.

Based on original version by Fabien Chouteau.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Long pending in my queue. Hope we can finally get these two in via
trivial (that's what they are).

 gdbstub.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/gdbstub.c b/gdbstub.c
index e4a1a79..da3e7cb 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -41,6 +41,12 @@
 #include "qemu/sockets.h"
 #include "sysemu/kvm.h"
 
+#ifdef CONFIG_USER_ONLY
+#define GDB_ATTACHED "0"
+#else
+#define GDB_ATTACHED "1"
+#endif
+
 static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
                                          uint8_t *buf, int len, bool is_write)
 {
@@ -1187,6 +1193,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
             put_packet_binary(s, buf, len + 1);
             break;
         }
+        if (strncmp(p, "Attached", 8) == 0) {
+            put_packet(s, GDB_ATTACHED);
+            break;
+        }
         /* Unrecognised 'q' command.  */
         goto unknown_command;
 
-- 
2.1.4


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[Qemu-devel] [PATCH 2/2] Revert "gdbstub: Do not kill target in system emulation mode"

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

From: Fabien Chouteau <chouteau@adacore.com>

The requirements described in this patch are implemented by "Add GDB
qAttached support".

This reverts commit 00e94dbc7fd0110b0555d59592b004333adfb4b8.

Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 gdbstub.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index da3e7cb..14e1f0c 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -880,11 +880,9 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
             goto unknown_command;
         }
     case 'k':
-#ifdef CONFIG_USER_ONLY
         /* Kill the target */
         fprintf(stderr, "\nQEMU: Terminated via GDBstub\n");
         exit(0);
-#endif
     case 'D':
         /* Detach packet */
         gdb_breakpoint_remove_all();
-- 
2.1.4


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 1/2] Add GDB qAttached support

[Pedro Alves]

Hi, I was skimming the list, and noticed:

On 01/31/2015 10:28 AM, Jan Kiszka wrote:
> @@ -1187,6 +1193,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>              put_packet_binary(s, buf, len + 1);
>              break;
>          }
> +        if (strncmp(p, "Attached", 8) == 0) {

This looks like it'd mishandle a future qAttached2 packet.

It should be doing something like:

       if (strncmp(p, "Attached", 8) == 0 &&
          (p[8] == '\0' || p[8] == ':')) {

or:

       if (strcmp(p, "Attached") == 0 || strncmp(p, "Attached:", 9) == 0) {


Likewise other packets, if they have the same issue.
(I'm not familiar with qemu's stub's internals.)

Thanks,
Pedro Alves

Re: [Qemu-devel] [PATCH 1/2] Add GDB qAttached support

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

On 2015-02-04 14:36, Pedro Alves wrote:
> Hi, I was skimming the list, and noticed:
> 
> On 01/31/2015 10:28 AM, Jan Kiszka wrote:
>> @@ -1187,6 +1193,10 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>>              put_packet_binary(s, buf, len + 1);
>>              break;
>>          }
>> +        if (strncmp(p, "Attached", 8) == 0) {
> 
> This looks like it'd mishandle a future qAttached2 packet.
> 
> It should be doing something like:
> 
>        if (strncmp(p, "Attached", 8) == 0 &&
>           (p[8] == '\0' || p[8] == ':')) {
> 
> or:
> 
>        if (strcmp(p, "Attached") == 0 || strncmp(p, "Attached:", 9) == 0) {
> 
> 
> Likewise other packets, if they have the same issue.
> (I'm not familiar with qemu's stub's internals.)

Thanks for the remark! Will update the patch using the easier readable
second variant.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH 1/2] Add GDB qAttached support

[Peter Maydell]

On 4 February 2015 at 13:36, Pedro Alves <palves@redhat.com> wrote:
>
> This looks like it'd mishandle a future qAttached2 packet.
>
> It should be doing something like:
>
>        if (strncmp(p, "Attached", 8) == 0 &&
>           (p[8] == '\0' || p[8] == ':')) {
>
> or:
>
>        if (strcmp(p, "Attached") == 0 || strncmp(p, "Attached:", 9) == 0) {
>
>
> Likewise other packets, if they have the same issue.
> (I'm not familiar with qemu's stub's internals.)

Looks like we get this wrong for a lot of our existing
query packet handling too... Maybe worth having a utility
function for "is this a foo query packet" rather than
raw strcmp/strncmp?

-- PMM

Re: [Qemu-devel] [PATCH 1/2] Add GDB qAttached support

[Jan Kiszka]

On 2013-03-12 15:31, Fabien Chouteau wrote:
> With this patch QEMU handles qAttached request from gdb. When QEMU
> replies 1, GDB sends a "detach" command at the end of a debugging
> session otherwise GDB sends "kill".
> 
> The default value for qAttached is 1 on system emulation and 0 on user
> emulation.
> 
> We introduce a new command line option. It's a generic option to
> customize the gdb server:

Again, please factor this out into a separate patch so that qAttached
can be merged independently (and we stop warning the gdb users
incorrectly about killing the target on quit).

> 
> -gdb-opts [attached=on|off]

This will still trigger additional discussions as it's still a new
top-level command line switch.

I think it is possible to merge chardev-unrelated options into the
parameter that -gdb takes and filter them out before handing the rest
over to the chardev layer.

Jan

> 
> The only parameter for now is "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              |   38 +++++++++++++++++++++++++++++++++++++-
>  include/exec/gdbstub.h |    2 ++
>  qemu-options.hx        |   17 +++++++++++++++++
>  vl.c                   |    3 +++
>  4 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index e414ad9..de95849 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,13 @@
>  #include "qemu/sockets.h"
>  #include "sysemu/kvm.h"
>  #include "qemu/bitops.h"
> +#include "exec/gdbstub.h"
> +
> +#ifdef CONFIG_USER_ONLY
> +static bool gdb_attached; /* false */
> +#else
> +static bool gdb_attached = true;
> +#endif
>  
>  #ifndef TARGET_CPU_MEMORY_RW_DEBUG
>  static inline int target_memory_rw_debug(CPUArchState *env, target_ulong addr,
> @@ -2491,6 +2497,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 +3065,29 @@ int gdbserver_start(const char *device)
>      return 0;
>  }
>  #endif
> +
> +static QemuOptsList qemu_gdb_opts = {
> +    .name = "gdb",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_gdb_opts.head),
> +    .desc = {
> +        {
> +            .name = "attached",
> +            .type = QEMU_OPT_BOOL,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
> +void gdb_set_opts(const char *opts_str)
> +{
> +    QemuOpts *opts;
> +
> +    opts = qemu_opts_parse(&qemu_gdb_opts, opts_str, 0);
> +    if (!opts) {
> +        exit(1);
> +    }
> +
> +    gdb_attached = qemu_opt_get_bool(opts, "attached", gdb_attached);
> +
> +    qemu_opts_del(opts);
> +}
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index ba20afa..86fc18b 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_opts(const char *opts_str);
> +
>  #endif
> diff --git a/qemu-options.hx b/qemu-options.hx
> index cd76f2a..49a480f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -2505,6 +2505,23 @@ within gdb and establish the connection via a pipe:
>  @end example
>  ETEXI
>  
> +DEF("gdb-opts", HAS_ARG, QEMU_OPTION_gdb_opts, \
> +    "-gdb-opts attached=on|off\n"\
> +    "                options for the gdb remote server\n", QEMU_ARCH_ALL)
> +STEXI
> +@item -gdb-opts @var{dev}
> +@findex -gdb-opts
> +Set options for the gdb remote server:
> +@table @option
> +@item attached=on|off:
> +
> +When 'on' (default), gdb sends a 'detach' command at the end of debugging
> +session, otherwise gdb sends a 'kill' command.
> +
> +@end table
> +
> +ETEXI
> +
>  DEF("s", 0, QEMU_OPTION_s, \
>      "-s              shorthand for -gdb tcp::" DEFAULT_GDBSTUB_PORT "\n",
>      QEMU_ARCH_ALL)
> diff --git a/vl.c b/vl.c
> index 154f7ba..cce96b6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3251,6 +3251,9 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_gdb:
>                  add_device_config(DEV_GDB, optarg);
>                  break;
> +            case QEMU_OPTION_gdb_opts:
> +                gdb_set_opts(optarg);
> +                break;
>              case QEMU_OPTION_L:
>                  data_dir = optarg;
>                  break;
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 1/2] Add GDB qAttached support

[Fabien Chouteau]

With this patch QEMU handles qAttached request from gdb. When QEMU
replies 1, GDB sends a "detach" command at the end of a debugging
session otherwise GDB sends "kill".

The default value for qAttached is 1 on system emulation and 0 on user
emulation.

We introduce a new command line option. It's a generic option to
customize the gdb server:

-gdb-opts [attached=on|off]

The only parameter for now is "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              |   38 +++++++++++++++++++++++++++++++++++++-
 include/exec/gdbstub.h |    2 ++
 qemu-options.hx        |   17 +++++++++++++++++
 vl.c                   |    3 +++
 4 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/gdbstub.c b/gdbstub.c
index e414ad9..de95849 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,13 @@
 #include "qemu/sockets.h"
 #include "sysemu/kvm.h"
 #include "qemu/bitops.h"
+#include "exec/gdbstub.h"
+
+#ifdef CONFIG_USER_ONLY
+static bool gdb_attached; /* false */
+#else
+static bool gdb_attached = true;
+#endif
 
 #ifndef TARGET_CPU_MEMORY_RW_DEBUG
 static inline int target_memory_rw_debug(CPUArchState *env, target_ulong addr,
@@ -2491,6 +2497,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 +3065,29 @@ int gdbserver_start(const char *device)
     return 0;
 }
 #endif
+
+static QemuOptsList qemu_gdb_opts = {
+    .name = "gdb",
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_gdb_opts.head),
+    .desc = {
+        {
+            .name = "attached",
+            .type = QEMU_OPT_BOOL,
+        },
+        { /* end of list */ }
+    },
+};
+
+void gdb_set_opts(const char *opts_str)
+{
+    QemuOpts *opts;
+
+    opts = qemu_opts_parse(&qemu_gdb_opts, opts_str, 0);
+    if (!opts) {
+        exit(1);
+    }
+
+    gdb_attached = qemu_opt_get_bool(opts, "attached", gdb_attached);
+
+    qemu_opts_del(opts);
+}
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index ba20afa..86fc18b 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_opts(const char *opts_str);
+
 #endif
diff --git a/qemu-options.hx b/qemu-options.hx
index cd76f2a..49a480f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2505,6 +2505,23 @@ within gdb and establish the connection via a pipe:
 @end example
 ETEXI
 
+DEF("gdb-opts", HAS_ARG, QEMU_OPTION_gdb_opts, \
+    "-gdb-opts attached=on|off\n"\
+    "                options for the gdb remote server\n", QEMU_ARCH_ALL)
+STEXI
+@item -gdb-opts @var{dev}
+@findex -gdb-opts
+Set options for the gdb remote server:
+@table @option
+@item attached=on|off:
+
+When 'on' (default), gdb sends a 'detach' command at the end of debugging
+session, otherwise gdb sends a 'kill' command.
+
+@end table
+
+ETEXI
+
 DEF("s", 0, QEMU_OPTION_s, \
     "-s              shorthand for -gdb tcp::" DEFAULT_GDBSTUB_PORT "\n",
     QEMU_ARCH_ALL)
diff --git a/vl.c b/vl.c
index 154f7ba..cce96b6 100644
--- a/vl.c
+++ b/vl.c
@@ -3251,6 +3251,9 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_gdb:
                 add_device_config(DEV_GDB, optarg);
                 break;
+            case QEMU_OPTION_gdb_opts:
+                gdb_set_opts(optarg);
+                break;
             case QEMU_OPTION_L:
                 data_dir = optarg;
                 break;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 1/2] Add GDB qAttached support

[Fabien Chouteau]

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,

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 1/2] Add GDB qAttached support

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1880 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [Qemu-devel] [PATCH 1/2] Add GDB qAttached support

[Fabien Chouteau]

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,

-- 
Fabien Chouteau

Re: [Qemu-devel] [PATCH 1/2] Add GDB qAttached support

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3510 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

[Qemu-devel] [PATCH 1/2] Add GDB qAttached support

[Fabien Chouteau]

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;
+        }
         /* 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);
             }
-- 
1.7.9.5