This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]
Other format:	[Raw text]

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

From: Simon Marchi <simon dot marchi at polymtl dot ca>
To: Kevin Buettner <kevinb at redhat dot com>
Cc: gdb-patches at sourceware dot org
Date: Sun, 23 Jul 2017 23:46:49 +0200
Subject: Re: [PATCH v3 6/7] Add thread_handle_to_thread_info support for remote targets
Authentication-results: sourceware.org; auth=none
References: <20170718174156.5da204b0@pinnacle.lan> <20170718175544.056dbec6@pinnacle.lan>

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 couldalways install the callback and it will just return false. If we canavoid 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 thesesituations. 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

References:
- [PATCH v3 0/7] Thread handle to thread info mapping
  - From: Kevin Buettner
- Re: [PATCH v3 6/7] Add thread_handle_to_thread_info support for remote targets
  - From: Kevin Buettner

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]