[PATCH 1/2] gdb: Workaround bad gdbserver qSupported:xmlRegisters=i386;UnknwnFeat+ handling

[PATCH 1/2] gdb: Workaround bad gdbserver qSupported:xmlRegisters=i386;UnknwnFeat+ handling

[Pedro Alves]

gdbserver's target_process_qsupported is called for each feature that
the gdbserver common code does not recognize.  The only current
implementation, for x86 Linux, does this:

  static void
  x86_linux_process_qsupported (const char *query)
  {
    /* Return if gdb doesn't support XML.  If gdb sends "xmlRegisters="
       with "i386" in qSupported query, it supports x86 XML target
       descriptions.  */
    use_xml = 0;
    if (query != NULL && startswith (query, "xmlRegisters="))
      {
	char *copy = xstrdup (query + 13);
	char *p;

	for (p = strtok (copy, ","); p != NULL; p = strtok (NULL, ","))
	  {
	    if (strcmp (p, "i386") == 0)
	      {
		use_xml = 1;
		break;
	      }
	  }

	free (copy);
      }

    x86_linux_update_xmltarget ();
  }

Notice that this clears use_xml and calls x86_linux_update_xmltarget
each time target_process_qsupported is called.  So if gdb sends in any
unknown feature after "xmlRegisters=i386", like e.g.,
"xmlRegisters=i386;UnknownFeature+" gdbserver ends up not reporting a
XML description...

Work around this by having GDB send the "xmlRegisters=" feature last.

gdb/ChangeLog:
2015-11-04  Pedro Alves  <palves@redhat.com>

	* remote.c (remote_query_supported): Send the "xmlRegisters="
	last.
---
 gdb/remote.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index fed397a..593bf63 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -4467,9 +4467,6 @@ remote_query_supported (void)
       if (packet_set_cmd_state (PACKET_hwbreak_feature) != AUTO_BOOLEAN_FALSE)
 	q = remote_query_supported_append (q, "hwbreak+");
 
-      if (remote_support_xml)
-	q = remote_query_supported_append (q, remote_support_xml);
-
       q = remote_query_supported_append (q, "qRelocInsn+");
 
       if (rs->extended)
@@ -4488,6 +4485,11 @@ remote_query_supported (void)
       if (packet_set_cmd_state (PACKET_vContSupported) != AUTO_BOOLEAN_FALSE)
 	q = remote_query_supported_append (q, "vContSupported+");
 
+      /* Keep this one last to work around a gdbserver <= 7.10 bug in
+	 the qSupported:xmlRegisters=i386 handling.  */
+      if (remote_support_xml != NULL)
+	q = remote_query_supported_append (q, remote_support_xml);
+
       q = reconcat (q, "qSupported:", q, (char *) NULL);
       putpkt (q);
 
-- 
1.9.3

[PATCH 0/2] Bad qSupported:xmlRegisters=i386;UnknownFeature+ handling

[Pedro Alves]

I noticed that gdbserver's qSupported:xmlRegisters=i386 handling is
buggy which results in broken backwards compatibility...

This fixes the gdbserver bug and adds a workaround to gdb, since it
was trivial.

Pedro Alves (2):
  gdb: Workaround bad gdbserver qSupported:xmlRegisters=i386;UnknwnFeat+
    handling
  gdbserver: Fix qSupported:xmlRegisters=i386;UnknownFeature+ handling

 gdb/gdbserver/linux-low.c     |  4 ++--
 gdb/gdbserver/linux-low.h     |  2 +-
 gdb/gdbserver/linux-x86-low.c | 28 +++++++++++++++++-----------
 gdb/gdbserver/server.c        | 19 +++++++++++++------
 gdb/gdbserver/target.h        |  9 +++++----
 gdb/remote.c                  |  8 +++++---
 6 files changed, 43 insertions(+), 27 deletions(-)

-- 
1.9.3

[PATCH 2/2] gdbserver: Fix qSupported:xmlRegisters=i386;UnknownFeature+ handling

[Pedro Alves]

The target_process_qsupported method is called for each qSupported
feature that the common code does not recognize.  The only current
implementation, for x86 Linux (x86_linux_process_qsupported), assumes
that it either is called with the "xmlRegisters=i386" feature, or that
it is isn't called at all, indicating the connected GDB predates x86
XML descriptions.

That's a bad assumption however.  If GDB sends in a new/unknown (to
core gdbserver) feature after "xmlRegisters=i386", say, something like
qSupported:xmlRegisters=i386;UnknownFeature+, then when
target_process_qsupported is called for "UnknownFeature+",
x86_linux_process_qsupported clears the 'use_xml' global and calls
x86_linux_update_xmltarget, and gdbserver ends up _not_ reporting a
XML description...

This commit changes the target_process_qsupported API to instead pass
down a vector of unprocessed qSupported features in one go.

(There's an early call to target_process_qsupported(NULL) that
indicates "starting qSupported processing".  There's no matching call
to mark the end of processing, though.  I first fixed this by passing
(char *)-1 to indicate that, and adjusted the x86 backend to only
clear 'use_xml' when qSupported processing starts, and then only call
x86_linux_update_xmltarget() when (char *)-1 was passed.  However, I
wasn't that happy with the hack and came up this alternative version.)

gdb/gdbserver/ChangeLog:
2015-11-04  Pedro Alves  <palves@redhat.com>

	* linux-low.c (linux_process_qsupported): Change prototype.
	Adjust.
	* linux-low.h (struct linux_target_ops) <process_qsupported>:
	Change prototype.
	* linux-x86-low.c (x86_linux_process_qsupported): Change prototype
	and adjust to loop over all features.
	* server.c (handle_query) <qSupported>: Adjust to call
	target_process_qsupported once, passing it a vector of unprocessed
	features.
	* target.h (struct target_ops) <process_qsupported>: Change
	prototype.
	(target_process_qsupported): Adjust.
---
 gdb/gdbserver/linux-low.c     |  4 ++--
 gdb/gdbserver/linux-low.h     |  2 +-
 gdb/gdbserver/linux-x86-low.c | 28 +++++++++++++++++-----------
 gdb/gdbserver/server.c        | 19 +++++++++++++------
 gdb/gdbserver/target.h        |  9 +++++----
 5 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 41ab510..e3a56a7 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -6128,10 +6128,10 @@ linux_read_loadmap (const char *annex, CORE_ADDR offset,
 #endif /* defined PT_GETDSBT || defined PTRACE_GETFDPIC */
 
 static void
-linux_process_qsupported (const char *query)
+linux_process_qsupported (char **features, int count)
 {
   if (the_low_target.process_qsupported != NULL)
-    the_low_target.process_qsupported (query);
+    the_low_target.process_qsupported (features, count);
 }
 
 static int
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index ccf4c94..f1d4f0f 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -199,7 +199,7 @@ struct linux_target_ops
   void (*prepare_to_resume) (struct lwp_info *);
 
   /* Hook to support target specific qSupported.  */
-  void (*process_qsupported) (const char *);
+  void (*process_qsupported) (char **, int count);
 
   /* Returns true if the low target supports tracepoints.  */
   int (*supports_tracepoints) (void);
diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c
index 89ec4e5..7f07194 100644
--- a/gdb/gdbserver/linux-x86-low.c
+++ b/gdb/gdbserver/linux-x86-low.c
@@ -1356,29 +1356,35 @@ x86_linux_update_xmltarget (void)
    PTRACE_GETREGSET.  */
 
 static void
-x86_linux_process_qsupported (const char *query)
+x86_linux_process_qsupported (char **features, int count)
 {
+  int i;
+
   /* Return if gdb doesn't support XML.  If gdb sends "xmlRegisters="
      with "i386" in qSupported query, it supports x86 XML target
      descriptions.  */
   use_xml = 0;
-  if (query != NULL && startswith (query, "xmlRegisters="))
+  for (i = 0; i < count; i++)
     {
-      char *copy = xstrdup (query + 13);
-      char *p;
+      const char *feature = features[i];
 
-      for (p = strtok (copy, ","); p != NULL; p = strtok (NULL, ","))
+      if (startswith (feature, "xmlRegisters="))
 	{
-	  if (strcmp (p, "i386") == 0)
+	  char *copy = xstrdup (feature + 13);
+	  char *p;
+
+	  for (p = strtok (copy, ","); p != NULL; p = strtok (NULL, ","))
 	    {
-	      use_xml = 1;
-	      break;
+	      if (strcmp (p, "i386") == 0)
+		{
+		  use_xml = 1;
+		  break;
+		}
 	    }
-	} 
 
-      free (copy);
+	  free (copy);
+	}
     }
-
   x86_linux_update_xmltarget ();
 }
 
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index fd2804b..7d6c9cc 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2054,9 +2054,6 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
       char *p = &own_buf[10];
       int gdb_supports_qRelocInsn = 0;
 
-      /* Start processing qSupported packet.  */
-      target_process_qsupported (NULL);
-
       /* Process each feature being provided by GDB.  The first
 	 feature will follow a ':', and latter features will follow
 	 ';'.  */
@@ -2064,6 +2061,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 	{
 	  char **qsupported = NULL;
 	  int count = 0;
+	  int unknown = 0;
 	  int i;
 
 	  /* Two passes, to avoid nested strtok calls in
@@ -2128,11 +2126,20 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 	      else if (strcmp (p, "vContSupported+") == 0)
 		vCont_supported = 1;
 	      else
-		target_process_qsupported (p);
-
-	      free (p);
+		{
+		  /* Move the unknown features all together.  */
+		  qsupported[i] = NULL;
+		  qsupported[unknown] = p;
+		  unknown++;
+		}
 	    }
 
+	  /* Give the target backend a chance to process the unknown
+	     features.  */
+	  target_process_qsupported (qsupported, unknown);
+
+	  for (i = 0; i < count; i++)
+	    free (qsupported[i]);
 	  free (qsupported);
 	}
 
diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
index 769c876..6fa454d 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -306,8 +306,9 @@ struct target_ops
   int (*read_loadmap) (const char *annex, CORE_ADDR offset,
 		       unsigned char *myaddr, unsigned int len);
 
-  /* Target specific qSupported support.  */
-  void (*process_qsupported) (const char *);
+  /* Target specific qSupported support.  FEATURES is an array of
+     features with COUNT elements.  */
+  void (*process_qsupported) (char **features, int count);
 
   /* Return 1 if the target supports tracepoints, 0 (or leave the
      callback NULL) otherwise.  */
@@ -519,11 +520,11 @@ int kill_inferior (int);
   (the_target->supports_multi_process ? \
    (*the_target->supports_multi_process) () : 0)
 
-#define target_process_qsupported(query)		\
+#define target_process_qsupported(features, count)	\
   do							\
     {							\
       if (the_target->process_qsupported)		\
-	the_target->process_qsupported (query);		\
+	the_target->process_qsupported (features, count); \
     } while (0)
 
 #define target_supports_tracepoints()			\
-- 
1.9.3

Re: [PATCH 0/2] Bad qSupported:xmlRegisters=i386;UnknownFeature+ handling

[Pedro Alves]

On 11/04/2015 04:48 PM, Pedro Alves wrote:
> I noticed that gdbserver's qSupported:xmlRegisters=i386 handling is
> buggy which results in broken backwards compatibility...
> 
> This fixes the gdbserver bug and adds a workaround to gdb, since it
> was trivial.
> 
> Pedro Alves (2):
>   gdb: Workaround bad gdbserver qSupported:xmlRegisters=i386;UnknwnFeat+
>     handling
>   gdbserver: Fix qSupported:xmlRegisters=i386;UnknownFeature+ handling

I've pushed this in now.

Thanks,
Pedro Alves