From 21fe1c752e254167d953fa8c846280f63a3a5290 Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Fri, 24 Nov 2017 10:40:13 -0500 Subject: [PATCH] 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. : Change type to std::string. : Initialize. : Make non-pointer. (thread_item_t): Remove typedef. (DEF_VEC_O(thread_item_t)): Remove. (threads_listing_context) : New method. : New method. : 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 | 33 +++++ gdb/Makefile.in | 2 + gdb/common/rsp-low.c | 13 ++ gdb/common/rsp-low.h | 6 + gdb/remote.c | 197 ++++++++++++------------------ gdb/unittests/rsp-low-selftests.c | 59 +++++++++ 6 files changed, 189 insertions(+), 121 deletions(-) create mode 100644 gdb/unittests/rsp-low-selftests.c diff --git a/gdb/ChangeLog b/gdb/ChangeLog index b0356dbde37..7165ec3180f 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,36 @@ +2017-11-24 Simon Marchi + + * remote.c (struct thread_item): Add constructor, disable copy + construction and copy assignment, define default move + construction and move assignment. + : Change type to std::string. + : Initialize. + : Make non-pointer. + (thread_item_t): Remove typedef. + (DEF_VEC_O(thread_item_t)): Remove. + (threads_listing_context) : New method. + : New method. + : 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 * inferior.h (private_inferior): Define structure type, add diff --git a/gdb/Makefile.in b/gdb/Makefile.in index ec9900ca1df..7198556e5cb 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 85987f70151..9948fbb762c 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 99dc93f4542..039f19c6fe6 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 5445bb298bf..8c76b7ee402 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 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 00000000000..e20fedf8179 --- /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 . */ + +#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); +} -- 2.30.2