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

[Hui Zhu <hui_zhu@mentor.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