Discussion:
Fix libgomp crash without TLS (PR42616)
Varvara Rainchik
2014-08-06 10:05:25 UTC
Permalink
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 <***@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
Varvara Rainchik
2014-08-13 08:13:50 UTC
Permalink
*Ping*

Thanks,
Varvara
Post by 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
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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42616;
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.
* 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
Varvara Rainchik
2014-08-29 11:21:26 UTC
Permalink
Hi again! I want to remind that issue is urgent for Android.
Post by Varvara Rainchik
*Ping*
Thanks,
Varvara
Post by 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
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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42616;
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.
* 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
Richard Henderson
2014-08-29 17:40:57 UTC
Permalink
Post by Varvara Rainchik
* 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~
Varvara Rainchik
2014-09-01 10:38:18 UTC
Permalink
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 ()
Post by Richard Henderson
Post by Varvara Rainchik
* 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~
Jakub Jelinek
2014-09-01 10:51:36 UTC
Permalink
Post by Richard Henderson
Post by Varvara Rainchik
* 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.
Post by Richard Henderson
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
Varvara Rainchik
2014-09-02 10:36:27 UTC
Permalink
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)?
Post by Jakub Jelinek
Post by Richard Henderson
Post by Varvara Rainchik
* 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.
Post by Richard Henderson
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
Varvara Rainchik
2014-09-19 11:41:58 UTC
Permalink
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 <***@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)
{
Post by 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)?
Post by Jakub Jelinek
Post by Richard Henderson
Post by Varvara Rainchik
* 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.
Post by Richard Henderson
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
Varvara Rainchik
2014-09-24 10:19:14 UTC
Permalink
*Ping*
Post by 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
/* 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.
* 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)
{
Post by 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)?
Post by Jakub Jelinek
Post by Richard Henderson
Post by Varvara Rainchik
* 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.
Post by Richard Henderson
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
Varvara Rainchik
2014-09-30 07:03:47 UTC
Permalink
Corrected patch: call pthread_setspecific (gomp_tls_key, NULL) in
gomp_thread_start if HAVE_TLS is not defined.

2014-09-19 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)
{
Post by Varvara Rainchik
*Ping*
Post by 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
/* 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.
* 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)
{
Post by 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)?
Post by Jakub Jelinek
Post by Richard Henderson
Post by Varvara Rainchik
* 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.
Post by Richard Henderson
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
Jakub Jelinek
2014-09-30 09:52:19 UTC
Permalink
Post by Varvara Rainchik
Corrected patch: call pthread_setspecific (gomp_tls_key, NULL) in
gomp_thread_start if HAVE_TLS is not defined.
* 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
2014-09-30 14:40:52 UTC
Permalink
Post by Jakub Jelinek
Post by Varvara Rainchik
Corrected patch: call pthread_setspecific (gomp_tls_key, NULL) in
gomp_thread_start if HAVE_TLS is not defined.
* 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~
Varvara Rainchik
2014-10-01 16:44:59 UTC
Permalink
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 <***@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
{
Post by Richard Henderson
Post by Jakub Jelinek
Post by Varvara Rainchik
Corrected patch: call pthread_setspecific (gomp_tls_key, NULL) in
gomp_thread_start if HAVE_TLS is not defined.
* 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~
Jakub Jelinek
2014-10-07 07:11:25 UTC
Permalink
Post by 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?
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).
Post by Varvara Rainchik
* libgomp.h (HAVE_TLS): Set to 1.
Jakub

Loading...