This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: PATCH: Add xmlRegisters= to qsupported query
On Monday 29 March 2010 19:01:36, H.J. Lu wrote:
> 2010-03-29 H.J. Lu <hongjiu.lu@intel.com>
>
> * i386-tdep.c: Include "remote.h".
> (i386_gdbarch_init): Call register_remote_support_xml.
>
> * remote.c (remote_support_xml): New.
> (register_remote_support_xml): Likewise.
> (remote_query_supported_append): Likewise.
> (remote_query_supported): Support remote_support_xml.
>
> * remote.h (register_remote_support_xml): New.
>
> gdb/doc/
>
> 2010-03-29 H.J. Lu <hongjiu.lu@intel.com>
>
> * gdb.texinfo (General Query Packets): Add xmlRegisters.
>
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 56dbe5d..15da725 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -30603,6 +30603,13 @@ extensions to the remote protocol. @value{GDBN} does not use such
> extensions unless the stub also reports that it supports them by
> including @samp{multiprocess+} in its @samp{qSupported} reply.
> @xref{multiprocess extensions}, for details.
> +
> +@item xmlRegisters
> +This feature indicates that @value{GDBN} supports the XML target
> +description. If the stub sees @samp{xmlRegisters=} with target
> +specific strings separated by a comma, it will send either an XML
> +target description or an XML stub that states only the architecture
> +and the OSABI.
> @end table
There's some confusion here. A target description can
describe more than the target's register set. If the
stub sees the xmlRegisters feature with a value included
its architecture, it may safely include the register
description in the target description, as GDB will
understand it. I don't think it's a good idea to
say here what _other_ things the stub will send. Why not
explain the problem we're solving there? You could even
copy Daniel's nice problem statement somewhere upthread.
I think this should also be mentioned in NEWS. We document
every new RSP packet and qSupported feature there.
>
> Stubs should ignore any unknown values for
> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 83275ac..0199026 100644
> --- a/gdb/i386-tdep.c
> +++ b/gdb/i386-tdep.c
> @@ -44,6 +44,7 @@
> #include "value.h"
> #include "dis-asm.h"
> #include "disasm.h"
> +#include "remote.h"
>
> #include "gdb_assert.h"
> #include "gdb_string.h"
> @@ -5943,6 +5944,9 @@ i386_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> set_gdbarch_fast_tracepoint_valid_at (gdbarch,
> i386_fast_tracepoint_valid_at);
>
> + /* Tell remote stub that we support XML target description. */
> + register_remote_support_xml ("i386");
> +
> return gdbarch;
> }
Daniel suggested doing this from _initialize_i386_tdep,
IIRC. Did that not work? That would make this new
registration function only called once, instead of once
for every i386 gdbarch.
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 0c791aa..25249d1 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -3464,6 +3464,57 @@ static struct protocol_feature remote_protocol_features[] = {
> PACKET_bs },
> };
>
> +static char *remote_support_xml;
> +
> +/* Register string appended to "xmlRegisters=" in qSupported query. */
> +
> +void
> +register_remote_support_xml (const char *xml ATTRIBUTE_UNUSED)
> +{
> +#if defined(HAVE_LIBEXPAT)
> + if (remote_support_xml == NULL)
> + remote_support_xml = concat ("xmlRegisters=", xml, NULL);
> + else
> + {
> + const char *str = strstr (remote_support_xml, xml);
> + int append;
> +
> + if (str)
> + {
> + if (str != remote_support_xml && *(str - 1) != ',')
> + append = 0;
> + else
> + {
> + size_t len = strlen (xml);
> + append = xml[len] != ',' && xml[len] != '\0';
> + }
I don't understand this. How can `xml[len]' ever be == ',' ?
Did uou mean `src[len]' ? I'd have to say that using strtok
to loop over all ',' separated archs, and strcmp to check if
it's already there would probably avoid such confusions.
Something like this:
tok = strtok (remote_support_xml, ","));
do
{
if (strcmp (tok, xml) == 0)
return; /* already there */
}
while ((tok = strtok (NULL, ",")) != NULL);
/* append */
char *p = concat (remote_support_xml, ",", xml, NULL);
xfree (remote_support_xml);
remote_support_xml = p;
> + }
> + else
> + append = 1;
> +
> + if (append)
> + {
> + char *p = concat (remote_support_xml, ",", xml, NULL);
> + xfree (remote_support_xml);
> + remote_support_xml = p;
> + }
> + }
> +#endif
> +}
> +
> +static char *
> +remote_query_supported_append (char *msg, const char *append)
> +{
> + if (msg)
> + {
> + char *p = concat (msg, ";", append, NULL);
> + xfree (msg);
> + return p;
> + }
> + else
> + return xstrdup (append);
> +}
> +
> static void
> remote_query_supported (void)
> {
> @@ -3482,24 +3533,27 @@ remote_query_supported (void)
> rs->buf[0] = 0;
> if (remote_protocol_packets[PACKET_qSupported].support != PACKET_DISABLE)
> {
> + char *q = NULL;
> const char *qsupported = gdbarch_qsupported (target_gdbarch);
> +
> + if (rs->extended)
> + q = remote_query_supported_append (q, "multiprocess+;");
Drop the ';' here as remote_query_supported_append takes care
of adding it.
--
Pedro Alves