Fix libgomp crash without TLS (PR42616)

Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

Hi,

The issue was firstly observed on NDK gcc since TLS is not supported
in Android bionic. I also see the same failure on gcc configured for
linux with –disable-tls, libgomp make check log:

FAIL: libgomp.c/affinity-1.c execution test
FAIL: libgomp.c/icv-2.c execution test
FAIL: libgomp.c/lock-3.c execution test
FAIL: libgomp.c/target-6.c execution test

These tests except affinity-1.c fail because gomp_thread () function
returns null pointer. I’ve found 2 bugs, first one addresses this
problem on Windows:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42616;
second one addresses original problem (for both cases, with and without TLS):
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36242.
Tests from both bugs fail with –disable-tls. So, it seems that non TLS
case was fixed just partially. The following patch solves the problem.
With this patch 3 tests from make check pass, affinity-1.c fails, but
I think it’s other non TLS problem.
Changes are bootstrapped and regtested on x86_64-linux.


2014-08-06  Varvara Rainchik  <varvara.rainchik@intel.com>

        * 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;
}
#endif

diff --git a/libgomp/team.c b/libgomp/team.c
index e6a6d8f..bf8bd4b 100644
--- a/libgomp/team.c
+++ b/libgomp/team.c
@@ -927,6 +927,17 @@ initialize_team (void)
     gomp_fatal ("could not create thread pool destructor.");
}

+#ifndef HAVE_TLS
+struct gomp_thread *create_non_tls_thread_data (void)
+{
+  struct gomp_thread *thr = gomp_malloc (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)
{
---


Is it ok?


Best regards,

Varvara

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

*Ping*

Thanks,
Varvara

2014-08-06 14:05 GMT+04:00 Varvara Rainchik <varvara.s.rainchik@gmail.com>:
> Hi,
>
> The issue was firstly observed on NDK gcc since TLS is not supported
> in Android bionic. I also see the same failure on gcc configured for
> linux with –disable-tls, libgomp make check log:
>
> FAIL: libgomp.c/affinity-1.c execution test
> FAIL: libgomp.c/icv-2.c execution test
> FAIL: libgomp.c/lock-3.c execution test
> FAIL: libgomp.c/target-6.c execution test
>
> These tests except affinity-1.c fail because gomp_thread () function
> returns null pointer. I’ve found 2 bugs, first one addresses this
> problem on Windows:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42616;
> second one addresses original problem (for both cases, with and without TLS):
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36242.
> Tests from both bugs fail with –disable-tls. So, it seems that non TLS
> case was fixed just partially. The following patch solves the problem.
> With this patch 3 tests from make check pass, affinity-1.c fails, but
> I think it’s other non TLS problem.
> Changes are bootstrapped and regtested on x86_64-linux.
>
>
> 2014-08-06  Varvara Rainchik  <varvara.rainchik@intel.com>
>
>         * 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;
> }
> #endif
>
> diff --git a/libgomp/team.c b/libgomp/team.c
> index e6a6d8f..bf8bd4b 100644
> --- a/libgomp/team.c
> +++ b/libgomp/team.c
> @@ -927,6 +927,17 @@ initialize_team (void)
>      gomp_fatal ("could not create thread pool destructor.");
> }
>
> +#ifndef HAVE_TLS
> +struct gomp_thread *create_non_tls_thread_data (void)
> +{
> +  struct gomp_thread *thr = gomp_malloc (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)
> {
> ---
>
>
> Is it ok?
>
>
> Best regards,
>
> Varvara

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

Hi again! I want to remind that issue is urgent for Android.

2014-08-13 12:13 GMT+04:00 Varvara Rainchik <varvara.s.rainchik@gmail.com>:
> *Ping*
>
> Thanks,
> Varvara
>
> 2014-08-06 14:05 GMT+04:00 Varvara Rainchik <varvara.s.rainchik@gmail.com>:
>> Hi,
>>
>> The issue was firstly observed on NDK gcc since TLS is not supported
>> in Android bionic. I also see the same failure on gcc configured for
>> linux with –disable-tls, libgomp make check log:
>>
>> FAIL: libgomp.c/affinity-1.c execution test
>> FAIL: libgomp.c/icv-2.c execution test
>> FAIL: libgomp.c/lock-3.c execution test
>> FAIL: libgomp.c/target-6.c execution test
>>
>> These tests except affinity-1.c fail because gomp_thread () function
>> returns null pointer. I’ve found 2 bugs, first one addresses this
>> problem on Windows:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42616;
>> second one addresses original problem (for both cases, with and without TLS):
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=36242.
>> Tests from both bugs fail with –disable-tls. So, it seems that non TLS
>> case was fixed just partially. The following patch solves the problem.
>> With this patch 3 tests from make check pass, affinity-1.c fails, but
>> I think it’s other non TLS problem.
>> Changes are bootstrapped and regtested on x86_64-linux.
>>
>>
>> 2014-08-06  Varvara Rainchik  <varvara.rainchik@intel.com>
>>
>>         * 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;
>> }
>> #endif
>>
>> diff --git a/libgomp/team.c b/libgomp/team.c
>> index e6a6d8f..bf8bd4b 100644
>> --- a/libgomp/team.c
>> +++ b/libgomp/team.c
>> @@ -927,6 +927,17 @@ initialize_team (void)
>>      gomp_fatal ("could not create thread pool destructor.");
>> }
>>
>> +#ifndef HAVE_TLS
>> +struct gomp_thread *create_non_tls_thread_data (void)
>> +{
>> +  struct gomp_thread *thr = gomp_malloc (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)
>> {
>> ---
>>
>>
>> Is it ok?
>>
>>
>> Best regards,
>>
>> Varvara

Re: Fix libgomp crash without TLS (PR42616)

[Richard Henderson]

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.

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?


r~

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

I've checked several tests, I see that for all tests failure occurs in
function gomp_icv (). E.g.:

icv-2:
#0  gomp_icv (write=true) at ../../../libgomp/libgomp.h:494
#1  omp_set_num_threads (n=6) at ../../../libgomp/env.c:1282
#2  0x0000000000404014 in tf ()
#3  0x000000000040d063 in start_thread ()
#4  0x0000000000450139 in clone ()

lock-3:
#0  gomp_icv (write=true) at ../../../libgomp/libgomp.h:494
#1  omp_test_nest_lock (lock=0x6dd580 <lock>) at
../../../libgomp/config/linux/lock.c:109
#2  0x0000000000403fbc in tf ()
#3  0x000000000040ccd3 in start_thread ()
#4  0x000000000044fda9 in clone ()

2014-08-29 21:40 GMT+04:00 Richard Henderson <rth@redhat.com>:
> 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.
>
> 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?
>
>
> r~
>

Re: Fix libgomp crash without TLS (PR42616)

[Jakub Jelinek]

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

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

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

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

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

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

*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

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

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.


---
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)
 {




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

Re: Fix libgomp crash without TLS (PR42616)

[Jakub Jelinek]

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

Re: Fix libgomp crash without TLS (PR42616)

[Richard Henderson]

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~

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

Ok, then here it is a new patch (tested and bootstrapped on linux).

On linux with --disable-tls now all libgomp make check tests pass; for
Android I've patched toolchain and tried test from one of the
mentioned bugs, test passes too.

Is there some benchmark to check performance?


2014-10-01  Varvara Rainchik  <varvara.rainchik@intel.com>

        * libgomp.h (HAVE_TLS): Set to 1.

--
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -45,6 +45,8 @@
 # pragma GCC visibility push(hidden)
 #endif

+#define HAVE_TLS 1
+
 /* If we were a C++ library, we'd get this from <std/atomic>.  */
 enum memmodel
 {

2014-09-30 18:40 GMT+04:00 Richard Henderson <rth@redhat.com>:
> 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~
>

Re: Fix libgomp crash without TLS (PR42616)

[Jakub Jelinek]

On Wed, Oct 01, 2014 at 08:44:59PM +0400, Varvara Rainchik wrote:
> Ok, then here it is a new patch (tested and bootstrapped on linux).
> 
> On linux with --disable-tls now all libgomp make check tests pass; for
> Android I've patched toolchain and tried test from one of the
> mentioned bugs, test passes too.

> Is there some benchmark to check performance?

There is SPEC OMP,
http://www.spec.org/hpg/omp2001/
EPCC,
http://www2.epcc.ed.ac.uk/computing/research_activities/openmpbench/openmp_index.html
NAS,
http://www.nas.nasa.gov/publications/npb.html
http://phase.hpcc.jp/Omni/benchmarks/NPB/
Rodinia,
https://www.cs.virginia.edu/~skadron/wiki/rodinia/index.php/Main_Page

Now, I wonder on which OS and why does config/tls.m4 CHECK_GCC_TLS
actually fail?  Can you figure that out?

If we get rid of HAVE_TLS code altogether, we might lose support of
some very old OSes, e.g. some Linux distros with a recent gcc and binutils
(so that emutls isn't used), but very old glibc (that doesn't support
TLS or supports it incorrectly, think of pre-2002 glibc).  So, if we get
rid of !HAVE_TLS code in libgomp, it would be nice if config/tls.m4 detected
it properly and we'd just fail at configure time.
And if we don't, just make sure that on Android, Darwin and/or M$Win (or
whatever other OS you had in mind which does support pthreads, but doesn't
support native TLS) find out why HAVE_AS_TLS is not defined (guess
config.log should explain that).

> 2014-10-01  Varvara Rainchik  <varvara.rainchik@intel.com>
> 
>         * libgomp.h (HAVE_TLS): Set to 1.

	Jakub

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

> Now, I wonder on which OS and why does config/tls.m4 CHECK_GCC_TLS
> actually fail?  Can you figure that out?
>

On Android check passes with --disable-tls (standard while building
gcc for Android as TLS is not supported in bionic) and fails with
--enable-tls (i686-linux-android/libgomp/conftest.c:32: undefined
reference to `___tls_get_addr'). So, HAVE_TLS is not defined in both
cases.

> If we get rid of HAVE_TLS code altogether, we might lose support of
> some very old OSes, e.g. some Linux distros with a recent gcc and binutils
> (so that emutls isn't used), but very old glibc (that doesn't support
> TLS or supports it incorrectly, think of pre-2002 glibc).  So, if we get
> rid of !HAVE_TLS code in libgomp, it would be nice if config/tls.m4 detected
> it properly and we'd just fail at configure time.

How can we check this in config/tls.m4? Can we just combine tests on
TLS and emutls? E.g. check whether HAVE_TLS and USE_EMUTLS are both
defined.

> And if we don't, just make sure that on Android, Darwin and/or M$Win (or
> whatever other OS you had in mind which does support pthreads, but doesn't
> support native TLS) find out why HAVE_AS_TLS is not defined (guess
> config.log should explain that).

HAVE_AS_TLS is also not defined for Android as it depends on --enable-tls.

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

*Ping*

2014-10-13 14:48 GMT+04:00 Varvara Rainchik <varvara.s.rainchik@gmail.com>:
>> Now, I wonder on which OS and why does config/tls.m4 CHECK_GCC_TLS
>> actually fail?  Can you figure that out?
>>
>
> On Android check passes with --disable-tls (standard while building
> gcc for Android as TLS is not supported in bionic) and fails with
> --enable-tls (i686-linux-android/libgomp/conftest.c:32: undefined
> reference to `___tls_get_addr'). So, HAVE_TLS is not defined in both
> cases.
>
>> If we get rid of HAVE_TLS code altogether, we might lose support of
>> some very old OSes, e.g. some Linux distros with a recent gcc and binutils
>> (so that emutls isn't used), but very old glibc (that doesn't support
>> TLS or supports it incorrectly, think of pre-2002 glibc).  So, if we get
>> rid of !HAVE_TLS code in libgomp, it would be nice if config/tls.m4 detected
>> it properly and we'd just fail at configure time.
>
> How can we check this in config/tls.m4? Can we just combine tests on
> TLS and emutls? E.g. check whether HAVE_TLS and USE_EMUTLS are both
> defined.
>
>> And if we don't, just make sure that on Android, Darwin and/or M$Win (or
>> whatever other OS you had in mind which does support pthreads, but doesn't
>> support native TLS) find out why HAVE_AS_TLS is not defined (guess
>> config.log should explain that).
>
> HAVE_AS_TLS is also not defined for Android as it depends on --enable-tls.

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

Hi Jakub,

Do you think this patch is ok for upstream:

2014-12-01  Varvara Rainchik  <varvara.rainchik@intel.com>

        * libgomp/libgomp.h: Eliminate case when HAVE_TLS is not defined:
        always use tls emulation.
        * libgomp/team.c: Likewise.

--
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index a1482cc..a659ebc 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -471,19 +471,11 @@ enum gomp_cancel_kind

 /* ... and here is that TLS data.  */

-#ifdef HAVE_TLS
 extern __thread struct gomp_thread gomp_tls_data;
 static inline struct gomp_thread *gomp_thread (void)
 {
   return &gomp_tls_data;
 }
-#else
-extern pthread_key_t gomp_tls_key;
-static inline struct gomp_thread *gomp_thread (void)
-{
-  return pthread_getspecific (gomp_tls_key);
-}
-#endif

 extern struct gomp_task_icv *gomp_new_icv (void);

diff --git a/libgomp/team.c b/libgomp/team.c
index e6a6d8f..2e8dc47 100644
--- a/libgomp/team.c
+++ b/libgomp/team.c
@@ -37,12 +37,7 @@ pthread_key_t gomp_thread_destructor;


 /* This is the libgomp per-thread data structure.  */
-#ifdef HAVE_TLS
 __thread struct gomp_thread gomp_tls_data;
-#else
-pthread_key_t gomp_tls_key;
-#endif
-

 /* This structure is used to communicate across pthread_create.  */

@@ -70,13 +65,7 @@ gomp_thread_start (void *xdata)
   void (*local_fn) (void *);
   void *local_data;

-#ifdef HAVE_TLS
   thr = &gomp_tls_data;
-#else
-  struct gomp_thread local_thr;
-  thr = &local_thr;
-  pthread_setspecific (gomp_tls_key, thr);
-#endif
   gomp_sem_init (&thr->release, 0);

   /* Extract what we need from data.  */
@@ -916,13 +905,6 @@ gomp_team_end (void)
 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_setspecific (gomp_tls_key, &initial_thread_tls_data);
-#endif
-
   if (pthread_key_create (&gomp_thread_destructor, gomp_free_thread) != 0)
     gomp_fatal ("could not create thread pool destructor.");
 }

Or should I add some extra checks in config/tls.m4? As far as I
understand you have mentioned case when both native tls and tls
emulation are not supported. So, is it sufficient to check that
HAVE_TLS and USE_EMUTLS are not defined to detect this case?

2014-11-10 16:15 GMT+03:00 Varvara Rainchik <varvara.s.rainchik@gmail.com>:
> *Ping*
>
> 2014-10-13 14:48 GMT+04:00 Varvara Rainchik <varvara.s.rainchik@gmail.com>:
>>> Now, I wonder on which OS and why does config/tls.m4 CHECK_GCC_TLS
>>> actually fail?  Can you figure that out?
>>>
>>
>> On Android check passes with --disable-tls (standard while building
>> gcc for Android as TLS is not supported in bionic) and fails with
>> --enable-tls (i686-linux-android/libgomp/conftest.c:32: undefined
>> reference to `___tls_get_addr'). So, HAVE_TLS is not defined in both
>> cases.
>>
>>> If we get rid of HAVE_TLS code altogether, we might lose support of
>>> some very old OSes, e.g. some Linux distros with a recent gcc and binutils
>>> (so that emutls isn't used), but very old glibc (that doesn't support
>>> TLS or supports it incorrectly, think of pre-2002 glibc).  So, if we get
>>> rid of !HAVE_TLS code in libgomp, it would be nice if config/tls.m4 detected
>>> it properly and we'd just fail at configure time.
>>
>> How can we check this in config/tls.m4? Can we just combine tests on
>> TLS and emutls? E.g. check whether HAVE_TLS and USE_EMUTLS are both
>> defined.
>>
>>> And if we don't, just make sure that on Android, Darwin and/or M$Win (or
>>> whatever other OS you had in mind which does support pthreads, but doesn't
>>> support native TLS) find out why HAVE_AS_TLS is not defined (guess
>>> config.log should explain that).
>>
>> HAVE_AS_TLS is also not defined for Android as it depends on --enable-tls.

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

Hi guys,

Could you please take a look at this issue? This fix is still urgent
for Android.

2014-12-01 18:25 GMT+03:00 Varvara Rainchik <varvara.s.rainchik@gmail.com>:
> Hi Jakub,
>
> Do you think this patch is ok for upstream:
>
> 2014-12-01  Varvara Rainchik  <varvara.rainchik@intel.com>
>
>         * libgomp/libgomp.h: Eliminate case when HAVE_TLS is not defined:
>         always use tls emulation.
>         * libgomp/team.c: Likewise.
>
> --
> diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
> index a1482cc..a659ebc 100644
> --- a/libgomp/libgomp.h
> +++ b/libgomp/libgomp.h
> @@ -471,19 +471,11 @@ enum gomp_cancel_kind
>
>  /* ... and here is that TLS data.  */
>
> -#ifdef HAVE_TLS
>  extern __thread struct gomp_thread gomp_tls_data;
>  static inline struct gomp_thread *gomp_thread (void)
>  {
>    return &gomp_tls_data;
>  }
> -#else
> -extern pthread_key_t gomp_tls_key;
> -static inline struct gomp_thread *gomp_thread (void)
> -{
> -  return pthread_getspecific (gomp_tls_key);
> -}
> -#endif
>
>  extern struct gomp_task_icv *gomp_new_icv (void);
>
> diff --git a/libgomp/team.c b/libgomp/team.c
> index e6a6d8f..2e8dc47 100644
> --- a/libgomp/team.c
> +++ b/libgomp/team.c
> @@ -37,12 +37,7 @@ pthread_key_t gomp_thread_destructor;
>
>
>  /* This is the libgomp per-thread data structure.  */
> -#ifdef HAVE_TLS
>  __thread struct gomp_thread gomp_tls_data;
> -#else
> -pthread_key_t gomp_tls_key;
> -#endif
> -
>
>  /* This structure is used to communicate across pthread_create.  */
>
> @@ -70,13 +65,7 @@ gomp_thread_start (void *xdata)
>    void (*local_fn) (void *);
>    void *local_data;
>
> -#ifdef HAVE_TLS
>    thr = &gomp_tls_data;
> -#else
> -  struct gomp_thread local_thr;
> -  thr = &local_thr;
> -  pthread_setspecific (gomp_tls_key, thr);
> -#endif
>    gomp_sem_init (&thr->release, 0);
>
>    /* Extract what we need from data.  */
> @@ -916,13 +905,6 @@ gomp_team_end (void)
>  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_setspecific (gomp_tls_key, &initial_thread_tls_data);
> -#endif
> -
>    if (pthread_key_create (&gomp_thread_destructor, gomp_free_thread) != 0)
>      gomp_fatal ("could not create thread pool destructor.");
>  }
>
> Or should I add some extra checks in config/tls.m4? As far as I
> understand you have mentioned case when both native tls and tls
> emulation are not supported. So, is it sufficient to check that
> HAVE_TLS and USE_EMUTLS are not defined to detect this case?
>
> 2014-11-10 16:15 GMT+03:00 Varvara Rainchik <varvara.s.rainchik@gmail.com>:
>> *Ping*
>>
>> 2014-10-13 14:48 GMT+04:00 Varvara Rainchik <varvara.s.rainchik@gmail.com>:
>>>> Now, I wonder on which OS and why does config/tls.m4 CHECK_GCC_TLS
>>>> actually fail?  Can you figure that out?
>>>>
>>>
>>> On Android check passes with --disable-tls (standard while building
>>> gcc for Android as TLS is not supported in bionic) and fails with
>>> --enable-tls (i686-linux-android/libgomp/conftest.c:32: undefined
>>> reference to `___tls_get_addr'). So, HAVE_TLS is not defined in both
>>> cases.
>>>
>>>> If we get rid of HAVE_TLS code altogether, we might lose support of
>>>> some very old OSes, e.g. some Linux distros with a recent gcc and binutils
>>>> (so that emutls isn't used), but very old glibc (that doesn't support
>>>> TLS or supports it incorrectly, think of pre-2002 glibc).  So, if we get
>>>> rid of !HAVE_TLS code in libgomp, it would be nice if config/tls.m4 detected
>>>> it properly and we'd just fail at configure time.
>>>
>>> How can we check this in config/tls.m4? Can we just combine tests on
>>> TLS and emutls? E.g. check whether HAVE_TLS and USE_EMUTLS are both
>>> defined.
>>>
>>>> And if we don't, just make sure that on Android, Darwin and/or M$Win (or
>>>> whatever other OS you had in mind which does support pthreads, but doesn't
>>>> support native TLS) find out why HAVE_AS_TLS is not defined (guess
>>>> config.log should explain that).
>>>
>>> HAVE_AS_TLS is also not defined for Android as it depends on --enable-tls.

Re: Fix libgomp crash without TLS (PR42616)

[Jakub Jelinek]

On Mon, Dec 08, 2014 at 04:30:46PM +0300, Varvara Rainchik wrote:
> Hi guys,
> 
> Could you please take a look at this issue? This fix is still urgent
> for Android.

I'm afraid it will break those targets where emutls is not on, but the C
library doesn't support TLS.  I think it is acceptable not to care about
#pragma omp from different pthread_create created threads in that case, but
stopping support completely might be too early.
So, can you instead arrange for HAVE_TLS to be defined if emutls is
supported (check for that during configure)?

	Jakub

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

Is it ok to add GCC_CHECK_EMUTLS test in libgomp/configure.ac or
should I add in tls.m3 a similair test that would be used only in
libgomp?

2014-12-08 17:03 GMT+03:00 Jakub Jelinek <jakub@redhat.com>:
> On Mon, Dec 08, 2014 at 04:30:46PM +0300, Varvara Rainchik wrote:
>> Hi guys,
>>
>> Could you please take a look at this issue? This fix is still urgent
>> for Android.
>
> I'm afraid it will break those targets where emutls is not on, but the C
> library doesn't support TLS.  I think it is acceptable not to care about
> #pragma omp from different pthread_create created threads in that case, but
> stopping support completely might be too early.
> So, can you instead arrange for HAVE_TLS to be defined if emutls is
> supported (check for that during configure)?
>
>         Jakub

Re: Fix libgomp crash without TLS (PR42616)

[Jakub Jelinek]

On Mon, Dec 08, 2014 at 07:01:09PM +0300, Varvara Rainchik wrote:
> Is it ok to add GCC_CHECK_EMUTLS test in libgomp/configure.ac or
> should I add in tls.m3 a similair test that would be used only in
> libgomp?

I think config/tls.m4 would be a better place, but doing it in some way
where the users of config/tls.m4 could actually by using different macros
from there choose what they want (either check solely for real TLS, or
only for emutls, or for both).  And libgomp/configure.ac would then choose
it is happy with both.

	Jakub

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

Can we instead of adding new macroses in config/tls.m4 use something
like that in libgomp:

#if defined (HAVE_TLS) && defined (USE_EMUTLS)

(with GCC_CHECK_EMUTLS in libgomp/configure.ac)?

2014-12-08 19:28 GMT+03:00 Jakub Jelinek <jakub@redhat.com>:
> On Mon, Dec 08, 2014 at 07:01:09PM +0300, Varvara Rainchik wrote:
>> Is it ok to add GCC_CHECK_EMUTLS test in libgomp/configure.ac or
>> should I add in tls.m3 a similair test that would be used only in
>> libgomp?
>
> I think config/tls.m4 would be a better place, but doing it in some way
> where the users of config/tls.m4 could actually by using different macros
> from there choose what they want (either check solely for real TLS, or
> only for emutls, or for both).  And libgomp/configure.ac would then choose
> it is happy with both.
>
>         Jakub

Re: Fix libgomp crash without TLS (PR42616)

[Jakub Jelinek]

On Tue, Dec 09, 2014 at 02:49:44PM +0300, Varvara Rainchik wrote:
> Can we instead of adding new macroses in config/tls.m4 use something
> like that in libgomp:
> 
> #if defined (HAVE_TLS) && defined (USE_EMUTLS)
> 
> (with GCC_CHECK_EMUTLS in libgomp/configure.ac)?

That would be fine too.

	Jakub

Re: Fix libgomp crash without TLS (PR42616)

[Varvara Rainchik]

Ok, the following patch works for Android:

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

        * config.h.in: Regenerate.
        * configure: Regenerate.
        * configure.ac: Add GCC_CHECK_EMUTLS.
        * libgomp.h: Add check for USE_EMUTLS: this case
        is equal to HAVE_TLS.
        * team.c: Likewise.

--

diff --git a/libgomp/configure.ac b/libgomp/configure.ac
index cea6366..16ec158 100644
--- a/libgomp/configure.ac
+++ b/libgomp/configure.ac
@@ -245,6 +245,9 @@ fi
 # See if we support thread-local storage.
 GCC_CHECK_TLS

+# See if we have emulated thread-local storage.
+GCC_CHECK_EMUTLS
+
 # See what sort of export controls are available.
 LIBGOMP_CHECK_ATTRIBUTE_VISIBILITY
 LIBGOMP_CHECK_ATTRIBUTE_DLLEXPORT
diff --git a/libgomp/libgomp.h b/libgomp/libgomp.h
index a1482cc..b694356 100644
--- a/libgomp/libgomp.h
+++ b/libgomp/libgomp.h
@@ -471,7 +471,7 @@ enum gomp_cancel_kind

 /* ... and here is that TLS data.  */

-#ifdef HAVE_TLS
+#if defined HAVE_TLS || defined USE_EMUTLS
 extern __thread struct gomp_thread gomp_tls_data;
 static inline struct gomp_thread *gomp_thread (void)
 {
diff --git a/libgomp/team.c b/libgomp/team.c
index e6a6d8f..594127c 100644
--- a/libgomp/team.c
+++ b/libgomp/team.c
@@ -37,7 +37,7 @@ pthread_key_t gomp_thread_destructor;


 /* This is the libgomp per-thread data structure.  */
-#ifdef HAVE_TLS
+#if defined HAVE_TLS || defined USE_EMUTLS
 __thread struct gomp_thread gomp_tls_data;
 #else
 pthread_key_t gomp_tls_key;
@@ -70,7 +70,7 @@ gomp_thread_start (void *xdata)
   void (*local_fn) (void *);
   void *local_data;

-#ifdef HAVE_TLS
+#if defined HAVE_TLS || defined USE_EMUTLS
   thr = &gomp_tls_data;
 #else
   struct gomp_thread local_thr;
@@ -916,7 +916,7 @@ gomp_team_end (void)
 static void __attribute__((constructor))
 initialize_team (void)
 {
-#ifndef HAVE_TLS
+#if !defined HAVE_TLS && !defined USE_EMUTLS
   static struct gomp_thread initial_thread_tls_data;

   pthread_key_create (&gomp_tls_key, NULL);

Changes are bootstrapped and regtested on linux, all make check tests
now also pass with --disable-tls.
Is it ok for upstream?

2014-12-09 15:12 GMT+03:00 Jakub Jelinek <jakub@redhat.com>:
> On Tue, Dec 09, 2014 at 02:49:44PM +0300, Varvara Rainchik wrote:
>> Can we instead of adding new macroses in config/tls.m4 use something
>> like that in libgomp:
>>
>> #if defined (HAVE_TLS) && defined (USE_EMUTLS)
>>
>> (with GCC_CHECK_EMUTLS in libgomp/configure.ac)?
>
> That would be fine too.
>
>         Jakub

Re: Fix libgomp crash without TLS (PR42616)

[Richard Henderson]

On 12/09/2014 06:53 AM, Varvara Rainchik wrote:
> 2014-12-09  Varvara Rainchik  <varvara.rainchik@intel.com>
> 
>         * config.h.in: Regenerate.
>         * configure: Regenerate.
>         * configure.ac: Add GCC_CHECK_EMUTLS.
>         * libgomp.h: Add check for USE_EMUTLS: this case
>         is equal to HAVE_TLS.
>         * team.c: Likewise.

Ok.


r~