PATCH: Add xmlRegisters= to qsupported query

PATCH: Add xmlRegisters= to qsupported query

[H.J. Lu]

Hi,

This patch adds xmlRegisters= to qsupported query.  Any comments?

Thanks.


H.J.
----
gdb/

2010-03-28  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): Support remote_support_xml.

	* remote.h (register_remote_support_xml): New.

gdb/doc/

2010-03-28  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..ad84d2c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -30603,6 +30603,12 @@ 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 supports the XML
+target description.  If the stub sees @samp{xmlRegisters=} with
+target specfic strings separated by comma, it can send @value{GDBN}
+the XML target description.
 @end table
 
 Stubs should ignore any unknown values for
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 83275ac..d7af84a 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 ("x86");
+
   return gdbarch;
 }
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 0c791aa..9637f53 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3464,6 +3464,41 @@ 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)
+{
+  if (remote_support_xml == NULL)
+    remote_support_xml = xstrdup (xml);
+  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] != ';'
+			&& xml[len] != '\0');
+	    }
+	}
+      else
+	append = 1;
+
+      if (append)
+	remote_support_xml = concat (xml, ",",  remote_support_xml,
+				     NULL);
+    }
+}
+
 static void
 remote_query_supported (void)
 {
@@ -3483,14 +3518,35 @@ remote_query_supported (void)
   if (remote_protocol_packets[PACKET_qSupported].support != PACKET_DISABLE)
     {
       const char *qsupported = gdbarch_qsupported (target_gdbarch);
-      if (qsupported)
+      if (qsupported || remote_support_xml)
 	{
-	  char *q;
+	  char *q, *x;
+	  char *p = NULL;
+	  const char *m;
+
 	  if (rs->extended)
-	    q = concat ("qSupported:multiprocess+;", qsupported, NULL);
+	    m = "multiprocess+;";
 	  else
-	    q = concat ("qSupported:", qsupported, NULL);
+	    m = "";
+
+	  if (remote_support_xml)
+	    {
+	      if (qsupported)
+		p = concat ("xmlRegisters=", remote_support_xml,
+			    ";", NULL);
+	      else
+		p = concat ("xmlRegisters=", remote_support_xml, NULL);
+	      x = p;
+	    }
+	  else
+	    x = "";
+
+	  q = concat ("qSupported:", m, x,
+		      qsupported ? qsupported : "",
+		      NULL);
 	  putpkt (q);
+	  if (p)
+	    xfree (p);
 	  xfree (q);
 	}
       else
diff --git a/gdb/remote.h b/gdb/remote.h
index a82d0d7..d480e26 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -66,6 +66,7 @@ extern void (*deprecated_target_wait_loop_hook) (void);
 
 void register_remote_g_packet_guess (struct gdbarch *gdbarch, int bytes,
 				     const struct target_desc *tdesc);
+void register_remote_support_xml (const char *);
 
 void remote_file_put (const char *local_file, const char *remote_file,
 		      int from_tty);

Re: PATCH: Add xmlRegisters= to qsupported query

[H.J. Lu]

On Sun, Mar 28, 2010 at 01:48:07PM -0700, H.J. Lu wrote:
> Hi,
> 
> This patch adds xmlRegisters= to qsupported query.  Any comments?
> 
> Thanks.
> 
> 

An updated patch.


H.J.
--
gdb/

2010-03-28  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): Support remote_support_xml.

	* remote.h (register_remote_support_xml): New.

gdb/doc/

2010-03-28  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..ad84d2c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -30603,6 +30603,12 @@ 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 supports the XML
+target description.  If the stub sees @samp{xmlRegisters=} with
+target specfic strings separated by comma, it can send @value{GDBN}
+the XML target description.
 @end table
 
 Stubs should ignore any unknown values for
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 83275ac..d7af84a 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 ("x86");
+
   return gdbarch;
 }
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 0c791aa..bceb477 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3464,6 +3464,39 @@ 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)
+{
+  if (remote_support_xml == NULL)
+    remote_support_xml = xstrdup (xml);
+  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';
+	    }
+	}
+      else
+	append = 1;
+
+      if (append)
+	remote_support_xml = concat (xml, ",",  remote_support_xml,
+				     NULL);
+    }
+}
+
 static void
 remote_query_supported (void)
 {
@@ -3483,14 +3516,35 @@ remote_query_supported (void)
   if (remote_protocol_packets[PACKET_qSupported].support != PACKET_DISABLE)
     {
       const char *qsupported = gdbarch_qsupported (target_gdbarch);
-      if (qsupported)
+      if (qsupported || remote_support_xml)
 	{
-	  char *q;
+	  char *q, *x;
+	  char *p = NULL;
+	  const char *m;
+
 	  if (rs->extended)
-	    q = concat ("qSupported:multiprocess+;", qsupported, NULL);
+	    m = "multiprocess+;";
 	  else
-	    q = concat ("qSupported:", qsupported, NULL);
+	    m = "";
+
+	  if (remote_support_xml)
+	    {
+	      if (qsupported)
+		p = concat ("xmlRegisters=", remote_support_xml,
+			    ";", NULL);
+	      else
+		p = concat ("xmlRegisters=", remote_support_xml, NULL);
+	      x = p;
+	    }
+	  else
+	    x = "";
+
+	  q = concat ("qSupported:", m, x,
+		      qsupported ? qsupported : "",
+		      NULL);
 	  putpkt (q);
+	  if (p)
+	    xfree (p);
 	  xfree (q);
 	}
       else
diff --git a/gdb/remote.h b/gdb/remote.h
index a82d0d7..d480e26 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -66,6 +66,7 @@ extern void (*deprecated_target_wait_loop_hook) (void);
 
 void register_remote_g_packet_guess (struct gdbarch *gdbarch, int bytes,
 				     const struct target_desc *tdesc);
+void register_remote_support_xml (const char *);
 
 void remote_file_put (const char *local_file, const char *remote_file,
 		      int from_tty);

Re: PATCH: Add xmlRegisters= to qsupported query

[Pedro Alves]

On Sunday 28 March 2010 21:52:07, H.J. Lu wrote:
>        const char *qsupported = gdbarch_qsupported (target_gdbarch);
> -      if (qsupported)
> +      if (qsupported || remote_support_xml)
>         {
> -         char *q;
> +         char *q, *x;
> +         char *p = NULL;
> +         const char *m;
> +
>           if (rs->extended)
> -           q = concat ("qSupported:multiprocess+;", qsupported, NULL);
> +           m = "multiprocess+;";
>           else
> -           q = concat ("qSupported:", qsupported, NULL);
> +           m = "";
> +
> +         if (remote_support_xml)
> +           {
> +             if (qsupported)
> +               p = concat ("xmlRegisters=", remote_support_xml,
> +                           ";", NULL);
> +             else
> +               p = concat ("xmlRegisters=", remote_support_xml, NULL);
> +             x = p;
> +           }
> +         else
> +           x = "";
> +
> +         q = concat ("qSupported:", m, x,
> +                     qsupported ? qsupported : "",
> +                     NULL);
>           putpkt (q);
> +         if (p)
> +           xfree (p);
>           xfree (q);

There's some hair here for handling gdbarch_qsupported, but,
is it going to end up used at all?  It isn't used today.

[personaly, I'd try to write the above in a way that avoids all
this if/then/if/then/else/else nesting everytime a few
qSupported feature is added.]

-- 
Pedro Alves

Re: PATCH: Add xmlRegisters= to qsupported query

[H.J. Lu]

On Sun, Mar 28, 2010 at 2:26 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Sunday 28 March 2010 21:52:07, H.J. Lu wrote:
>>        const char *qsupported = gdbarch_qsupported (target_gdbarch);
>> -      if (qsupported)
>> +      if (qsupported || remote_support_xml)
>>         {
>> -         char *q;
>> +         char *q, *x;
>> +         char *p = NULL;
>> +         const char *m;
>> +
>>           if (rs->extended)
>> -           q = concat ("qSupported:multiprocess+;", qsupported, NULL);
>> +           m = "multiprocess+;";
>>           else
>> -           q = concat ("qSupported:", qsupported, NULL);
>> +           m = "";
>> +
>> +         if (remote_support_xml)
>> +           {
>> +             if (qsupported)
>> +               p = concat ("xmlRegisters=", remote_support_xml,
>> +                           ";", NULL);
>> +             else
>> +               p = concat ("xmlRegisters=", remote_support_xml, NULL);
>> +             x = p;
>> +           }
>> +         else
>> +           x = "";
>> +
>> +         q = concat ("qSupported:", m, x,
>> +                     qsupported ? qsupported : "",
>> +                     NULL);
>>           putpkt (q);
>> +         if (p)
>> +           xfree (p);
>>           xfree (q);
>
> There's some hair here for handling gdbarch_qsupported, but,
> is it going to end up used at all?  It isn't used today.

I added it to prepare for AVX support. But I don't need it anymore
since I will use xmlRegisters=.

> [personaly, I'd try to write the above in a way that avoids all
> this if/then/if/then/else/else nesting everytime a few
> qSupported feature is added.]
>

I can try something.

-- 
H.J.

Re: PATCH: Add xmlRegisters= to qsupported query

[H.J. Lu]

On Sun, Mar 28, 2010 at 01:52:07PM -0700, H.J. Lu wrote:
> On Sun, Mar 28, 2010 at 01:48:07PM -0700, H.J. Lu wrote:
> > Hi,
> > 
> > This patch adds xmlRegisters= to qsupported query.  Any comments?
> > 
> > Thanks.
> > 
> > 
> 
> An updated patch.
> 
> 

Here is a new patch to simplify extending qsupported query and fix a
memory leak.  Any comments?

Thanks.


H.J.
---
gdb/

2010-03-28  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-28  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..ad84d2c 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -30603,6 +30603,12 @@ 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 supports the XML
+target description.  If the stub sees @samp{xmlRegisters=} with
+target specfic strings separated by comma, it can send @value{GDBN}
+the XML target description.
 @end table
 
 Stubs should ignore any unknown values for
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 83275ac..d7af84a 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 ("x86");
+
   return gdbarch;
 }
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 0c791aa..2dcf079 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3464,6 +3464,55 @@ 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)
+{
+  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';
+	    }
+	}
+      else
+	append = 1;
+
+      if (append)
+	{
+	  char *p = concat (remote_support_xml, ",", xml, NULL);
+	  xfree (remote_support_xml);
+	  remote_support_xml = p;
+	}
+    }
+}
+
+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 +3531,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+;");
+      
       if (qsupported)
+	q = remote_query_supported_append (q, qsupported);
+
+      if (remote_support_xml)
+	q = remote_query_supported_append (q, remote_support_xml);
+
+      if (q)
 	{
-	  char *q;
-	  if (rs->extended)
-	    q = concat ("qSupported:multiprocess+;", qsupported, NULL);
-	  else
-	    q = concat ("qSupported:", qsupported, NULL);
-	  putpkt (q);
+	  char *p = concat ("qSupported:", q, NULL);
 	  xfree (q);
+	  putpkt (p);
+	  xfree (p);
 	}
       else
-	{
-	  if (rs->extended)
-	    putpkt ("qSupported:multiprocess+");
-	  else
-	    putpkt ("qSupported");
-	}
+	putpkt ("qSupported");
 
       getpkt (&rs->buf, &rs->buf_size, 0);
 
diff --git a/gdb/remote.h b/gdb/remote.h
index a82d0d7..d480e26 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -66,6 +66,7 @@ extern void (*deprecated_target_wait_loop_hook) (void);
 
 void register_remote_g_packet_guess (struct gdbarch *gdbarch, int bytes,
 				     const struct target_desc *tdesc);
+void register_remote_support_xml (const char *);
 
 void remote_file_put (const char *local_file, const char *remote_file,
 		      int from_tty);

Re: PATCH: Add xmlRegisters= to qsupported query

[H.J. Lu]

On Sun, Mar 28, 2010 at 2:26 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> [personaly, I'd try to write the above in a way that avoids all
> this if/then/if/then/else/else nesting everytime a few
> qSupported feature is added.]

An updated patch is at

http://sourceware.org/ml/gdb-patches/2010-03/msg00963.html



-- 
H.J.

Re: PATCH: Add xmlRegisters= to qsupported query

[Eli Zaretskii]

> Date: Sun, 28 Mar 2010 16:46:43 -0700
> From: "H.J. Lu" <hongjiu.lu@intel.com>
> 
> 2010-03-28  H.J. Lu  <hongjiu.lu@intel.com>
> 
> 	* gdb.texinfo (General Query Packets): Add xmlRegisters.

I have a few comments to the documentation part:

> +@item xmlRegisters
> +This feature indicates that @value{GDBN} supports supports the XML
                                            ^^^^^^^^^^^^^^^^^
two "supports" in a row.

> +target description.  If the stub sees @samp{xmlRegisters=} with
> +target specfic strings separated by comma, it can send @value{GDBN}
                                    ^^^^^^^^
"by a comma".  Also, what do you mean by "it can send", why "can"?
Doesn't it always send the XML description?

> @@ -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 ("x86");

A possibly dumb question: what effect will this change in i386-tdep.c
have on i386 targets that don't support remote debugging?  How about
if GDB was built without libexpat?

Re: PATCH: Add xmlRegisters= to qsupported query

[H.J. Lu]

On Sun, Mar 28, 2010 at 11:38 PM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Sun, 28 Mar 2010 16:46:43 -0700
>> From: "H.J. Lu" <hongjiu.lu@intel.com>
>>
>> 2010-03-28  H.J. Lu  <hongjiu.lu@intel.com>
>>
>>       * gdb.texinfo (General Query Packets): Add xmlRegisters.
>
> I have a few comments to the documentation part:
>
>> +@item xmlRegisters
>> +This feature indicates that @value{GDBN} supports supports the XML
>                                            ^^^^^^^^^^^^^^^^^
> two "supports" in a row.

I will fix it.

>> +target description.  If the stub sees @samp{xmlRegisters=} with
>> +target specfic strings separated by comma, it can send @value{GDBN}
>                                    ^^^^^^^^
> "by a comma".  Also, what do you mean by "it can send", why "can"?

I will add `a'.

> Doesn't it always send the XML description?

Before XML was enabled on x86, we sent

"@<target>\
<architecture>i386</architecture>\
<osabi>GNU/Linux</osabi>\
</target>"

It isn't the "real" XML target description since it doesn't describe anything.

>> @@ -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 ("x86");
>
> A possibly dumb question: what effect will this change in i386-tdep.c
> have on i386 targets that don't support remote debugging?  How about

Did you mean remote.o wasn't linked in? Can that happen?

> if GDB was built without libexpat?
>

I will add check for HAVE_LIBEXPAT.

Thanks.


-- 
H.J.

Re: PATCH: Add xmlRegisters= to qsupported query

[Eli Zaretskii]

> Date: Mon, 29 Mar 2010 07:13:28 -0700
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Cc: gdb-patches@sourceware.org
> 
> >> +target description.  If the stub sees @samp{xmlRegisters=} with
> >> +target specfic strings separated by comma, it can send @value{GDBN}
> >                                    ^^^^^^^^
> > "by a comma".  Also, what do you mean by "it can send", why "can"?
> 
> I will add `a'.
> 
> > Doesn't it always send the XML description?
> 
> Before XML was enabled on x86, we sent
> 
> "@<target>\
> <architecture>i386</architecture>\
> <osabi>GNU/Linux</osabi>\
> </target>"
> 
> It isn't the "real" XML target description since it doesn't describe anything.

So how about this text instead:

 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.

> >> +  /* Tell remote stub that we support XML target description.  */
> >> +  register_remote_support_xml ("x86");
> >
> > A possibly dumb question: what effect will this change in i386-tdep.c
> > have on i386 targets that don't support remote debugging?  How about
> 
> Did you mean remote.o wasn't linked in? Can that happen?

I don't know.  The question is, will register_remote_support_xml be
always available fro an i386 target?

Thanks.

Re: PATCH: Add xmlRegisters= to qsupported query

[H.J. Lu]

On Mon, Mar 29, 2010 at 7:31 AM, Eli Zaretskii <eliz@gnu.org> wrote:
>> Date: Mon, 29 Mar 2010 07:13:28 -0700
>> From: "H.J. Lu" <hjl.tools@gmail.com>
>> Cc: gdb-patches@sourceware.org
>>
>> >> +target description.  If the stub sees @samp{xmlRegisters=} with
>> >> +target specfic strings separated by comma, it can send @value{GDBN}
>> >                                    ^^^^^^^^
>> > "by a comma".  Also, what do you mean by "it can send", why "can"?
>>
>> I will add `a'.
>>
>> > Doesn't it always send the XML description?
>>
>> Before XML was enabled on x86, we sent
>>
>> "@<target>\
>> <architecture>i386</architecture>\
>> <osabi>GNU/Linux</osabi>\
>> </target>"
>>
>> It isn't the "real" XML target description since it doesn't describe anything.
>
> So how about this text instead:
>
>  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.

I will make the change.

>> >> +  /* Tell remote stub that we support XML target description.  */
>> >> +  register_remote_support_xml ("x86");
>> >
>> > A possibly dumb question: what effect will this change in i386-tdep.c
>> > have on i386 targets that don't support remote debugging?  How about
>>
>> Did you mean remote.o wasn't linked in? Can that happen?
>
> I don't know.  The question is, will register_remote_support_xml be
> always available fro an i386 target?

Yes, register_remote_support_xml is available since remote.o is always
linked in for any i386 target.



-- 
H.J.

Re: PATCH: Add xmlRegisters= to qsupported query

[H.J. Lu]

On Sun, Mar 28, 2010 at 04:46:43PM -0700, H.J. Lu wrote:
> On Sun, Mar 28, 2010 at 01:52:07PM -0700, H.J. Lu wrote:
> > On Sun, Mar 28, 2010 at 01:48:07PM -0700, H.J. Lu wrote:
> > > Hi,
> > > 
> > > This patch adds xmlRegisters= to qsupported query.  Any comments?
> > > 
> > > Thanks.
> > > 
> > > 
> > 
> > An updated patch.
> > 
> > 
> 
> Here is a new patch to simplify extending qsupported query and fix a
> memory leak.  Any comments?
> 

This patch updated documentation based on Eli's feedbacks. OK to
install?

Thanks.


H.J.
---
gdb/

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
 
 Stubs should ignore any unknown values for
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 83275ac..d7af84a 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 ("x86");
+
   return gdbarch;
 }
 
diff --git a/gdb/remote.c b/gdb/remote.c
index 0c791aa..2dcf079 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3464,6 +3464,55 @@ 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)
+{
+  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';
+	    }
+	}
+      else
+	append = 1;
+
+      if (append)
+	{
+	  char *p = concat (remote_support_xml, ",", xml, NULL);
+	  xfree (remote_support_xml);
+	  remote_support_xml = p;
+	}
+    }
+}
+
+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 +3531,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+;");
+      
       if (qsupported)
+	q = remote_query_supported_append (q, qsupported);
+
+      if (remote_support_xml)
+	q = remote_query_supported_append (q, remote_support_xml);
+
+      if (q)
 	{
-	  char *q;
-	  if (rs->extended)
-	    q = concat ("qSupported:multiprocess+;", qsupported, NULL);
-	  else
-	    q = concat ("qSupported:", qsupported, NULL);
-	  putpkt (q);
+	  char *p = concat ("qSupported:", q, NULL);
 	  xfree (q);
+	  putpkt (p);
+	  xfree (p);
 	}
       else
-	{
-	  if (rs->extended)
-	    putpkt ("qSupported:multiprocess+");
-	  else
-	    putpkt ("qSupported");
-	}
+	putpkt ("qSupported");
 
       getpkt (&rs->buf, &rs->buf_size, 0);
 
diff --git a/gdb/remote.h b/gdb/remote.h
index a82d0d7..d480e26 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -66,6 +66,7 @@ extern void (*deprecated_target_wait_loop_hook) (void);
 
 void register_remote_g_packet_guess (struct gdbarch *gdbarch, int bytes,
 				     const struct target_desc *tdesc);
+void register_remote_support_xml (const char *);
 
 void remote_file_put (const char *local_file, const char *remote_file,
 		      int from_tty);

Re: PATCH: Add xmlRegisters= to qsupported query

[H.J. Lu]

On Mon, Mar 29, 2010 at 07:55:49AM -0700, H.J. Lu wrote:
> On Sun, Mar 28, 2010 at 04:46:43PM -0700, H.J. Lu wrote:
> > On Sun, Mar 28, 2010 at 01:52:07PM -0700, H.J. Lu wrote:
> > > On Sun, Mar 28, 2010 at 01:48:07PM -0700, H.J. Lu wrote:
> > > > Hi,
> > > > 
> > > > This patch adds xmlRegisters= to qsupported query.  Any comments?
> > > > 
> > > > Thanks.
> > > > 
> > > > 
> > > 
> > > An updated patch.
> > > 
> > > 
> > 
> > Here is a new patch to simplify extending qsupported query and fix a
> > memory leak.  Any comments?
> > 
> 
> This patch updated documentation based on Eli's feedbacks. OK to
> install?
> 
> Thanks.
> 

We should check if libexpat is available. Any comments/suggestions?

Thanks.


H.J.
---
gdb/

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
 
 Stubs should ignore any unknown values for
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 83275ac..d7af84a 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 ("x86");
+
   return 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';
+	    }
+	}
+      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+;");
+      
       if (qsupported)
+	q = remote_query_supported_append (q, qsupported);
+
+      if (remote_support_xml)
+	q = remote_query_supported_append (q, remote_support_xml);
+
+      if (q)
 	{
-	  char *q;
-	  if (rs->extended)
-	    q = concat ("qSupported:multiprocess+;", qsupported, NULL);
-	  else
-	    q = concat ("qSupported:", qsupported, NULL);
-	  putpkt (q);
+	  char *p = concat ("qSupported:", q, NULL);
 	  xfree (q);
+	  putpkt (p);
+	  xfree (p);
 	}
       else
-	{
-	  if (rs->extended)
-	    putpkt ("qSupported:multiprocess+");
-	  else
-	    putpkt ("qSupported");
-	}
+	putpkt ("qSupported");
 
       getpkt (&rs->buf, &rs->buf_size, 0);
 
diff --git a/gdb/remote.h b/gdb/remote.h
index a82d0d7..d480e26 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -66,6 +66,7 @@ extern void (*deprecated_target_wait_loop_hook) (void);
 
 void register_remote_g_packet_guess (struct gdbarch *gdbarch, int bytes,
 				     const struct target_desc *tdesc);
+void register_remote_support_xml (const char *);
 
 void remote_file_put (const char *local_file, const char *remote_file,
 		      int from_tty);

Re: PATCH: Add xmlRegisters= to qsupported query

[Mark Kettenis]

> Date: Mon, 29 Mar 2010 09:19:10 -0700
> From: "H.J. Lu" <hongjiu.lu@intel.com>
> 
> We should check if libexpat is available. Any comments/suggestions?

> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
> index 83275ac..d7af84a 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 ("x86");

Please use "i386" here.

Re: PATCH: Add xmlRegisters= to qsupported query

[H.J. Lu]

On Mon, Mar 29, 2010 at 09:19:10AM -0700, H.J. Lu wrote:
> On Mon, Mar 29, 2010 at 07:55:49AM -0700, H.J. Lu wrote:
> > On Sun, Mar 28, 2010 at 04:46:43PM -0700, H.J. Lu wrote:
> > > On Sun, Mar 28, 2010 at 01:52:07PM -0700, H.J. Lu wrote:
> > > > On Sun, Mar 28, 2010 at 01:48:07PM -0700, H.J. Lu wrote:
> > > > > Hi,
> > > > > 
> > > > > This patch adds xmlRegisters= to qsupported query.  Any comments?
> > > > > 
> > > > > Thanks.
> > > > > 
> > > > > 
> > > > 
> > > > An updated patch.
> > > > 
> > > > 
> > > 
> > > Here is a new patch to simplify extending qsupported query and fix a
> > > memory leak.  Any comments?
> > > 
> > 
> > This patch updated documentation based on Eli's feedbacks. OK to
> > install?
> > 
> > Thanks.
> > 
> 
> We should check if libexpat is available. Any comments/suggestions?
> 

Updated patch to pass "i386" instead of "x86" to
register_remote_support_xml.  OK to install?

Thanks.


H.J.
---
gdb/

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
 
 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;
 }
 
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';
+	    }
+	}
+      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+;");
+      
       if (qsupported)
+	q = remote_query_supported_append (q, qsupported);
+
+      if (remote_support_xml)
+	q = remote_query_supported_append (q, remote_support_xml);
+
+      if (q)
 	{
-	  char *q;
-	  if (rs->extended)
-	    q = concat ("qSupported:multiprocess+;", qsupported, NULL);
-	  else
-	    q = concat ("qSupported:", qsupported, NULL);
-	  putpkt (q);
+	  char *p = concat ("qSupported:", q, NULL);
 	  xfree (q);
+	  putpkt (p);
+	  xfree (p);
 	}
       else
-	{
-	  if (rs->extended)
-	    putpkt ("qSupported:multiprocess+");
-	  else
-	    putpkt ("qSupported");
-	}
+	putpkt ("qSupported");
 
       getpkt (&rs->buf, &rs->buf_size, 0);
 
diff --git a/gdb/remote.h b/gdb/remote.h
index a82d0d7..d480e26 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -66,6 +66,7 @@ extern void (*deprecated_target_wait_loop_hook) (void);
 
 void register_remote_g_packet_guess (struct gdbarch *gdbarch, int bytes,
 				     const struct target_desc *tdesc);
+void register_remote_support_xml (const char *);
 
 void remote_file_put (const char *local_file, const char *remote_file,
 		      int from_tty);

Re: PATCH: Add xmlRegisters= to qsupported query

[Pedro Alves]

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

Re: PATCH: Add xmlRegisters= to qsupported query

[H.J. Lu]

On Mon, Mar 29, 2010 at 11:01:36AM -0700, H.J. Lu wrote:
> On Mon, Mar 29, 2010 at 09:19:10AM -0700, H.J. Lu wrote:
> > On Mon, Mar 29, 2010 at 07:55:49AM -0700, H.J. Lu wrote:
> > > On Sun, Mar 28, 2010 at 04:46:43PM -0700, H.J. Lu wrote:
> > > > On Sun, Mar 28, 2010 at 01:52:07PM -0700, H.J. Lu wrote:
> > > > > On Sun, Mar 28, 2010 at 01:48:07PM -0700, H.J. Lu wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > This patch adds xmlRegisters= to qsupported query.  Any comments?
> > > > > > 
> > > > > > Thanks.
> > > > > > 
> > > > > > 
> > > > > 
> > > > > An updated patch.
> > > > > 
> > > > > 
> > > > 
> > > > Here is a new patch to simplify extending qsupported query and fix a
> > > > memory leak.  Any comments?
> > > > 
> > > 
> > > This patch updated documentation based on Eli's feedbacks. OK to
> > > install?
> > > 
> > > Thanks.
> > > 
> > 
> > We should check if libexpat is available. Any comments/suggestions?
> > 
> 
> Updated patch to pass "i386" instead of "x86" to
> register_remote_support_xml.  OK to install?
> 

Here is the updated patch.  OK to install?

Thanks.


H.J.
---
gdb/

2010-03-30  H.J. Lu  <hongjiu.lu@intel.com>

	* NEWS: Mention xmlRegisters= in qSupported packet.

	* i386-tdep.c: Include "remote.h".
	(_initialize_i386_tdep): 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-30  H.J. Lu  <hongjiu.lu@intel.com>

	* gdb.texinfo (General Query Packets): Add xmlRegisters.

diff --git a/gdb/NEWS b/gdb/NEWS
index f01b55e..0d27faa 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,9 @@
 
 *** Changes since GDB 7.1
 
+* GDB now sends xmlRegisters= in qSupported packet to indicate that
+  it understands register description.
+
 * The --batch flag now disables pagination and queries.
 
 * X86 general purpose registers
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index ef51ed1..fab06a8 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -30633,6 +30633,12 @@ 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 report register
+description.
 @end table
 
 Stubs should ignore any unknown values for
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index 83275ac..9b4c93e 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"
@@ -6000,4 +6001,7 @@ is \"default\"."),
 
   /* Initialize the standard target descriptions.  */
   initialize_tdesc_i386 ();
+
+  /* Tell remote stub that we support XML target description.  */
+  register_remote_support_xml ("i386");
 }
diff --git a/gdb/remote.c b/gdb/remote.c
index dcae72c..03644d1 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3467,6 +3467,53 @@ static struct protocol_feature remote_protocol_features[] = {
     PACKET_TracepointSource },
 };
 
+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
+    {
+      char *copy = xstrdup (remote_support_xml + 13);
+      char *p = strtok (copy, ",");
+
+      do
+	{
+	  if (strcmp (p, xml) == 0)
+	    {
+	      /* already there */
+	      xfree (copy);
+	      return;
+	    }
+	}
+      while ((p = strtok (NULL, ",")) != NULL);
+      xfree (copy);
+
+      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)
 {
@@ -3485,24 +3532,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+");
+      
       if (qsupported)
+	q = remote_query_supported_append (q, qsupported);
+
+      if (remote_support_xml)
+	q = remote_query_supported_append (q, remote_support_xml);
+
+      if (q)
 	{
-	  char *q;
-	  if (rs->extended)
-	    q = concat ("qSupported:multiprocess+;", qsupported, NULL);
-	  else
-	    q = concat ("qSupported:", qsupported, NULL);
-	  putpkt (q);
+	  char *p = concat ("qSupported:", q, NULL);
 	  xfree (q);
+	  putpkt (p);
+	  xfree (p);
 	}
       else
-	{
-	  if (rs->extended)
-	    putpkt ("qSupported:multiprocess+");
-	  else
-	    putpkt ("qSupported");
-	}
+	putpkt ("qSupported");
 
       getpkt (&rs->buf, &rs->buf_size, 0);
 
diff --git a/gdb/remote.h b/gdb/remote.h
index a82d0d7..d480e26 100644
--- a/gdb/remote.h
+++ b/gdb/remote.h
@@ -66,6 +66,7 @@ extern void (*deprecated_target_wait_loop_hook) (void);
 
 void register_remote_g_packet_guess (struct gdbarch *gdbarch, int bytes,
 				     const struct target_desc *tdesc);
+void register_remote_support_xml (const char *);
 
 void remote_file_put (const char *local_file, const char *remote_file,
 		      int from_tty);

Re: PATCH: Add xmlRegisters= to qsupported query

[Pedro Alves]

On Tuesday 30 March 2010 15:58:47, H.J. Lu wrote:

> Here is the updated patch.  OK to install?

This version is fine with me.

Thanks.

-- 
Pedro Alves