From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 933E13828939 for ; Thu, 15 Sep 2022 20:46:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 933E13828939 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1663274787; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=UaUGmcwFgWgA6QyhIELzn0rhIAW0+qI1ZA40bs763P8=; b=FFAjwUdh+skRRyBBuZTIWPWEwesS4YgDSZPOQ2kb89uDNEk9heheFY2VHvmuuXk4SdHQIc HehBG64O71dHLFQdzSJHj/qNyS2EyeDNtLPrEHbeNjv5ENgN825pS9iNPEEheG0pHGnXLK BD3tQcQPT6dtxSwLAM/IIdndl+IOs1w= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-635-g8eTbt1xPUyP3aq-skAeMg-1; Thu, 15 Sep 2022 16:46:25 -0400 X-MC-Unique: g8eTbt1xPUyP3aq-skAeMg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 12DE9882822; Thu, 15 Sep 2022 20:46:25 +0000 (UTC) Received: from localhost (unknown [10.33.36.214]) by smtp.corp.redhat.com (Postfix) with ESMTP id CDFFC10EB8; Thu, 15 Sep 2022 20:46:24 +0000 (UTC) From: Jonathan Wakely To: libstdc++@gcc.gnu.org Cc: gcc-patches@gcc.gnu.org Subject: [committed] libstdc++: Tweak TSan annotations for std::atomic> Date: Thu, 15 Sep 2022 21:46:23 +0100 Message-Id: <20220915204623.407931-1-jwakely@redhat.com> In-Reply-To: References: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Wed, 14 Sept 2022 at 23:28, Jonathan Wakely wrote: > > On Wed, 14 Sept 2022 at 23:25, Jonathan Wakely wrote: > > > > On Wed, 14 Sept 2022 at 23:05, Jonathan Wakely via Libstdc++ > > wrote: > > > @@ -377,6 +401,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > ~_Atomic_count() > > > { > > > auto __val = _M_val.load(memory_order_relaxed); > > > + _GLIBCXX_TSAN_MUTEX_DESTROY(&_M_val); > > > > After further thought, I'm not sure this is right. This tells tsan > > that the "mutex" at &_M_val cannot be locked or unlocked again after > > this. But what happens if the address is reused by a different > > atomic> which happens to be at the same memory address? > > Will tsan think that's an invalid use of the original "mutex" after > > its destruction? > > We can't easily add a call to __tsan_mutex_create, which would begin > the lifetime of a new object at that address, because the default > constructor is constexpr, and the create function isn't. > > > > > I will investigate. I investigated. There is a bug in my commit, but only that I pass __tsan_mutex_not_static to the unlock annotations, and it's only valid for the create and lock annotations. But it appears to simply be ignored by the unlock functions, so it's harmless. It seems that if __tsan_mutex_create has not been called, passing __tsan_mutex_not_static to the lock functions implicitly begins the lifetime of a lock. That means it's fine to "destroy" with an address that then gets reused later for a second object, because we'll implicitly create a new mutex (in tsan's mind) when we first lock it. But it also means that tsan doesn't complain about this: using A = std::atomic>; alignas(A) unsigned char buf[sizeof(A)]; A* a = new(buf) A(); a->load(); a->~A(); a->load(); The second load() uses a destroyed mutex, but tsan just beings the lifetime of a new one when we call __tsan_mutex_pre_lock(&_M_val, __tsan_mutex_not_static). I don't think we can do anything about that, but it's not tsan's job to detect use-after-free anyway. Here's a patch to fix the incorrect flags being passed to the pre/post unlock functions, and to make the lock annotations more fine-grained. Tested powerpc64le-linux, pushed to trunk. -- >8 -- Do not use the __tsan_mutex_not_static flag for annotation functions where it's not a valid flag. Also use the try_lock and try_lock_failed flags to more precisely annotate the CAS loop used to acquire a lock. libstdc++-v3/ChangeLog: * include/bits/shared_ptr_atomic.h (_GLIBCXX_TSAN_MUTEX_PRE_LOCK): Replace with ... (_GLIBCXX_TSAN_MUTEX_TRY_LOCK): ... this, add try_lock flag. (_GLIBCXX_TSAN_MUTEX_TRY_LOCK_FAILED): New macro using try_lock_failed flag (_GLIBCXX_TSAN_MUTEX_POST_LOCK): Rename to ... (_GLIBCXX_TSAN_MUTEX_LOCKED): ... this. (_GLIBCXX_TSAN_MUTEX_PRE_UNLOCK): Remove invalid flag. (_GLIBCXX_TSAN_MUTEX_POST_UNLOCK): Remove invalid flag. (_Sp_atomic::_Atomic_count::lock): Use new macros. --- libstdc++-v3/include/bits/shared_ptr_atomic.h | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/libstdc++-v3/include/bits/shared_ptr_atomic.h b/libstdc++-v3/include/bits/shared_ptr_atomic.h index 4580807f42c..55d193d4bda 100644 --- a/libstdc++-v3/include/bits/shared_ptr_atomic.h +++ b/libstdc++-v3/include/bits/shared_ptr_atomic.h @@ -32,24 +32,26 @@ #include +// Annotations for the custom locking in atomic>. #if defined _GLIBCXX_TSAN && __has_include() #include #define _GLIBCXX_TSAN_MUTEX_DESTROY(X) \ __tsan_mutex_destroy(X, __tsan_mutex_not_static) -#define _GLIBCXX_TSAN_MUTEX_PRE_LOCK(X) \ - __tsan_mutex_pre_lock(X, __tsan_mutex_not_static) -#define _GLIBCXX_TSAN_MUTEX_POST_LOCK(X) \ +#define _GLIBCXX_TSAN_MUTEX_TRY_LOCK(X) \ + __tsan_mutex_pre_lock(X, __tsan_mutex_not_static|__tsan_mutex_try_lock) +#define _GLIBCXX_TSAN_MUTEX_TRY_LOCK_FAILED(X) __tsan_mutex_post_lock(X, \ + __tsan_mutex_not_static|__tsan_mutex_try_lock_failed, 0) +#define _GLIBCXX_TSAN_MUTEX_LOCKED(X) \ __tsan_mutex_post_lock(X, __tsan_mutex_not_static, 0) -#define _GLIBCXX_TSAN_MUTEX_PRE_UNLOCK(X) \ - __tsan_mutex_pre_unlock(X, __tsan_mutex_not_static) -#define _GLIBCXX_TSAN_MUTEX_POST_UNLOCK(X) \ - __tsan_mutex_post_unlock(X, __tsan_mutex_not_static) +#define _GLIBCXX_TSAN_MUTEX_PRE_UNLOCK(X) __tsan_mutex_pre_unlock(X, 0) +#define _GLIBCXX_TSAN_MUTEX_POST_UNLOCK(X) __tsan_mutex_post_unlock(X, 0) #define _GLIBCXX_TSAN_MUTEX_PRE_SIGNAL(X) __tsan_mutex_pre_signal(X, 0) #define _GLIBCXX_TSAN_MUTEX_POST_SIGNAL(X) __tsan_mutex_post_signal(X, 0) #else #define _GLIBCXX_TSAN_MUTEX_DESTROY(X) -#define _GLIBCXX_TSAN_MUTEX_PRE_LOCK(X) -#define _GLIBCXX_TSAN_MUTEX_POST_LOCK(X) +#define _GLIBCXX_TSAN_MUTEX_TRY_LOCK(X) +#define _GLIBCXX_TSAN_MUTEX_TRY_LOCK_FAILED(X) +#define _GLIBCXX_TSAN_MUTEX_LOCKED(X) #define _GLIBCXX_TSAN_MUTEX_PRE_UNLOCK(X) #define _GLIBCXX_TSAN_MUTEX_POST_UNLOCK(X) #define _GLIBCXX_TSAN_MUTEX_PRE_SIGNAL(X) @@ -431,19 +433,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __current = _M_val.load(memory_order_relaxed); } - _GLIBCXX_TSAN_MUTEX_PRE_LOCK(&_M_val); + _GLIBCXX_TSAN_MUTEX_TRY_LOCK(&_M_val); while (!_M_val.compare_exchange_strong(__current, __current | _S_lock_bit, __o, memory_order_relaxed)) { + _GLIBCXX_TSAN_MUTEX_TRY_LOCK_FAILED(&_M_val); #if __cpp_lib_atomic_wait __detail::__thread_relax(); #endif __current = __current & ~_S_lock_bit; + _GLIBCXX_TSAN_MUTEX_TRY_LOCK(&_M_val); } - _GLIBCXX_TSAN_MUTEX_POST_LOCK(&_M_val); + _GLIBCXX_TSAN_MUTEX_LOCKED(&_M_val); return reinterpret_cast(__current); } -- 2.37.3