Re: RFC: Remote "qSupported" features probe

Daniel Jacobowitz
2006-06-20T20:51:09Z

On Tue, Jun 13, 2006 at 02:43:13PM -0400, Daniel Jacobowitz wrote:
> On Tue, Jun 13, 2006 at 09:41:19PM +0300, Eli Zaretskii wrote:
> > > Date: Tue, 13 Jun 2006 09:07:28 -0400
> > > From: Daniel Jacobowitz <drow@false.org>
> > > Cc: gdb-patches@sourceware.org
> > >
> > > > Can you clarify something for me? If a stub recieves a 'qSupported:foo'
> > > > packet, should it still respond with any default information it might have
> > > > -- such as the PacketSize response? Or should such default responses only
> > > > be sent for an unadorned qSupported packet?
> > >
> > > qSupported:foo means "I'm GDB, I support the "foo" feature, what do you
> > > support?". The stub's response should be independent of the arguments
> > > in the packet.
> >
> > Daniel, if Nathan didn't understand this from the text, it's a clear
> > sign that something like the above should be added to make things more
> > self-explanatory.
>
> Yes, I'll plan to add a note about this and repost; I'm just not going
> to do it today.

Like so. Nathan, Eli, does this look fine? If so, I plan to commit
it.

--
Daniel Jacobowitz
CodeSourcery

2006-06-12 Daniel Jacobowitz <dan@codesourcery.com>

* NEWS: Mention qSupported.
* remote.c (struct remote_state): Add explicit_packet_size.
(get_remote_packet_size): Check explicit_packet_size.
(get_memory_packet_size): Likewise.
(PACKET_qSupported): New enum value.
(struct protocol_feature, remote_supported_packet)
(remote_packet_size, remote_protocol_features)
(remote_query_supported): New.
(remote_open_1): Reset explicit_packet_size. Call
remote_query_supported.
(_initialize_remote): Register qSupported.

2006-06-12 Daniel Jacobowitz <dan@codesourcery.com>

* gdb.texinfo (Remote configuration): Document set / show
remote supported-packets.
(General Query Packets): Document qSupported packet.

2006-06-12 Daniel Jacobowitz <dan@codesourcery.com>

* server.c (handle_query): Handle qSupported.

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c 2006-06-20 16:33:25.000000000 -0400
+++ src/gdb/remote.c 2006-06-20 16:34:53.000000000 -0400
@@ -215,6 +215,12 @@ struct remote_state
packets. */
char *buf;
long buf_size;
+
+ /* If we negotiated packet size explicitly (and thus can bypass
+ heuristics for the largest packet size that will not overflow
+ a buffer in the stub), this will be set to that packet size.
+ Otherwise zero, meaning to use the guessed size. */
+ long explicit_packet_size;
};

/* This data could be associated with a target, but we do not always
@@ -340,8 +346,12 @@ init_remote_state (struct gdbarch *gdbar
static long
get_remote_packet_size (void)
{
+ struct remote_state *rs = get_remote_state ();
struct remote_arch_state *rsa = get_remote_arch_state ();

+ if (rs->explicit_packet_size)
+ return rs->explicit_packet_size;
+
return rsa->remote_packet_size;
}

@@ -484,10 +494,13 @@ get_memory_packet_size (struct memory_pa
if (config->size > 0
&& what_they_get > config->size)
what_they_get = config->size;
- /* Limit it to the size of the targets ``g'' response. */
- if ((rsa->actual_register_packet_size) > 0
- && what_they_get > (rsa->actual_register_packet_size))
- what_they_get = (rsa->actual_register_packet_size);
+
+ /* Limit it to the size of the targets ``g'' response unless we have
+ permission from the stub to use a larger packet size. */
+ if (rs->explicit_packet_size == 0
+ && rsa->actual_register_packet_size > 0
+ && what_they_get > rsa->actual_register_packet_size)
+ what_they_get = rsa->actual_register_packet_size;
}
if (what_they_get > MAX_REMOTE_PACKET_SIZE)
what_they_get = MAX_REMOTE_PACKET_SIZE;
@@ -802,6 +815,7 @@ enum {
PACKET_Z4,
PACKET_qPart_auxv,
PACKET_qGetTLSAddr,
+ PACKET_qSupported,
PACKET_MAX
};

@@ -2065,6 +2079,222 @@ Some events may be lost, rendering furth
return serial_open (name);
}

+/* This type describes each known response to the qSupported
+ packet. */
+struct protocol_feature
+{
+ /* The name of this protocol feature. */
+ const char *name;
+
+ /* The default for this protocol feature. */
+ enum packet_support default_support;
+
+ /* The function to call when this feature is reported, or after
+ qSupported processing if the feature is not supported.
+ The first argument points to this structure. The second
+ argument indicates whether the packet requested support be
+ enabled, disabled, or probed (or the default, if this function
+ is being called at the end of processing and this feature was
+ not reported). The third argument may be NULL; if not NULL, it
+ is a NUL-terminated string taken from the packet following
+ this feature's name and an equals sign. */
+ void (*func) (const struct protocol_feature *, enum packet_support,
+ const char *);
+
+ /* The corresponding packet for this feature. Only used if
+ FUNC is remote_supported_packet. */
+ int packet;
+};
+
+#if 0
+static void
+remote_supported_packet (const struct protocol_feature *feature,
+ enum packet_support support,
+ const char *argument)
+{
+ if (argument)
+ {
+ warning (_("Remote qSupported response supplied an unexpected value for"
+ " \"%s\"."), feature->name);
+ return;
+ }
+
+ if (remote_protocol_packets[feature->packet].support
+ == PACKET_SUPPORT_UNKNOWN)
+ remote_protocol_packets[feature->packet].support = support;
+}
+#endif
+
+static void
+remote_packet_size (const struct protocol_feature *feature,
+ enum packet_support support, const char *value)
+{
+ struct remote_state *rs = get_remote_state ();
+
+ int packet_size;
+ char *value_end;
+
+ if (support != PACKET_ENABLE)
+ return;
+
+ if (value == NULL || *value == '\0')
+ {
+ warning (_("Remote target reported \"%s\" without a size."),
+ feature->name);
+ return;
+ }
+
+ errno = 0;
+ packet_size = strtol (value, &value_end, 16);
+ if (errno != 0 || *value_end != '\0' || packet_size < 0)
+ {
+ warning (_("Remote target reported \"%s\" with a bad size: \"%s\"."),
+ feature->name, value);
+ return;
+ }
+
+ if (packet_size > MAX_REMOTE_PACKET_SIZE)
+ {
+ warning (_("limiting remote suggested packet size (%d bytes) to %d"),
+ packet_size, MAX_REMOTE_PACKET_SIZE);
+ packet_size = MAX_REMOTE_PACKET_SIZE;
+ }
+
+ /* Record the new maximum packet size. */
+ rs->explicit_packet_size = packet_size;
+}
+
+static struct protocol_feature remote_protocol_features[] = {
+ { "PacketSize", PACKET_DISABLE, remote_packet_size, -1 }
+};
+
+static void
+remote_query_supported (void)
+{
+ struct remote_state *rs = get_remote_state ();
+ char *next;
+ int i;
+ unsigned char seen [ARRAY_SIZE (remote_protocol_features)];
+
+ /* The packet support flags are handled differently for this packet
+ than for most others. We treat an error, a disabled packet, and
+ an empty response identically: any features which must be reported
+ to be used will be automatically disabled. An empty buffer
+ accomplishes this, since that is also the representation for a list
+ containing no features. */
+
+ rs->buf[0] = 0;
+ if (remote_protocol_packets[PACKET_qSupported].support != PACKET_DISABLE)
+ {
+ putpkt ("qSupported");
+ getpkt (&rs->buf, &rs->buf_size, 0);
+
+ /* If an error occured, warn, but do not return - just reset the
+ buffer to empty and go on to disable features. */
+ if (packet_ok (rs->buf, &remote_protocol_packets[PACKET_qSupported])
+ == PACKET_ERROR)
+ {
+ warning (_("Remote failure reply: %s"), rs->buf);
+ rs->buf[0] = 0;
+ }
+ }
+
+ memset (seen, 0, sizeof (seen));
+
+ next = rs->buf;
+ while (*next)
+ {
+ enum packet_support is_supported;
+ char *p, *end, *name_end, *value;
+
+ /* First separate out this item from the rest of the packet. If
+ there's another item after this, we overwrite the separator
+ (terminated strings are much easier to work with). */
+ p = next;
+ end = strchr (p, ';');
+ if (end == NULL)
+ {
+ end = p + strlen (p);
+ next = end;
+ }
+ else
+ {
+ if (end == p)
+ {
+ warning (_("empty item in \"qSupported\" response"));
+ continue;
+ }
+
+ *end = '\0';
+ next = end + 1;
+ }
+
+ name_end = strchr (p, '=');
+ if (name_end)
+ {
+ /* This is a name=value entry. */
+ is_supported = PACKET_ENABLE;
+ value = name_end + 1;
+ *name_end = '\0';
+ }
+ else
+ {
+ value = NULL;
+ switch (end[-1])
+ {
+ case '+':
+ is_supported = PACKET_ENABLE;
+ break;
+
+ case '-':
+ is_supported = PACKET_DISABLE;
+ break;
+
+ case '?':
+ is_supported = PACKET_SUPPORT_UNKNOWN;
+ break;
+
+ default:
+ warning (_("unrecognized item \"%s\" in \"qSupported\" response"), p);
+ continue;
+ }
+ end[-1] = '\0';
+ }
+
+ for (i = 0; i < ARRAY_SIZE (remote_protocol_features); i++)
+ if (strcmp (remote_protocol_features[i].name, p) == 0)
+ {
+ const struct protocol_feature *feature;
+
+ seen[i] = 1;
+ feature = &remote_protocol_features[i];
+ feature->func (feature, is_supported, value);
+ break;
+ }
+ }
+
+ /* If we increased the packet size, make sure to increase the global
+ buffer size also. We delay this until after parsing the entire
+ qSupported packet, because this is the same buffer we were
+ parsing. */
+ if (rs->buf_size < rs->explicit_packet_size)
+ {
+ rs->buf_size = rs->explicit_packet_size;
+ rs->buf = xrealloc (rs->buf, rs->buf_size);
+ }
+
+ /* 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);
+ }
+}
+
+
static void
remote_open_1 (char *name, int from_tty, struct target_ops *target,
int extended_p, int async_p)
@@ -2119,7 +2349,10 @@ remote_open_1 (char *name, int from_tty,
}
push_target (target); /* Switch to using remote target now. */

+ /* Reset the target state; these things will be queried either by
+ remote_query_supported or as they are needed. */
init_all_packet_configs ();
+ rs->explicit_packet_size = 0;

general_thread = -2;
continue_thread = -2;
@@ -2128,6 +2361,11 @@ remote_open_1 (char *name, int from_tty,
use_threadinfo_query = 1;
use_threadextra_query = 1;

+ /* The first packet we send to the target is the optional "supported
+ packets" request. If the target can answer this, it will tell us
+ which later probes to skip. */
+ remote_query_supported ();
+
/* Without this, some commands which require an active target (such
as kill) won't work. This variable serves (at least) double duty
as both the pid of the target process (if it has such), and as a
@@ -5675,6 +5913,9 @@ Show the maximum size of the address (in
"qGetTLSAddr", "get-thread-local-storage-address",
0);

+ add_packet_config_cmd (&remote_protocol_packets[PACKET_qSupported],
+ "qSupported", "supported-packets", 0);
+
/* 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
Index: src/gdb/doc/gdb.texinfo
===================================================================
--- src.orig/gdb/doc/gdb.texinfo 2006-06-20 16:33:26.000000000 -0400
+++ src/gdb/doc/gdb.texinfo 2006-06-20 16:45:36.000000000 -0400
@@ -12782,6 +12782,17 @@ packet.
@item show remote get-thread-local-storage-address
@kindex show remote get-thread-local-storage-address
Show the current setting of @samp{qGetTLSAddr} packet usage.
+
+@item set remote supported-packets
+@kindex set remote supported-packets
+@cindex query supported packets of remote targets
+This command enables or disables the use of the @samp{qSupported}
+request packet. @xref{General Query Packets, qSupported}, for more
+details about this packet. The default is to use @samp{qSupported}.
+
+@item show remote supported-packets
+@kindex show remote supported-packets
+Show the current setting of @samp{qSupported} packet usage.
@end table

@node remote stub
@@ -23623,6 +23634,129 @@ command by a @samp{,}, not a @samp{:}, c
conventions above. Please don't use this packet as a model for new
packets.)

+@item qSupported @r{[}:@var{gdbfeature} @r{[};@var{gdbfeature}@r{]}@dots{} @r{]}
+@cindex supported packets, remote query
+@cindex features of the remote protocol
+@cindex @samp{qSupported} packet
+Tell the remote stub about features supported by @value{GDBN}, and
+query the stub for features it supports. This packet allows
+@value{GDBN} and the remote stub to take advantage of each others'
+features. @samp{qSupported} also consolidates multiple feature probes
+at startup, to improve @value{GDBN} performance---a single larger
+packet performs better than multiple smaller probe packets on
+high-latency links. Some features may enable behavior which must not
+be on by default, e.g.@: because it would confuse older clients or
+stubs. Other features may describe packets which could be
+automatically probed for, but are not. These features must be
+reported before @value{GDBN} will use them. This ``default
+unsupported'' behavior is not appropriate for all packets, but it
+helps to keep the initial connection time under control with new
+versions of @value{GDBN} which support increasing numbers of packets.
+
+Reply:
+@table @samp
+@item @var{stubfeature} @r{[};@var{stubfeature}@r{]}@dots{}
+The stub supports or does not support each returned @var{stubfeature},
+depending on the form of each @var{stubfeature} (see below for the
+possible forms).
+@item
+An empty reply indicates that @samp{qSupported} is not recognized,
+or that no features needed to be reported to @value{GDBN}.
+@end table
+
+The allowed forms for each feature (either a @var{gdbfeature} in the
+@samp{qSupported} packet, or a @var{stubfeature} in the response)
+are:
+
+@table @samp
+@item @var{name}=@var{value}
+The remote protocol feature @var{name} is supported, and associated
+with the specified @var{value}. The format of @var{value} depends
+on the feature, but it must not include a semicolon.
+@item @var{name}+
+The remote protocol feature @var{name} is supported, and does not
+need an associated value.
+@item @var{name}-
+The remote protocol feature @var{name} is not supported.
+@item @var{name}?
+The remote protocol feature @var{name} may be supported, and
+@value{GDBN} should auto-detect support in some other way when it is
+needed. This form will not be used for @var{gdbfeature} notifications,
+but may be used for @var{stubfeature} responses.
+@end table
+
+Whenever the stub receives a @samp{qSupported} request, the
+supplied set of @value{GDBN} features should override any previous
+request. This allows @value{GDBN} to put the stub in a known
+state, even if the stub had previously been communicating with
+a different version of @value{GDBN}.
+
+No values of @var{gdbfeature} (for the packet sent by @value{GDBN})
+are defined yet. Stubs should ignore any unknown values for
+@var{gdbfeature}. Any @value{GDBN} which sends a @samp{qSupported}
+packet supports receiving packets of unlimited length (earlier
+versions of @value{GDBN} may reject overly long responses). Values
+for @var{gdbfeature} may be defined in the future to let the stub take
+advantage of new features in @value{GDBN}, e.g.@: incompatible
+improvements in the remote protocol---support for unlimited length
+responses would be a @var{gdbfeature} example, if it were not implied by
+the @samp{qSupported} query. The stub's reply should be independent
+of the @var{gdbfeature} entries sent by @value{GDBN}; first @value{GDBN}
+describes all the features it supports, and then the stub replies with
+all the features it supports.
+
+Similarly, @value{GDBN} will silently ignore unrecognized stub feature
+responses, as long as each response uses one of the standard forms.
+
+Some features are flags. A stub which supports a flag feature
+should respond with a @samp{+} form response. Other features
+require values, and the stub should respond with an @samp{=}
+form response.
+
+Each feature has a default value, which @value{GDBN} will use if
+@samp{qSupported} is not available or if the feature is not mentioned
+in the @samp{qSupported} response. The default values are fixed; a
+stub is free to omit any feature responses that match the defaults.
+
+Not all features can be probed, but for those which can, the probing
+mechanism is useful: in some cases, a stub's internal
+architecture may not allow the protocol layer to know some information
+about the underlying target in advance. This is especially common in
+stubs which may be configured for multiple targets.
+
+These are the currently defined stub features and their properties:
+
+@multitable @columnfractions 0.25 0.2 0.2 0.2
+@c NOTE: The first row should be @headitem, but we do not yet require
+@c a new enough version of Texinfo (4.7) to use @headitem.
+@item Packet Name
+@tab Value Required
+@tab Default
+@tab Probe Allowed
+
+@item @samp{PacketSize}
+@tab Yes
+@tab @samp{-}
+@tab No
+
+@end multitable
+
+These are the currently defined stub features, in more detail:
+
+@table @samp
+@cindex packet size, remote protocol
+@item PacketSize=@var{bytes}
+The remote stub can accept packets up to at least @var{bytes} in
+length. @value{GDBN} will send packets up to this size for bulk
+transfers, and will never send larger packets. This is a limit on the
+data characters in the packet, including the frame and checksum.
+There is no trailing NUL byte in a remote protocol packet; if the stub
+stores packets in a NUL-terminated format, it should allow an extra
+byte in its buffer for the NUL. If this stub feature is not supported,
+@value{GDBN} guesses based on the size of the @samp{g} packet response.
+
+@end table
+
@item qSymbol::
@cindex symbol lookup, remote request
@cindex @samp{qSymbol} packet
Index: src/gdb/gdbserver/server.c
===================================================================
--- src.orig/gdb/gdbserver/server.c 2006-06-20 16:33:07.000000000 -0400
+++ src/gdb/gdbserver/server.c 2006-06-20 16:34:53.000000000 -0400
@@ -163,6 +163,14 @@ handle_query (char *own_buf)
return;
}

+ /* Protocol features query. */
+ if (strncmp ("qSupported", own_buf, 10) == 0
+ && (own_buf[10] == ':' || own_buf[10] == '\0'))
+ {
+ sprintf (own_buf, "PacketSize=%x", PBUFSIZ - 1);
+ return;
+ }
+
/* Otherwise we didn't know what packet it was. Say we didn't
understand it. */
own_buf[0] = 0;
Index: src/gdb/NEWS
===================================================================
--- src.orig/gdb/NEWS 2006-06-20 16:33:07.000000000 -0400
+++ src/gdb/NEWS 2006-06-20 16:34:53.000000000 -0400
@@ -7,6 +7,15 @@

The ARM Demon monitor support (RDP protocol, "target rdp").

+* New remote packets
+
+qSupported:
+ Tell a stub about GDB client features, and request remote target features.
+ The first feature implemented is PacketSize, which allows the target to
+ specify the size of packets it can handle - to minimize the number of
+ packets required and improve performance when connected to a remote
+ target.
+
*** Changes in GDB 6.5

* New targets