Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik <varvara dot s dot rainchik at gmail dot com>]

I've corrected my patch accordingly to what you said. To diffirentiate
second case in destructor I've added pthread_setspecific
(gomp_tls_key, NULL) at the end of gomp_thread_start. So, destructor
can simply skip the case when pthread_getspecific (gomp_tls_key)
returns 0. I also think that it's better to set 0 in gomp_thread_start
explicitly as thread data is initialized by a local variable in this
function.

But, I see that pthread_getspecific always returns 0 in destrucor
because data pointer is implicitly set to 0 before destructor call in
glibc:

(pthread_create.c):

/* Always clear the data. */
level2[inner].data = NULL;

/* Make sure the data corresponds to a valid
key. This test fails if the key was
deallocated and also if it was
re-allocated. It is the user's
responsibility to free the memory in this
case. */
if (level2[inner].seq
   == __pthread_keys[idx].seq
   /* It is not necessary to register a destructor
      function. */
 && __pthread_keys[idx].destr != NULL)
/* Call the user-provided destructor. */
__pthread_keys[idx].destr (data);

I suppose it's not necessary if everything is cleaned up in
gomp_thread_start  and destructor. What do you think?


Changes are bootstrapped and regtested on x86_64-linux.

2014-09-19  Varvara Rainchik  <varvara.rainchik@intel.com>

        * libgomp.h (gomp_thread): For non TLS case create thread data.
        * team.c (non_tls_thread_data_destructor,
create_non_tls_thread_data): New functions.


---
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index bcd5b34..2f33d99 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -467,9 +467,15 @@ static inline struct gomp_thread *gomp_thread (void)
 }
 #else
 extern pthread_key_t gomp_tls_key;
-static inline struct gomp_thread *gomp_thread (void)
+extern struct gomp_thread *create_non_tls_thread_data (void);
+static struct gomp_thread *gomp_thread (void)
 {
-  return pthread_getspecific (gomp_tls_key);
+  struct gomp_thread *thr = pthread_getspecific (gomp_tls_key);
+  if (thr == NULL)
+  {
+    thr = create_non_tls_thread_data ();
+  }
+  return thr;
 }
 #endif

diff --git a/libgomp/team.c b/libgomp/team.c
index e6a6d8f..a692df8 100644
--- a/libgomp/team.c
+++ b/libgomp/team.c
@@ -41,6 +41,7 @@ pthread_key_t gomp_thread_destructor;
 __thread struct gomp_thread gomp_tls_data;
 #else
 pthread_key_t gomp_tls_key;
+struct gomp_thread initial_thread_tls_data;
 #endif


@@ -130,6 +131,7 @@ gomp_thread_start (void *xdata)
   gomp_sem_destroy (&thr->release);
   thr->thread_pool = NULL;
   thr->task = NULL;
+  pthread_setspecific (gomp_tls_key, NULL);
   return NULL;
 }

@@ -222,8 +224,16 @@ gomp_free_pool_helper (void *thread_pool)
 void
 gomp_free_thread (void *arg __attribute__((unused)))
 {
-  struct gomp_thread *thr = gomp_thread ();
-  struct gomp_thread_pool *pool = thr->thread_pool;
+  struct gomp_thread *thr;
+  struct gomp_thread_pool *pool;
+#ifdef HAVE_TLS
+  thr = gomp_thread ();
+#else
+  thr = pthread_getspecific (gomp_tls_key);
+  if (thr == NULL)
+    return;
+#endif
+  pool = thr->thread_pool;
   if (pool)
     {
       if (pool->threads_used > 0)
@@ -910,6 +920,21 @@ gomp_team_end (void)
     }
 }

+/* Destructor for data created in create_non_tls_thread_data.  */
+
+#ifndef HAVE_TLS
+void
+non_tls_thread_data_destructor (void *arg __attribute__((unused)))
+{
+  struct gomp_thread *thr = pthread_getspecific (gomp_tls_key);
+  if (thr != NULL && thr != &initial_thread_tls_data)
+  {
+    gomp_free_thread (arg);
+    free (thr);
+    pthread_setspecific (gomp_tls_key, NULL);
+  }
+}
+#endif

 /* Constructors for this file.  */

@@ -917,9 +942,7 @@ static void __attribute__((constructor))
 initialize_team (void)
 {
 #ifndef HAVE_TLS
-  static struct gomp_thread initial_thread_tls_data;
-
-  pthread_key_create (&gomp_tls_key, NULL);
+  pthread_key_create (&gomp_tls_key, non_tls_thread_data_destructor);
   pthread_setspecific (gomp_tls_key, &initial_thread_tls_data);
 #endif

@@ -927,6 +950,19 @@ initialize_team (void)
     gomp_fatal ("could not create thread pool destructor.");
 }

+/* Create data for thread created by pthread_create.  */
+
+#ifndef HAVE_TLS
+struct gomp_thread *create_non_tls_thread_data (void)
+{
+  struct gomp_thread *thr = gomp_malloc_cleared (sizeof (struct gomp_thread));
+  pthread_setspecific (gomp_tls_key, thr);
+  gomp_sem_init (&thr->release, 0);
+
+  return thr;
+}
+#endif
+
 static void __attribute__((destructor))
 team_destructor (void)
{

2014-09-02 14:36 GMT+04:00 Varvara Rainchik <varvara.s.rainchik@gmail.com>:
> May I use gomp_free_thread as a destructor for pthread_key_create?
> Then I'll make initial_thread_tls_data global for the first case, but
> how can I differentiate thread created by gomp_thread_start (second
> case)?
>
> 2014-09-01 14:51 GMT+04:00 Jakub Jelinek <jakub@redhat.com>:
>> On Fri, Aug 29, 2014 at 10:40:57AM -0700, Richard Henderson wrote:
>>> On 08/06/2014 03:05 AM, Varvara Rainchik wrote:
>>> >         * libgomp.h (gomp_thread): For non TLS case create thread data.
>>> >         * team.c (create_non_tls_thread_data): New function.
>>> >
>>> >
>>> > ---
>>> > diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
>>> > index a1482cc..cf3ec8f 100644
>>> > --- a/libgomp/libgomp.h
>>> > +++ b/libgomp/libgomp.h
>>> > @@ -479,9 +479,15 @@ static inline struct gomp_thread *gomp_thread (void)
>>> > }
>>> > #else
>>> > extern pthread_key_t gomp_tls_key;
>>> > +extern struct gomp_thread *create_non_tls_thread_data (void);
>>> > static inline struct gomp_thread *gomp_thread (void)
>>> > {
>>> > -  return pthread_getspecific (gomp_tls_key);
>>> > +  struct gomp_thread *thr = pthread_getspecific (gomp_tls_key);
>>> > +  if (thr == NULL)
>>> > +  {
>>> > +    thr = create_non_tls_thread_data ();
>>> > +  }
>>> > +  return thr;
>>> > }
>>>
>>> This should never happen.
>>
>> I guess it can happen if you mix up explicit pthread_create and libgomp APIs.
>> initialize_team will only initialize it in the initial thread, while if you
>> use #pragma omp ... or omp_* calls from a thread created with
>> pthread_create, in the !HAVE_TLS case pthread_getspecific will return NULL.
>>
>> Now, the patch doesn't handle that case completely though (and is badly
>> formatted), the problem is that if we allocate in the !HAVE_TLS case
>> in non-initial thread the TLS data, we want to free them again, so that
>> would mean pthread_key_create with non-NULL destructor, and then we need to
>> differentiate in between the 3 cases - key equal to &initial_thread_tls_data
>> (would need to move out of the block context), no freeing needed, thread
>> created by gomp_thread_start, no freeing needed, otherwise free.
>>
>>> The thread-specific data is set in gomp_thread_start and initialize_team.
>>>
>>> Where are you getting a call to gomp_thread that hasn't been through one of
>>> those functions?
>>
>>         Jakub