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

[Joel Brobecker <brobecker@adacore.com>]

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

Hi Pedro,

> gdb/
> 2014-04-01  Pedro Alves  <palves@redhat.com>
> 
> 	* remote.c (struct remote_state): Remove multi_process_aware,
[...]

Unfortunately, your patch seems to be introducing 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: 

What happens, here, it seems, is that GDB thinks it's OK to use
the QNonStop even though it wasn't specified in the qSupported
reply. Looking at the code that sets the feature's status when
not seen in the qSupported reply, we see:

    /* Handle the defaults for unmentioned features.  */
    for (i = 0; i < ARRAY_SIZE (remote_protocol_features); i++)
      if (!seen[i])
        {
          const struct protocol_feature *feature;

          feature = &remote_protocol_features[i];
          feature->func (feature, feature->default_support, NULL);
        }

In the case of PACKET_QNonStop, we have the following definition...

  { "QNonStop", PACKET_DISABLE, remote_supported_packet, PACKET_QNonStop },

... so feature->func is remote_supported_packet, which then sets
the feature as:

    static void
    remote_supported_packet (const struct protocol_feature *feature,
                             enum packet_support support,
                             const char *argument)
    {
      [warning + return if argument not NULL]
      remote_protocol_packets[feature->packet].support = support;
    }

However, looking at the packet_support function, it basically
relies on packet_config_support, which itself only looks at
the support field of our feature if "detect" was set to AUTO_BOOLEAN_AUTO:

    switch (config->detect)
      {
      case AUTO_BOOLEAN_TRUE:
        return PACKET_ENABLE;
      case AUTO_BOOLEAN_FALSE:
        return PACKET_DISABLE;
      case AUTO_BOOLEAN_AUTO:
        return config->support;
      default:
        gdb_assert_not_reached (_("bad switch"));
      }

However, in the case of this option, it's set to AUTO_BOOLEAN_TRUE.
So the config->support value that we set earlier has no effect.

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.

Thanks!
-- 
Joel

[-- Attachment #2: remote.c.diff --]
[-- Type: text/x-diff, Size: 554 bytes --]

diff --git a/gdb/remote.c b/gdb/remote.c
index dcd3cdd..14dd8c9 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -11878,6 +11878,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (&remote_protocol_packets[PACKET_QPassSignals],
 			 "QPassSignals", "pass-signals", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_QNonStop],
+			 "QNonStop", "non-stop", 0);
+
   add_packet_config_cmd (&remote_protocol_packets[PACKET_QProgramSignals],
 			 "QProgramSignals", "program-signals", 0);