remote: C++ify thread_item and threads_listing_context
authorSimon Marchi <simon.marchi@polymtl.ca>
Fri, 24 Nov 2017 15:40:13 +0000 (10:40 -0500)
committerSimon Marchi <simon.marchi@ericsson.com>
Fri, 24 Nov 2017 15:40:15 +0000 (10:40 -0500)
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.

gdb/ChangeLog
gdb/Makefile.in
gdb/common/rsp-low.c
gdb/common/rsp-low.h
gdb/remote.c
gdb/unittests/rsp-low-selftests.c [new file with mode: 0644]

index b0356dbde37415cf6cea1c33f38b264f3e33dfa2..7165ec3180f1d4dc846d78c09c4facf85cc138e1 100644 (file)
@@ -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
index ec9900ca1dfef843f0c3357c34615bc41926f6df..7198556e5cbe12d65f57d1d41d78ca205f56b7e2 100644 (file)
@@ -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
 
index 85987f701519b44550d73e218b349dbc36c2f792..9948fbb762cf26b99b6954c11d6b3f9a9a23c4ec 100644 (file)
@@ -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)
 {
index 99dc93f45428747ccd3d7cb815fe37aadf99ca56..039f19c6fe6416a76ce7d5230f84f3282a95a882 100644 (file)
@@ -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);
index 5445bb298bf6162d0cc23a6b64dc9e81edd49365..8c76b7ee402eceae6399d56bcf0b7658a51c6ddc 100644 (file)
@@ -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 (file)
index 0000000..e20fedf
--- /dev/null
@@ -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);
+}