From patchwork Fri Nov 24 04:34:14 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Marchi X-Patchwork-Id: 24488 Received: (qmail 18360 invoked by alias); 24 Nov 2017 04:34:39 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 18276 invoked by uid 89); 24 Nov 2017 04:34:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, KB_WAM_FROM_NAME_SINGLEWORD, RCVD_IN_DNSWL_NONE, SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy= X-HELO: barracuda.ebox.ca Received: from barracuda.ebox.ca (HELO barracuda.ebox.ca) (96.127.255.19) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 24 Nov 2017 04:34:35 +0000 X-ASG-Debug-ID: 1511498068-0c856e65d43caa420001-fS2M51 Received: from smtp.ebox.ca (smtp.electronicbox.net [96.127.255.82]) by barracuda.ebox.ca with ESMTP id 7geFbPoSTeWTu5Wi (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 23 Nov 2017 23:34:28 -0500 (EST) X-Barracuda-Envelope-From: simon.marchi@polymtl.ca X-Barracuda-RBL-Trusted-Forwarder: 96.127.255.82 Received: from simark.lan (192-222-251-162.qc.cable.ebox.net [192.222.251.162]) by smtp.ebox.ca (Postfix) with ESMTP id 56420441D64; Thu, 23 Nov 2017 23:34:25 -0500 (EST) From: Simon Marchi X-Barracuda-Effective-Source-IP: 192-222-251-162.qc.cable.ebox.net[192.222.251.162] X-Barracuda-Apparent-Source-IP: 192.222.251.162 X-Barracuda-RBL-IP: 192.222.251.162 To: gdb-patches@sourceware.org Cc: Simon Marchi Subject: [PATCH v2 2/4] remote: C++ify thread_item and threads_listing_context Date: Thu, 23 Nov 2017 23:34:14 -0500 X-ASG-Orig-Subj: [PATCH v2 2/4] remote: C++ify thread_item and threads_listing_context Message-Id: <20171124043416.20706-2-simon.marchi@polymtl.ca> In-Reply-To: <20171124043416.20706-1-simon.marchi@polymtl.ca> References: <20171124043416.20706-1-simon.marchi@polymtl.ca> X-Barracuda-Connect: smtp.electronicbox.net[96.127.255.82] X-Barracuda-Start-Time: 1511498068 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://96.127.255.19:443/cgi-mod/mark.cgi X-Barracuda-Scan-Msg-Size: 17076 X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 1.50 X-Barracuda-Spam-Status: No, SCORE=1.50 using global scores of TAG_LEVEL=1000.0 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=8.0 tests=BSF_SC0_MV0713, BSF_SC0_MV0713_2 X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.45158 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.50 BSF_SC0_MV0713 Custom rule MV0713 1.00 BSF_SC0_MV0713_2 BSF_SC0_MV0713_2 X-IsSubscribed: yes New in v2: - Make thread_item::thread_handle non-pointer - Make thread_item constructor explicit 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/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 ++++++++++++ 5 files changed, 156 insertions(+), 121 deletions(-) create mode 100644 gdb/unittests/rsp-low-selftests.c diff --git a/gdb/Makefile.in b/gdb/Makefile.in index 0a1a769541..26942e9af8 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 85987f7015..9948fbb762 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 99dc93f454..039f19c6fe 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 5445bb298b..8c76b7ee40 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 0000000000..e20fedf817 --- /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); +}