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 EC36A3858C62 for ; Thu, 1 Sep 2022 22:58:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EC36A3858C62 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=1662073085; 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: in-reply-to:in-reply-to:references:references; bh=4GWHH0/gf9eS6MMx5uAiqsEh8L5fy+rdVOQDoIw9Mo0=; b=Pv0nERfxcxGHN1rcEyed2s+sjymKbBX/4SjczsAYrHDG5flpuyWpgnv4E7vIL/vraAxxf3 vrcJBkYG2dC1/Tbxu6xnkl9VEEcdg/o0s8uWZA5+toVINEkAvPnY3i+TxpN5Ssa5Y78Rvf xJT3lXwbaeY018B4kU0FK7fx5UNH91k= Received: from mail-io1-f70.google.com (mail-io1-f70.google.com [209.85.166.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-523-s3aOwflxPDKtiJr_4WNEjw-1; Thu, 01 Sep 2022 18:58:01 -0400 X-MC-Unique: s3aOwflxPDKtiJr_4WNEjw-1 Received: by mail-io1-f70.google.com with SMTP id l15-20020a0566022dcf00b00688e70a26deso216182iow.12 for ; Thu, 01 Sep 2022 15:58:01 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=4GWHH0/gf9eS6MMx5uAiqsEh8L5fy+rdVOQDoIw9Mo0=; b=Quq7JwkjCT4Qc2vO7pCR3DAspokKD1f/fidU95gh2GRHNnG/HAtpaJvX6ZcDDBTp4q 89Qrktlxy+D6rSl5ss5oH25hLNQb3jcXgsv/wUDlN3ziIW2DDImA8hscUk5rWiKpVoGD sLhs+blS4G9s/QRiRMsDaR98zZvOheHPTt9BtNgrXKrchORPqC0/qxGvwuzJ8dAJKAc8 AN/wlpCIvVx0JZdM7TCD4Cq9ZOF53CBffP8BbR3EssXL91C/6MsCgAli93RoM4F/d8p/ lgx0A8fDUSXExnXdEBx6nBpsObr0leO0uhWkVta0AnII2rNLIJwGpEx73wUFAyR7cUeW 27qg== X-Gm-Message-State: ACgBeo2DRvKf3wNh3IWTcaL0F2b8l0E3jloJL/DoGwu2bcNYWgIk1Ocy LfLqpQUAPyC7tm1XGJj15KevlNOwRALr8H43j2niyRntOT75miV0Xet7XNiLVgx22+VkdZKwNZ5 pWit35DwjHvRnaz+C/U+xSgnN7tR3xVc= X-Received: by 2002:a02:c779:0:b0:34a:bc59:8e97 with SMTP id k25-20020a02c779000000b0034abc598e97mr6978184jao.158.1662073080954; Thu, 01 Sep 2022 15:58:00 -0700 (PDT) X-Google-Smtp-Source: AA6agR4WxgPPjpGhlCTtM6aUQRsI9XXdpq3aT3FQlLhUX9RGBDjpaURlhfGh8MYF2U+n15VvP6PObfZIwXKsxzHZg9I= X-Received: by 2002:a02:c779:0:b0:34a:bc59:8e97 with SMTP id k25-20020a02c779000000b0034abc598e97mr6978172jao.158.1662073080717; Thu, 01 Sep 2022 15:58:00 -0700 (PDT) MIME-Version: 1.0 References: <20210923180837.633173-1-rodgert@appliantology.com> <20210927141031.651313-1-rodgert@appliantology.com> In-Reply-To: From: Thomas Rodgers Date: Thu, 1 Sep 2022 15:57:50 -0700 Message-ID: Subject: Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange) To: Jakub Jelinek Cc: Jonathan Wakely , gcc Patches , "libstdc++" , Thomas Rodgers X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="000000000000913c7105e7a58bd6" X-Spam-Status: No, score=-6.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,HTML_MESSAGE,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --000000000000913c7105e7a58bd6 Content-Type: text/plain; charset="UTF-8" Sorry for the delay in getting to this. I am currently working on moving the bulk of the atomic wait implementation into the .so. I'd like to get that work to a stable state before revisiting this patch, but obviously if we want this to make it into GCC13, it needs to happen sooner rather than later. On Thu, Aug 25, 2022 at 3:11 AM Jakub Jelinek wrote: > 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. > > --- 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; > +} > > > Jakub > > --000000000000913c7105e7a58bd6--