From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38216 invoked by alias); 23 Jul 2017 20:39:33 -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 38207 invoked by uid 89); 23 Jul 2017 20:39:32 -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 20:39:30 +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 v6NKdN4u025459 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Sun, 23 Jul 2017 16:39:28 -0400 Received: by simark.ca (Postfix, from userid 112) id 8D68B1E5E6; Sun, 23 Jul 2017 16:39:23 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 0BA5A1E009; Sun, 23 Jul 2017 16:39:22 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sun, 23 Jul 2017 20:39:00 -0000 From: Simon Marchi To: Kevin Buettner Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v3 1/7] Add target method for converting thread handle to thread_info struct pointer In-Reply-To: <20170718175430.30431119@pinnacle.lan> References: <20170718174156.5da204b0@pinnacle.lan> <20170718175430.30431119@pinnacle.lan> Message-ID: <2bbc4bf22765912f3121e119bde37777@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 20:39:23 +0000 X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg00343.txt.bz2 Hi Kevin, Just some formatting stuff, otherwise the patch LGTM. On 2017-07-19 02:54, Kevin Buettner wrote: > This patch adds a target method named > `to_thread_handle_to_thread_info'. > It is intended to map a thread library specific thread handle (such as > pthread_t for the pthread library) to the corresponding GDB internal > thread_info struct (pointer). > > An implementation is provided for Linux pthreads; see > linux-thread-db.c. > > gdb/ChangeLog: > > * target.h (struct target_ops): Add > to_thread_handle_to_thread_info. > (target_thread_handle_to_thread_info): Declare. > * target.c (target_thread_handle_to_thread_info): New function. > * target-delegates.c: Regenerate. > * gdbthread.h (find_thread_by_handle): Declare. > * thread.c (find_thread_by_handle): New function. > * linux-thread-db.c (thread_db_thread_handle_to_thread_info): New > function. > (init_thread_db_ops): Register > thread_db_thread_handle_to_thread_info. > --- > gdb/gdbthread.h | 4 ++++ > gdb/linux-thread-db.c | 31 +++++++++++++++++++++++++++++++ > gdb/target-delegates.c | 37 +++++++++++++++++++++++++++++++++++++ > gdb/target.c | 9 +++++++++ > gdb/target.h | 11 +++++++++++ > gdb/thread.c | 10 ++++++++++ > 6 files changed, 102 insertions(+) > > diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h > index 046bf95..c16a0d2 100644 > --- a/gdb/gdbthread.h > +++ b/gdb/gdbthread.h > @@ -448,6 +448,10 @@ extern struct thread_info *find_thread_ptid > (ptid_t ptid); > /* Find thread by GDB global thread ID. */ > struct thread_info *find_thread_global_id (int global_id); > > +/* Find thread by thread library specific handle in inferior INF. */ > +struct thread_info *find_thread_by_handle (struct value > *thread_handle, > + struct inferior *inf); > + > /* Finds the first thread of the inferior given by PID. If PID is -1, > returns the first thread in the list. */ > struct thread_info *first_thread_of_process (int pid); > diff --git a/gdb/linux-thread-db.c b/gdb/linux-thread-db.c > index 86254f8..14dc5d2 100644 > --- a/gdb/linux-thread-db.c > +++ b/gdb/linux-thread-db.c > @@ -1410,6 +1410,36 @@ thread_db_extra_thread_info (struct target_ops > *self, > return NULL; > } > > +/* Return pointer to the thread_info struct which corresponds to > + THREAD_HANDLE (having length HANDLE_LEN). */ Please insert a newline between the comment and function (I know there are tons of bad examples). > +static struct thread_info * > +thread_db_thread_handle_to_thread_info (struct target_ops *ops, > + const gdb_byte *thread_handle, > + int handle_len, > + struct inferior *inf) > +{ > + struct thread_info *tp; > + thread_t handle_tid; > + > + /* Thread handle sizes must match in order to proceed. We don't use > an > + assert here because the resulting internal error will cause GDB > to > + exit. This isn't necessarily an internal error due to the > possibility > + of garbage being passed as the thread handle via the python > interface. */ > + if (handle_len != sizeof (handle_tid)) > + error (_("Thread handle size mismatch: %d vs %d (from > libthread_db)"), > + handle_len, (int) sizeof (handle_tid)); You could use %zu for printing the sizeof. I just learned that the z length modifier is standard in C++11, so we might as well use it. > + > + handle_tid = * (const thread_t *) thread_handle; > + > + ALL_NON_EXITED_THREADS (tp) > + { > + if (tp->inf == inf && tp->priv != NULL && handle_tid == > tp->priv->tid) > + return tp; > + } > + > + return NULL; > +} > + > /* Get the address of the thread local variable in load module LM > which > is stored at OFFSET within the thread local storage for thread > PTID. */ > > @@ -1687,6 +1717,7 @@ init_thread_db_ops (void) > = thread_db_get_thread_local_address; > thread_db_ops.to_extra_thread_info = thread_db_extra_thread_info; > thread_db_ops.to_get_ada_task_ptid = thread_db_get_ada_task_ptid; > + thread_db_ops.to_thread_handle_to_thread_info = > thread_db_thread_handle_to_thread_info; > thread_db_ops.to_magic = OPS_MAGIC; > > complete_target_initialization (&thread_db_ops); > diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c > index 88e3e0b..dea1d9f 100644 > --- a/gdb/target-delegates.c > +++ b/gdb/target-delegates.c > @@ -1583,6 +1583,39 @@ debug_thread_name (struct target_ops *self, > struct thread_info *arg1) > return result; > } > > +static struct thread_info * > +delegate_thread_handle_to_thread_info (struct target_ops *self, const > gdb_byte *arg1, int arg2, struct inferior *arg3) > +{ > + self = self->beneath; > + return self->to_thread_handle_to_thread_info (self, arg1, arg2, > arg3); > +} > + > +static struct thread_info * > +tdefault_thread_handle_to_thread_info (struct target_ops *self, const > gdb_byte *arg1, int arg2, struct inferior *arg3) > +{ > + return NULL; > +} > + > +static struct thread_info * > +debug_thread_handle_to_thread_info (struct target_ops *self, const > gdb_byte *arg1, int arg2, struct inferior *arg3) > +{ > + struct thread_info * result; > + fprintf_unfiltered (gdb_stdlog, "-> > %s->to_thread_handle_to_thread_info (...)\n", > debug_target.to_shortname); > + result = debug_target.to_thread_handle_to_thread_info > (&debug_target, arg1, arg2, arg3); > + fprintf_unfiltered (gdb_stdlog, "<- > %s->to_thread_handle_to_thread_info (", debug_target.to_shortname); > + target_debug_print_struct_target_ops_p (&debug_target); > + fputs_unfiltered (", ", gdb_stdlog); > + target_debug_print_const_gdb_byte_p (arg1); > + fputs_unfiltered (", ", gdb_stdlog); > + target_debug_print_int (arg2); > + fputs_unfiltered (", ", gdb_stdlog); > + target_debug_print_struct_inferior_p (arg3); > + fputs_unfiltered (") = ", gdb_stdlog); > + target_debug_print_struct_thread_info_p (result); > + fputs_unfiltered ("\n", gdb_stdlog); > + return result; > +} > + > static void > delegate_stop (struct target_ops *self, ptid_t arg1) > { > @@ -4267,6 +4300,8 @@ install_delegators (struct target_ops *ops) > ops->to_extra_thread_info = delegate_extra_thread_info; > if (ops->to_thread_name == NULL) > ops->to_thread_name = delegate_thread_name; > + if (ops->to_thread_handle_to_thread_info == NULL) > + ops->to_thread_handle_to_thread_info = > delegate_thread_handle_to_thread_info; > if (ops->to_stop == NULL) > ops->to_stop = delegate_stop; > if (ops->to_interrupt == NULL) > @@ -4522,6 +4557,7 @@ install_dummy_methods (struct target_ops *ops) > ops->to_pid_to_str = default_pid_to_str; > ops->to_extra_thread_info = tdefault_extra_thread_info; > ops->to_thread_name = tdefault_thread_name; > + ops->to_thread_handle_to_thread_info = > tdefault_thread_handle_to_thread_info; > ops->to_stop = tdefault_stop; > ops->to_interrupt = tdefault_interrupt; > ops->to_pass_ctrlc = default_target_pass_ctrlc; > @@ -4681,6 +4717,7 @@ init_debug_target (struct target_ops *ops) > ops->to_pid_to_str = debug_pid_to_str; > ops->to_extra_thread_info = debug_extra_thread_info; > ops->to_thread_name = debug_thread_name; > + ops->to_thread_handle_to_thread_info = > debug_thread_handle_to_thread_info; > ops->to_stop = debug_stop; > ops->to_interrupt = debug_interrupt; > ops->to_pass_ctrlc = debug_pass_ctrlc; > diff --git a/gdb/target.c b/gdb/target.c > index e526bcc..23f4b9f 100644 > --- a/gdb/target.c > +++ b/gdb/target.c > @@ -2315,6 +2315,15 @@ target_thread_name (struct thread_info *info) > return current_target.to_thread_name (¤t_target, info); > } > > +struct thread_info * > +target_thread_handle_to_thread_info (const gdb_byte * thread_handle, No space after *. > + int handle_len, > + struct inferior *inf) > +{ > + return current_target.to_thread_handle_to_thread_info > + (¤t_target, thread_handle, handle_len, inf); > +} > + > void > target_resume (ptid_t ptid, int step, enum gdb_signal signal) > { > diff --git a/gdb/target.h b/gdb/target.h > index c0155c1..1bb8c0f 100644 > --- a/gdb/target.h > +++ b/gdb/target.h > @@ -646,6 +646,11 @@ struct target_ops > TARGET_DEFAULT_RETURN (NULL); > const char *(*to_thread_name) (struct target_ops *, struct > thread_info *) > TARGET_DEFAULT_RETURN (NULL); > + struct thread_info *(*to_thread_handle_to_thread_info) (struct > target_ops *, > + const > gdb_byte *, > + int, > + struct inferior *inf) > + TARGET_DEFAULT_RETURN (NULL); > void (*to_stop) (struct target_ops *, ptid_t) > TARGET_DEFAULT_IGNORE (); > void (*to_interrupt) (struct target_ops *, ptid_t) > @@ -1857,6 +1862,12 @@ extern const char *normal_pid_to_str (ptid_t > ptid); > > extern const char *target_thread_name (struct thread_info *); > > +/* Given a pointer to a thread library specific thread handle and > + its length, return a pointer to the corresponding thread_info > struct. */ > + > +extern struct thread_info *target_thread_handle_to_thread_info > + (const gdb_byte * thread_handle, int handle_len, struct inferior *); No space after *. Could you name the last parameter as well? > + > /* Attempts to find the pathname of the executable file > that was run to create a specified process. > > diff --git a/gdb/thread.c b/gdb/thread.c > index 3cf1b94..456b1ad 100644 > --- a/gdb/thread.c > +++ b/gdb/thread.c > @@ -534,6 +534,16 @@ find_thread_ptid (ptid_t ptid) > return NULL; > } > > +/* Find a thread_info by matching thread libary specific handle. */ That comment should only refer to the header, which already contains a comment for that function. /* See gdbthread.h. */ Also, newline after the comment. > +struct thread_info * > +find_thread_by_handle (struct value *thread_handle, struct inferior > *inf) > +{ > + return target_thread_handle_to_thread_info > + (value_contents_all (thread_handle), > + TYPE_LENGTH (value_type (thread_handle)), > + inf); Those last two lines should be intended with one more space (aligned with value_contents_all). > +} > + > /* > * Thread iterator function. > * Thanks! Simon