[PATCH v2 0/7] Thread handle to thread info mapping

[PATCH v2 0/7] Thread handle to thread info mapping

[Kevin Buettner]

This patch set introduces support for mapping thread handles to the
thread_info structs which GDB uses to keep track of threads in the
inferiors which it's debugging.  I need this in order to find the GDB
thread which corresponds to a saved thread handle (e.g. pthread_t)
within an implementation of a thread library built atop pthreads.
The mechanism is general enough however to support other thread
handle representations as well.

Part 1 introduces a target method which maps a thread handle to
the corresponding internal GDB thread object, i.e. something of type
`struct thread_info *'.  An implementation of this new method is
provided for the Linux thread target.  Additional work will be
required, over time, for other thread targets.

Part 2 adds a python interface for the mechanism introduced in part 1.

Part 3 is a documentation patch.  It has been adjusted to address Eli's
concerns in the earlier patch series.

Part 4 adds a test case. I've extended this test case slightly from the
patch in the earlier series.

Part 5 is a bug fix for a problem discovered while working on part 6.

Part 6 adds support for remote targets.

Part 7 is a documentation patch for the remote protocol changes that
were implemented in part 6.

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

[Kevin Buettner]

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        |  3 +++
 gdb/linux-thread-db.c  | 24 ++++++++++++++++++++++++
 gdb/target-delegates.c | 35 +++++++++++++++++++++++++++++++++++
 gdb/target.c           |  8 ++++++++
 gdb/target.h           | 10 ++++++++++
 gdb/thread.c           |  9 +++++++++
 6 files changed, 89 insertions(+)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 9a16fe6..27b326b 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -427,6 +427,9 @@ 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.  */
+struct thread_info *find_thread_by_handle (struct value *thread_handle);
+
 /* 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 b131fc2..892a83e 100644
--- a/gdb/linux-thread-db.c
+++ b/gdb/linux-thread-db.c
@@ -1410,6 +1410,29 @@ 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).  */
+static struct thread_info *
+thread_db_thread_handle_to_thread_info (struct target_ops *ops,
+					const gdb_byte *thread_handle,
+					int handle_len)
+{
+  struct thread_info *tp;
+  thread_t handle_tid;
+
+  gdb_assert (handle_len == sizeof (handle_tid));
+
+  handle_tid = * (const thread_t *) thread_handle;
+
+  ALL_NON_EXITED_THREADS (tp)
+    {
+      if (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.  */
 
@@ -1688,6 +1711,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 470b7e4..08c08a9 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -1583,6 +1583,37 @@ 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)
+{
+  self = self->beneath;
+  return self->to_thread_handle_to_thread_info (self, arg1, arg2);
+}
+
+static struct thread_info *
+tdefault_thread_handle_to_thread_info (struct target_ops *self, const gdb_byte *arg1, int arg2)
+{
+  return NULL;
+}
+
+static struct thread_info *
+debug_thread_handle_to_thread_info (struct target_ops *self, const gdb_byte *arg1, int arg2)
+{
+  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);
+  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_thread_info_p (result);
+  fputs_unfiltered ("\n", gdb_stdlog);
+  return result;
+}
+
 static void
 delegate_stop (struct target_ops *self, ptid_t arg1)
 {
@@ -4267,6 +4298,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 +4555,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 +4715,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 7c286ab..ed58065 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2315,6 +2315,14 @@ 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,
+                                     int handle_len)
+{
+  return current_target.to_thread_handle_to_thread_info
+           (&current_target, thread_handle, handle_len);
+}
+
 void
 target_resume (ptid_t ptid, int step, enum gdb_signal signal)
 {
diff --git a/gdb/target.h b/gdb/target.h
index 943a0e2..8082a01 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -645,6 +645,10 @@ 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)
+      TARGET_DEFAULT_RETURN (NULL);
     void (*to_stop) (struct target_ops *, ptid_t)
       TARGET_DEFAULT_IGNORE ();
     void (*to_interrupt) (struct target_ops *, ptid_t)
@@ -1867,6 +1871,12 @@ extern 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);
+
 /* 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 fd9022f..fa973af6 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -524,6 +524,15 @@ find_thread_ptid (ptid_t ptid)
   return NULL;
 }
 
+/* Find a thread_info by matching thread libary specific handle.  */
+struct thread_info *
+find_thread_by_handle (struct value *thread_handle)
+{
+  return target_thread_handle_to_thread_info
+           (value_contents_all (thread_handle),
+           TYPE_LENGTH (value_type (thread_handle)));
+}
+
 /*
  * Thread iterator function.
  *

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

[Simon Marchi]

On 2017-04-09 02:06, Kevin Buettner wrote:
> --- a/gdb/linux-thread-db.c
> +++ b/gdb/linux-thread-db.c
> @@ -1410,6 +1410,29 @@ 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).  */
> +static struct thread_info *
> +thread_db_thread_handle_to_thread_info (struct target_ops *ops,
> +					const gdb_byte *thread_handle,
> +					int handle_len)
> +{
> +  struct thread_info *tp;
> +  thread_t handle_tid;
> +
> +  gdb_assert (handle_len == sizeof (handle_tid));

I assume this is always true, because if you are using libthread_db, it 
implies that GDB is of the exact same architecture (32 bits vs 64 bits) 
as the inferior?

Simon

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

[Kevin Buettner]

On Thu, 04 May 2017 23:26:20 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> On 2017-04-09 02:06, Kevin Buettner wrote:
> > --- a/gdb/linux-thread-db.c
> > +++ b/gdb/linux-thread-db.c
> > @@ -1410,6 +1410,29 @@ 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).  */
> > +static struct thread_info *
> > +thread_db_thread_handle_to_thread_info (struct target_ops *ops,
> > +					const gdb_byte *thread_handle,
> > +					int handle_len)
> > +{
> > +  struct thread_info *tp;
> > +  thread_t handle_tid;
> > +
> > +  gdb_assert (handle_len == sizeof (handle_tid));  
> 
> I assume this is always true, because if you are using libthread_db, it 
> implies that GDB is of the exact same architecture (32 bits vs 64 bits) 
> as the inferior?

I'm about to submit a new patch set, but I decided that I ought to
address this question...

I had first thought that the answer to your question was "yes", but
after thinking about it some more, I decided that it's not appropriate
to generate an internal error (via a failed assert) for this case. 
It's possible to get here via a call from the python interface.  We
don't want user written code to generate internal errors.  Therefore,
I changed it to call error() when there's a handle size mismatch.

Thanks for this review and all of the others too.  They were very much
appreciated.

Kevin

[PATCH v2 7/7] Documentation for qXfer:threads:read handle attribute

[Kevin Buettner]

gdb/doc/ChangeLog:
    
    	* gdb.texinfo (qXfer:threads:read): Add documentation for handle
    	attribute.
---
 gdb/doc/gdb.texinfo | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 300d78e..b7b9632 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -40396,7 +40396,9 @@ identifies the thread (@pxref{thread-id syntax}).  The
 the thread was last executing on.  The @samp{name} attribute, if
 present, specifies the human-readable name of the thread.  The content
 of the of @samp{thread} element is interpreted as human-readable
-auxiliary information.
+auxiliary information.  The @samp{handle} attribute, if present,
+is a hex encoded representation of the thread handle.
+
 
 @node Traceframe Info Format
 @section Traceframe Info Format

Re: [PATCH v2 7/7] Documentation for qXfer:threads:read handle attribute

[Eli Zaretskii]

> Date: Sat, 8 Apr 2017 23:07:38 -0700
> From: Kevin Buettner <kevinb@redhat.com>
> 
> gdb/doc/ChangeLog:
>     
>     	* gdb.texinfo (qXfer:threads:read): Add documentation for handle
>     	attribute.

OK.

[PATCH v2 4/7] Test case for gdb.thread_from_thread_handle

[Kevin Buettner]

As the title says, this is a test case for
gdb.thread_from_thread_handle, a python function which will, given a
thread library dependent thread handle, find the GDB thread which
corresponds to that thread handle.

The C file for this test case causes the thread handles for the
main thread and two child threads to be placed into an array.  The
test case runs to one of the functions (do_something()) at which point,
it retrieves the thread handles from the array and attempts to find the
correponding thread in GDB's internal thread list.

I use a barrier to make sure that both threads have actually started;
execution will stop when one of the threads breaks at do_something.

The one concern I have about what I've written is with the last three
invocations of gdb_test.  I don't know that we can be certain that
thrs[1] will always map to GDB thread 2 and that thrs[2] will map to
GDB thread 3.  It seems likely, but some perverse pthreads implementation
could change the order in which newly created threads are actually started.
If anyone thinks this is a problem, I can tweak it so that the test case
simply verifies that reasonable output is produced and another test can
verify that the two child thread numbers are actually different.

gdb/testsuite/ChangeLog:
    
    	* gdb.python/py-thrhandle.c, gdb.python/py-thrhandle.exp: New
    	files.
---
 gdb/testsuite/gdb.python/py-thrhandle.c   | 76 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.python/py-thrhandle.exp | 52 +++++++++++++++++++++
 2 files changed, 128 insertions(+)

diff --git a/gdb/testsuite/gdb.python/py-thrhandle.c b/gdb/testsuite/gdb.python/py-thrhandle.c
new file mode 100644
index 0000000..93d7dee
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-thrhandle.c
@@ -0,0 +1,76 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2016 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see  <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <pthread.h>
+#include <unistd.h>
+#include <alloca.h>
+#include <memory.h>
+
+#define NTHR 3
+pthread_t thrs[NTHR+2];
+pthread_barrier_t barrier;
+pthread_mutex_t mutex;
+
+void
+do_something (int n)
+{
+  pthread_mutex_lock (&mutex);
+  printf ("%d\n", n);
+  pthread_mutex_unlock (&mutex);
+}
+
+void *
+do_work (void *data)
+{
+  int num = * (int *) data;
+
+  pthread_barrier_wait (&barrier);
+
+  do_something (num);
+
+  pthread_exit (NULL);
+}
+
+int
+main (int argc, char **argv)
+{
+  int i;
+
+  pthread_barrier_init (&barrier, NULL, NTHR-1);
+  pthread_mutex_init (&mutex, NULL);
+
+  thrs[0] = pthread_self ();
+
+  for (i=1; i< NTHR; i++)
+    {
+      int *iptr = alloca (sizeof (int));
+      
+      *iptr = i;
+      pthread_create (&thrs[i], NULL, do_work, iptr);
+    }
+
+  /* Create two bogus thread handles.  */
+  memset (&thrs[NTHR], 0, sizeof (pthread_t));
+  memset (&thrs[NTHR+1], 0xaa, sizeof (pthread_t));
+
+  for (i=1; i< NTHR; i++)
+    {
+      pthread_join (thrs[i], NULL);
+    }
+  printf ("Done!");
+}
diff --git a/gdb/testsuite/gdb.python/py-thrhandle.exp b/gdb/testsuite/gdb.python/py-thrhandle.exp
new file mode 100644
index 0000000..d542734
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-thrhandle.exp
@@ -0,0 +1,52 @@
+# Copyright (C) 2016 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Please email any bugs, comments, and/or additions to this file to:
+# bug-gdb@gnu.org
+
+# This file verifies that gdb.thread_from_thread_handle works as expected.
+
+standard_testfile
+
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable debug] != "" } {
+    return -1
+}
+
+clean_restart ${binfile}
+runto_main
+
+gdb_test "break do_something" \
+    "Breakpoint 2 at .*: file .*${srcfile}, line .*" \
+         "breakpoint on do_something"
+
+gdb_test "continue" \
+	"Breakpoint 2, do_something .*" \
+	"run to do_something"
+
+gdb_test "python print gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[0\]')).num" \
+	"1" 
+
+gdb_test "python print gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[1\]')).num" \
+	"2"
+
+gdb_test "python print gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[2\]')).num" \
+	"3"
+
+gdb_test "python print gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[3\]'))" \
+	"None"
+
+gdb_test "python print gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[4\]'))" \
+	"None"

Re: [PATCH v2 4/7] Test case for gdb.thread_from_thread_handle

[Simon Marchi]

On 2017-04-09 02:07, Kevin Buettner wrote:
> As the title says, this is a test case for
> gdb.thread_from_thread_handle, a python function which will, given a
> thread library dependent thread handle, find the GDB thread which
> corresponds to that thread handle.
> 
> The C file for this test case causes the thread handles for the
> main thread and two child threads to be placed into an array.  The
> test case runs to one of the functions (do_something()) at which point,
> it retrieves the thread handles from the array and attempts to find the
> correponding thread in GDB's internal thread list.
> 
> I use a barrier to make sure that both threads have actually started;
> execution will stop when one of the threads breaks at do_something.
> 
> The one concern I have about what I've written is with the last three
> invocations of gdb_test.  I don't know that we can be certain that
> thrs[1] will always map to GDB thread 2 and that thrs[2] will map to
> GDB thread 3.  It seems likely, but some perverse pthreads 
> implementation
> could change the order in which newly created threads are actually 
> started.
> If anyone thinks this is a problem, I can tweak it so that the test 
> case
> simply verifies that reasonable output is produced and another test can
> verify that the two child thread numbers are actually different.

One case I can think of where thread numbers could be unstable is with a 
remote target.  AFAIK, new threads are only reported to GDB when there 
is a stop.  Depending on the remote target implementation, it could be 
possible that the order in which it reports the threads changes.

When I test the full series with --target_board=native-gdbserver, I 
actually see it.  I added an "info threads" in the test:

   1    Thread 27095.27095 "py-thrhandle" 0x00007ffff7bc457d in 
pthread_join () from target:/usr/lib/libpthread.so.0^M
* 2    Thread 27095.27100 "py-thrhandle" do_something (n=2) at 
/home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-thrhandle.c:32^M
   3    Thread 27095.27099 "py-thrhandle" do_something (n=1) at 
/home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-thrhandle.c:32^M

And I see FAILs for thrs[1] and [2].

I think a good fix would be to force the thread numbering to be stable 
by hitting a breakpoint after each thread start.  That would force the 
target to report the threads in the order they really appear.  That 
should be done easily by having a barrier of 2, to be hit by the main 
thread and the newly-spawned thread.  You can have a breakpoint in main 
just after that barrier, to which you "continue" as many times as there 
are threads.  You would need to make sure that the previously started 
threads don't exit while you're busy starting other threads, so you can 
make them sleep(30) for example.

> gdb/testsuite/ChangeLog:
> 
>     	* gdb.python/py-thrhandle.c, gdb.python/py-thrhandle.exp: New
>     	files.
> ---
>  gdb/testsuite/gdb.python/py-thrhandle.c   | 76 
> +++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.python/py-thrhandle.exp | 52 +++++++++++++++++++++
>  2 files changed, 128 insertions(+)
> 
> diff --git a/gdb/testsuite/gdb.python/py-thrhandle.c
> b/gdb/testsuite/gdb.python/py-thrhandle.c
> new file mode 100644
> index 0000000..93d7dee
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-thrhandle.c
> @@ -0,0 +1,76 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2016 Free Software Foundation, Inc.

2017?

> +
> +   This program is free software; you can redistribute it and/or 
> modify
> +   it under the terms of the GNU General Public License as published 
> by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see  
> <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <pthread.h>
> +#include <unistd.h>
> +#include <alloca.h>
> +#include <memory.h>
> +
> +#define NTHR 3
> +pthread_t thrs[NTHR+2];

It was not clear to me why the +2.  Maybe you could split it in two 
arrays, threads and invalid_threads?

> +pthread_barrier_t barrier;
> +pthread_mutex_t mutex;
> +
> +void
> +do_something (int n)
> +{
> +  pthread_mutex_lock (&mutex);
> +  printf ("%d\n", n);
> +  pthread_mutex_unlock (&mutex);

Is this mutex (and printf) actually useful?  I think it would be better 
to keep the test program as simple as it can be.  Getting rid of the 
printf  would also allow getting rid of the alloca in main.

> +}
> +
> +void *
> +do_work (void *data)
> +{
> +  int num = * (int *) data;
> +
> +  pthread_barrier_wait (&barrier);
> +
> +  do_something (num);
> +
> +  pthread_exit (NULL);
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> +  int i;
> +
> +  pthread_barrier_init (&barrier, NULL, NTHR-1);
> +  pthread_mutex_init (&mutex, NULL);
> +
> +  thrs[0] = pthread_self ();
> +
> +  for (i=1; i< NTHR; i++)

We try to respect the C coding style even for tests, so here that would 
mean spaces around the binary operators ...

> +    {
> +      int *iptr = alloca (sizeof (int));
> +
> +      *iptr = i;
> +      pthread_create (&thrs[i], NULL, do_work, iptr);
> +    }
> +
> +  /* Create two bogus thread handles.  */
> +  memset (&thrs[NTHR], 0, sizeof (pthread_t));
> +  memset (&thrs[NTHR+1], 0xaa, sizeof (pthread_t));

... and here too I guess ...

> +
> +  for (i=1; i< NTHR; i++)

... and here too.  You can also remove the curly braces.

> +    {
> +      pthread_join (thrs[i], NULL);
> +    }
> +  printf ("Done!");
> +}
> diff --git a/gdb/testsuite/gdb.python/py-thrhandle.exp
> b/gdb/testsuite/gdb.python/py-thrhandle.exp
> new file mode 100644
> index 0000000..d542734
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-thrhandle.exp
> @@ -0,0 +1,52 @@
> +# Copyright (C) 2016 Free Software Foundation, Inc.

2017?

> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see 
> <http://www.gnu.org/licenses/>.
> +
> +# Please email any bugs, comments, and/or additions to this file to:
> +# bug-gdb@gnu.org
> +
> +# This file verifies that gdb.thread_from_thread_handle works as 
> expected.
> +
> +standard_testfile
> +
> +
> +if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}"
> "${binfile}" executable debug] != "" } {
> +    return -1
> +}
> +
> +clean_restart ${binfile}
> +runto_main
> +
> +gdb_test "break do_something" \
> +    "Breakpoint 2 at .*: file .*${srcfile}, line .*" \
> +         "breakpoint on do_something"

The indentation look inconsistent between these two lines.

Note that you can use gdb_breakpoint also.

> +
> +gdb_test "continue" \
> +	"Breakpoint 2, do_something .*" \
> +	"run to do_something"

gdb_continue_to_breakpoint can be useful here.

> +gdb_test "python print
> gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[0\]')).num" \
> +	"1"

Please use parenthesis with print so that the test works with Python 3.

> +
> +gdb_test "python print
> gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[1\]')).num" \
> +	"2"
> +
> +gdb_test "python print
> gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[2\]')).num" \
> +	"3"
> +
> +gdb_test "python print
> gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[3\]'))" \
> +	"None"
> +
> +gdb_test "python print
> gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[4\]'))" \
> +	"None"

Something I realized while testing is that the assert in 
linux-thread-db.c is not ideal, since the user can input any kind of 
value through Python:

>>> print(gdb.thread_from_thread_handle(gdb.parse_and_eval('2')).num)
/home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1423: 
internal-error: thread_info* 
thread_db_thread_handle_to_thread_info(target_ops*, const gdb_byte*, 
int): Assertion `handle_len == sizeof (handle_tid)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

So perhaps returning NULL would be a better choice if the length doesn't 
match.

While on the subject, instead of passing a buffer and a length to the 
target method, how about passing the struct value directly and checking 
if the type is the right one (pthread_t in this case), returning NULL if 
it isn't?

Thanks,

Simon

[PATCH v2 2/7] Add `thread_from_thread_handle' function to (Python) gdb module

[Kevin Buettner]

gdb/ChangeLog:
    	* python/py-infthread.c (gdbpy_thread_from_thread_handle): New
    	function.
    	* python/python-internal.h (thread_object_type): Declare.
    	(gdbpy_thread_from_thread_handle): Declare.
    	* python/python.c (thread_from_thread_handle): Register.
---
 gdb/python/py-infthread.c    | 41 +++++++++++++++++++++++++++++++++++++++++
 gdb/python/python-internal.h |  3 +++
 gdb/python/python.c          |  4 ++++
 3 files changed, 48 insertions(+)

diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
index 5482bf9..5739984 100644
--- a/gdb/python/py-infthread.c
+++ b/gdb/python/py-infthread.c
@@ -294,6 +294,47 @@ gdbpy_selected_thread (PyObject *self, PyObject *args)
   Py_RETURN_NONE;
 }
 
+/* Implementation of gdb.thread_from_thread_handle (handle) 
+                        ->  gdb.InferiorThread.  */
+
+PyObject *
+gdbpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
+{
+  PyObject *handle_obj, *result;
+  static char *keywords[] = { "thread_handle", NULL };
+
+  if (! PyArg_ParseTupleAndKeywords (args, kw, "O", keywords, &handle_obj))
+    return NULL;
+
+  result = Py_None;
+
+  if (gdbpy_is_value_object (handle_obj))
+    {
+      TRY
+	{
+	  struct thread_info *thread_info;
+	  struct value *val = value_object_to_value (handle_obj);
+
+	  thread_info = find_thread_by_handle (val);
+	  if (thread_info != NULL)
+	    {
+	      result = (PyObject *) find_thread_object (thread_info->ptid);
+	      if (result)
+		Py_INCREF (result);
+	    }
+	}
+      CATCH (except, RETURN_MASK_ALL)
+	{
+	  if (except.reason < 0)
+	    gdbpy_convert_exception (except);
+	  return NULL;
+	}
+      END_CATCH
+    }
+
+  return result;
+}
+
 int
 gdbpy_initialize_thread (void)
 {
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 4dd413d..68c3a8e 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -258,6 +258,8 @@ extern PyTypeObject breakpoint_object_type
     CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("breakpoint_object");
 extern PyTypeObject frame_object_type
     CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("frame_object");
+extern PyTypeObject thread_object_type
+    CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("thread_object");
 
 typedef struct gdbpy_breakpoint_object
 {
@@ -385,6 +387,7 @@ PyObject *gdbpy_create_lazy_string_object (CORE_ADDR address, long length,
 PyObject *gdbpy_inferiors (PyObject *unused, PyObject *unused2);
 PyObject *gdbpy_create_ptid_object (ptid_t ptid);
 PyObject *gdbpy_selected_thread (PyObject *self, PyObject *args);
+PyObject *gdbpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw);
 PyObject *gdbpy_selected_inferior (PyObject *self, PyObject *args);
 PyObject *gdbpy_string_to_argv (PyObject *self, PyObject *args);
 PyObject *gdbpy_parameter_value (enum var_types type, void *var);
diff --git a/gdb/python/python.c b/gdb/python/python.c
index a7aff53..972def2 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -1993,6 +1993,10 @@ Arguments are separate by spaces and may be quoted."
   { "selected_thread", gdbpy_selected_thread, METH_NOARGS,
     "selected_thread () -> gdb.InferiorThread.\n\
 Return the selected thread object." },
+  { "thread_from_thread_handle", (PyCFunction) gdbpy_thread_from_thread_handle,
+     METH_VARARGS | METH_KEYWORDS,
+     "thread_from_thread_handle (handle) -> gdb.InferiorThread.\n\
+Return thread object corresponding to thread handle." },
   { "selected_inferior", gdbpy_selected_inferior, METH_NOARGS,
     "selected_inferior () -> gdb.Inferior.\n\
 Return the selected inferior object." },

Re: [PATCH v2 2/7] Add `thread_from_thread_handle' function to (Python) gdb module

[Phil Muldoon]

On 09/04/17 07:06, Kevin Buettner wrote:
> gdb/ChangeLog:
>     	* python/py-infthread.c (gdbpy_thread_from_thread_handle): New
>     	function.
>     	* python/python-internal.h (thread_object_type): Declare.
>     	(gdbpy_thread_from_thread_handle): Declare.
>     	* python/python.c (thread_from_thread_handle): Register.
> ---
>  gdb/python/py-infthread.c    | 41 +++++++++++++++++++++++++++++++++++++++++
>  gdb/python/python-internal.h |  3 +++
>  gdb/python/python.c          |  4 ++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
> index 5482bf9..5739984 100644
> --- a/gdb/python/py-infthread.c
> +++ b/gdb/python/py-infthread.c
> @@ -294,6 +294,47 @@ gdbpy_selected_thread (PyObject *self, PyObject *args)
>    Py_RETURN_NONE;
>  }
>  
> +/* Implementation of gdb.thread_from_thread_handle (handle) 
> +                        ->  gdb.InferiorThread.  */
> +
> +PyObject *
> +gdbpy_thread_from_thread_handle (PyObject *self, PyObject *args, PyObject *kw)
> +{
> +  PyObject *handle_obj, *result;
> +  static char *keywords[] = { "thread_handle", NULL };
> +
> +  if (! PyArg_ParseTupleAndKeywords (args, kw, "O", keywords, &handle_obj))
> +    return NULL;
> +
> +  result = Py_None;
> +
> +  if (gdbpy_is_value_object (handle_obj))
> +    {
> +      TRY
> +	{
> +	  struct thread_info *thread_info;
> +	  struct value *val = value_object_to_value (handle_obj);
> +
> +	  thread_info = find_thread_by_handle (val);
> +	  if (thread_info != NULL)
> +	    {
> +	      result = (PyObject *) find_thread_object (thread_info->ptid);
> +	      if (result)
> +		Py_INCREF (result);

Initially this caught me off guard and wondered why find_thread_object required a reference count. But it does return a borrowed reference, though it does not seem to be documented as such.  I think the code surrounding this should be documented, but not in this patch. I'll add it to one of my TODOs.
> +	    }
> +	}
> +      CATCH (except, RETURN_MASK_ALL)
> +	{
> +	  if (except.reason < 0)
> +	    gdbpy_convert_exception (except);
> +	  return NULL;
> +	}
> +      END_CATCH
> +    }
> +
> +  return result;
> +}

The Python bits look good to me, as do the tests and documentation.  Please wait on a maintainer to give the overall patch a yes/no before proceeding.

Cheers

Phil

Re: [PATCH v2 2/7] Add `thread_from_thread_handle' function to (Python) gdb module

[Simon Marchi]

On 2017-04-09 02:06, Kevin Buettner wrote:
> gdb/ChangeLog:
>     	* python/py-infthread.c (gdbpy_thread_from_thread_handle): New
>     	function.
>     	* python/python-internal.h (thread_object_type): Declare.
>     	(gdbpy_thread_from_thread_handle): Declare.
>     	* python/python.c (thread_from_thread_handle): Register.
> ---
>  gdb/python/py-infthread.c    | 41 
> +++++++++++++++++++++++++++++++++++++++++
>  gdb/python/python-internal.h |  3 +++
>  gdb/python/python.c          |  4 ++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
> index 5482bf9..5739984 100644
> --- a/gdb/python/py-infthread.c
> +++ b/gdb/python/py-infthread.c
> @@ -294,6 +294,47 @@ gdbpy_selected_thread (PyObject *self, PyObject 
> *args)
>    Py_RETURN_NONE;
>  }
> 
> +/* Implementation of gdb.thread_from_thread_handle (handle)
> +                        ->  gdb.InferiorThread.  */
> +
> +PyObject *
> +gdbpy_thread_from_thread_handle (PyObject *self, PyObject *args, 
> PyObject *kw)
> +{
> +  PyObject *handle_obj, *result;
> +  static char *keywords[] = { "thread_handle", NULL };
> +
> +  if (! PyArg_ParseTupleAndKeywords (args, kw, "O", keywords, 
> &handle_obj))
> +    return NULL;

To adapt the patch to the current GDB master, you'll need to make 
keyword a const char * [], and use gdb_PyArg_ParseTupleAndKeywords.

> +
> +  result = Py_None;
> +
> +  if (gdbpy_is_value_object (handle_obj))
> +    {
> +      TRY
> +	{
> +	  struct thread_info *thread_info;
> +	  struct value *val = value_object_to_value (handle_obj);
> +
> +	  thread_info = find_thread_by_handle (val);
> +	  if (thread_info != NULL)
> +	    {
> +	      result = (PyObject *) find_thread_object (thread_info->ptid);
> +	      if (result)

if (result != NULL)

> +		Py_INCREF (result);
> +	    }
> +	}
> +      CATCH (except, RETURN_MASK_ALL)
> +	{
> +	  if (except.reason < 0)
> +	    gdbpy_convert_exception (except);
> +	  return NULL;

Should you use GDB_PY_HANDLE_EXCEPTION?  The behavior is a little bit 
different than your code, in that it will only return if an exception 
was set.  The Python doc says: "If the return value is NULL, an 
exception shall have been set.".  Here, it's possible that we return 
NULL without having set an exception.

Thanks,

Simon

[PATCH v2 3/7] Documentation for gdb.thread_from_thread_handle

[Kevin Buettner]

gdb/doc/ChangeLog:
    
    	* python.texi (Threads In Python): Add description for function
    	gdb.thread_from_thread_handle.
---
 gdb/doc/python.texi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index ce5810e..81543d3 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3008,6 +3008,12 @@ This function returns the thread object for the selected thread.  If there
 is no selected thread, this will return @code{None}.
 @end defun
 
+@defun gdb.thread_from_thread_handle (thread_handle)
+Return the thread object corresponding to the thread handle,
+@var{thread_handle}, a thread library specific data structure such as
+@code{pthread_t} for pthreads library implementations.
+@end defun
+
 A @code{gdb.InferiorThread} object has the following attributes:
 
 @defvar InferiorThread.name

Re: [PATCH v2 3/7] Documentation for gdb.thread_from_thread_handle

[Simon Marchi]

On 2017-04-09 02:07, Kevin Buettner wrote:
> gdb/doc/ChangeLog:
> 
>     	* python.texi (Threads In Python): Add description for function
>     	gdb.thread_from_thread_handle.
> ---
>  gdb/doc/python.texi | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index ce5810e..81543d3 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -3008,6 +3008,12 @@ This function returns the thread object for the
> selected thread.  If there
>  is no selected thread, this will return @code{None}.
>  @end defun
> 
> +@defun gdb.thread_from_thread_handle (thread_handle)
> +Return the thread object corresponding to the thread handle,
> +@var{thread_handle}, a thread library specific data structure such as

The comma before @var{thread_handle} feels unnecessary.  And I think you 
could shorten it to "... corresponding to @var{thread_handle}, ..." 
without loss of clarity.

> +@code{pthread_t} for pthreads library implementations.
> +@end defun
> +
>  A @code{gdb.InferiorThread} object has the following attributes:
> 
>  @defvar InferiorThread.name

Eli, do you have something to say about this patch?

Thanks,

Simon

Re: [PATCH v2 3/7] Documentation for gdb.thread_from_thread_handle

[Eli Zaretskii]

> Date: Fri, 05 May 2017 00:01:31 -0400
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Cc: gdb-patches@sourceware.org, eliz@gnu.org
> 
> > +@defun gdb.thread_from_thread_handle (thread_handle)
> > +Return the thread object corresponding to the thread handle,
> > +@var{thread_handle}, a thread library specific data structure such as
> 
> The comma before @var{thread_handle} feels unnecessary.  And I think you 
> could shorten it to "... corresponding to @var{thread_handle}, ..." 
> without loss of clarity.

Both variants are OK.

> Eli, do you have something to say about this patch?

Sorry, I missed it at the time.  It's fine with me.

Thanks.

Re: [PATCH v2 3/7] Documentation for gdb.thread_from_thread_handle

[Kevin Buettner]

On Fri, 05 May 2017 00:01:31 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> > +@defun gdb.thread_from_thread_handle (thread_handle)
> > +Return the thread object corresponding to the thread handle,
> > +@var{thread_handle}, a thread library specific data structure such as  
> 
> The comma before @var{thread_handle} feels unnecessary.  And I think you 
> could shorten it to "... corresponding to @var{thread_handle}, ..." 
> without loss of clarity.

I like your proposed (re)wording.  Since it's okay with Eli, that's
what I've incorporated into the new patch which I will post shortly.

Kevin

[PATCH v2 5/7] Add thread_db_notice_clone to gdbserver

[Kevin Buettner]

While working on a patch for fetching a thread handle in gdbserver, I
ran into a circumstance in which tests in gdb.mi/mi-nsmoribund.exp
would occasionally fail.  Over a large enough number of runs, it would
fail roughly 2% of the time.

That thread handle patch caused find_one_thread() to be called on
every stop.  find_one_thread() calls td_ta_map_lwp2thr() which, in
turn, can cause ps_get_thread_area() to be called.
ps_get_thread_area() makes a call to ptrace() for getting the thread
area address.  If this should happen when the thread is not stopped,
the call to ptrace will return error which in turn propogates back to
find_one_thread().  find_one_thread() calls error() in this instance
which causes the program to die.

This patch causes find_one_thread() to be called upon reciept of a
clone event.  Since the clone is stopped, the circumstances described
above cannot occur.

gdb/gdbserver/ChangeLog:
    
    	* linux-low.c (handle_extended_wait): Call thread_db_notice_clone().
    	* linux-low.h (thread_db_notice_clone): Declare.
    	* thread-db.c (thread_db_notice_clone): New function.
---
 gdb/gdbserver/linux-low.c |  2 ++
 gdb/gdbserver/linux-low.h |  1 +
 gdb/gdbserver/thread-db.c | 14 ++++++++++++++
 3 files changed, 17 insertions(+)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index e27cbf8..6f06eb8 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -653,6 +653,8 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat)
 	  new_lwp->status_pending = status;
 	}
 
+      thread_db_notice_clone (get_thread_process (event_thr), ptid);
+
       /* Don't report the event.  */
       return 1;
     }
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 7dcbfcf..87ce53a 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -409,5 +409,6 @@ int thread_db_handle_monitor_command (char *);
 int thread_db_get_tls_address (struct thread_info *thread, CORE_ADDR offset,
 			       CORE_ADDR load_module, CORE_ADDR *address);
 int thread_db_look_up_one_symbol (const char *name, CORE_ADDR *addrp);
+void thread_db_notice_clone (struct process_info *proc, ptid_t lwp);
 
 extern int have_ptrace_getregset;
diff --git a/gdb/gdbserver/thread-db.c b/gdb/gdbserver/thread-db.c
index 1ffb79d..eff1914 100644
--- a/gdb/gdbserver/thread-db.c
+++ b/gdb/gdbserver/thread-db.c
@@ -864,3 +864,17 @@ thread_db_handle_monitor_command (char *mon)
   /* Tell server.c to perform default processing.  */
   return 0;
 }
+
+void
+thread_db_notice_clone (struct process_info *proc, ptid_t ptid)
+{
+  struct thread_db *thread_db = proc->priv->thread_db;
+
+  /* If the thread layer isn't initialized, return.  It may just
+     be that the program uses clone, but does not use libthread_db.  */
+  if (thread_db == NULL || !thread_db->all_symbols_looked_up)
+    return;
+
+  if (!find_one_thread (ptid))
+    warning ("Cannot find thread after clone.\n");
+}

Re: [PATCH v2 5/7] Add thread_db_notice_clone to gdbserver

[Simon Marchi]

On 2017-04-09 02:07, Kevin Buettner wrote:
> While working on a patch for fetching a thread handle in gdbserver, I
> ran into a circumstance in which tests in gdb.mi/mi-nsmoribund.exp
> would occasionally fail.  Over a large enough number of runs, it would
> fail roughly 2% of the time.
> 
> That thread handle patch caused find_one_thread() to be called on
> every stop.  find_one_thread() calls td_ta_map_lwp2thr() which, in
> turn, can cause ps_get_thread_area() to be called.
> ps_get_thread_area() makes a call to ptrace() for getting the thread
> area address.  If this should happen when the thread is not stopped,
> the call to ptrace will return error which in turn propogates back to
> find_one_thread().  find_one_thread() calls error() in this instance
> which causes the program to die.
> 
> This patch causes find_one_thread() to be called upon reciept of a
> clone event.  Since the clone is stopped, the circumstances described
> above cannot occur.

This patch looks good to me, although I'm not very knowledgeable in the 
thread handling area.

Just one comment below.

> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index e27cbf8..6f06eb8 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -653,6 +653,8 @@ handle_extended_wait (struct lwp_info
> **orig_event_lwp, int wstat)
>  	  new_lwp->status_pending = status;
>  	}
> 
> +      thread_db_notice_clone (get_thread_process (event_thr), ptid);
> +
>        /* Don't report the event.  */
>        return 1;
>      }
> diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
> index 7dcbfcf..87ce53a 100644
> --- a/gdb/gdbserver/linux-low.h
> +++ b/gdb/gdbserver/linux-low.h
> @@ -409,5 +409,6 @@ int thread_db_handle_monitor_command (char *);
>  int thread_db_get_tls_address (struct thread_info *thread, CORE_ADDR 
> offset,
>  			       CORE_ADDR load_module, CORE_ADDR *address);
>  int thread_db_look_up_one_symbol (const char *name, CORE_ADDR *addrp);
> +void thread_db_notice_clone (struct process_info *proc, ptid_t lwp);

Could you add a comment to document this function?

Thanks,

Simon

[PATCH v2 6/7] Add thread_handle_to_thread_info support for remote targets

[Kevin Buettner]

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_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 |  3 +++
 gdb/gdbserver/server.c    | 10 +++++++++
 gdb/gdbserver/target.h    | 10 +++++++++
 gdb/gdbserver/thread-db.c | 30 +++++++++++++++++++++++++++
 gdb/remote.c              | 53 +++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 111 insertions(+)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 6f06eb8..4ed5a63 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7700,6 +7700,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
 };
 
 #ifdef HAVE_LINUX_REGSETS
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index 87ce53a..5d6ee40 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;
+  /* The pthread_t handle.  */
+  thread_t thread_handle;
 #endif
 
   /* Arch-specific additions.  */
@@ -410,5 +412,6 @@ int thread_db_get_tls_address (struct thread_info *thread, CORE_ADDR offset,
 			       CORE_ADDR load_module, CORE_ADDR *address);
 int thread_db_look_up_one_symbol (const char *name, CORE_ADDR *addrp);
 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);
 
 extern int have_ptrace_getregset;
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 4bc7f71..0ad6c65 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1572,6 +1572,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);
 
   write_ptid (ptid_s, ptid);
 
@@ -1586,6 +1589,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 3cc2bc4..db8b8d7 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -474,6 +474,11 @@ struct target_ops
 
   /* Return tdesc index for IPA.  */
   int (*get_ipa_tdesc_idx) (void);
+
+  /* Thread ID to (numeric) thread handle: Return a non-zero status on
+     success, 0 for failure.  Return pointer to thread handle via HANDLE
+     and the handle's length via HANDLE_LEN.  */
+  int (*thread_handle) (ptid_t ptid, gdb_byte **handle, int *handle_len);
 };
 
 extern struct target_ops *the_target;
@@ -692,6 +697,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)
+
 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..8f8bf20 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;
 }
 
+int
+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 0;
+
+  thread_db = get_thread_process (thread)->priv->thread_db;
+
+  if (thread_db == NULL)
+    return 0;
+
+  lwp = get_thread_lwp (thread);
+
+  if (!lwp->thread_known && !find_one_thread (thread->entry.id))
+    return 0;
+
+  gdb_assert (lwp->thread_known);
+
+  *handle = (gdb_byte *) &lwp->thread_handle;
+  *handle_len = sizeof (lwp->thread_handle);
+  return 1;
+}
+
 #ifdef USE_LIBTHREAD_DB_DIRECTLY
 
 static int
diff --git a/gdb/remote.c b/gdb/remote.c
index 73a2e51..cdac7a1 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;
+
   /* Whether the target stopped for a breakpoint/watchpoint.  */
   enum target_stop_reason stop_reason;
 
@@ -482,6 +487,7 @@ free_private_thread_info (struct private_thread_info *info)
 {
   xfree (info->extra);
   xfree (info->name);
+  delete info->thread_handle;
   xfree (info);
 }
 
@@ -1967,6 +1973,7 @@ get_private_info_thread (struct thread_info *thread)
       priv->last_resume_step = 0;
       priv->last_resume_sig = GDB_SIGNAL_0;
       priv->vcont_resumed = 0;
+      priv->thread_handle = nullptr;
     }
 
   return thread->priv;
@@ -2993,6 +3000,10 @@ typedef struct thread_item
 
   /* The core the thread was running on.  -1 if not known.  */
   int core;
+
+  /* The thread handle associated with the thread.  */
+  std::vector<gdb_byte> *thread_handle;
+
 } thread_item_t;
 DEF_VEC_O(thread_item_t);
 
@@ -3020,6 +3031,7 @@ clear_threads_listing_context (void *p)
     {
       xfree (item->extra);
       xfree (item->name);
+      delete item->thread_handle;
     }
 
   VEC_free (thread_item_t, context->items);
@@ -3058,6 +3070,7 @@ remote_newthread_step (threadref *ref, void *data)
   item.core = -1;
   item.name = NULL;
   item.extra = NULL;
+  item.thread_handle = nullptr;
 
   VEC_safe_push (thread_item_t, context->items, &item);
 
@@ -3128,6 +3141,17 @@ start_thread (struct gdb_xml_parser *parser,
   attr = xml_find_attribute (attributes, "name");
   item.name = attr != NULL ? xstrdup ((const char *) attr->value) : NULL;
 
+  attr = xml_find_attribute (attributes, "handle");
+  if (attr != NULL)
+    {
+      item.thread_handle = new std::vector<gdb_byte>
+                             (strlen ((const char *) attr->value) / 2);
+      hex2bin ((const char *) attr->value, item.thread_handle->data (),
+               item.thread_handle->size ());
+    }
+  else
+    item.thread_handle = nullptr;
+
   item.extra = 0;
 
   VEC_safe_push (thread_item_t, data->items, &item);
@@ -3149,6 +3173,7 @@ const struct gdb_xml_attribute thread_attributes[] = {
   { "id", GDB_XML_AF_NONE, NULL, NULL },
   { "core", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL },
   { "name", GDB_XML_AF_OPTIONAL, NULL, NULL },
+  { "handle", GDB_XML_AF_OPTIONAL, NULL, NULL },
   { NULL, GDB_XML_AF_NONE, NULL, NULL }
 };
 
@@ -3224,6 +3249,7 @@ remote_get_threads_with_qthreadinfo (struct target_ops *ops,
 		  item.core = -1;
 		  item.name = NULL;
 		  item.extra = NULL;
+		  item.thread_handle = nullptr;
 
 		  VEC_safe_push (thread_item_t, context->items, &item);
 		}
@@ -3329,6 +3355,8 @@ remote_update_thread_list (struct target_ops *ops)
 	      item->extra = NULL;
 	      info->name = item->name;
 	      item->name = NULL;
+	      info->thread_handle = item->thread_handle;
+	      item->thread_handle = nullptr;
 	    }
 	}
     }
@@ -13505,6 +13533,29 @@ 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).  */
+static struct thread_info *
+remote_thread_handle_to_thread_info (struct target_ops *ops,
+				     const gdb_byte *thread_handle,
+				     int handle_len)
+{
+  enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
+  struct thread_info *tp;
+
+  ALL_NON_EXITED_THREADS (tp)
+    {
+      struct private_thread_info *priv = get_private_info_thread (tp);
+
+      if (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)
 {
@@ -13653,6 +13704,8 @@ Specify the serial device it is connected to\n\
   remote_ops.to_insert_exec_catchpoint = remote_insert_exec_catchpoint;
   remote_ops.to_remove_exec_catchpoint = remote_remove_exec_catchpoint;
   remote_ops.to_execution_direction = remote_execution_direction;
+  remote_ops.to_thread_handle_to_thread_info =
+    remote_thread_handle_to_thread_info;
 }
 
 /* Set up the extended remote vector by making a copy of the standard

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

[Simon Marchi]

On 2017-04-09 02:07, 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_update_thread_list): Update thread_handle.
>     	(remote_thread_handle_to_thread_info): New function.
>     	(init_remote_ops): Initialize to_thread_handle_to_thread_info.

That looks good to me.  I was a bit thrown off by the std::vector 
pointers in the remote structures, but I think it's a good trade-off 
until we make the structures apt to have non trivial fields.

> diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h
> index 3cc2bc4..db8b8d7 100644
> --- a/gdb/gdbserver/target.h
> +++ b/gdb/gdbserver/target.h
> @@ -474,6 +474,11 @@ struct target_ops
> 
>    /* Return tdesc index for IPA.  */
>    int (*get_ipa_tdesc_idx) (void);
> +
> +  /* Thread ID to (numeric) thread handle: Return a non-zero status on
> +     success, 0 for failure.  Return pointer to thread handle via 
> HANDLE
> +     and the handle's length via HANDLE_LEN.  */
> +  int (*thread_handle) (ptid_t ptid, gdb_byte **handle, int 
> *handle_len);

Return a bool?

> @@ -13505,6 +13533,29 @@ 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).  */
> +static struct thread_info *
> +remote_thread_handle_to_thread_info (struct target_ops *ops,
> +				     const gdb_byte *thread_handle,
> +				     int handle_len)
> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());

byte_order is unused.

> +  struct thread_info *tp;
> +
> +  ALL_NON_EXITED_THREADS (tp)
> +    {
> +      struct private_thread_info *priv = get_private_info_thread (tp);
> +
> +      if (priv != NULL && handle_len == priv->thread_handle->size ()
> +          && memcmp (thread_handle, priv->thread_handle->data (),
> +	             handle_len) == 0)
> +        return tp;
> +    }
> +
> +  return NULL;
> +}
> +

Thanks,

Simon

Re: [PATCH v2 0/7] Thread handle to thread info mapping

[Kevin Buettner]

Ping!

Any other thoughts on this patch series?

(Thanks to Eli and Phil for their reviews...)

Kevin

On Sat, 8 Apr 2017 22:49:59 -0700
Kevin Buettner <kevin@buettner.to> wrote:

> This patch set introduces support for mapping thread handles to the
> thread_info structs which GDB uses to keep track of threads in the
> inferiors which it's debugging.  I need this in order to find the GDB
> thread which corresponds to a saved thread handle (e.g. pthread_t)
> within an implementation of a thread library built atop pthreads.
> The mechanism is general enough however to support other thread
> handle representations as well.
> 
> Part 1 introduces a target method which maps a thread handle to
> the corresponding internal GDB thread object, i.e. something of type
> `struct thread_info *'.  An implementation of this new method is
> provided for the Linux thread target.  Additional work will be
> required, over time, for other thread targets.
> 
> Part 2 adds a python interface for the mechanism introduced in part 1.
> 
> Part 3 is a documentation patch.  It has been adjusted to address Eli's
> concerns in the earlier patch series.
> 
> Part 4 adds a test case. I've extended this test case slightly from the
> patch in the earlier series.
> 
> Part 5 is a bug fix for a problem discovered while working on part 6.
> 
> Part 6 adds support for remote targets.
> 
> Part 7 is a documentation patch for the remote protocol changes that
> were implemented in part 6.

Re: [PATCH v2 0/7] Thread handle to thread info mapping

[Kevin Buettner]

Ping!

On Sat, 8 Apr 2017 22:49:59 -0700
Kevin Buettner <kevin@buettner.to> wrote:

> This patch set introduces support for mapping thread handles to the
> thread_info structs which GDB uses to keep track of threads in the
> inferiors which it's debugging.  I need this in order to find the GDB
> thread which corresponds to a saved thread handle (e.g. pthread_t)
> within an implementation of a thread library built atop pthreads.
> The mechanism is general enough however to support other thread
> handle representations as well.
> 
> Part 1 introduces a target method which maps a thread handle to
> the corresponding internal GDB thread object, i.e. something of type
> `struct thread_info *'.  An implementation of this new method is
> provided for the Linux thread target.  Additional work will be
> required, over time, for other thread targets.
> 
> Part 2 adds a python interface for the mechanism introduced in part 1.
> 
> Part 3 is a documentation patch.  It has been adjusted to address Eli's
> concerns in the earlier patch series.
> 
> Part 4 adds a test case. I've extended this test case slightly from the
> patch in the earlier series.
> 
> Part 5 is a bug fix for a problem discovered while working on part 6.
> 
> Part 6 adds support for remote targets.
> 
> Part 7 is a documentation patch for the remote protocol changes that
> were implemented in part 6.

Re: [PATCH v2 0/7] Thread handle to thread info mapping

[Kevin Buettner]

Ping!

On Sat, 8 Apr 2017 22:49:59 -0700
Kevin Buettner <kevin@buettner.to> wrote:

> This patch set introduces support for mapping thread handles to the
> thread_info structs which GDB uses to keep track of threads in the
> inferiors which it's debugging.  I need this in order to find the GDB
> thread which corresponds to a saved thread handle (e.g. pthread_t)
> within an implementation of a thread library built atop pthreads.
> The mechanism is general enough however to support other thread
> handle representations as well.
> 
> Part 1 introduces a target method which maps a thread handle to
> the corresponding internal GDB thread object, i.e. something of type
> `struct thread_info *'.  An implementation of this new method is
> provided for the Linux thread target.  Additional work will be
> required, over time, for other thread targets.
> 
> Part 2 adds a python interface for the mechanism introduced in part 1.
> 
> Part 3 is a documentation patch.  It has been adjusted to address Eli's
> concerns in the earlier patch series.
> 
> Part 4 adds a test case. I've extended this test case slightly from the
> patch in the earlier series.
> 
> Part 5 is a bug fix for a problem discovered while working on part 6.
> 
> Part 6 adds support for remote targets.
> 
> Part 7 is a documentation patch for the remote protocol changes that
> were implemented in part 6.