From patchwork Fri Sep 1 15:02:19 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jonathan Wakely X-Patchwork-Id: 137400 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:c792:0:b0:3f2:4152:657d with SMTP id b18csp947801vqu; Fri, 1 Sep 2023 08:04:36 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH+xw7S8i08EFGGXBGJrYu3YasX7NrozDK4KuRkkBwMhZXEQrHwhSWU5wqMCymWU2fkcEi7 X-Received: by 2002:a17:906:2252:b0:9a1:bd33:4389 with SMTP id 18-20020a170906225200b009a1bd334389mr2238460ejr.74.1693580676627; Fri, 01 Sep 2023 08:04:36 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1693580676; cv=none; d=google.com; s=arc-20160816; b=DwFPvjQmTuyUsQAdx2Ebuvu9fJ5M27TvSMcV7PGJG8yiTR0fXrYCHVCcQVy+rBNK2F XZKncwis2CHjU7IO1ziuYmh3V6enQ/cvkg2vQCW8EMd7Xj9+jEDCI5yBIEkhXrTPgCm1 lNynOf86HQ2GSv9kh2LNvqy8ZMkLO66rj7nh58kw0n0EjE03c635WxspuR0hCSPncO+j JLGs/Pk69gIe+7J6ujBDi5YXYeERSaNgOIp9aluW9LTVSlP88PRnnbxXEqEHIaVQ6BWq tjYOWicOOuCQZJB8w7z8xIzZTdjcxFdBFbMNxj/QdZt1FBBW4NIccwQVe3paezftNntu NhNQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:reply-to:from:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence :content-transfer-encoding:mime-version:message-id:date:subject:to :dmarc-filter:delivered-to:dkim-signature:dkim-filter; bh=GY3rS6bJZVZ9u7cP9Js0Sqh8rImqzuGrStAZpLfIkvA=; fh=sJ+2/4g29YdyXkoRrFZSpsL2zxijepB7X/1rB0LDDh8=; b=Eh3INMhljrobgsQfHfsZl8kMWA17Q7LWiTtfK3IsXXvyDWq+Mkr+b32wEow/QEddd1 eHNNPEiy105K6zt14utEtPoGTnF9FWCiVFFyeolpIBcKVjxAgNyhBqbJI+oHJNMnj8xg 0+GYJAsmyAsdE9wJtDCxe3xA3MYWRHWpfdYGVlJ/iSftap8P1Xu/p+TtkJ9U0aWzmz9B BN9Th1xsHkZ2b3f5ljU4ZWvBqu8WSpsZhMw//6tTTSRNMCQjwATTc9U2SO7AErE2inZK 4xLrI8rpSffh+ppwKQx8z6xbNiI2M3nPpOWyjfeUlNRmi9NALf+ktHYOYBFGJbxuZWWd 7MOw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=E99P5WEd; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c 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 (server2.sourceware.org. [2620:52:3:1:0:246e:9693:128c]) by mx.google.com with ESMTPS id gw18-20020a170906f15200b0099a1fb56239si2807293ejb.476.2023.09.01.08.04.36 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 01 Sep 2023 08:04:36 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) client-ip=2620:52:3:1:0:246e:9693:128c; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=E99P5WEd; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c 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 3DBB43853837 for ; Fri, 1 Sep 2023 15:03:44 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3DBB43853837 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1693580624; bh=GY3rS6bJZVZ9u7cP9Js0Sqh8rImqzuGrStAZpLfIkvA=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=E99P5WEd++4zXG1FixYcNCaO8RsZItQH5b6kLfh+aX7g16FvYjSLpME0OPcr38IdE tFbSc/t9czSKe65H5dcOKIB1/V+aNti68FiLcDqAPygifgFb5ftXzkj33SSZGLkNSB MiStUJME3Fl+8zmDSNLmnc1ulUP8PO9GtymOS+/I= 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 CB7AD38555B4 for ; Fri, 1 Sep 2023 15:02:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CB7AD38555B4 Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-5-t2KLOV3oNR6pIDysWybA9w-1; Fri, 01 Sep 2023 11:02:46 -0400 X-MC-Unique: t2KLOV3oNR6pIDysWybA9w-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B75EE29AB414; Fri, 1 Sep 2023 15:02:45 +0000 (UTC) Received: from localhost (unknown [10.42.28.181]) by smtp.corp.redhat.com (Postfix) with ESMTP id 60E711402C0A; Fri, 1 Sep 2023 15:02:45 +0000 (UTC) To: libstdc++@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: [committed] libstdc++: Use a loop in atomic_ref::compare_exchange_strong [PR111077] Date: Fri, 1 Sep 2023 16:02:19 +0100 Message-ID: <20230901150244.253651-1-jwakely@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-40.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, 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 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.30 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 Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org Sender: "Gcc-patches" X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1775848051885041186 X-GMAIL-MSGID: 1775848051885041186 Tested x86_64-linux. Pushed to trunk. Backport to gcc-13 needed too. -- >8 -- We need to use a loop in std::atomic_ref::compare_exchange_strong in order to properly implement the C++20 requirement that padding bits do not participate when checking the value for equality. The variable being modified by a std::atomic_ref might have an initial value with non-zero padding bits, so when the __atomic_compare_exchange built-in returns false we need to check whether that was only because of non-equal padding bits that are not part of the value representation. If the value bits differ, it's just a failed compare-exchange. If the value bits are the same, we need to retry the __atomic_compare_exchange using the value that was just read by the previous failed call. As noted in the comments, it's possible for that second try to also fail due to another thread storing the same value but with differences in padding. Because it's undefined to access a variable directly while it's held by a std::atomic_ref, and because std::atomic_ref will only ever store values with zeroed padding, we know that padding bits will never go from zero to non-zero during the lifetime of a std::atomic_ref. They can only go from an initial non-zero state to zero. This means the loop will terminate, rather than looping indefinitely as padding bits flicker on and off. In theory users could call __atomic_store etc. directly and write a value with non-zero padding bits, but we don't need to support that. Users doing that should ensure they do not write non-zero padding, to be compatibile with our std::atomic_ref's invariants. This isn't a problem for std::atomic::compare_exchange_strong because the initial value (and all later stores to the variable) are performed by the library, so we ensure that stored values always have padding bits cleared. That means we can simply clear the padding bits of the 'expected' value and we will be comparing two values with equal padding bits. This means we don't need the loop for std::atomic, so update the __atomic_impl::__compare_exchange function to take a bool parameter that says whether it's being used by std::atomic_ref. If not, we can use a simpler, non-looping implementation. libstdc++-v3/ChangeLog: PR libstdc++/111077 * include/bits/atomic_base.h (__atomic_impl::__compare_exchange): Add _AtomicRef non-type template parameter and use a loop if it is true. (__atomic_impl::compare_exchange_weak): Add _AtomicRef NTTP. (__atomic_impl::compare_exchange_strong): Likewise. (atomic_ref::compare_exchange_weak): Use true for NTTP. (atomic_ref::compare_exchange_strong): Use true for NTTP. * testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc: Fix test to not rely on atomic_ref::load() to return an object with padding preserved. --- libstdc++-v3/include/bits/atomic_base.h | 147 ++++++++++++------ .../atomic_ref/compare_exchange_padding.cc | 75 ++++++--- 2 files changed, 150 insertions(+), 72 deletions(-) diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h index 4ce04a02dd0..974872ad7a6 100644 --- a/libstdc++-v3/include/bits/atomic_base.h +++ b/libstdc++-v3/include/bits/atomic_base.h @@ -985,7 +985,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template using _Val = typename remove_volatile<_Tp>::type; - template +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wc++17-extensions" + + template _GLIBCXX_ALWAYS_INLINE bool __compare_exchange(_Tp& __val, _Val<_Tp>& __e, _Val<_Tp>& __i, bool __is_weak, @@ -994,27 +997,79 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __glibcxx_assert(__is_valid_cmpexch_failure_order(__f)); using _Vp = _Val<_Tp>; + _Tp* const __pval = std::__addressof(__val); - if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Vp>()) + if constexpr (!__atomic_impl::__maybe_has_padding<_Vp>()) { - // We must not modify __e on success, so cannot clear its padding. - // Copy into a buffer and clear that, then copy back on failure. - alignas(_Vp) unsigned char __buf[sizeof(_Vp)]; - _Vp* __exp = ::new((void*)__buf) _Vp(__e); - __atomic_impl::__clear_padding(*__exp); - if (__atomic_compare_exchange(std::__addressof(__val), __exp, - __atomic_impl::__clear_padding(__i), + return __atomic_compare_exchange(__pval, std::__addressof(__e), + std::__addressof(__i), __is_weak, + int(__s), int(__f)); + } + else if constexpr (!_AtomicRef) // std::atomic + { + // Clear padding of the value we want to set: + _Vp* const __pi = __atomic_impl::__clear_padding(__i); + // Only allowed to modify __e on failure, so make a copy: + _Vp __exp = __e; + // Clear padding of the expected value: + _Vp* const __pexp = __atomic_impl::__clear_padding(__exp); + + // For std::atomic we know that the contained value will already + // have zeroed padding, so trivial memcmp semantics are OK. + if (__atomic_compare_exchange(__pval, __pexp, __pi, __is_weak, int(__s), int(__f))) return true; - __builtin_memcpy(std::__addressof(__e), __exp, sizeof(_Vp)); + // Value bits must be different, copy from __exp back to __e: + __builtin_memcpy(std::__addressof(__e), __pexp, sizeof(_Vp)); return false; } - else - return __atomic_compare_exchange(std::__addressof(__val), - std::__addressof(__e), - std::__addressof(__i), - __is_weak, int(__s), int(__f)); + else // std::atomic_ref where T has padding bits. + { + // Clear padding of the value we want to set: + _Vp* const __pi = __atomic_impl::__clear_padding(__i); + + // Only allowed to modify __e on failure, so make a copy: + _Vp __exp = __e; + // Optimistically assume that a previous store had zeroed padding + // so that zeroing it in the expected value will match first time. + _Vp* const __pexp = __atomic_impl::__clear_padding(__exp); + + // compare_exchange is specified to compare value representations. + // Need to check whether a failure is 'real' or just due to + // differences in padding bits. This loop should run no more than + // three times, because the worst case scenario is: + // First CAS fails because the actual value has non-zero padding. + // Second CAS fails because another thread stored the same value, + // but now with padding cleared. Third CAS succeeds. + // We will never need to loop a fourth time, because any value + // written by another thread (whether via store, exchange or + // compare_exchange) will have had its padding cleared. + while (true) + { + // Copy of the expected value so we can clear its padding. + _Vp __orig = __exp; + + if (__atomic_compare_exchange(__pval, __pexp, __pi, + __is_weak, int(__s), int(__f))) + return true; + + // Copy of the actual value so we can clear its padding. + _Vp __curr = __exp; + + // Compare value representations (i.e. ignoring padding). + if (__builtin_memcmp(__atomic_impl::__clear_padding(__orig), + __atomic_impl::__clear_padding(__curr), + sizeof(_Vp))) + { + // Value representations compare unequal, real failure. + __builtin_memcpy(std::__addressof(__e), __pexp, + sizeof(_Vp)); + return false; + } + } + } } +#pragma GCC diagnostic pop } // namespace __atomic_impl #if __cplusplus > 201703L @@ -1061,24 +1116,26 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return *__dest; } - template + template _GLIBCXX_ALWAYS_INLINE bool compare_exchange_weak(_Tp* __ptr, _Val<_Tp>& __expected, _Val<_Tp> __desired, memory_order __success, - memory_order __failure) noexcept + memory_order __failure, + bool __check_padding = false) noexcept { - return __atomic_impl::__compare_exchange(*__ptr, __expected, __desired, - true, __success, __failure); + return __atomic_impl::__compare_exchange<_AtomicRef>( + *__ptr, __expected, __desired, true, __success, __failure); } - template + template _GLIBCXX_ALWAYS_INLINE bool compare_exchange_strong(_Tp* __ptr, _Val<_Tp>& __expected, _Val<_Tp> __desired, memory_order __success, - memory_order __failure) noexcept + memory_order __failure, + bool __ignore_padding = false) noexcept { - return __atomic_impl::__compare_exchange(*__ptr, __expected, __desired, - false, __success, __failure); + return __atomic_impl::__compare_exchange<_AtomicRef>( + *__ptr, __expected, __desired, false, __success, __failure); } #if __cpp_lib_atomic_wait @@ -1485,9 +1542,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION memory_order __success, memory_order __failure) const noexcept { - return __atomic_impl::compare_exchange_weak(_M_ptr, - __expected, __desired, - __success, __failure); + return __atomic_impl::compare_exchange_weak( + _M_ptr, __expected, __desired, __success, __failure); } bool @@ -1495,9 +1551,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION memory_order __success, memory_order __failure) const noexcept { - return __atomic_impl::compare_exchange_strong(_M_ptr, - __expected, __desired, - __success, __failure); + return __atomic_impl::compare_exchange_strong( + _M_ptr, __expected, __desired, __success, __failure); } bool @@ -1600,9 +1655,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION memory_order __success, memory_order __failure) const noexcept { - return __atomic_impl::compare_exchange_weak(_M_ptr, - __expected, __desired, - __success, __failure); + return __atomic_impl::compare_exchange_weak( + _M_ptr, __expected, __desired, __success, __failure); } bool @@ -1610,9 +1664,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION memory_order __success, memory_order __failure) const noexcept { - return __atomic_impl::compare_exchange_strong(_M_ptr, - __expected, __desired, - __success, __failure); + return __atomic_impl::compare_exchange_strong( + _M_ptr, __expected, __desired, __success, __failure); } bool @@ -1775,19 +1828,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION memory_order __success, memory_order __failure) const noexcept { - return __atomic_impl::compare_exchange_weak(_M_ptr, - __expected, __desired, - __success, __failure); + return __atomic_impl::compare_exchange_weak( + _M_ptr, __expected, __desired, __success, __failure); } bool compare_exchange_strong(_Fp& __expected, _Fp __desired, - memory_order __success, - memory_order __failure) const noexcept + memory_order __success, + memory_order __failure) const noexcept { - return __atomic_impl::compare_exchange_strong(_M_ptr, - __expected, __desired, - __success, __failure); + return __atomic_impl::compare_exchange_strong( + _M_ptr, __expected, __desired, __success, __failure); } bool @@ -1904,9 +1955,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION memory_order __success, memory_order __failure) const noexcept { - return __atomic_impl::compare_exchange_weak(_M_ptr, - __expected, __desired, - __success, __failure); + return __atomic_impl::compare_exchange_weak( + _M_ptr, __expected, __desired, __success, __failure); } bool @@ -1914,9 +1964,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION memory_order __success, memory_order __failure) const noexcept { - return __atomic_impl::compare_exchange_strong(_M_ptr, - __expected, __desired, - __success, __failure); + return __atomic_impl::compare_exchange_strong( + _M_ptr, __expected, __desired, __success, __failure); } bool diff --git a/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc index e9f8a4bdf2a..0dab8a23e10 100644 --- a/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc +++ b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc @@ -6,39 +6,68 @@ #include -struct S { char c; short s; }; +struct S +{ + char c; + alignas(2) short s; +}; void __attribute__((noinline,noipa)) -fill_struct(S& s) -{ __builtin_memset(&s, 0xff, sizeof(S)); } +set_padding(S& s, unsigned char x) +{ reinterpret_cast(&s)[1] = x; } -bool -compare_struct(const S& a, const S& b) -{ return __builtin_memcmp(&a, &b, sizeof(S)) == 0; } +unsigned char __attribute__((noinline,noipa)) +get_padding(S& s) +{ return reinterpret_cast(&s)[1]; } -int -main () +void +test01() { S s; - S ss{ s }; - fill_struct(ss); + S ss; ss.c = 'a'; ss.s = 42; + set_padding(ss, 0xff); - std::atomic_ref as{ s }; - as.store(ss); - auto ts = as.load(); - VERIFY( !compare_struct(ss, ts) ); // padding cleared on store - as.exchange(ss); - auto es = as.load(); - VERIFY( compare_struct(ts, es) ); // padding cleared on exchange + { + std::atomic_ref as{ s }; + as.store(ss); // copy value bits, clear padding bits + } + VERIFY( get_padding(s) == 0 ); // padding was cleared on store + ss.c = 'b'; + set_padding(ss, 0x11); + VERIFY( get_padding(ss) == 0x11 ); + { + std::atomic_ref as{ s }; + as.exchange(ss); // copy value bits, clear padding bits + } + VERIFY( get_padding(s) == 0 ); // padding was cleared on store + + S exp = s; + set_padding(exp, 0xaa); + set_padding(s, 0xbb); S n; - fill_struct(n); - n.c = 'b'; + n.c = 'c'; n.s = 71; - // padding cleared on compexchg - VERIFY( as.compare_exchange_weak(s, n) ); - VERIFY( as.compare_exchange_strong(n, s) ); - return 0; + set_padding(n, 0xcc); + + // padding cleared on cmpexchg + { + std::atomic_ref as{ s }; + // This assumes no spurious failures, hopefully true without contention. + VERIFY( as.compare_exchange_weak(exp, n) ); // padding in exp ignored + } + VERIFY( get_padding(s) == 0 ); // padding in n was not copied to s + + { + std::atomic_ref as{ s }; + VERIFY( as.compare_exchange_strong(n, exp) ); // padding in n ignored + } + VERIFY( get_padding(s) == 0 ); // padding in exp was not copied to s +} + +int main() +{ + test01(); }