[PATCH] Add fields to struct gomp_thread for debugging purposes

[PATCH] Add fields to struct gomp_thread for debugging purposes

[Kevin Buettner]

This patch adds a new member named "pthread_id" to the gomp_thread
struct.  It is initialized in team.c.

It also adds a field named "parent" which is initialized to the thread
which created the thread in question.  For non-nested parallel
regions, this is always the master thread.

These new fields serve no purpose in a normally running OpenMP
program.  They are intended to be used by a debugger for identifying
threads and for finding the parent thread.

I've done a "make bootstrap" and have regression tested these changes
with no regressions found.

libgomp/ChangeLog:
    
    	* libgomp.h (struct gomp_thread): Add new member "pthread_id"
    	and "parent".
    	* team.c (struct gomp_thread_start_data): Add field "parent".
    	(gomp_thread_start): Set parent and pthread_id.
    	(gomp_team_start): Initialize master thread.  Initialize parent
    	in the start data.
---
 libgomp/libgomp.h |  7 +++++++
 libgomp/team.c    | 13 +++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index 940b5b8..7fa64f7 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -611,6 +611,13 @@ struct gomp_thread
      to any place.  */
   unsigned int place;
 
+  /* The pthread id associated with this thread.  This is required for
+     debugging.  */
+  pthread_t pthread_id;
+
+  /* Thread which spawned this one.  This is required for debugging.  */
+  struct gomp_thread *parent;
+
   /* User pthread thread pool */
   struct gomp_thread_pool *thread_pool;
 };
diff --git a/libgomp/team.c b/libgomp/team.c
index 676614a..17a0b3d 100644
--- a/libgomp/team.c
+++ b/libgomp/team.c
@@ -58,6 +58,7 @@ struct gomp_thread_start_data
   struct gomp_thread_pool *thread_pool;
   unsigned int place;
   bool nested;
+  struct gomp_thread *parent;
 };
 
 
@@ -89,9 +90,12 @@ gomp_thread_start (void *xdata)
   thr->ts = data->ts;
   thr->task = data->task;
   thr->place = data->place;
+  thr->parent = data->parent;
 
   thr->ts.team->ordered_release[thr->ts.team_id] = &thr->release;
 
+  thr->pthread_id = pthread_self ();
+
   /* Make thread pool local. */
   pool = thr->thread_pool;
 
@@ -718,6 +722,14 @@ gomp_team_start (void (*fn) (void *), void *data, unsigned nthreads,
       attr = &thread_attr;
     }
 
+  /* Add the master thread to threads[] and record its pthread id too.  */
+  if (pool->threads[0] == NULL)
+    {
+      pool->threads[0] = thr;
+      thr->pthread_id = pthread_self ();
+      thr->parent = NULL;
+    }
+
   start_data = gomp_alloca (sizeof (struct gomp_thread_start_data)
 			    * (nthreads-i));
 
@@ -812,6 +824,7 @@ gomp_team_start (void (*fn) (void *), void *data, unsigned nthreads,
       team->implicit_task[i].icv.bind_var = bind_var;
       start_data->thread_pool = pool;
       start_data->nested = nested;
+      start_data->parent = thr;
 
       attr = gomp_adjust_thread_attr (attr, &thread_attr);
       err = pthread_create (&pt, attr, gomp_thread_start, start_data++);

Re: [PATCH] Add fields to struct gomp_thread for debugging purposes

[Kevin Buettner]

Below is some additional information about the work I've been doing. 
It may be useful in understanding where I'm going with my libgomp
patch and other patches still to come...

I've been working on improvements to gdb, gcc, and libgomp which make
GDB able to better access variables in an OpenMP program.  Access to
some variables was already possible even before starting my work.  One
case which did not work is access to variables which are not used in
an OpenMP parallel region.

Consider this program:

    void foo (int a1) {}

    int
    main (void)
    {
      static int s1 = -41;
      int i1 = 11, i2;

      for (i2 = 1; i2 <= 2; i2++)
        {
          int pass = i2;
    #pragma omp parallel num_threads (2) firstprivate (i1)
          {
            foo (i1);
          }
          foo(pass);
        }
      foo (s1); foo (i2);
    }

When using GDB, if a breakpoint is placed on the "foo (i1)" line, GDB
was unable to access variables `s1', `i2', or `pass' in either of the
two threads that are created.

I recently committed a patch to GCC which now allows GDB to access all
of these variables, but at the moment, GDB is only able to find stack
based variables (e.g. i2 and pass) for the master thread.  Finding
static variables (e.g. s1) now works from any thread.

In order to find stack based variables from non-master thread(s), GDB
needs to be able to find the parent thread and then look for the
variables on the stack of the parent thread.

I've recently committed/pushed patches to GDB which map a thread
handle to one of GDB's internal thread identifiers.  This work
also exposes the mapping functionality via GDB's Python interface.
I provide an example of how this might be used later on.

On the GDB side of things, I have two other patches.

The first one implements an interface for a python function which I've
named `thread_parent'.  The idea here is that we implement
thread_parent in Python and that GDB then uses this implementation,
when appropriate, to locate a parent thread.

The second patch makes calls to the thread_parent to find the thread
to search for stack based variables which are not found on the stack
of the current thread under consideration.

I also have a patch to libgomp which adds pthread id and parent
fields to the gomp_thread struct.  These fields make it possible
for a debugger to know which pthread_t identifier corresponds to
a particular gomp thread.  It also makes it possible for the debugger
to discern the parent / child relationship among threads.

Using the above work, thread_parent can be implemented in Python
as follows:

def thr_parent (thr):
    try:
	h = gdb.parse_and_eval("gomp_tls_data->parent->pthread_id")
	parent = gdb.selected_inferior().thread_from_thread_handle(h)
	return parent
    except:
	return None

gdb.thread_parent = thr_parent

Note the ease with with the thread handle is obtained from libgomp. 
GDB's expression evaluator (which relies on having accurate DWARF
info) is used to fetch the thread handle via this expression:

    gomp_tls_data->parent->pthread_id

This is the very expression that one might use within libgomp to
access the thread handle of the parent.  The master thread has a NULL
parent, so attempting to access pthread_id for NULL throws an
exception, causing None to be returned.

The file in which this python code resides is placed in one of the
standard locations for python plugins for GDB.  It must follow the
naming conventions which will cause it to be loaded when the libgomp
shared object is loaded.  For the current version of libgomp, the file
is named libgomp.so.1.0.0-gdb.py.  I've been placing it within the
same directory as libgomp.so, which is just a symlink to
libgomp.so.1.0.0.  This file may also be placed a location relative to
auto-load in GDB's installed data directory.  Also, it is my
understanding that this script could be placed in a .debug_gdb_scripts
section of the libgomp shared library.

It is expected that some of this work will (still) prove useful when
an OMPD library is implemented for libgomp.  Some of it will, of
course, have to be discarded, but in the interim, it will improve
GDB's capability for debugging OpenMP programs.

Re: [PATCH] Add fields to struct gomp_thread for debugging purposes

[Jakub Jelinek]

On Mon, Oct 30, 2017 at 04:06:15PM -0700, Kevin Buettner wrote:
> This patch adds a new member named "pthread_id" to the gomp_thread
> struct.  It is initialized in team.c.

That part is reasonable, though it is unclear how the debugger will
query it (through OMPD, or through hardcoded name lookup of the struct and
field in libgomp's debug info, something else).  But the field certainly
has to be guarded by #ifdef LIBGOMP_USE_PTHREADS, otherwise it will break
NVPTX offloading or any other pthread-less libgomp ports.
Another question is exact placement of the field, struct gomp_thread
vs. struct gomp_team_state etc.  Maybe it is ok, as the pthread_id is
the same once the thread is created, doesn't change when we create more
levels.

> It also adds a field named "parent" which is initialized to the thread
> which created the thread in question.  For non-nested parallel
> regions, this is always the master thread.

What do you need it for and why isn't the current way of querying
parent (see e.g. omp_get_ancestor_thread_num or omp_get_team_size)
sufficient for the debugger?  Even if gomp_team_state doesn't contain
pthread_id, perhaps it would be more space and performance efficient
to store some pointer into struct gomp_team, gomp_team_state/gomp_thread
structs are in TLS which should be kept as small as possible.
Why do you care about which thread called pthread_create, rather than
what actually owns it right now (is the master thread)?

	Jakub

Re: [PATCH] Add fields to struct gomp_thread for debugging purposes

[Kevin Buettner]

On Tue, 31 Oct 2017 08:03:22 +0100
Jakub Jelinek <jakub@redhat.com> wrote:

> On Mon, Oct 30, 2017 at 04:06:15PM -0700, Kevin Buettner wrote:
> > This patch adds a new member named "pthread_id" to the gomp_thread
> > struct.  It is initialized in team.c.  
> 
> That part is reasonable, though it is unclear how the debugger will
> query it (through OMPD, or through hardcoded name lookup of the struct and
> field in libgomp's debug info, something else).  But the field certainly
> has to be guarded by #ifdef LIBGOMP_USE_PTHREADS, otherwise it will break
> NVPTX offloading or any other pthread-less libgomp ports.
> Another question is exact placement of the field, struct gomp_thread
> vs. struct gomp_team_state etc.  Maybe it is ok, as the pthread_id is
> the same once the thread is created, doesn't change when we create more
> levels.

Assuming we can figure out how to work the rest of it out, I'll submit
a new patch with the appropriate ifdef.

> > It also adds a field named "parent" which is initialized to the thread
> > which created the thread in question.  For non-nested parallel
> > regions, this is always the master thread.  
> 
> What do you need it for and why isn't the current way of querying
> parent (see e.g. omp_get_ancestor_thread_num or omp_get_team_size)
> sufficient for the debugger?  Even if gomp_team_state doesn't contain
> pthread_id, perhaps it would be more space and performance efficient
> to store some pointer into struct gomp_team, gomp_team_state/gomp_thread
> structs are in TLS which should be kept as small as possible.
> Why do you care about which thread called pthread_create, rather than
> what actually owns it right now (is the master thread)?

The cases that I'm considering are variants of this example:

#include <stdio.h>
#include <omp.h>

int
main (int artc, char **argv)
{
  int i = 42;
  int x = 10;
  omp_set_nested (1);
  omp_set_dynamic (0);
#pragma omp parallel num_threads (2) firstprivate(x)
  {
    int j = 43;
    int y = 20;
    x += omp_get_thread_num ();
#pragma omp parallel num_threads (2) firstprivate(y)
    {
      y += omp_get_thread_num ();
      #pragma omp critical
      printf ("inner threads: x=%d, y=%d\n", x, y);
    }
    #pragma omp critical
    printf ("outer threads: x=%d, y=%d, j=%d\n", x, y, j);
  }
  printf ("i = %d\n", i);
}

For this example, neither i nor j appear in the innermost parallel
region.

The variable i will be found on the stack of the master thread.

j, however, is a bit more interesting.  If the GDB user sets a
breakpoint on the "inner threads..." printf, j will either be found on
the on the stack of the master thread or on the stack of the first
thread spawned by the master thread.

So, in order for the debugger to find j, it should first see if it can
be found in the current thread (under consideration).  Next, it should
check the stack of the parent, and then the parent's parent, etc.

I've looked at the implementation of omp_get_ancestor_thread_num(),
but do not see a way to map the value returned to a gomp_thread. 
(Actually, it might be made to work for this example, but if we add
one more level of nesting, we'll need to find a parent thread which
does not appear in the thread pool.) However, I admit that there's
much that I don't understand about libgomp, so it's possible that I've
missed something.  I'd appreciate a pointer on how the existing
mechanisms might be used if that's the case.

(I should also note that in my testing of the above code, it can't work
at the moment anyway since j is optimized out.  I'm assuming that
this can be fixed so that it's not a moot point.)

Kevin

Re: [PATCH] Add fields to struct gomp_thread for debugging purposes

[Kevin Buettner]

On Sat, 4 Nov 2017 21:39:14 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> On Tue, 31 Oct 2017 08:03:22 +0100
> Jakub Jelinek <jakub@redhat.com> wrote:
> 
> > On Mon, Oct 30, 2017 at 04:06:15PM -0700, Kevin Buettner wrote:  
> > > This patch adds a new member named "pthread_id" to the gomp_thread
> > > struct.  It is initialized in team.c.    
> > 
> > That part is reasonable, though it is unclear how the debugger will
> > query it (through OMPD, or through hardcoded name lookup of the struct and
> > field in libgomp's debug info, something else).  But the field certainly
> > has to be guarded by #ifdef LIBGOMP_USE_PTHREADS, otherwise it will break
> > NVPTX offloading or any other pthread-less libgomp ports.
> > Another question is exact placement of the field, struct gomp_thread
> > vs. struct gomp_team_state etc.  Maybe it is ok, as the pthread_id is
> > the same once the thread is created, doesn't change when we create more
> > levels.  
> 
> Assuming we can figure out how to work the rest of it out, I'll submit
> a new patch with the appropriate ifdef.

I've decided to try for a more incremental approach.  This patch,
below, retains the portion that looked reasonable to you.  I've
stripped out the portions for finding the thread parent and have
added the ifdef guards that you asked for.

Is this part okay?

___

Add field to struct gomp_thread for debugging purposes

This patch adds a new member named "pthread_id" to the gomp_thread
struct.  It is initialized in team.c.

This new field serves no purpose in a normally running OpenMP
program.  It is intended to be used by a debugger for identifying
threads.

I've done a "make bootstrap" and have regression tested these changes
with no regressions found.

libgomp/ChangeLog:
    
    	* libgomp.h (struct gomp_thread): Add new member "pthread_id".
    	* team.c (gomp_thread_start): Set pthread_id.
    	(gomp_team_start): Initialize master thread.
---
 libgomp/libgomp.h |  6 ++++++
 libgomp/team.c    | 13 +++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index 940b5b8..5dafb3c 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -611,6 +611,12 @@ struct gomp_thread
      to any place.  */
   unsigned int place;
 
+#ifdef LIBGOMP_USE_PTHREADS
+  /* The pthread id associated with this thread.  This is required for
+     debugging.  */
+  pthread_t pthread_id;
+#endif
+
   /* User pthread thread pool */
   struct gomp_thread_pool *thread_pool;
 };
diff --git a/libgomp/team.c b/libgomp/team.c
index 676614a..6292b22 100644
--- a/libgomp/team.c
+++ b/libgomp/team.c
@@ -92,6 +92,10 @@ gomp_thread_start (void *xdata)
 
   thr->ts.team->ordered_release[thr->ts.team_id] = &thr->release;
 
+#ifdef LIBGOMP_USE_PTHREADS
+  thr->pthread_id = pthread_self ();
+#endif
+
   /* Make thread pool local. */
   pool = thr->thread_pool;
 
@@ -718,6 +722,15 @@ gomp_team_start (void (*fn) (void *), void *data, unsigned nthreads,
       attr = &thread_attr;
     }
 
+  /* Add the master thread to threads[] and record its pthread id too.  */
+  if (pool->threads[0] == NULL)
+    {
+      pool->threads[0] = thr;
+#ifdef LIBGOMP_USE_PTHREADS
+      thr->pthread_id = pthread_self ();
+#endif
+    }
+
   start_data = gomp_alloca (sizeof (struct gomp_thread_start_data)
 			    * (nthreads-i));
 

Re: [PATCH] Add fields to struct gomp_thread for debugging purposes

[Jakub Jelinek]

On Wed, Nov 22, 2017 at 08:25:35AM -0700, Kevin Buettner wrote:
> On Sat, 4 Nov 2017 21:39:14 -0700
> Kevin Buettner <kevinb@redhat.com> wrote:
> 
> > On Tue, 31 Oct 2017 08:03:22 +0100
> > Jakub Jelinek <jakub@redhat.com> wrote:
> > 
> > > On Mon, Oct 30, 2017 at 04:06:15PM -0700, Kevin Buettner wrote:  
> > > > This patch adds a new member named "pthread_id" to the gomp_thread
> > > > struct.  It is initialized in team.c.    
> > > 
> > > That part is reasonable, though it is unclear how the debugger will
> > > query it (through OMPD, or through hardcoded name lookup of the struct and
> > > field in libgomp's debug info, something else).  But the field certainly
> > > has to be guarded by #ifdef LIBGOMP_USE_PTHREADS, otherwise it will break
> > > NVPTX offloading or any other pthread-less libgomp ports.
> > > Another question is exact placement of the field, struct gomp_thread
> > > vs. struct gomp_team_state etc.  Maybe it is ok, as the pthread_id is
> > > the same once the thread is created, doesn't change when we create more
> > > levels.  
> > 
> > Assuming we can figure out how to work the rest of it out, I'll submit
> > a new patch with the appropriate ifdef.
> 
> I've decided to try for a more incremental approach.  This patch,
> below, retains the portion that looked reasonable to you.  I've
> stripped out the portions for finding the thread parent and have
> added the ifdef guards that you asked for.
> 
> Is this part okay?

Actually, isn't this redundant information at least on Linux?
How would the debugger find the pthread_id field in the TLS?

If it is TLS and libgomp uses -ftls-model=initial-exec on Linux, then
the difference between the base of the TLS area (which is I believe
pthread_self ()) and struct gomp_thread is fixed (and can be found in the
debug info on most linux targets, except e.g. aarch64 which doesn't have
as/ld support for dtoff-ish relocations).  So, if the debugger can find
the gomp_thread, then it can find pthread_self () and vice versa.  Dunno
if libthread_db or infinity notes in libpthread provide any info for this,
and/or infinity notes in libgomp could add some further info so that the
debugger can do this portably.

Of course, for targets where this isn't possible I'm not against your patch
if it is conditionalized on those targets that can't do better.
In any case, IMHO the debugger shouldn't hardcode that stuff but
ask infinity notes in libgomp and libpthread how to compute it.
Or as temporary alternative to the infinity notes it could be even a
function in the library that given struct gomp_thread returns corresponding
pthread_t.  Though, it would be nice to know exactly what the debugger wants
to do.  Perhaps it is also already part of OMPD and we could just implement
subset of it.

	Jakub