Re: [PATCH v3 6/7] Add thread_handle_to_thread_info support for remote targets

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

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 <algorithm>
> +#include <vector>
> 
>  /* 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<gdb_byte> *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