Re: [PATCH v3 1/7] Add target method for converting thread handle to thread_info struct pointer

[Simon Marchi <simon.marchi@polymtl.ca>]

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 (&current_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
> +           (&current_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