[PATCH 1/4] Merge remote thread listing methods

[Pedro Alves <palves@redhat.com>]

We have three methods to list the current remote thread list:

1. The qXfer:threads:read method (the prefered one nowadays), builds a
remote thread list while parsing the XML, and then after the XML
parsing is done, goes over the built list and adds threads GDB doesn't
know about yet to GDB's list.

2. If the qXfer method isn't available, we fallback to using the
qfThreadInfo/qsThreadInfo packets.  When we do this, we adds threads
to GDB's list immediately as we parse the qfThreadInfo/qsThreadInfo
packet replies.

3. And then if the previous method isn't available either, we try the
old deprecated qL packet.  This path is already looking somewhat
broken for not using remote_notice_new_inferior to add threads to
GDB's list.

This patch makes all variants work in two passes, like the qXfer
method, and then makes all variants share the code path that adds
threads to GDB's list.

Tested on x86_64 Fedora 20 with native gdbserver.

gdb/
2014-10-02  Pedro Alves  <palves@redhat.com>

	* remote.c (remote_get_threadlist, remote_threadlist_iterator):
	Add describing comment.  Return -1 if the qL packet is not
	supported.
	(struct thread_item, thread_item_t): Move higher up in
	the file.  Add comments.
	(struct threads_parsing_context): Move higher up in
	the file, add comments, and remote to ...
	(struct threads_listing_context): ... this.
	(remote_newthread_step): Don't add the thread to GDB's thread
	database here.  Instead push it to the thread_listing_context
	list.
	(remote_find_new_threads): Rename to ...
	(remote_get_threads_with_ql): ... this.  Add target_ops and
	targets_listing_context parameters.  Pass down context.
	(start_thread): Adjust.
	(clear_threads_parsing_context): Rename to ...
	(clear_threads_listing_context): ... this.
	(remote_get_threads_with_qxfer): New, with parts salvaged from old
	remote_threads_info.
	(remote_get_threads_with_qthreadinfo): Ditto.
	(remote_threads_info): Reimplement.
---
 gdb/remote.c | 298 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 161 insertions(+), 137 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 41ea012..e4776cc 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -169,8 +169,6 @@ static int stub_unpack_int (char *buff, int fieldlength);
 
 static ptid_t remote_current_thread (ptid_t oldptid);
 
-static void remote_find_new_threads (void);
-
 static int putpkt_binary (const char *buf, int cnt);
 
 static void check_binary_download (CORE_ADDR addr);
@@ -2428,6 +2426,9 @@ parse_threadlist_response (char *pkt, int result_limit,
   return resultcount;
 }
 
+/* Fetch the next batch of threads from the remote.  Returns -1 if the
+   qL packet is not supported, 0 on error and 1 on success.  */
+
 static int
 remote_get_threadlist (int startflag, threadref *nextthread, int result_limit,
 		       int *done, int *result_count, threadref *threadlist)
@@ -2443,13 +2444,15 @@ remote_get_threadlist (int startflag, threadref *nextthread, int result_limit,
   pack_threadlist_request (rs->buf, startflag, result_limit, nextthread);
   putpkt (rs->buf);
   getpkt (&rs->buf, &rs->buf_size, 0);
-
   if (*rs->buf == '\0')
-    return 0;
-  else
-    *result_count =
-      parse_threadlist_response (rs->buf + 2, result_limit,
-				 &rs->echo_nextthread, threadlist, done);
+    {
+      /* Packet not supported.  */
+      return -1;
+    }
+
+  *result_count =
+    parse_threadlist_response (rs->buf + 2, result_limit,
+			       &rs->echo_nextthread, threadlist, done);
 
   if (!threadmatch (&rs->echo_nextthread, nextthread))
     {
@@ -2482,15 +2485,11 @@ remote_get_threadlist (int startflag, threadref *nextthread, int result_limit,
   return result;
 }
 
-/* This is the interface between remote and threads, remotes upper
-   interface.  */
-
-/* remote_find_new_threads retrieves the thread list and for each
-   thread in the list, looks up the thread in GDB's internal list,
-   adding the thread if it does not already exist.  This involves
-   getting partial thread lists from the remote target so, polling the
-   quit_flag is required.  */
-
+/* Fetch the list of remote threads, with the qL packet, and call
+   STEPFUNCTION for each thread found.  Stops iterating and returns 1
+   if STEPFUNCTION returns true.  Stops iterating and returns 0 if the
+   STEPFUNCTION returns false.  If the packet is not supported,
+   returns -1.  */
 
 static int
 remote_threadlist_iterator (rmt_thread_action stepfunction, void *context,
@@ -2511,13 +2510,12 @@ remote_threadlist_iterator (rmt_thread_action stepfunction, void *context,
 	  warning (_("Remote fetch threadlist -infinite loop-."));
 	  break;
 	}
-      if (!remote_get_threadlist (startflag, &rs->nextthread,
-				  MAXTHREADLISTRESULTS,
-				  &done, &result_count, rs->resultthreadlist))
-	{
-	  result = 0;
-	  break;
-	}
+      result = remote_get_threadlist (startflag, &rs->nextthread,
+				      MAXTHREADLISTRESULTS,
+				      &done, &result_count,
+				      rs->resultthreadlist);
+      if (result <= 0)
+	break;
       /* Clear for later iterations.  */
       startflag = 0;
       /* Setup to resume next batch of thread references, set nextthread.  */
@@ -2526,20 +2524,55 @@ remote_threadlist_iterator (rmt_thread_action stepfunction, void *context,
 			&rs->resultthreadlist[result_count - 1]);
       i = 0;
       while (result_count--)
-	if (!(result = (*stepfunction) (&rs->resultthreadlist[i++], context)))
-	  break;
+	{
+	  if (!(*stepfunction) (&rs->resultthreadlist[i++], context))
+	    {
+	      result = 0;
+	      break;
+	    }
+	}
     }
   return result;
 }
 
+/* A thread found on the remote target.  */
+
+typedef struct thread_item
+{
+  /* The thread's PTID.  */
+  ptid_t ptid;
+
+  /* The thread's extra info.  May be NULL.  */
+  char *extra;
+
+  /* The core the thread was running on.  -1 if not known.  */
+  int core;
+} thread_item_t;
+DEF_VEC_O(thread_item_t);
+
+/* Context passed around to the various methods listing remote
+   threads.  As new threads are found, they're added to the ITEMS
+   vector.  */
+
+struct threads_listing_context
+{
+  /* The threads found on the remote target.  */
+  VEC (thread_item_t) *items;
+};
+
 static int
-remote_newthread_step (threadref *ref, void *context)
+remote_newthread_step (threadref *ref, void *data)
 {
+  struct threads_listing_context *context = data;
+  struct thread_item item;
   int pid = ptid_get_pid (inferior_ptid);
-  ptid_t ptid = ptid_build (pid, threadref_to_int (ref), 0);
 
-  if (!in_thread_list (ptid))
-    add_thread (ptid);
+  item.ptid = ptid_build (pid, threadref_to_int (ref), 0);
+  item.core = -1;
+  item.extra = NULL;
+
+  VEC_safe_push (thread_item_t, context->items, &item);
+
   return 1;			/* continue iterator */
 }
 
@@ -2558,38 +2591,27 @@ remote_current_thread (ptid_t oldpid)
     return oldpid;
 }
 
-/* Find new threads for info threads command.
- * Original version, using John Metzler's thread protocol.
- */
+/* List remote threads using the deprecated qL packet.  */
 
-static void
-remote_find_new_threads (void)
+static int
+remote_get_threads_with_ql (struct target_ops *ops,
+			    struct threads_listing_context *context)
 {
-  remote_threadlist_iterator (remote_newthread_step, 0,
-			      CRAZY_MAX_THREADS);
+  if (remote_threadlist_iterator (remote_newthread_step, context,
+				  CRAZY_MAX_THREADS) >= 0)
+    return 1;
+
+  return 0;
 }
 
 #if defined(HAVE_LIBEXPAT)
 
-typedef struct thread_item
-{
-  ptid_t ptid;
-  char *extra;
-  int core;
-} thread_item_t;
-DEF_VEC_O(thread_item_t);
-
-struct threads_parsing_context
-{
-  VEC (thread_item_t) *items;
-};
-
 static void
 start_thread (struct gdb_xml_parser *parser,
 	      const struct gdb_xml_element *element,
 	      void *user_data, VEC(gdb_xml_value_s) *attributes)
 {
-  struct threads_parsing_context *data = user_data;
+  struct threads_listing_context *data = user_data;
 
   struct thread_item item;
   char *id;
@@ -2614,7 +2636,7 @@ end_thread (struct gdb_xml_parser *parser,
 	    const struct gdb_xml_element *element,
 	    void *user_data, const char *body_text)
 {
-  struct threads_parsing_context *data = user_data;
+  struct threads_listing_context *data = user_data;
 
   if (body_text && *body_text)
     VEC_last (thread_item_t, data->items)->extra = xstrdup (body_text);
@@ -2646,9 +2668,9 @@ const struct gdb_xml_element threads_elements[] = {
 /* Discard the contents of the constructed thread info context.  */
 
 static void
-clear_threads_parsing_context (void *p)
+clear_threads_listing_context (void *p)
 {
-  struct threads_parsing_context *context = p;
+  struct threads_listing_context *context = p;
   int i;
   struct thread_item *item;
 
@@ -2660,124 +2682,126 @@ clear_threads_parsing_context (void *p)
 
 #endif
 
-/*
- * Find all threads for info threads command.
- * Uses new thread protocol contributed by Cisco.
- * Falls back and attempts to use the older method (above)
- * if the target doesn't respond to the new method.
- */
+/* List remote threads using qXfer:threads:read.  */
 
-static void
-remote_threads_info (struct target_ops *ops)
+static int
+remote_get_threads_with_qxfer (struct target_ops *ops,
+			       struct threads_listing_context *context)
 {
-  struct remote_state *rs = get_remote_state ();
-  char *bufp;
-  ptid_t new_thread;
-
-  if (rs->remote_desc == 0)		/* paranoia */
-    error (_("Command can only be used when connected to the remote target."));
-
 #if defined(HAVE_LIBEXPAT)
   if (packet_support (PACKET_qXfer_threads) == PACKET_ENABLE)
     {
-      char *xml = target_read_stralloc (&current_target,
-					 TARGET_OBJECT_THREADS, NULL);
-
+      char *xml = target_read_stralloc (ops, TARGET_OBJECT_THREADS, NULL);
       struct cleanup *back_to = make_cleanup (xfree, xml);
 
-      if (xml && *xml)
+      if (xml != NULL && *xml != '\0')
 	{
-	  struct threads_parsing_context context;
-
-	  context.items = NULL;
-	  make_cleanup (clear_threads_parsing_context, &context);
-
-	  if (gdb_xml_parse_quick (_("threads"), "threads.dtd",
-				   threads_elements, xml, &context) == 0)
-	    {
-	      int i;
-	      struct thread_item *item;
-
-	      for (i = 0;
-		   VEC_iterate (thread_item_t, context.items, i, item);
-		   ++i)
-		{
-		  if (!ptid_equal (item->ptid, null_ptid))
-		    {
-		      struct private_thread_info *info;
-		      /* In non-stop mode, we assume new found threads
-			 are running until proven otherwise with a
-			 stop reply.  In all-stop, we can only get
-			 here if all threads are stopped.  */
-		      int running = non_stop ? 1 : 0;
-
-		      remote_notice_new_inferior (item->ptid, running);
-
-		      info = demand_private_info (item->ptid);
-		      info->core = item->core;
-		      info->extra = item->extra;
-		      item->extra = NULL;
-		    }
-		}
-	    }
+	  gdb_xml_parse_quick (_("threads"), "threads.dtd",
+			       threads_elements, xml, context);
 	}
 
       do_cleanups (back_to);
-      return;
+      return 1;
     }
 #endif
 
+  return 0;
+}
+
+/* List remote threads using qfThreadInfo/qsThreadInfo.  */
+
+static int
+remote_get_threads_with_qthreadinfo (struct target_ops *ops,
+				     struct threads_listing_context *context)
+{
+  struct remote_state *rs = get_remote_state ();
+
   if (rs->use_threadinfo_query)
     {
+      char *bufp;
+
       putpkt ("qfThreadInfo");
       getpkt (&rs->buf, &rs->buf_size, 0);
       bufp = rs->buf;
       if (bufp[0] != '\0')		/* q packet recognized */
 	{
-	  struct cleanup *old_chain;
-	  char *saved_reply;
-
-	  /* remote_notice_new_inferior (in the loop below) may make
-	     new RSP calls, which clobber rs->buf.  Work with a
-	     copy.  */
-	  bufp = saved_reply = xstrdup (rs->buf);
-	  old_chain = make_cleanup (free_current_contents, &saved_reply);
-
 	  while (*bufp++ == 'm')	/* reply contains one or more TID */
 	    {
 	      do
 		{
-		  new_thread = read_ptid (bufp, &bufp);
-		  if (!ptid_equal (new_thread, null_ptid))
-		    {
-		      /* In non-stop mode, we assume new found threads
-			 are running until proven otherwise with a
-			 stop reply.  In all-stop, we can only get
-			 here if all threads are stopped.  */
-		      int running = non_stop ? 1 : 0;
+		  struct thread_item item;
 
-		      remote_notice_new_inferior (new_thread, running);
-		    }
+		  item.ptid = read_ptid (bufp, &bufp);
+		  item.core = -1;
+		  item.extra = NULL;
+
+		  VEC_safe_push (thread_item_t, context->items, &item);
 		}
 	      while (*bufp++ == ',');	/* comma-separated list */
-	      free_current_contents (&saved_reply);
 	      putpkt ("qsThreadInfo");
 	      getpkt (&rs->buf, &rs->buf_size, 0);
-	      bufp = saved_reply = xstrdup (rs->buf);
+	      bufp = rs->buf;
 	    }
-	  do_cleanups (old_chain);
-	  return;	/* done */
+	  return 1;
+	}
+      else
+	{
+	  /* Packet not recognized.  */
+	  rs->use_threadinfo_query = 0;
 	}
     }
 
-  /* Only qfThreadInfo is supported in non-stop mode.  */
-  if (non_stop)
-    return;
+  return 0;
+}
+
+/* Implement the to_find_new_threads function for the remote
+   targets.  */
+
+static void
+remote_threads_info (struct target_ops *ops)
+{
+  struct remote_state *rs = get_remote_state ();
+  struct threads_listing_context context;
+  struct cleanup *old_chain;
+
+  context.items = NULL;
+  old_chain = make_cleanup (clear_threads_listing_context, &context);
+
+  /* We have a few different mechanisms to fetch the thread list.  Try
+     them all, starting with the most preferred one first, falling
+     back to older methods.  */
+  if (remote_get_threads_with_qxfer (ops, &context)
+      || remote_get_threads_with_qthreadinfo (ops, &context)
+      || remote_get_threads_with_ql (ops, &context))
+    {
+      int i;
+      struct thread_item *item;
 
-  /* Else fall back to old method based on jmetzler protocol.  */
-  rs->use_threadinfo_query = 0;
-  remote_find_new_threads ();
-  return;
+      /* Add threads we don't know about yet to our list.  */
+      for (i = 0;
+	   VEC_iterate (thread_item_t, context.items, i, item);
+	   ++i)
+	{
+	  if (!ptid_equal (item->ptid, null_ptid))
+	    {
+	      struct private_thread_info *info;
+	      /* In non-stop mode, we assume new found threads are
+		 running until proven otherwise with a stop reply.  In
+		 all-stop, we can only get here if all threads are
+		 stopped.  */
+	      int running = non_stop ? 1 : 0;
+
+	      remote_notice_new_inferior (item->ptid, running);
+
+	      info = demand_private_info (item->ptid);
+	      info->core = item->core;
+	      info->extra = item->extra;
+	      item->extra = NULL;
+	    }
+	}
+    }
+
+  do_cleanups (old_chain);
 }
 
 /*
-- 
1.9.3