remote: C++ify thread_item and threads_listing_context

This patch C++ifies the thread_item and threads_listing_context
structures in remote.c.  thread_item::{extra,name} are changed to
std::string.  As a result, there's a bit of awkwardness in
remote_update_thread_list, where we have to xstrdup those strings when
filling the private_thread_info structure.  This is removed in the
following patch, where private_thread_info is also C++ified and its
corresponding fields made std::string too.  The xstrdup then becomes an
std::move.

Other than that there's nothing really special, it's a usual day-to-day
VEC -> vector and char* -> std::string change.  It allows removing a
cleanup in remote_update_thread_list.

Note that an overload of hex2bin that returns a gdb::byte_vector is
added, with corresponding selftests.

gdb/ChangeLog:

	* remote.c (struct thread_item): Add constructor, disable copy
	construction and copy assignment, define default move
	construction and move assignment.
	<extra, name>: Change type to std::string.
	<core>: Initialize.
	<thread_handle>: Make non-pointer.
	(thread_item_t): Remove typedef.
	(DEF_VEC_O(thread_item_t)): Remove.
	(threads_listing_context) <contains_thread>: New method.
	<remove_thread>: New method.
	<items>: Change type to std::vector.
	(clear_threads_listing_context): Remove.
	(threads_listing_context_remove): Remove.
	(remote_newthread_step): Use thread_item constructor, adjust to
	change to std::vector.
	(start_thread): Use thread_item constructor, adjust to change to
	std::vector.
	(end_thread): Adjust to change to std::vector and std::string.
	(remote_get_threads_with_qthreadinfo): Use thread_item
	constructor, adjust to std::vector.
	(remote_update_thread_list): Adjust to change to std::vector and
	std::string, use threads_listing_context methods.
	(remove_child_of_pending_fork): Adjust.
	(remove_new_fork_children): Adjust.
	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add rsp-low-selftests.c.
	(SUBDIR_UNITTESTS_OBS): Add rsp-low-selftests.o.
	* unittests/rsp-low-selftests.c: New file.
	* common/rsp-low.h: Include common/byte-vector.h.
	(hex2bin): New overload.
	* common/rsp-low.c (hex2bin): New overload.

6 files changed, 189 insertions, 121 deletions

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index b0356db..7165ec3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,36 @@
+2017-11-24  Simon Marchi  <simon.marchi@polymtl.ca>
+
+	* remote.c (struct thread_item): Add constructor, disable copy
+	construction and copy assignment, define default move
+	construction and move assignment.
+	<extra, name>: Change type to std::string.
+	<core>: Initialize.
+	<thread_handle>: Make non-pointer.
+	(thread_item_t): Remove typedef.
+	(DEF_VEC_O(thread_item_t)): Remove.
+	(threads_listing_context) <contains_thread>: New method.
+	<remove_thread>: New method.
+	<items>: Change type to std::vector.
+	(clear_threads_listing_context): Remove.
+	(threads_listing_context_remove): Remove.
+	(remote_newthread_step): Use thread_item constructor, adjust to
+	change to std::vector.
+	(start_thread): Use thread_item constructor, adjust to change to
+	std::vector.
+	(end_thread): Adjust to change to std::vector and std::string.
+	(remote_get_threads_with_qthreadinfo): Use thread_item
+	constructor, adjust to std::vector.
+	(remote_update_thread_list): Adjust to change to std::vector and
+	std::string, use threads_listing_context methods.
+	(remove_child_of_pending_fork): Adjust.
+	(remove_new_fork_children): Adjust.
+	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add rsp-low-selftests.c.
+	(SUBDIR_UNITTESTS_OBS): Add rsp-low-selftests.o.
+	* unittests/rsp-low-selftests.c: New file.
+	* common/rsp-low.h: Include common/byte-vector.h.
+	(hex2bin): New overload.
+	* common/rsp-low.c (hex2bin): New overload.
+
 2017-11-24  Simon Marchi  <simon.marchi@ericsson.com>
 
 	* inferior.h (private_inferior): Define structure type, add
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index ec9900c..7198556 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -539,6 +539,7 @@ SUBDIR_UNITTESTS_SRCS = \
 	unittests/offset-type-selftests.c \
 	unittests/optional-selftests.c \
 	unittests/ptid-selftests.c \
+	unittests/rsp-low-selftests.c \
 	unittests/scoped_restore-selftests.c \
 	unittests/xml-utils-selftests.c
 
@@ -553,6 +554,7 @@ SUBDIR_UNITTESTS_OBS = \
 	offset-type-selftests.o \
 	optional-selftests.o \
 	ptid-selftests.o \
+	rsp-low-selftests.o \
 	scoped_restore-selftests.o \
 	xml-utils-selftests.o
 
diff --git a/gdb/common/rsp-low.c b/gdb/common/rsp-low.c
index 85987f7..9948fbb 100644
--- a/gdb/common/rsp-low.c
+++ b/gdb/common/rsp-low.c
@@ -132,6 +132,19 @@ hex2bin (const char *hex, gdb_byte *bin, int count)
 
 /* See rsp-low.h.  */
 
+gdb::byte_vector
+hex2bin (const char *hex)
+{
+  size_t bin_len = strlen (hex) / 2;
+  gdb::byte_vector bin (bin_len);
+
+  hex2bin (hex, bin.data (), bin_len);
+
+  return bin;
+}
+
+/* See rsp-low.h.  */
+
 std::string
 hex2str (const char *hex)
 {
diff --git a/gdb/common/rsp-low.h b/gdb/common/rsp-low.h
index 99dc93f..039f19c 100644
--- a/gdb/common/rsp-low.h
+++ b/gdb/common/rsp-low.h
@@ -20,6 +20,8 @@
 #ifndef COMMON_RSP_LOW_H
 #define COMMON_RSP_LOW_H
 
+#include "common/byte-vector.h"
+
 /* Convert hex digit A to a number, or throw an exception.  */
 
 extern int fromhex (int a);
@@ -52,6 +54,10 @@ extern const char *unpack_varlen_hex (const char *buff, ULONGEST *result);
 
 extern int hex2bin (const char *hex, gdb_byte *bin, int count);
 
+/* Like the above, but return a gdb::byte_vector.  */
+
+gdb::byte_vector hex2bin (const char *hex);
+
 /* Like hex2bin, but return a std::string.  */
 
 extern std::string hex2str (const char *hex);
diff --git a/gdb/remote.c b/gdb/remote.c
index 5445bb2..8c76b7e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2972,25 +2972,33 @@ remote_threadlist_iterator (rmt_thread_action stepfunction, void *context,
 
 /* A thread found on the remote target.  */
 
-typedef struct thread_item
+struct thread_item
 {
+  explicit thread_item (ptid_t ptid_)
+  : ptid (ptid_)
+  {}
+
+  thread_item (thread_item &&other) = default;
+  thread_item &operator= (thread_item &&other) = default;
+
+  DISABLE_COPY_AND_ASSIGN (thread_item);
+
   /* The thread's PTID.  */
   ptid_t ptid;
 
-  /* The thread's extra info.  May be NULL.  */
-  char *extra;
+  /* The thread's extra info.  */
+  std::string extra;
 
-  /* The thread's name.  May be NULL.  */
-  char *name;
+  /* The thread's name.  */
+  std::string name;
 
   /* The core the thread was running on.  -1 if not known.  */
-  int core;
+  int core = -1;
 
   /* The thread handle associated with the thread.  */
-  gdb::byte_vector *thread_handle;
+  gdb::byte_vector thread_handle;
 
-} 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
@@ -2998,66 +3006,54 @@ DEF_VEC_O(thread_item_t);
 
 struct threads_listing_context
 {
-  /* The threads found on the remote target.  */
-  VEC (thread_item_t) *items;
-};
+  /* Return true if this object contains an entry for a thread with ptid
+     PTID.  */
 
-/* Discard the contents of the constructed thread listing context.  */
+  bool contains_thread (ptid_t ptid) const
+  {
+    auto match_ptid = [&] (const thread_item &item)
+      {
+	return item.ptid == ptid;
+      };
 
-static void
-clear_threads_listing_context (void *p)
-{
-  struct threads_listing_context *context
-    = (struct threads_listing_context *) p;
-  int i;
-  struct thread_item *item;
+    auto it = std::find_if (this->items.begin (),
+			    this->items.end (),
+			    match_ptid);
 
-  for (i = 0; VEC_iterate (thread_item_t, context->items, i, item); ++i)
-    {
-      xfree (item->extra);
-      xfree (item->name);
-      delete item->thread_handle;
-    }
+    return it != this->items.end ();
+  }
 
-  VEC_free (thread_item_t, context->items);
-}
+  /* Remove the thread with ptid PTID.  */
 
-/* Remove the thread specified as the related_pid field of WS
-   from the CONTEXT list.  */
+  void remove_thread (ptid_t ptid)
+  {
+    auto match_ptid = [&] (const thread_item &item)
+      {
+        return item.ptid == ptid;
+      };
 
-static void
-threads_listing_context_remove (struct target_waitstatus *ws,
-				struct threads_listing_context *context)
-{
-  struct thread_item *item;
-  int i;
-  ptid_t child_ptid = ws->value.related_pid;
+    auto it = std::remove_if (this->items.begin (),
+			      this->items.end (),
+			      match_ptid);
 
-  for (i = 0; VEC_iterate (thread_item_t, context->items, i, item); ++i)
-    {
-      if (ptid_equal (item->ptid, child_ptid))
-	{
-	  VEC_ordered_remove (thread_item_t, context->items, i);
-	  break;
-	}
-    }
-}
+    if (it != this->items.end ())
+      this->items.erase (it);
+  }
+
+  /* The threads found on the remote target.  */
+  std::vector<thread_item> items;
+};
 
 static int
 remote_newthread_step (threadref *ref, void *data)
 {
   struct threads_listing_context *context
     = (struct threads_listing_context *) data;
-  struct thread_item item;
-  int pid = ptid_get_pid (inferior_ptid);
-
-  item.ptid = ptid_build (pid, threadref_to_int (ref), 0);
-  item.core = -1;
-  item.name = NULL;
-  item.extra = NULL;
-  item.thread_handle = nullptr;
+  int pid = inferior_ptid.pid ();
+  int lwp = threadref_to_int (ref);
+  ptid_t ptid (pid, lwp);
 
-  VEC_safe_push (thread_item_t, context->items, &item);
+  context->items.emplace_back (ptid);
 
   return 1;			/* continue iterator */
 }
@@ -3109,37 +3105,25 @@ start_thread (struct gdb_xml_parser *parser,
 {
   struct threads_listing_context *data
     = (struct threads_listing_context *) user_data;
-
-  struct thread_item item;
-  char *id;
   struct gdb_xml_value *attr;
 
-  id = (char *) xml_find_attribute (attributes, "id")->value;
-  item.ptid = read_ptid (id, NULL);
+  char *id = (char *) xml_find_attribute (attributes, "id")->value;
+  ptid_t ptid = read_ptid (id, NULL);
+
+  data->items.emplace_back (ptid);
+  thread_item &item = data->items.back ();
 
   attr = xml_find_attribute (attributes, "core");
   if (attr != NULL)
     item.core = *(ULONGEST *) attr->value;
-  else
-    item.core = -1;
 
   attr = xml_find_attribute (attributes, "name");
-  item.name = attr != NULL ? xstrdup ((const char *) attr->value) : NULL;
+  if (attr != NULL)
+    item.name = (const char *) attr->value;
 
   attr = xml_find_attribute (attributes, "handle");
   if (attr != NULL)
-    {
-      item.thread_handle = new gdb::byte_vector
-                             (strlen ((const char *) attr->value) / 2);
-      hex2bin ((const char *) attr->value, item.thread_handle->data (),
-               item.thread_handle->size ());
-    }
-  else
-    item.thread_handle = nullptr;
-
-  item.extra = 0;
-
-  VEC_safe_push (thread_item_t, data->items, &item);
+    item.thread_handle = hex2bin ((const char *) attr->value);
 }
 
 static void
@@ -3150,8 +3134,8 @@ end_thread (struct gdb_xml_parser *parser,
   struct threads_listing_context *data
     = (struct threads_listing_context *) user_data;
 
-  if (body_text && *body_text)
-    VEC_last (thread_item_t, data->items)->extra = xstrdup (body_text);
+  if (body_text != NULL && *body_text != '\0')
+    data->items.back ().extra = body_text;
 }
 
 const struct gdb_xml_attribute thread_attributes[] = {
@@ -3227,15 +3211,8 @@ remote_get_threads_with_qthreadinfo (struct target_ops *ops,
 	    {
 	      do
 		{
-		  struct thread_item item;
-
-		  item.ptid = read_ptid (bufp, &bufp);
-		  item.core = -1;
-		  item.name = NULL;
-		  item.extra = NULL;
-		  item.thread_handle = nullptr;
-
-		  VEC_safe_push (thread_item_t, context->items, &item);
+		  ptid_t ptid = read_ptid (bufp, &bufp);
+		  context->items.emplace_back (ptid);
 		}
 	      while (*bufp++ == ',');	/* comma-separated list */
 	      putpkt ("qsThreadInfo");
@@ -3261,12 +3238,8 @@ static void
 remote_update_thread_list (struct target_ops *ops)
 {
   struct threads_listing_context context;
-  struct cleanup *old_chain;
   int got_list = 0;
 
-  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.  */
@@ -3275,12 +3248,11 @@ remote_update_thread_list (struct target_ops *ops)
       || remote_get_threads_with_ql (ops, &context))
     {
       int i;
-      struct thread_item *item;
       struct thread_info *tp, *tmp;
 
       got_list = 1;
 
-      if (VEC_empty (thread_item_t, context.items)
+      if (context.items.empty ()
 	  && remote_thread_always_alive (ops, inferior_ptid))
 	{
 	  /* Some targets don't really support threads, but still
@@ -3288,7 +3260,6 @@ remote_update_thread_list (struct target_ops *ops)
 	     listing packets, instead of replying "packet not
 	     supported".  Exit early so we don't delete the main
 	     thread.  */
-	  do_cleanups (old_chain);
 	  return;
 	}
 
@@ -3297,15 +3268,7 @@ remote_update_thread_list (struct target_ops *ops)
 	 target.  */
       ALL_THREADS_SAFE (tp, tmp)
 	{
-	  for (i = 0;
-	       VEC_iterate (thread_item_t, context.items, i, item);
-	       ++i)
-	    {
-	      if (ptid_equal (item->ptid, tp->ptid))
-		break;
-	    }
-
-	  if (i == VEC_length (thread_item_t, context.items))
+	  if (!context.contains_thread (tp->ptid))
 	    {
 	      /* Not found.  */
 	      delete_thread (tp->ptid);
@@ -3318,11 +3281,9 @@ remote_update_thread_list (struct target_ops *ops)
       remove_new_fork_children (&context);
 
       /* And now add threads we don't know about yet to our list.  */
-      for (i = 0;
-	   VEC_iterate (thread_item_t, context.items, i, item);
-	   ++i)
+      for (thread_item &item : context.items)
 	{
-	  if (!ptid_equal (item->ptid, null_ptid))
+	  if (item.ptid != null_ptid)
 	    {
 	      struct private_thread_info *info;
 	      /* In non-stop mode, we assume new found threads are
@@ -3331,16 +3292,14 @@ remote_update_thread_list (struct target_ops *ops)
 		 stopped.  */
 	      int executing = target_is_non_stop_p () ? 1 : 0;
 
-	      remote_notice_new_inferior (item->ptid, executing);
+	      remote_notice_new_inferior (item.ptid, executing);
 
-	      info = get_private_info_ptid (item->ptid);
-	      info->core = item->core;
-	      info->extra = item->extra;
-	      item->extra = NULL;
-	      info->name = item->name;
-	      item->name = NULL;
-	      info->thread_handle = item->thread_handle;
-	      item->thread_handle = nullptr;
+	      info = get_private_info_ptid (item.ptid);
+	      info->core = item.core;
+	      info->extra = xstrdup (item.extra.c_str ());
+	      info->name = xstrdup (item.name.c_str ());
+	      info->thread_handle
+		= new gdb::byte_vector (std::move (item.thread_handle));
 	    }
 	}
     }
@@ -3353,8 +3312,6 @@ remote_update_thread_list (struct target_ops *ops)
 	 no-op.  See remote_thread_alive.  */
       prune_threads ();
     }
-
-  do_cleanups (old_chain);
 }
 
 /*
@@ -6521,7 +6478,7 @@ remove_child_of_pending_fork (QUEUE (stop_reply_p) *q,
   if (event->ws.kind == TARGET_WAITKIND_FORKED
       || event->ws.kind == TARGET_WAITKIND_VFORKED
       || event->ws.kind == TARGET_WAITKIND_THREAD_EXITED)
-    threads_listing_context_remove (&event->ws, context);
+    context->remove_thread (event->ws.value.related_pid);
 
   return 1;
 }
@@ -6547,9 +6504,7 @@ remove_new_fork_children (struct threads_listing_context *context)
       struct target_waitstatus *ws = thread_pending_fork_status (thread);
 
       if (is_pending_fork_parent (ws, pid, thread->ptid))
-	{
-	  threads_listing_context_remove (ws, context);
-	}
+	context->remove_thread (ws->value.related_pid);
     }
 
   /* Check for any pending fork events (not reported or processed yet)
diff --git a/gdb/unittests/rsp-low-selftests.c b/gdb/unittests/rsp-low-selftests.c
new file mode 100644
index 0000000..e20fedf
--- /dev/null
+++ b/gdb/unittests/rsp-low-selftests.c
@@ -0,0 +1,59 @@
+/* Unit tests for the rsp-low.c file.
+
+   Copyright (C) 2017 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "selftest.h"
+#include "common/rsp-low.h"
+
+namespace selftests {
+namespace rsp_low {
+
+/* Test the variant of hex2bin that returns a byte_vector.  */
+
+static void test_hex2bin_byte_vector ()
+{
+  gdb::byte_vector bv;
+
+  /* Test an empty string.  */
+  bv = hex2bin ("");
+  SELF_CHECK (bv.size () == 0);
+
+  /* Test a well-formated hex string.  */
+  bv = hex2bin ("abcd01");
+  SELF_CHECK (bv.size () == 3);
+  SELF_CHECK (bv[0] == 0xab);
+  SELF_CHECK (bv[1] == 0xcd);
+  SELF_CHECK (bv[2] == 0x01);
+
+  /* Test an odd-length hex string.  */
+  bv = hex2bin ("0123c");
+  SELF_CHECK (bv.size () == 2);
+  SELF_CHECK (bv[0] == 0x01);
+  SELF_CHECK (bv[1] == 0x23);
+}
+
+} /* namespace rsp_low */
+} /* namespace selftests */
+
+void
+_initialize_rsp_low_selftests ()
+{
+  selftests::register_test ("hex2bin_byte_vector",
+			    selftests::rsp_low::test_hex2bin_byte_vector);
+}