| Message ID | CAAp9h921vFhhu+-6-SsRnN9kqP1SGfO+enciwXw4FoP26UfmcA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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
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
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
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."); }
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. -- 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.