* Fix libgomp crash without TLS (PR42616) @ 2014-08-06 10:05 Varvara Rainchik 2014-08-13 8:14 ` Varvara Rainchik 2014-08-29 17:41 ` Richard Henderson 0 siblings, 2 replies; 25+ messages in thread From: Varvara Rainchik @ 2014-08-06 10:05 UTC (permalink / raw) To: gcc-patches, jakub 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-08-06 10:05 Fix libgomp crash without TLS (PR42616) Varvara Rainchik @ 2014-08-13 8:14 ` Varvara Rainchik 2014-08-29 11:21 ` Varvara Rainchik 2014-08-29 17:41 ` Richard Henderson 1 sibling, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-08-13 8:14 UTC (permalink / raw) To: gcc-patches, jakub *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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-08-13 8:14 ` Varvara Rainchik @ 2014-08-29 11:21 ` Varvara Rainchik 0 siblings, 0 replies; 25+ messages in thread From: Varvara Rainchik @ 2014-08-29 11:21 UTC (permalink / raw) To: gcc-patches, jakub 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-08-06 10:05 Fix libgomp crash without TLS (PR42616) Varvara Rainchik 2014-08-13 8:14 ` Varvara Rainchik @ 2014-08-29 17:41 ` Richard Henderson 2014-09-01 10:38 ` Varvara Rainchik 2014-09-01 10:51 ` Jakub Jelinek 1 sibling, 2 replies; 25+ messages in thread From: Richard Henderson @ 2014-08-29 17:41 UTC (permalink / raw) To: Varvara Rainchik, gcc-patches, jakub 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~ ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-08-29 17:41 ` Richard Henderson @ 2014-09-01 10:38 ` Varvara Rainchik 2014-09-01 10:51 ` Jakub Jelinek 1 sibling, 0 replies; 25+ messages in thread From: Varvara Rainchik @ 2014-09-01 10:38 UTC (permalink / raw) To: Richard Henderson; +Cc: gcc-patches, jakub 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~ > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-08-29 17:41 ` Richard Henderson 2014-09-01 10:38 ` Varvara Rainchik @ 2014-09-01 10:51 ` Jakub Jelinek 2014-09-02 10:37 ` Varvara Rainchik 1 sibling, 1 reply; 25+ messages in thread From: Jakub Jelinek @ 2014-09-01 10:51 UTC (permalink / raw) To: Richard Henderson; +Cc: Varvara Rainchik, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-09-01 10:51 ` Jakub Jelinek @ 2014-09-02 10:37 ` Varvara Rainchik 2014-09-19 11:42 ` Varvara Rainchik 0 siblings, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-09-02 10:37 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-09-02 10:37 ` Varvara Rainchik @ 2014-09-19 11:42 ` Varvara Rainchik 2014-09-24 10:19 ` Varvara Rainchik 0 siblings, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-09-19 11:42 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-09-19 11:42 ` Varvara Rainchik @ 2014-09-24 10:19 ` Varvara Rainchik 2014-09-30 7:03 ` Varvara Rainchik 0 siblings, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-09-24 10:19 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches *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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-09-24 10:19 ` Varvara Rainchik @ 2014-09-30 7:03 ` Varvara Rainchik 2014-09-30 9:52 ` Jakub Jelinek 0 siblings, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-09-30 7:03 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-09-30 7:03 ` Varvara Rainchik @ 2014-09-30 9:52 ` Jakub Jelinek 2014-09-30 14:40 ` Richard Henderson 0 siblings, 1 reply; 25+ messages in thread From: Jakub Jelinek @ 2014-09-30 9:52 UTC (permalink / raw) To: Varvara Rainchik; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-09-30 9:52 ` Jakub Jelinek @ 2014-09-30 14:40 ` Richard Henderson 2014-10-01 16:45 ` Varvara Rainchik 0 siblings, 1 reply; 25+ messages in thread From: Richard Henderson @ 2014-09-30 14:40 UTC (permalink / raw) To: Jakub Jelinek, Varvara Rainchik; +Cc: gcc-patches 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~ ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-09-30 14:40 ` Richard Henderson @ 2014-10-01 16:45 ` Varvara Rainchik 2014-10-07 7:11 ` Jakub Jelinek 0 siblings, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-10-01 16:45 UTC (permalink / raw) To: Richard Henderson; +Cc: Jakub Jelinek, gcc-patches 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~ > ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-10-01 16:45 ` Varvara Rainchik @ 2014-10-07 7:11 ` Jakub Jelinek 2014-10-13 10:49 ` Varvara Rainchik 0 siblings, 1 reply; 25+ messages in thread From: Jakub Jelinek @ 2014-10-07 7:11 UTC (permalink / raw) To: Varvara Rainchik; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-10-07 7:11 ` Jakub Jelinek @ 2014-10-13 10:49 ` Varvara Rainchik 2014-11-10 13:32 ` Varvara Rainchik 0 siblings, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-10-13 10:49 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches > 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-10-13 10:49 ` Varvara Rainchik @ 2014-11-10 13:32 ` Varvara Rainchik 2014-12-01 15:25 ` Varvara Rainchik 0 siblings, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-11-10 13:32 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches *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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-11-10 13:32 ` Varvara Rainchik @ 2014-12-01 15:25 ` Varvara Rainchik 2014-12-08 13:30 ` Varvara Rainchik 0 siblings, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-12-01 15:25 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-12-01 15:25 ` Varvara Rainchik @ 2014-12-08 13:30 ` Varvara Rainchik 2014-12-08 14:03 ` Jakub Jelinek 0 siblings, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-12-08 13:30 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-12-08 13:30 ` Varvara Rainchik @ 2014-12-08 14:03 ` Jakub Jelinek 2014-12-08 16:01 ` Varvara Rainchik 0 siblings, 1 reply; 25+ messages in thread From: Jakub Jelinek @ 2014-12-08 14:03 UTC (permalink / raw) To: Varvara Rainchik; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-12-08 14:03 ` Jakub Jelinek @ 2014-12-08 16:01 ` Varvara Rainchik 2014-12-08 16:28 ` Jakub Jelinek 0 siblings, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-12-08 16:01 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-12-08 16:01 ` Varvara Rainchik @ 2014-12-08 16:28 ` Jakub Jelinek 2014-12-09 11:49 ` Varvara Rainchik 0 siblings, 1 reply; 25+ messages in thread From: Jakub Jelinek @ 2014-12-08 16:28 UTC (permalink / raw) To: Varvara Rainchik; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-12-08 16:28 ` Jakub Jelinek @ 2014-12-09 11:49 ` Varvara Rainchik 2014-12-09 12:12 ` Jakub Jelinek 0 siblings, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-12-09 11:49 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-12-09 11:49 ` Varvara Rainchik @ 2014-12-09 12:12 ` Jakub Jelinek 2014-12-09 14:53 ` Varvara Rainchik 0 siblings, 1 reply; 25+ messages in thread From: Jakub Jelinek @ 2014-12-09 12:12 UTC (permalink / raw) To: Varvara Rainchik; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-12-09 12:12 ` Jakub Jelinek @ 2014-12-09 14:53 ` Varvara Rainchik 2014-12-09 16:37 ` Richard Henderson 0 siblings, 1 reply; 25+ messages in thread From: Varvara Rainchik @ 2014-12-09 14:53 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: Fix libgomp crash without TLS (PR42616) 2014-12-09 14:53 ` Varvara Rainchik @ 2014-12-09 16:37 ` Richard Henderson 0 siblings, 0 replies; 25+ messages in thread From: Richard Henderson @ 2014-12-09 16:37 UTC (permalink / raw) To: Varvara Rainchik, Jakub Jelinek; +Cc: gcc-patches 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~ ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2014-12-09 16:37 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-06 10:05 Fix libgomp crash without TLS (PR42616) Varvara Rainchik 2014-08-13 8:14 ` Varvara Rainchik 2014-08-29 11:21 ` Varvara Rainchik 2014-08-29 17:41 ` Richard Henderson 2014-09-01 10:38 ` Varvara Rainchik 2014-09-01 10:51 ` Jakub Jelinek 2014-09-02 10:37 ` Varvara Rainchik 2014-09-19 11:42 ` Varvara Rainchik 2014-09-24 10:19 ` Varvara Rainchik 2014-09-30 7:03 ` Varvara Rainchik 2014-09-30 9:52 ` Jakub Jelinek 2014-09-30 14:40 ` Richard Henderson 2014-10-01 16:45 ` Varvara Rainchik 2014-10-07 7:11 ` Jakub Jelinek 2014-10-13 10:49 ` Varvara Rainchik 2014-11-10 13:32 ` Varvara Rainchik 2014-12-01 15:25 ` Varvara Rainchik 2014-12-08 13:30 ` Varvara Rainchik 2014-12-08 14:03 ` Jakub Jelinek 2014-12-08 16:01 ` Varvara Rainchik 2014-12-08 16:28 ` Jakub Jelinek 2014-12-09 11:49 ` Varvara Rainchik 2014-12-09 12:12 ` Jakub Jelinek 2014-12-09 14:53 ` Varvara Rainchik 2014-12-09 16:37 ` Richard Henderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).