[v2] Fix several "set remote foo-packet on/off" commands.
Commit Message
Hi Joel,
Bummer, sorry for the trouble.
On 04/28/2014 08:16 PM, Joel Brobecker wrote:
> I worked around the issue by making this package a configurable
> packet (diff attached) but I have a feeling that there is either
> something I am not getting, or perhaps a hole in the current
> design. Or perhaps something else? I am not quite clear on what
> should be done, yet.
I think the design is sound. See more info in the patch below.
I'd be fine with either:
- restoring things to how they've "always" been immediately.
That is, push the patch below. We can then incrementally add the
missing associated commands, along with corresponding manual and
possibly testsuite changes/additions, as a non-priority task.
- or, adding all the missing commands now, and add an assertion just
like in the patch below, but with no exception list, of course.
(but TBC, I can't offer to work on that myself now.)
Let me know what you think.
-------------
>From 37a80ecbfd5cab03a3a88f73d7d06bc6a4f757b9 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 29 Apr 2014 01:14:22 +0100
Subject: [PATCH] Fix remote connection to targets that don't support the
QNonStop packet.
... and others. The recent patch that fixed several "set remote
foo-packet on/off" commands introduced a regression, observable when
connecting GDB to QEMU. For instance:
(gdb) set debug remote 1
(gdb) tar rem :4444
Remote debugging using :4444
Sending packet: $qSupported:multiprocess+;qRelocInsn+#2a...Ack
Packet received: PacketSize=1000;qXfer:features:read+
Packet qSupported (supported-packets) is supported
Sending packet: $Hgp0.0#ad...Ack
Packet received: OK
Sending packet: $qXfer:features:read:target.xml:0,ffb#79...Ack
Packet received: [...]
Sending packet: $qXfer:features:read:arm-core.xml:0,ffb#08...Ack
Packet received: [...]
!!! -> Sending packet: $QNonStop:0#8c...Ack
Packet received:
Remote refused setting all-stop mode with:
The "QNonStop" feature is associated with the PACKET_QNonStop packet,
with a default of PACKET_DISABLE, so GDB should not be sending the
packet at all.
The patch that introduced the regression decoupled packet_config's
'detect' and 'support' fields, making the former (an auto_boolean)
purely the associated "set remote foo-packet" command's variable. In
the example above, the packet config's 'supported' field does end up
correctly set to PACKET_DISABLE. However, nothing is presently
initializing packet configs that don't actually have a command
associated. Those configs's 'detect' field then ends up set to
AUTO_BOOLEAN_TRUE, simply because that happens to be 0. This forces
GDB to assume the packet is supported, irrespective of what the target
claims it supports, just like if the user had done "set remote
foo-packet on" (this being the associated command, if there was one).
Ideally, all packet configs would have a command associated. While
that isn't true, make sure all packet configs are initialized, even if
no command is associated, and add an assertion that prevents adding
more packets/features without an associated command.
Tested on x86_64 Fedora 17, against pristine gdbserver, and against a
gdbserver with the QNonStop packet/feature disabled with a local hack.
gdb/
2014-04-29 Pedro Alves <palves@redhat.com>
* remote.c (struct packet_config) <detect>: Extend comment.
(add_packet_config_cmd): Don't set the config's detect or support
fields here.
(init_all_packet_configs): Also initialize the config's 'detect'
field.
(reset_all_packet_configs_support): New function.
(remote_open_1): Call reset_all_packet_configs_support instead of
init_all_packet_configs.
(_initialize_remote): Initialize all packet configs. Assert that
all packets have an associated command, except a few known
outliers.
---
gdb/remote.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 54 insertions(+), 6 deletions(-)
Comments
On 04/29/14 09:01, Pedro Alves wrote:
> Hi Joel,
>
> Bummer, sorry for the trouble.
>
> On 04/28/2014 08:16 PM, Joel Brobecker wrote:
>
>> I worked around the issue by making this package a configurable
>> packet (diff attached) but I have a feeling that there is either
>> something I am not getting, or perhaps a hole in the current
>> design. Or perhaps something else? I am not quite clear on what
>> should be done, yet.
>
> I think the design is sound. See more info in the patch below.
>
> I'd be fine with either:
>
> - restoring things to how they've "always" been immediately.
> That is, push the patch below. We can then incrementally add the
> missing associated commands, along with corresponding manual and
> possibly testsuite changes/additions, as a non-priority task.
This patch fixed my issue with qemu that donn't support qnonstop.
Thanks,
Hui
>
> - or, adding all the missing commands now, and add an assertion just
> like in the patch below, but with no exception list, of course.
> (but TBC, I can't offer to work on that myself now.)
>
> Let me know what you think.
>
> -------------
> From 37a80ecbfd5cab03a3a88f73d7d06bc6a4f757b9 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Tue, 29 Apr 2014 01:14:22 +0100
> Subject: [PATCH] Fix remote connection to targets that don't support the
> QNonStop packet.
>
> ... and others. The recent patch that fixed several "set remote
> foo-packet on/off" commands introduced a regression, observable when
> connecting GDB to QEMU. For instance:
>
> (gdb) set debug remote 1
> (gdb) tar rem :4444
> Remote debugging using :4444
> Sending packet: $qSupported:multiprocess+;qRelocInsn+#2a...Ack
> Packet received: PacketSize=1000;qXfer:features:read+
> Packet qSupported (supported-packets) is supported
> Sending packet: $Hgp0.0#ad...Ack
> Packet received: OK
> Sending packet: $qXfer:features:read:target.xml:0,ffb#79...Ack
> Packet received: [...]
> Sending packet: $qXfer:features:read:arm-core.xml:0,ffb#08...Ack
> Packet received: [...]
> !!! -> Sending packet: $QNonStop:0#8c...Ack
> Packet received:
> Remote refused setting all-stop mode with:
>
> The "QNonStop" feature is associated with the PACKET_QNonStop packet,
> with a default of PACKET_DISABLE, so GDB should not be sending the
> packet at all.
>
> The patch that introduced the regression decoupled packet_config's
> 'detect' and 'support' fields, making the former (an auto_boolean)
> purely the associated "set remote foo-packet" command's variable. In
> the example above, the packet config's 'supported' field does end up
> correctly set to PACKET_DISABLE. However, nothing is presently
> initializing packet configs that don't actually have a command
> associated. Those configs's 'detect' field then ends up set to
> AUTO_BOOLEAN_TRUE, simply because that happens to be 0. This forces
> GDB to assume the packet is supported, irrespective of what the target
> claims it supports, just like if the user had done "set remote
> foo-packet on" (this being the associated command, if there was one).
>
> Ideally, all packet configs would have a command associated. While
> that isn't true, make sure all packet configs are initialized, even if
> no command is associated, and add an assertion that prevents adding
> more packets/features without an associated command.
>
> Tested on x86_64 Fedora 17, against pristine gdbserver, and against a
> gdbserver with the QNonStop packet/feature disabled with a local hack.
>
> gdb/
> 2014-04-29 Pedro Alves <palves@redhat.com>
>
> * remote.c (struct packet_config) <detect>: Extend comment.
> (add_packet_config_cmd): Don't set the config's detect or support
> fields here.
> (init_all_packet_configs): Also initialize the config's 'detect'
> field.
> (reset_all_packet_configs_support): New function.
> (remote_open_1): Call reset_all_packet_configs_support instead of
> init_all_packet_configs.
> (_initialize_remote): Initialize all packet configs. Assert that
> all packets have an associated command, except a few known
> outliers.
> ---
> gdb/remote.c | 60
++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 54 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index dcd3cdd..4177b39 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -1067,7 +1067,8 @@ struct packet_config
> /* If auto, GDB auto-detects support for this packet or feature,
> either through qSupported, or by trying the packet and looking
> at the response. If true, GDB assumes the target supports this
> - packet. If false, the packet is disabled. */
> + packet. If false, the packet is disabled. Configs that don't
> + have an associated command always have this set to auto. */
> enum auto_boolean detect;
>
> /* Does the target support this packet? */
> @@ -1129,8 +1130,6 @@ add_packet_config_cmd (struct packet_config
*config, const char *name,
>
> config->name = name;
> config->title = title;
> - config->detect = AUTO_BOOLEAN_AUTO;
> - config->support = PACKET_SUPPORT_UNKNOWN;
> set_doc = xstrprintf ("Set use of remote protocol `%s' (%s) packet",
> name, title);
> show_doc = xstrprintf ("Show current use of remote "
> @@ -3632,10 +3631,11 @@ extended_remote_open (char *name, int from_tty)
> remote_open_1 (name, from_tty, &extended_remote_ops, 1
/*extended_p */);
> }
>
> -/* Generic code for opening a connection to a remote target. */
> +/* Reset all packets back to "unknown support". Called when opening a
> + new connection to a remote target. */
>
> static void
> -init_all_packet_configs (void)
> +reset_all_packet_configs_support (void)
> {
> int i;
>
> @@ -3643,6 +3643,20 @@ init_all_packet_configs (void)
> remote_protocol_packets[i].support = PACKET_SUPPORT_UNKNOWN;
> }
>
> +/* Initialize all packet configs. */
> +
> +static void
> +init_all_packet_configs (void)
> +{
> + int i;
> +
> + for (i = 0; i < PACKET_MAX; i++)
> + {
> + remote_protocol_packets[i].detect = AUTO_BOOLEAN_AUTO;
> + remote_protocol_packets[i].support = PACKET_SUPPORT_UNKNOWN;
> + }
> +}
> +
> /* Symbol look-up. */
>
> static void
> @@ -4192,7 +4206,7 @@ remote_open_1 (char *name, int from_tty,
>
> /* Reset the target state; these things will be queried either by
> remote_query_supported or as they are needed. */
> - init_all_packet_configs ();
> + reset_all_packet_configs_support ();
> rs->cached_wait_status = 0;
> rs->explicit_packet_size = 0;
> rs->noack_mode = 0;
> @@ -11869,6 +11883,8 @@ Show the maximum size of the address (in
bits) in a memory packet."), NULL,
> NULL, /* FIXME: i18n: */
> &setlist, &showlist);
>
> + init_all_packet_configs ();
> +
> add_packet_config_cmd (&remote_protocol_packets[PACKET_X],
> "X", "binary-download", 1);
>
> @@ -12052,6 +12068,38 @@ Show the maximum size of the address (in
bits) in a memory packet."), NULL,
> add_packet_config_cmd (&remote_protocol_packets[PACKET_qXfer_btrace],
> "qXfer:btrace", "read-btrace", 0);
>
> + /* Assert that we've registered commands for all packet configs. */
> + {
> + int i;
> +
> + for (i = 0; i < PACKET_MAX; i++)
> + {
> + /* Ideally all configs would have a command associated. Some
> + still don't though. */
> + int excepted;
> +
> + switch (i)
> + {
> + case PACKET_QNonStop:
> + case PACKET_multiprocess_feature:
> + case PACKET_EnableDisableTracepoints_feature:
> + case PACKET_tracenz_feature:
> + case PACKET_DisconnectedTracing_feature:
> + case PACKET_augmented_libraries_svr4_read_feature:
> + /* Additions to this list need to be well justified. */
> + excepted = 1;
> + break;
> + default:
> + excepted = 0;
> + break;
> + }
> +
> + /* This catches both forgetting to add a config command, and
> + forgetting to remove a packet from the exception list. */
> + gdb_assert (excepted == (remote_protocol_packets[i].name == NULL));
> + }
> + }
> +
> /* Keep the old ``set remote Z-packet ...'' working. Each individual
> Z sub-packet has its own set and show commands, but users may
> have sets to this variable in their .gdbinit files (or in their
Hi Pedro,
> Bummer, sorry for the trouble.
No problem at all!
> I think the design is sound. See more info in the patch below.
>
> I'd be fine with either:
>
> - restoring things to how they've "always" been immediately.
> That is, push the patch below. We can then incrementally add the
> missing associated commands, along with corresponding manual and
> possibly testsuite changes/additions, as a non-priority task.
>
> - or, adding all the missing commands now, and add an assertion just
> like in the patch below, but with no exception list, of course.
> (but TBC, I can't offer to work on that myself now.)
Either approach would be fine with me too. I could even see a two-step
approach where we apply your first patch as a stop-gap, and then
implement everything as a setting (I think having the setting for
every packet could prove useful to interact with difficult remote
stubs).
Thanks!
On 04/29/2014 01:53 PM, Joel Brobecker wrote:
> Hi Pedro,
>
>> Bummer, sorry for the trouble.
>
> No problem at all!
>
>> I think the design is sound. See more info in the patch below.
>>
>> I'd be fine with either:
>>
>> - restoring things to how they've "always" been immediately.
>> That is, push the patch below. We can then incrementally add the
>> missing associated commands, along with corresponding manual and
>> possibly testsuite changes/additions, as a non-priority task.
>>
>> - or, adding all the missing commands now, and add an assertion just
>> like in the patch below, but with no exception list, of course.
>> (but TBC, I can't offer to work on that myself now.)
>
> Either approach would be fine with me too. I could even see a two-step
> approach where we apply your first patch as a stop-gap, and then
> implement everything as a setting (I think having the setting for
> every packet could prove useful to interact with difficult remote
> stubs).
Alright, so I just went ahead and pushed the patch in.
Thanks,
> >> I'd be fine with either:
> >>
> >> - restoring things to how they've "always" been immediately.
> >> That is, push the patch below. We can then incrementally add the
> >> missing associated commands, along with corresponding manual and
> >> possibly testsuite changes/additions, as a non-priority task.
> >>
> >> - or, adding all the missing commands now, and add an assertion just
> >> like in the patch below, but with no exception list, of course.
> >> (but TBC, I can't offer to work on that myself now.)
> >
> > Either approach would be fine with me too. I could even see a two-step
> > approach where we apply your first patch as a stop-gap, and then
> > implement everything as a setting (I think having the setting for
> > every packet could prove useful to interact with difficult remote
> > stubs).
>
> Alright, so I just went ahead and pushed the patch in.
Thanks, Pedro!
@@ -1067,7 +1067,8 @@ struct packet_config
/* If auto, GDB auto-detects support for this packet or feature,
either through qSupported, or by trying the packet and looking
at the response. If true, GDB assumes the target supports this
- packet. If false, the packet is disabled. */
+ packet. If false, the packet is disabled. Configs that don't
+ have an associated command always have this set to auto. */
enum auto_boolean detect;
/* Does the target support this packet? */
@@ -1129,8 +1130,6 @@ add_packet_config_cmd (struct packet_config *config, const char *name,
config->name = name;
config->title = title;
- config->detect = AUTO_BOOLEAN_AUTO;
- config->support = PACKET_SUPPORT_UNKNOWN;
set_doc = xstrprintf ("Set use of remote protocol `%s' (%s) packet",
name, title);
show_doc = xstrprintf ("Show current use of remote "
@@ -3632,10 +3631,11 @@ extended_remote_open (char *name, int from_tty)
remote_open_1 (name, from_tty, &extended_remote_ops, 1 /*extended_p */);
}
-/* Generic code for opening a connection to a remote target. */
+/* Reset all packets back to "unknown support". Called when opening a
+ new connection to a remote target. */
static void
-init_all_packet_configs (void)
+reset_all_packet_configs_support (void)
{
int i;
@@ -3643,6 +3643,20 @@ init_all_packet_configs (void)
remote_protocol_packets[i].support = PACKET_SUPPORT_UNKNOWN;
}
+/* Initialize all packet configs. */
+
+static void
+init_all_packet_configs (void)
+{
+ int i;
+
+ for (i = 0; i < PACKET_MAX; i++)
+ {
+ remote_protocol_packets[i].detect = AUTO_BOOLEAN_AUTO;
+ remote_protocol_packets[i].support = PACKET_SUPPORT_UNKNOWN;
+ }
+}
+
/* Symbol look-up. */
static void
@@ -4192,7 +4206,7 @@ remote_open_1 (char *name, int from_tty,
/* Reset the target state; these things will be queried either by
remote_query_supported or as they are needed. */
- init_all_packet_configs ();
+ reset_all_packet_configs_support ();
rs->cached_wait_status = 0;
rs->explicit_packet_size = 0;
rs->noack_mode = 0;
@@ -11869,6 +11883,8 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
NULL, /* FIXME: i18n: */
&setlist, &showlist);
+ init_all_packet_configs ();
+
add_packet_config_cmd (&remote_protocol_packets[PACKET_X],
"X", "binary-download", 1);
@@ -12052,6 +12068,38 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
add_packet_config_cmd (&remote_protocol_packets[PACKET_qXfer_btrace],
"qXfer:btrace", "read-btrace", 0);
+ /* Assert that we've registered commands for all packet configs. */
+ {
+ int i;
+
+ for (i = 0; i < PACKET_MAX; i++)
+ {
+ /* Ideally all configs would have a command associated. Some
+ still don't though. */
+ int excepted;
+
+ switch (i)
+ {
+ case PACKET_QNonStop:
+ case PACKET_multiprocess_feature:
+ case PACKET_EnableDisableTracepoints_feature:
+ case PACKET_tracenz_feature:
+ case PACKET_DisconnectedTracing_feature:
+ case PACKET_augmented_libraries_svr4_read_feature:
+ /* Additions to this list need to be well justified. */
+ excepted = 1;
+ break;
+ default:
+ excepted = 0;
+ break;
+ }
+
+ /* This catches both forgetting to add a config command, and
+ forgetting to remove a packet from the exception list. */
+ gdb_assert (excepted == (remote_protocol_packets[i].name == NULL));
+ }
+ }
+
/* Keep the old ``set remote Z-packet ...'' working. Each individual
Z sub-packet has its own set and show commands, but users may
have sets to this variable in their .gdbinit files (or in their