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

[Hui Zhu <hui_zhu at mentor dot com>]

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