diff mbox

Fix libgomp crash without TLS (PR42616)

Message ID CAAp9h91Gd6nauh5Vm4bStXwKcMDdc_fDs+xHtZa1c7DziCpv3g@mail.gmail.com
State New
Headers show

Commit Message

Varvara Rainchik Sept. 30, 2014, 7:03 a.m. UTC 
Corrected patch: call pthread_setspecific (gomp_tls_key, NULL) in
gomp_thread_start if HAVE_TLS is not defined.

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.


---




2014-09-24 14:19 GMT+04:00 Varvara Rainchik <varvara.s.rainchik@gmail.com>:
> *Ping*
>
> 2014-09-19 15:41 GMT+04:00 Varvara Rainchik <varvara.s.rainchik@gmail.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

Comments

Jakub Jelinek Sept. 30, 2014, 9:52 a.m. UTC | #1 
On Tue, Sep 30, 2014 at 11:03:47AM +0400, Varvara Rainchik wrote:
> Corrected patch: call pthread_setspecific (gomp_tls_key, NULL) in
> gomp_thread_start if HAVE_TLS is not defined.
> 
> 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.

I actually wonder when we have emutls support in libgcc if it wouldn't
be better to just define HAVE_TLS always to 1 (i.e. remove all the
conditionals on it), then you wouldn't need to bother with this at all.

I don't have an OS which doesn't support native TLS though, so somebody with
such a system would need to test it and benchmark if it doesn't make things
slower.

Richard, thoughts on this?

	Jakub
Richard Henderson Sept. 30, 2014, 2:40 p.m. UTC | #2 
On 09/30/2014 02:52 AM, Jakub Jelinek wrote:
> On Tue, Sep 30, 2014 at 11:03:47AM +0400, Varvara Rainchik wrote:
>> Corrected patch: call pthread_setspecific (gomp_tls_key, NULL) in
>> gomp_thread_start if HAVE_TLS is not defined.
>>
>> 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.
> 
> I actually wonder when we have emutls support in libgcc if it wouldn't
> be better to just define HAVE_TLS always to 1 (i.e. remove all the
> conditionals on it), then you wouldn't need to bother with this at all.
> 
> I don't have an OS which doesn't support native TLS though, so somebody with
> such a system would need to test it and benchmark if it doesn't make things
> slower.
> 
> Richard, thoughts on this?

I like that idea better as well.


r~
diff mbox

Patch

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..1854d8a 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,9 @@  gomp_thread_start (void *xdata)
   gomp_sem_destroy (&thr->release);
   thr->thread_pool = NULL;
   thr->task = NULL;
+#ifndef HAVE_TLS
+  pthread_setspecific (gomp_tls_key, NULL);
+#endif
   return NULL;
 }

@@ -222,8 +226,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 +922,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 +944,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 +952,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)
 {