Re: PATCH: Add xmlRegisters= to qsupported query

[Pedro Alves <pedro at codesourcery dot com>]

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