From patchwork Thu Jul 9 18:07:24 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Siddhesh Poyarekar X-Patchwork-Id: 493513 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 837D514076A for ; Fri, 10 Jul 2015 04:07:38 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.b=gz8Q38w9; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id :mime-version:content-type; q=dns; s=default; b=v57OHlxM7MKFu+Qv ef4K32aCpwRNRFWUKAaj/wjfX1ruh5O/1x+nMk56Ktc2aI34icWtpyyHe22J6uBX lW0tJGMfF6jssuhtwmQYZ7nkHihy5TjQwten7+TNNGrWoIf7r6iatytk5dSR9DoY TdLi/abn83/KmZpOHFNPdJiZIhY= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id :mime-version:content-type; s=default; bh=0koqOTN+5Dcp/NS+5zf5/m i8AGM=; b=gz8Q38w9wwAvvyZ4JjxRiE+cx5YUqya4k+zRp5F2z0a72G+9r1fxaw KFp1IouotxiUUT1EjEjxgVgrZBPedq4I+sMfeFeeuL3nnogtHj4yhC7uYEWx9AbC 2EZmSvye0bXJ5+CI9ANyzjwA3pGZOwjyfsjD+UwVEN7DYgm20L2ZY= Received: (qmail 18222 invoked by alias); 9 Jul 2015 18:07:33 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 18213 invoked by uid 89); 9 Jul 2015 18:07:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.3 required=5.0 tests=AWL, BAYES_20, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Date: Thu, 9 Jul 2015 23:37:24 +0530 From: Siddhesh Poyarekar To: libc-alpha@sourceware.org Cc: triegel@redhat.com, carlos@redhat.com Subject: [PATCH] Also use l_tls_dtor_count to decide on object unload Message-ID: <20150709180724.GA8552@spoyarek.pnq.redhat.com> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.23 (2014-03-12) When an TLS destructor is registered, we set the DF_1_NODELETE flag to signal that the object should not be destroyed. We then clear the DF_1_NODELETE flag when all destructors are called, which is wrong - the flag could have been set by other means too. This patch replaces this use of the flag by using l_tls_dtor_count directly to determine whether it is safe to unload the object. This change has the added advantage of eliminating the lock taking when calling the destructors, which could result in a deadlock. The patch also fixes the test case tst-tls-atexit - it was making an invalid dlclose call, which would just return an error silently. Change verified on x86_64; the test suite does not show any regressions due to the patch. ChangeLog: * elf/dl-close.c (_dl_close_worker): Don't unload DSO if there are pending TLS destructor calls. * stdlib/cxa_thread_atexit_impl.c (__cxa_thread_atexit_impl): Don't touch the link map flag. Atomically increment l_tls_dtor_count. (__call_tls_dtors): Atomically decrement l_tls_dtor_count. Avoid taking the load lock and don't touch the link map flag. * stdlib/tst-tls-atexit.c (do_test): dlopen tst-tls-atexit-lib.so again before dlclose. --- elf/dl-close.c | 9 ++++++++- stdlib/cxa_thread_atexit_impl.c | 25 +++++++------------------ stdlib/tst-tls-atexit.c | 9 ++++++++- 3 files changed, 23 insertions(+), 20 deletions(-) diff --git a/elf/dl-close.c b/elf/dl-close.c index 2104674..30e30e2 100644 --- a/elf/dl-close.c +++ b/elf/dl-close.c @@ -153,7 +153,11 @@ _dl_close_worker (struct link_map *map, bool force) maps[idx] = l; ++idx; - /* Clear DF_1_NODELETE to force object deletion. */ + /* Clear DF_1_NODELETE to force object deletion. We don't need to touch + l_tls_dtor_count because forced object deletion only happens when an + error occurs during object load. Destructor registration for TLS + non-POD objects should not have happened till then for this + object. */ if (force) l->l_flags_1 &= ~DF_1_NODELETE; } @@ -173,10 +177,13 @@ _dl_close_worker (struct link_map *map, bool force) /* Already handled. */ continue; + size_t tls_dtor_count = atomic_load_relaxed (&l->l_tls_dtor_count); + /* Check whether this object is still used. */ if (l->l_type == lt_loaded && l->l_direct_opencount == 0 && (l->l_flags_1 & DF_1_NODELETE) == 0 + && tls_dtor_count == 0 && !used[done_index]) continue; diff --git a/stdlib/cxa_thread_atexit_impl.c b/stdlib/cxa_thread_atexit_impl.c index 9120162..fac2cc9 100644 --- a/stdlib/cxa_thread_atexit_impl.c +++ b/stdlib/cxa_thread_atexit_impl.c @@ -50,27 +50,25 @@ __cxa_thread_atexit_impl (dtor_func func, void *obj, void *dso_symbol) tls_dtor_list = new; /* See if we already encountered the DSO. */ - __rtld_lock_lock_recursive (GL(dl_load_lock)); - if (__glibc_unlikely (dso_symbol_cache != dso_symbol)) { ElfW(Addr) caller = (ElfW(Addr)) dso_symbol; + /* _dl_find_dso_for_object assumes that we have the dl_load_lock. */ + __rtld_lock_lock_recursive (GL(dl_load_lock)); struct link_map *l = _dl_find_dso_for_object (caller); + __rtld_lock_unlock_recursive (GL(dl_load_lock)); /* If the address is not recognized the call comes from the main program (we hope). */ lm_cache = l ? l : GL(dl_ns)[LM_ID_BASE]._ns_loaded; } + /* A destructor could result in a thread_local construction and the former could have cleared the flag. */ - if (lm_cache->l_type == lt_loaded && lm_cache->l_tls_dtor_count == 0) - lm_cache->l_flags_1 |= DF_1_NODELETE; + atomic_increment (&lm_cache->l_tls_dtor_count); new->map = lm_cache; - new->map->l_tls_dtor_count++; - - __rtld_lock_unlock_recursive (GL(dl_load_lock)); return 0; } @@ -83,19 +81,10 @@ __call_tls_dtors (void) while (tls_dtor_list) { struct dtor_list *cur = tls_dtor_list; - tls_dtor_list = tls_dtor_list->next; + tls_dtor_list = tls_dtor_list->next; cur->func (cur->obj); - - __rtld_lock_lock_recursive (GL(dl_load_lock)); - - /* Allow DSO unload if count drops to zero. */ - cur->map->l_tls_dtor_count--; - if (cur->map->l_tls_dtor_count == 0 && cur->map->l_type == lt_loaded) - cur->map->l_flags_1 &= ~DF_1_NODELETE; - - __rtld_lock_unlock_recursive (GL(dl_load_lock)); - + atomic_decrement (&cur->map->l_tls_dtor_count); free (cur); } } diff --git a/stdlib/tst-tls-atexit.c b/stdlib/tst-tls-atexit.c index 68247d1..ba70790 100644 --- a/stdlib/tst-tls-atexit.c +++ b/stdlib/tst-tls-atexit.c @@ -82,7 +82,14 @@ do_test (void) if (thr_ret != NULL) return 1; - /* Now this should unload the DSO. */ + /* Now this sequence should unload the DSO. */ + handle = dlopen ("$ORIGIN/tst-tls-atexit-lib.so", RTLD_LAZY); + if (!handle) + { + printf ("main thread: Unable to load DSO: %s\n", dlerror ()); + return 1; + } + dlclose (handle); /* Run through our maps and ensure that the DSO is unloaded. */