Re: [PATCH v2] Fix several "set remote foo-packet on/off" commands.

Hui Zhu
2014-04-29T02:10:16Z

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