From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23994 invoked by alias); 23 Jul 2017 21:47:00 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 23966 invoked by uid 89); 23 Jul 2017 21:46:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 23 Jul 2017 21:46:57 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v6NLkoIE012978 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Sun, 23 Jul 2017 17:46:55 -0400 Received: by simark.ca (Postfix, from userid 112) id E621A1E5E4; Sun, 23 Jul 2017 17:46:50 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id ABF861E009; Sun, 23 Jul 2017 17:46:49 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sun, 23 Jul 2017 21:47:00 -0000 From: Simon Marchi To: Kevin Buettner Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v3 6/7] Add thread_handle_to_thread_info support for remote targets In-Reply-To: <20170718175544.056dbec6@pinnacle.lan> References: <20170718174156.5da204b0@pinnacle.lan> <20170718175544.056dbec6@pinnacle.lan> Message-ID: <9c3ac4259fba5e9ece279b3db6cbbdcf@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Sun, 23 Jul 2017 21:46:51 +0000 X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg00347.txt.bz2 Hi Kevin, On 2017-07-19 02:55, Kevin Buettner wrote: > This patch adds support to remote targets for converting a thread > handle to a thread_info struct pointer. > > A thread handle is fetched via a "handle" attribute which has been > added to the qXfer:threads:read query packet. An implementation is > provided in gdbserver for targets using the Linux kernel. > > gdb/gdbserver/ChangeLog: > > * linux-low.h (struct lwp_info): Add new field, thread_handle. > (thread_db_thread_handle): Declare. > * linux-low.c (linux_target_ops): Initialize thread_handle. > * server.c (handle_qxfer_threads_worker): Add support for > "handle" attribute. > * target.h (struct target_ops): Add new function pointer, > thread_handle. > (target_thread_handle): Define. > * thread-db.c (find_one_thread, attach_thread): Set thread_handle > field in lwp. > (thread_db_thread_handle): New function. > > gdb/ChangeLog: > > * remote.c (vector): Include. > (struct private_thread_info): Add field, thread_handle. > (free_private_thread_info): Deallocate storage associated with > thread handle. > (get_private_info_thread): Initialize `thread_handle' field. > (struct thread_item): Add field, thread_handle. > (clear_threads_listing_context): Deallocate storage associated > with thread handle. > (start_thread): Add support for "handle" attribute. > (thread_attributes): Add "handle". > (remote_get_threads_with_qthreadinfo): Initialize thread_handle > field. > (remote_update_thread_list): Update thread_handle. > (remote_thread_handle_to_thread_info): New function. > (init_remote_ops): Initialize to_thread_handle_to_thread_info. > --- > gdb/gdbserver/linux-low.c | 5 +++++ > gdb/gdbserver/linux-low.h | 4 +++- > gdb/gdbserver/server.c | 10 +++++++++ > gdb/gdbserver/target.h | 10 +++++++++ > gdb/gdbserver/thread-db.c | 30 ++++++++++++++++++++++++++ > gdb/remote.c | 54 > +++++++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 112 insertions(+), 1 deletion(-) > > diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c > index 9d831e7..1a15a74 100644 > --- a/gdb/gdbserver/linux-low.c > +++ b/gdb/gdbserver/linux-low.c > @@ -7696,6 +7696,11 @@ static struct target_ops linux_target_ops = { > linux_supports_software_single_step, > linux_supports_catch_syscall, > linux_get_ipa_tdesc_idx, > +#if USE_THREAD_DB > + thread_db_thread_handle, > +#else > + NULL, > +#endif Given your implementation of thread_db_thread_handle, I think we could always install the callback and it will just return false. If we can avoid a preprocessor conditional, I think it's preferable. > }; > > #ifdef HAVE_LINUX_REGSETS > diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h > index 86cfe51..8e3eecb 100644 > --- a/gdb/gdbserver/linux-low.h > +++ b/gdb/gdbserver/linux-low.h > @@ -374,6 +374,8 @@ struct lwp_info > /* The thread handle, used for e.g. TLS access. Only valid if > THREAD_KNOWN is set. */ > td_thrhandle_t th; New line between the two fields? > + /* The pthread_t handle. */ > + thread_t thread_handle; > #endif > > /* Arch-specific additions. */ > @@ -415,6 +417,6 @@ int thread_db_look_up_one_symbol (const char > *name, CORE_ADDR *addrp); > whatever is required have the clone under thread_db's control. */ > void thread_db_notice_clone (struct process_info *proc, ptid_t lwp); > > -int thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int > *handle_len); > +bool thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int > *handle_len); > > extern int have_ptrace_getregset; > diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c > index 3838351..ac8f812 100644 > --- a/gdb/gdbserver/server.c > +++ b/gdb/gdbserver/server.c > @@ -1526,6 +1526,9 @@ handle_qxfer_threads_worker (struct > inferior_list_entry *inf, void *arg) > int core = target_core_of_thread (ptid); > char core_s[21]; > const char *name = target_thread_name (ptid); > + int handle_len; > + gdb_byte *handle; > + int handle_status = target_thread_handle (ptid, &handle, > &handle_len); bool > > write_ptid (ptid_s, ptid); > > @@ -1540,6 +1543,13 @@ handle_qxfer_threads_worker (struct > inferior_list_entry *inf, void *arg) > if (name != NULL) > buffer_xml_printf (buffer, " name=\"%s\"", name); > > + if (handle_status) > + { > + char *handle_s = (char *) alloca (handle_len * 2 + 1); > + bin2hex (handle, handle_s, handle_len); > + buffer_xml_printf (buffer, " handle=\"%s\"", handle_s); > + } > + > buffer_xml_printf (buffer, "/>\n"); > } > > diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h > index be89258..ebff52e 100644 > --- a/gdb/gdbserver/target.h > +++ b/gdb/gdbserver/target.h > @@ -475,6 +475,11 @@ struct target_ops > > /* Return tdesc index for IPA. */ > int (*get_ipa_tdesc_idx) (void); > + > + /* Thread ID to (numeric) thread handle: Return true on success and > + false for failure. Return pointer to thread handle via HANDLE > + and the handle's length via HANDLE_LEN. */ > + bool (*thread_handle) (ptid_t ptid, gdb_byte **handle, int > *handle_len); > }; > > extern struct target_ops *the_target; > @@ -693,6 +698,11 @@ void done_accessing_memory (void); > (the_target->thread_name ? (*the_target->thread_name) (ptid) \ > : NULL) > > +#define target_thread_handle(ptid, handle, handle_len) \ > + (the_target->thread_handle ? (*the_target->thread_handle) \ > + (ptid, handle, handle_len) \ > + : 0) Nit: this should be false. > + > int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, > int len); > > int write_inferior_memory (CORE_ADDR memaddr, const unsigned char > *myaddr, > diff --git a/gdb/gdbserver/thread-db.c b/gdb/gdbserver/thread-db.c > index eff1914..3a9ebbb 100644 > --- a/gdb/gdbserver/thread-db.c > +++ b/gdb/gdbserver/thread-db.c > @@ -200,6 +200,7 @@ find_one_thread (ptid_t ptid) > > lwp->thread_known = 1; > lwp->th = th; > + lwp->thread_handle = ti.ti_tid; > > return 1; > } > @@ -231,6 +232,7 @@ attach_thread (const td_thrhandle_t *th_p, > td_thrinfo_t *ti_p) > gdb_assert (lwp != NULL); > lwp->thread_known = 1; > lwp->th = *th_p; > + lwp->thread_handle = ti_p->ti_tid; > > return 1; > } > @@ -439,6 +441,34 @@ thread_db_get_tls_address (struct thread_info > *thread, CORE_ADDR offset, > return err; > } > Add /* See linux-low.h. */ > +bool > +thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int > *handle_len) > +{ > + struct thread_db *thread_db; > + struct lwp_info *lwp; > + struct thread_info *thread > + = (struct thread_info *) find_inferior_id (&all_threads, ptid); > + > + if (thread == NULL) > + return false; > + > + thread_db = get_thread_process (thread)->priv->thread_db; > + > + if (thread_db == NULL) > + return false; > + > + lwp = get_thread_lwp (thread); > + > + if (!lwp->thread_known && !find_one_thread (thread->entry.id)) > + return false; > + > + gdb_assert (lwp->thread_known); > + > + *handle = (gdb_byte *) &lwp->thread_handle; > + *handle_len = sizeof (lwp->thread_handle); > + return true; > +} > + > #ifdef USE_LIBTHREAD_DB_DIRECTLY > > static int > diff --git a/gdb/remote.c b/gdb/remote.c > index 8e8ee6f..8cf65e7 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -72,6 +72,7 @@ > #include "btrace.h" > #include "record-btrace.h" > #include > +#include > > /* Temp hacks for tracepoint encoding migration. */ > static char *target_buf; > @@ -451,6 +452,10 @@ struct private_thread_info > char *name; > int core; > > + /* Thread handle, perhaps a pthread_t or thread_t value, stored as a > + sequence of bytes. */ > + std::vector *thread_handle; We now have a gdb::byte_vector (common/byte-vector.h) to use in these situations. It should be a drop-in replacement. > @@ -13515,6 +13543,30 @@ remote_execution_direction (struct target_ops > *self) > return rs->last_resume_exec_dir; > } > > +/* Return pointer to the thread_info struct which corresponds to > + THREAD_HANDLE (having length HANDLE_LEN). */ New line here. > +static struct thread_info * > +remote_thread_handle_to_thread_info (struct target_ops *ops, > + const gdb_byte *thread_handle, > + int handle_len, > + struct inferior *inf) > +{ > + struct thread_info *tp; > + > + ALL_NON_EXITED_THREADS (tp) > + { > + struct private_thread_info *priv = get_private_info_thread (tp); > + > + if (tp->inf == inf && priv != NULL > + && handle_len == priv->thread_handle->size () > + && memcmp (thread_handle, priv->thread_handle->data (), > + handle_len) == 0) > + return tp; > + } > + > + return NULL; > +} > + > static void > init_remote_ops (void) > { Thanks! Simon