From patchwork Thu Sep 15 20:46:23 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 1242 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5044:0:0:0:0:0 with SMTP id h4csp451383wrt; Thu, 15 Sep 2022 13:47:22 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7fKNRTMqeefvUUep70DJRClrRIb73cHdZPMEWwX+7Wu8faJ6Uw+YmxxmxBMegYe0ZNRyOi X-Received: by 2002:a17:907:2cf0:b0:77b:2ad7:121b with SMTP id hz16-20020a1709072cf000b0077b2ad7121bmr1099650ejc.577.1663274842517; Thu, 15 Sep 2022 13:47:22 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1663274842; cv=none; d=google.com; s=arc-20160816; b=ZYznN0jNWqvALeF7K1mnW9sRl1tP6yk1ZH3P0AYFtVJgKyoKh9Q4dwekWsmcRYJppM +Hbh8uzB0NGC8ZJbY9rvZZOzFUC8z0c3CfQMLvOkWosxJwKXRf8GgdSDw20rigYO+vfJ RYfcj1o9vGrwh16iRXVdN4cwogAzfqc21hScBXnGUv8eN2zcbeVNTfKou44uGmsiKtIw VqnBhVX52K4Zxb9cJ2NN+uJ/N4queWCtFlYHAVv5d13i8gsLBslOxPkdDtuENPX6L9/w Oz2gpdZ1NmgZyaumvIT5tSW6wR3c1xRSwOrYp5/KYfA8pDY+S3s+LM+18ZzzEVP8wIV1 W+jQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:cc:reply-to:from:list-subscribe:list-help :list-post:list-archive:list-unsubscribe:list-id:precedence :content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:dmarc-filter:delivered-to:dkim-signature :dkim-filter; bh=UaUGmcwFgWgA6QyhIELzn0rhIAW0+qI1ZA40bs763P8=; b=AkS0cciH41f4RV+QupEeD/6RpiaSCVIPXML3utee1T/krcBG4CdtNqGExPexPahtUW nCxAvjPFfl8aUesYksNMhUKzPh6Jm0M0waV5mfMq3tuuT9fvVPxIiIBznjtoW0BOaPet rrqU0sXZw9smJOFav6YF/RHRfvSGRfj9oyga385xkYxEVXr2iwzWeNKiK4/MThyi1Wwv eLXQPdh7M63KCmfC0IE4ZmPHc/hAc+GMNvzgU39MaxTtkgw1UbxEmLts4F/IwWDDK6HN d6ERi2oH+Av1Gzh+fUL1XBWMYtf+cosgn8PBdVI2xcNHhqcgodrT7GKUntio4KNQOupy ABYw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=gjCDliLx; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from sourceware.org (server2.sourceware.org. [8.43.85.97]) by mx.google.com with ESMTPS id y9-20020a056402270900b00445f3ed936csi350584edd.595.2022.09.15.13.47.22 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 15 Sep 2022 13:47:22 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) client-ip=8.43.85.97; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=gjCDliLx; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D4180381FE5E for ; Thu, 15 Sep 2022 20:47:14 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D4180381FE5E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1663274834; bh=UaUGmcwFgWgA6QyhIELzn0rhIAW0+qI1ZA40bs763P8=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=gjCDliLxRv58EqpoLHo0VkStF5O9oG/jfTQvooBw0S4571kE1C7iVHfO2y5MHAXpL qDXHctCJqV1N5KmUAVkjOkd5YiSf5f+av9MZE1/s0uAid1iDqTTJ7A2+KezZUNbZ+Q m0xGISbf2IT+mjH3AA6jeG3OlFxA+PafvUD3mbd0= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org 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 350583821FE7 for ; Thu, 15 Sep 2022 20:46:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 350583821FE7 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) To: libstdc++@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 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=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jonathan Wakely via Gcc-patches From: Jonathan Wakely Reply-To: Jonathan Wakely Cc: gcc-patches@gcc.gnu.org Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org Sender: "Gcc-patches" X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1744070080870289943?= X-GMAIL-MSGID: =?utf-8?q?1744070080870289943?= 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); }