From patchwork Fri Sep 19 11:41:58 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Varvara Rainchik X-Patchwork-Id: 391222 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id CA8B71401AF for ; Fri, 19 Sep 2014 21:42:11 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=XnYoH46uyGACF3ucgC XMRoTKSv7AFF2HCpoT53++hk33Iuco3fYlqIsuN03X/l3kE4LXTYwVBmc2dPAled 2eYB+0AwAcN8NeR/DIRyU7eKORtrK6rrLR2XOWACW2Htw0MZfhx76svE4QThBkcq nhJZkyJc+/NBiIeHgYtmDL2Qs= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=Fs+4y7pk6kQCuE+KuQatCqUF i/M=; b=lXRAcnWbvITXE0Z2pXeCDjXOCjiK1k6eSFEGtHT/3zJVP4OMkcW4Bw25 A27uZKrcRwG/zJ085vUtNX6lGXIA48CzALanowmyjAoyxFIjJEM0MflpzgVYtsGF vx3W2S6sQfnULlh4hqJidyi0mwUfWLiGzN48ls/vwr5RWHeNkyM= Received: (qmail 13642 invoked by alias); 19 Sep 2014 11:42:04 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 13630 invoked by uid 89); 19 Sep 2014 11:42:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wi0-f176.google.com Received: from mail-wi0-f176.google.com (HELO mail-wi0-f176.google.com) (209.85.212.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 19 Sep 2014 11:42:02 +0000 Received: by mail-wi0-f176.google.com with SMTP id ex7so4731222wid.3 for ; Fri, 19 Sep 2014 04:41:59 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.194.103.41 with SMTP id ft9mr442611wjb.93.1411126919076; Fri, 19 Sep 2014 04:41:59 -0700 (PDT) Received: by 10.194.36.4 with HTTP; Fri, 19 Sep 2014 04:41:58 -0700 (PDT) In-Reply-To: References: <5400BB29.2010205@redhat.com> <20140901105136.GM17454@tucnak.redhat.com> Date: Fri, 19 Sep 2014 15:41:58 +0400 Message-ID: Subject: Re: Fix libgomp crash without TLS (PR42616) From: Varvara Rainchik To: Jakub Jelinek Cc: Richard Henderson , gcc-patches@gcc.gnu.org X-IsSubscribed: yes 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 * libgomp.h (gomp_thread): For non TLS case create thread data. * team.c (non_tls_thread_data_destructor, create_non_tls_thread_data): New functions. --- 2014-09-02 14:36 GMT+04:00 Varvara Rainchik : > May I use gomp_free_thread as a destructor for pthread_key_create? > Then I'll make initial_thread_tls_data global for the first case, but > how can I differentiate thread created by gomp_thread_start (second > case)? > > 2014-09-01 14:51 GMT+04:00 Jakub Jelinek : >> 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 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) {