From patchwork Thu Aug 25 10:11:40 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 754 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:ecc5:0:0:0:0:0 with SMTP id s5csp166032wro; Thu, 25 Aug 2022 03:12:55 -0700 (PDT) X-Google-Smtp-Source: AA6agR6uIeumZKADVRImVe0+wymW0pi3OwQTJmi4hd9T20C7vKcdBXRG6SJmxiESvaQjHxqHlx0W X-Received: by 2002:aa7:d6c8:0:b0:447:b278:5ba5 with SMTP id x8-20020aa7d6c8000000b00447b2785ba5mr1847070edr.217.1661422374855; Thu, 25 Aug 2022 03:12:54 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1661422374; cv=none; d=google.com; s=arc-20160816; b=KsodQVzwncCaP0ohO+J8iyl+VAxilVk+KgRBSPrrlsXACexXcYg5FMBLAMTIEGb6Uu Q1Snz+jNZ79sS1eupcARK5gGWoTTvnxss+pKCc2m6moZ0eFKcVbTpJYUjCrSfn2ONBCv yO2dASGJj4K9rG3cPog/fZeqhhfvCUXbuK6IS5pWhdpHBECFw0yNh8TUXh/pcTaFVzgw 4+gpyx9zUq3G8rRVLbGQifOlQRukcgHKVRNh2HTfYoy1JtIONs9uFK42IrJeygTN31/s czr20OSbXB8vbsKw4pL2vtEd4x19VqpIQi0mIrFEYEFzYxv9SfcyvnqNWiNhgNcaBW2K ffhA== 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-disposition:in-reply-to:mime-version:references:message-id :subject:to:date:dmarc-filter:delivered-to:dkim-signature :dkim-filter; bh=4WRk+OUaaenfWm1/pl0TYmqfB47FnAD/SG2QGRWAEC8=; b=l8ZbUvkds+YqhFE8jhlcqyaOrWCD2wmcFrdaduoWl0GwaGQp8rPduydPtC81pWZkXC 9uuVt2QubZ4Zwfuz/plSAIQZe8+YD+0KrAE4PKt90GwljOT/9RVzBP7F6mClDOR9Zo0R X9Yo87RnTUOIu8RryrY/JM3e1o2QthZCJ5EZU8ZGDVBmcWn4EspmFSrq5im/pIRS0J3W uZCNeND3XxzkqUkz6Y+TgxtilKppNUeCGf8o99wn+MJuRDHwU+ilk8U9kL+5B6QGGqVb 643slKtbM+HAMxCcxrij4G0wP923mFTsln+kUfDrnamq4WTrJjMkL5Y7J8AOHEO/6apl jeuQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=bz3evvJR; 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 sourceware.org (server2.sourceware.org. [2620:52:3:1:0:246e:9693:128c]) by mx.google.com with ESMTPS id qt7-20020a170906ece700b0073cd848ae8asi3054568ejb.321.2022.08.25.03.12.54 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 25 Aug 2022 03:12:54 -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=bz3evvJR; 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 200A6385140A for ; Thu, 25 Aug 2022 10:12:34 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 200A6385140A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1661422354; bh=4WRk+OUaaenfWm1/pl0TYmqfB47FnAD/SG2QGRWAEC8=; h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=bz3evvJRvuXvn81g6go1fWb1vAUSzL2hi5lTCkVrEmGByc7hLuf6mgy9l/OT+0s1l 1SPI8bFfPGAJwwpGNumQVYoCl6imPqcLUl+d6Z8tVcnzFrZTv1A+NwxwUTazlrBzWD YBAXhWKxNInJaED6HUskyxZFDMgWDs/knmHiaSzc= 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.133.124]) by sourceware.org (Postfix) with ESMTPS id EED163858C2F for ; Thu, 25 Aug 2022 10:11:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EED163858C2F Received: from mimecast-mx02.redhat.com (mx3-rdu2.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-456-oClvkHODO0itc7Bu0m9HoQ-1; Thu, 25 Aug 2022 06:11:46 -0400 X-MC-Unique: oClvkHODO0itc7Bu0m9HoQ-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D504B1C08978; Thu, 25 Aug 2022 10:11:45 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.41]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 905052166B2A; Thu, 25 Aug 2022 10:11:45 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 27PABgBA2547175 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 25 Aug 2022 12:11:43 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 27PABf4M2547174; Thu, 25 Aug 2022 12:11:41 +0200 Date: Thu, 25 Aug 2022 12:11:40 +0200 To: Thomas Rodgers , Jonathan Wakely Subject: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange) Message-ID: References: <20210923180837.633173-1-rodgert@appliantology.com> <20210927141031.651313-1-rodgert@appliantology.com> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE 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.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jakub Jelinek via Gcc-patches From: Jakub Jelinek Reply-To: Jakub Jelinek Cc: libstdc++ , gcc-patches@gcc.gnu.org, Thomas Rodgers 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?1742127628057044358?= X-GMAIL-MSGID: =?utf-8?q?1742127628057044358?= On Tue, Jan 18, 2022 at 09:48:19PM +0000, Jonathan Wakely via Gcc-patches wrote: > On Tue, 2 Nov 2021 at 01:26, Thomas Rodgers wrote: > > > This should address Jonathan's feedback and adds support for atomic_ref > > > > > >This change implements P0528 which requires that padding bits not > >participate in atomic compare exchange operations. All arguments to the > >generic template are 'sanitized' by the __builtin_clearpadding intrisic > > The name of the intrinsic and the word "instrinsic" have typos. I'd like to ping this patch. To make some progress, I've tried to incorporate some of Jonathan's review comments below, but it is incomplete. ChangeLog + wording above it fixed. > > > > explicit > > __atomic_ref(_Tp& __t) : _M_ptr(std::__addressof(__t)) > >- { __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) == 0); } > >+ { > >+ __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) == 0); > >+#if __cplusplus > 201402L && __has_builtin(__builtin_clear_padding) > >+ __builtin_clear_padding(_M_ptr); > >+#endif > >+ } > > Is this safe to do? > > What if multiple threads all create a std::atomic_ref round the same object > at once, they'll all try to clear padding, and so race, won't they? > I don't think we can clear padding on atomic_ref construction, only on > store and RMW operations. Didn't touch the above. > > > >--- a/libstdc++-v3/include/std/atomic > >+++ b/libstdc++-v3/include/std/atomic The patch against this file doesn't apply it all. > >--- /dev/null > >+++ > b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc > >@@ -0,0 +1,43 @@ > >+// { dg-options "-std=gnu++2a" } > >+// { dg-do run { target c++2a } } > > This new test is using "2a" not "20". Fixed thus, but the other testcase wasn't in the patch at all. Here it is: libstdc++: Clear padding bits in atomic compare_exchange This change implements P0528 which requires that padding bits not participate in atomic compare exchange operations. All arguments to the generic template are 'sanitized' by the __builtin_clear_padding intrinsic before they are used in comparisons. This requires that any stores also sanitize the incoming value. Signed-off-by: Thomas Rodgers libstdc++-v3/ChangeLog: * include/std/atomic (atomic::atomic(_Tp)): Clear padding for __cplusplus > 201703L. (atomic::store()): Clear padding. (atomic::exchange()): Likewise. (atomic::compare_exchange_weak()): Likewise. (atomic::compare_exchange_strong()): Likewise. * include/bits/atomic_base.h (__atomic_impl::__clear_padding()): New function. (__atomic_impl::__maybe_has_padding()): Likewise. (__atomic_impl::__compare_exchange()): Likewise. (__atomic_impl::compare_exchange_weak()): Delegate to __compare_exchange(). (__atomic_impl::compare_exchange_strong()): Likewise. * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New test. * testsuite/28_atomics/atomic_ref/compare_exchange_padding.cc: Likewise. Jakub --- a/libstdc++-v3/include/bits/atomic_base.h.jj 2022-05-16 09:46:02.361059682 +0200 +++ b/libstdc++-v3/include/bits/atomic_base.h 2022-08-25 12:06:13.758883172 +0200 @@ -954,6 +954,87 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION /// @endcond + // Implementation details of atomic padding handling + namespace __atomic_impl + { + template + _GLIBCXX_ALWAYS_INLINE _Tp* + __clear_padding(_Tp& __val) noexcept + { + auto* __ptr = std::__addressof(__val); +#if __has_builtin(__builtin_clear_padding) + __builtin_clear_padding(std::__addressof(__val)); +#endif + return __ptr; + } + + template + constexpr bool + __maybe_has_padding() + { +#if ! __has_builtin(__builtin_clear_padding) + return false; +#elif __has_builtin(__has_unique_object_representations) + return !__has_unique_object_representations(_Tp) + && !is_floating_point<_Tp>::value; +#else + return true; +#endif + } + + template + _GLIBCXX_ALWAYS_INLINE bool + __compare_exchange(_Tp& __val, _Tp& __e, _Tp& __i, bool __weak, + memory_order __s, memory_order __f) noexcept + { + __glibcxx_assert(__is_valid_cmpexch_failure_order(__f)); + + if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>()) + { + alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; + _Tp* __exp = ::new((void*)__buf) _Tp(__e); + __exp = __atomic_impl::__clear_padding(*__exp); + auto* __des = __atomic_impl::__clear_padding(__i); + if (__atomic_compare_exchange(std::__addressof(__val), __exp, __des, __weak, + int(__s), int(__f))) + return true; + __builtin_memcpy(std::addressof(__e), __exp, sizeof(_Tp)); + return false; + } + else + return __atomic_compare_exchange(std::__addressof(__val), + std::__addressof(__e), + std::__addressof(__i), + __weak, int(__s), int(__f)); + } + + template + _GLIBCXX_ALWAYS_INLINE bool + __compare_exchange(_Tp volatile& __val, _Tp& __e, _Tp& __i, bool __weak, + memory_order __s, memory_order __f) noexcept + { + __glibcxx_assert(__is_valid_cmpexch_failure_order(__f)); + + if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>()) + { + alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; + _Tp* __exp = ::new((void*)__buf) _Tp(__e); + __exp = __atomic_impl::__clear_padding(*__exp); + auto* __des = __atomic_impl::__clear_padding(__i); + if (__atomic_compare_exchange(std::__addressof(__val), __exp, __des, __weak, + int(__s), int(__f))) + return true; + __builtin_memcpy(std::addressof(__e), __exp, sizeof(_Tp)); + return false; + } + else + return __atomic_compare_exchange(std::__addressof(__val), + std::__addressof(__e), + std::__addressof(__i), + __weak, int(__s), int(__f)); + } + } // namespace __atomic_impl + #if __cplusplus > 201703L /// @cond undocumented @@ -979,7 +1060,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template _GLIBCXX_ALWAYS_INLINE void store(_Tp* __ptr, _Val<_Tp> __t, memory_order __m) noexcept - { __atomic_store(__ptr, std::__addressof(__t), int(__m)); } + { + if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>()) + __atomic_impl::__clear_padding(__t); + __atomic_store(__ptr, std::__addressof(__t), int(__m)); + } template _GLIBCXX_ALWAYS_INLINE _Val<_Tp> @@ -997,6 +1082,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; auto* __dest = reinterpret_cast<_Val<_Tp>*>(__buf); + __atomic_impl::__clear_padding(__desired); __atomic_exchange(__ptr, std::__addressof(__desired), __dest, int(__m)); return *__dest; } @@ -1007,11 +1093,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Val<_Tp> __desired, memory_order __success, memory_order __failure) noexcept { - __glibcxx_assert(__is_valid_cmpexch_failure_order(__failure)); - - return __atomic_compare_exchange(__ptr, std::__addressof(__expected), - std::__addressof(__desired), true, - int(__success), int(__failure)); + return __atomic_impl::__compare_exchange(*__ptr, __expected, __desired, + true, __success, __failure); } template @@ -1020,11 +1103,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Val<_Tp> __desired, memory_order __success, memory_order __failure) noexcept { - __glibcxx_assert(__is_valid_cmpexch_failure_order(__failure)); - - return __atomic_compare_exchange(__ptr, std::__addressof(__expected), - std::__addressof(__desired), false, - int(__success), int(__failure)); + return __atomic_impl::__compare_exchange(*__ptr, __expected, __desired, + false, __success, __failure); } #if __cpp_lib_atomic_wait @@ -1396,7 +1476,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION explicit __atomic_ref(_Tp& __t) : _M_ptr(std::__addressof(__t)) - { __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) == 0); } + { + __glibcxx_assert(((uintptr_t)_M_ptr % required_alignment) == 0); +#if __cplusplus > 201402L && __has_builtin(__builtin_clear_padding) + __builtin_clear_padding(_M_ptr); +#endif + } __atomic_ref(const __atomic_ref&) noexcept = default; --- a/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc.jj 2022-08-25 11:54:56.094694979 +0200 +++ b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc 2022-08-25 11:54:56.094694979 +0200 @@ -0,0 +1,43 @@ +// { dg-options "-std=gnu++20" } +// { dg-do run { target c++20 } } +// { dg-add-options libatomic } + +#include + +#include + +struct S { char c; short s; }; + +void __attribute__((noinline,noipa)) +fill_struct(S& s) +{ __builtin_memset(&s, 0xff, sizeof(S)); } + +bool +compare_struct(const S& a, const S& b) +{ return __builtin_memcmp(&a, &b, sizeof(S)) == 0; } + +int +main () +{ + S s; + fill_struct(s); + s.c = 'a'; + s.s = 42; + + S ss{ s }; + std::atomic_ref as{ s }; + auto ts = as.load(); + VERIFY( !compare_struct(ss, ts) ); // padding cleared on construction + as.exchange(ss); + auto es = as.load(); + VERIFY( compare_struct(ts, es) ); // padding cleared on exchange + + S n; + fill_struct(n); + n.c = 'b'; + n.s = 71; + // padding cleared on compexchg + VERIFY( as.compare_exchange_weak(s, n) ); + VERIFY( as.compare_exchange_strong(n, s) ); + return 0; +}