Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
On Fri, 9 Sept 2022 at 20:01, Thomas Rodgers wrote: > > s/__weak/__is_weak/g perhaps? Yes, that'll do. Fixed by the attached, with a test to avoid it happening again. Tested x86_64-linux, pushed to trunk. > > On Fri, Sep 9, 2022 at 11:46 AM Iain Sandoe via Libstdc++ > wrote: >> >> >> >> > On 9 Sep 2022, at 19:36, Rainer Orth wrote: >> > >> >> >> Here's a complete patch that combines the various incremental patches >> >> that have been going around. I'm testing this now. >> >> >> >> Please take a look. >> > >> > unfortunately, this patch broke macOS bootstrap (seen on >> > x86_64-apple-darwin11.4.2): >> > >> > In file included from >> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/shared_ptr_atomic.h:33, >> > from >> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/memory:78, >> > from >> > /vol/gcc/src/hg/master/darwin/libstdc++-v3/include/precompiled/stdc++.h:82: >> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h: >> > In function 'bool std::__atomic_impl::__compare_exchange(_Tp&, >> > _Val<_Tp>&, _Val<_Tp>&, bool, std::memory_order, std::memory_order)': >> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1008:49: >> > error: expected primary-expression before ',' token >> > 1008 | __weak, int(__s), >> > int(__f))) >> > | ^ >> > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1017:50: >> > error: expected primary-expression before ',' token >> > 1017 |__weak, int(__s), >> > int(__f)); >> > | ^ >> > >> > Darwin gcc predefines __weak= in gcc/config/darwin-c.cc >> > (darwin_cpp_builtins). >> >> yes, __weak and __strong are Objective C things (in principle, applicable to >> non-Darwin targets >> using NeXT runtime - there is at least one such target). >> >> Iain >> commit 007680f946eaffa3c6321624129e1ec18e673091 Author: Jonathan Wakely Date: Fri Sep 9 21:03:58 2022 libstdc++: Rename parameter to avoid darwin __weak qualifier libstdc++-v3/ChangeLog: * include/bits/atomic_base.h (__atomic_impl::__compare_exchange): Rename __weak to __is_weak. * testsuite/17_intro/names.cc: Add __weak and __strong. diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h index 29315547aab..6ea3268fdf0 100644 --- a/libstdc++-v3/include/bits/atomic_base.h +++ b/libstdc++-v3/include/bits/atomic_base.h @@ -990,7 +990,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION template _GLIBCXX_ALWAYS_INLINE bool __compare_exchange(_Tp& __val, _Val<_Tp>& __e, _Val<_Tp>& __i, -bool __weak, memory_order __s, memory_order __f) noexcept +bool __is_weak, +memory_order __s, memory_order __f) noexcept { __glibcxx_assert(__is_valid_cmpexch_failure_order(__f)); @@ -1005,7 +1006,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION __atomic_impl::__clear_padding(*__exp); if (__atomic_compare_exchange(std::__addressof(__val), __exp, __atomic_impl::__clear_padding(__i), - __weak, int(__s), int(__f))) + __is_weak, int(__s), int(__f))) return true; __builtin_memcpy(std::__addressof(__e), __exp, sizeof(_Vp)); return false; @@ -1014,7 +1015,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION return __atomic_compare_exchange(std::__addressof(__val), std::__addressof(__e), std::__addressof(__i), - __weak, int(__s), int(__f)); + __is_weak, int(__s), int(__f)); } } // namespace __atomic_impl diff --git a/libstdc++-v3/testsuite/17_intro/names.cc b/libstdc++-v3/testsuite/17_intro/names.cc index ede2fe8caa7..86fb8f8999b 100644 --- a/libstdc++-v3/testsuite/17_intro/names.cc +++ b/libstdc++-v3/testsuite/17_intro/names.cc @@ -129,6 +129,10 @@ // This clashes with newlib so don't use it. # define __lockablecannot be used as an identifier +#ifndef __APPLE__ +#define __weak predefined qualifier on darwin +#define __strong predefined qualifier on darwin +#endif // Common template parameter names #define OutputIterator OutputIterator is not a reserved name
Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
s/__weak/__is_weak/g perhaps? On Fri, Sep 9, 2022 at 11:46 AM Iain Sandoe via Libstdc++ < libstd...@gcc.gnu.org> wrote: > > > > On 9 Sep 2022, at 19:36, Rainer Orth > wrote: > > > > >> Here's a complete patch that combines the various incremental patches > >> that have been going around. I'm testing this now. > >> > >> Please take a look. > > > > unfortunately, this patch broke macOS bootstrap (seen on > > x86_64-apple-darwin11.4.2): > > > > In file included from > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/shared_ptr_atomic.h:33, > > from > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/memory:78, > > from > /vol/gcc/src/hg/master/darwin/libstdc++-v3/include/precompiled/stdc++.h:82: > > > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h: > In function 'bool std::__atomic_impl::__compare_exchange(_Tp&, _Val<_Tp>&, > _Val<_Tp>&, bool, std::memory_order, std::memory_order)': > > > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1008:49: > error: expected primary-expression before ',' token > > 1008 | __weak, int(__s), > int(__f))) > > | ^ > > > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1017:50: > error: expected primary-expression before ',' token > > 1017 |__weak, int(__s), > int(__f)); > > | ^ > > > > Darwin gcc predefines __weak= in gcc/config/darwin-c.cc > (darwin_cpp_builtins). > > yes, __weak and __strong are Objective C things (in principle, applicable > to non-Darwin targets > using NeXT runtime - there is at least one such target). > > Iain > >
Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
> On 9 Sep 2022, at 19:36, Rainer Orth wrote: > >> Here's a complete patch that combines the various incremental patches >> that have been going around. I'm testing this now. >> >> Please take a look. > > unfortunately, this patch broke macOS bootstrap (seen on > x86_64-apple-darwin11.4.2): > > In file included from > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/shared_ptr_atomic.h:33, > from > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/memory:78, > from > /vol/gcc/src/hg/master/darwin/libstdc++-v3/include/precompiled/stdc++.h:82: > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h: > In function 'bool std::__atomic_impl::__compare_exchange(_Tp&, _Val<_Tp>&, > _Val<_Tp>&, bool, std::memory_order, std::memory_order)': > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1008:49: > error: expected primary-expression before ',' token > 1008 | __weak, int(__s), int(__f))) > | ^ > /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1017:50: > error: expected primary-expression before ',' token > 1017 |__weak, int(__s), int(__f)); > | ^ > > Darwin gcc predefines __weak= in gcc/config/darwin-c.cc (darwin_cpp_builtins). yes, __weak and __strong are Objective C things (in principle, applicable to non-Darwin targets using NeXT runtime - there is at least one such target). Iain
Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
Hi Jonathan, > Here's a complete patch that combines the various incremental patches > that have been going around. I'm testing this now. > > Please take a look. unfortunately, this patch broke macOS bootstrap (seen on x86_64-apple-darwin11.4.2): In file included from /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/shared_ptr_atomic.h:33, from /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/memory:78, from /vol/gcc/src/hg/master/darwin/libstdc++-v3/include/precompiled/stdc++.h:82: /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h: In function 'bool std::__atomic_impl::__compare_exchange(_Tp&, _Val<_Tp>&, _Val<_Tp>&, bool, std::memory_order, std::memory_order)': /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1008:49: error: expected primary-expression before ',' token 1008 | __weak, int(__s), int(__f))) | ^ /var/gcc/regression/master/10.7-gcc/build/x86_64-apple-darwin11.4.2/libstdc++-v3/include/bits/atomic_base.h:1017:50: error: expected primary-expression before ',' token 1017 |__weak, int(__s), int(__f)); | ^ Darwin gcc predefines __weak= in gcc/config/darwin-c.cc (darwin_cpp_builtins). Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
Looks good to me. Tom. On Wed, Sep 7, 2022 at 4:56 AM Jonathan Wakely wrote: > Here's a complete patch that combines the various incremental patches > that have been going around. I'm testing this now. > > Please take a look. >
Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
Here's a complete patch that combines the various incremental patches that have been going around. I'm testing this now. Please take a look. commit 4a0a8ec5bc2a890a1568f99eace666e9f72d Author: Thomas Rodgers Date: Thu Aug 25 11:11:40 2022 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. Co-authored-by: Jakub Jelinek Co-authored-by: Jonathan Wakely Signed-off-by: Thomas Rodgers libstdc++-v3/ChangeLog: * include/bits/atomic_base.h (__atomic_impl::__maybe_has_padding): New function. (__atomic_impl::clear_padding): Likewise. (__atomic_impl::__compare_exchange): Likewise. (__atomic_impl::compare_exchange_weak): Delegate to __compare_exchange. (__atomic_impl::compare_exchange_strong): Likewise. * include/std/atomic (atomic::atomic(T)): Clear padding when possible in a constexpr function. (atomic::store): Clear padding. (atomic::exchange): Likewise. (atomic::compare_exchange_weak): Use __compare_exchange. (atomic::compare_exchange_strong): Likewise. * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New test. * testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc: New test. diff --git a/libstdc++-v3/include/bits/atomic_base.h b/libstdc++-v3/include/bits/atomic_base.h index d29e4434177..29315547aab 100644 --- a/libstdc++-v3/include/bits/atomic_base.h +++ b/libstdc++-v3/include/bits/atomic_base.h @@ -33,6 +33,7 @@ #pragma GCC system_header #include +#include // For placement new #include #include #include @@ -952,19 +953,76 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return __atomic_fetch_sub(&_M_p, _M_type_size(__d), int(__m)); } }; - /// @endcond + namespace __atomic_impl + { +// Implementation details of atomic padding handling + +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_same<_Tp, float>::value && !is_same<_Tp, double>::value; +#else + return true; +#endif + } + +template + _GLIBCXX_ALWAYS_INLINE _Tp* + __clear_padding(_Tp& __val) noexcept + { + auto* __ptr = std::__addressof(__val); +#if __has_builtin(__builtin_clear_padding) + if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>()) + __builtin_clear_padding(__ptr); +#endif + return __ptr; + } + +// Remove volatile and create a non-deduced context for value arguments. +template + using _Val = typename remove_volatile<_Tp>::type; + +template + _GLIBCXX_ALWAYS_INLINE bool + __compare_exchange(_Tp& __val, _Val<_Tp>& __e, _Val<_Tp>& __i, +bool __weak, memory_order __s, memory_order __f) noexcept + { + __glibcxx_assert(__is_valid_cmpexch_failure_order(__f)); + + using _Vp = _Val<_Tp>; + + if _GLIBCXX17_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), + __weak, int(__s), int(__f))) + return true; + __builtin_memcpy(std::__addressof(__e), __exp, sizeof(_Vp)); + 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 - // Implementation details of atomic_ref and atomic. namespace __atomic_impl { -// Remove volatile and create a non-deduced context for value arguments. -template - using _Val = remove_volatile_t<_Tp>; - -// As above, but for difference_type arguments. +// Like _Val above, but for difference_type arguments. template
Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
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 +, 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.jj2022-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
Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
On Tue, Jan 18, 2022 at 09:48:19PM +, 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.jj2022-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_compar
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
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. >before they are used in comparisons. This alrequires that any stores >also sanitize the incoming value. > >Signed-off-by: Thomas Rodgers > >libstdc++=v3/ChangeLog: Typo > > * include/std/atomic (atomic::atomic(_Tp): clear padding for Unclosed paren. >+#if __has_builtin(__builtin_clear_padding) Instead of checking this built-in at every call site, can't we just make __maybe_has_padding return false if the built-in isn't supported? __clear_padding already handles the case where the built-in isn't supported. >+template >+ constexpr bool >+ __maybe_has_padding() >+ { >+#if __has_builtin(__has_unique_object_representations) >+return !__has_unique_object_representations(_Tp) >+&& !is_floating_point<_Tp>::value; >+#else >+return true; >+#endif >+ } So make that: 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 } >+ if _GLIBCXX14_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>()) >+ { This needs to be _GLIBCXX17_CONSTEXPR (everywhere that `if constexpr` is used). >+ { >+alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; >+__builtin_memcpy(__buf, std::__addressof(__e), sizeof(_Tp)); alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; _Tp* __exp = ::new((void*)__buf) _Tp(__e); >+auto* __exp = __atomic_impl::__clear_padding(*reinterpret_cast<_Tp*>(__buf)); And then you don't need the reinterpret_cast: __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; > template > _GLIBCXX_ALWAYS_INLINE void > store(_Tp* __ptr, _Val<_Tp> __t, memory_order __m) noexcept >- { __atomic_store(__ptr, std::__addressof(__t), int(__m)); } >+ { >+#if __has_builtin(__builtin_clear_padding) >+ if _GLIBCXX14_CONSTEXPR (__maybe_has_padding<_Tp>()) >+ __clear_padding(__t); >+#endif >+ __atomic_store(__ptr, std::__addressof(__t), int(__m)); >+ } > All calls to __clear_padding need to be qualified. >+ return __compare_exchange(*__ptr, __expected, __desired, true, >+ __success, __failure); So do calls to __compare_exchange. > > 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. >--- a/libstdc++-v3/include/std/atomic >+++ b/libstdc++-v3/include/std/atomic >@@ -228,13 +228,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > atomic& operator=(const atomic&) = delete; > atomic& operator=(const atomic&) volatile = delete; > >-#if __cplusplus > 201703L && __has_builtin(__builtin_clear_padding) > constexpr atomic(_Tp __i) noexcept : _M_i(__i) >- { __builtin_clear_padding(std::__addressof(_M_i)); } >-#else >- constexpr atomic(_Tp __i) noexcept : _M_i(__i) >- { } >+ { >+#if __cplusplus > 201402L && __has_builtin(__builtin_clear_padding) >+ __builtin_clear_padding(std::__addressof(_M_i)); > #endif >+ } > Is this an incremental patch relative to the first one? The changes to this file look correct. >--- /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".
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
This version of the patch specifically doesn’t deal with long double. Mostly looking for Jonathan’s review to ensure his previous feedback is addressed. I plan to rev the patch to handle long doubles after some further discussion with you and Jonathan. On Tue, Nov 2, 2021 at 12:49 AM Jakub Jelinek wrote: > On Mon, Nov 01, 2021 at 06:25:45PM -0700, Thomas Rodgers via Gcc-patches > wrote: > > +template > > + constexpr bool > > + __maybe_has_padding() > > + { > > +#if __has_builtin(__has_unique_object_representations) > > + return !__has_unique_object_representations(_Tp) > > + && !is_floating_point<_Tp>::value; > > +#else > > + return true; > > +#endif > > I'm not sure I understand the && !is_floating_point<_Tp>::value. > Yes, float and double will never have padding, but long double often > will, e.g. on x86 or ia64 (but e.g. not on ppc, s390x, etc.). > So, unless we want to play with numeric_limits, it should be either > just return !__has_unique_object_representations(_Tp); > or return !__has_unique_object_representations(_Tp) > && (!is_floating_point<_Tp>::value > || is_same<__remove_cv_t<_Tp>,long double>::value); > or, with numeric_limits test numeric_limits<_Tp>::digits == 64 > (but I'm sure Jonathan will not want including such a header dependency > unless it is already included). > Or I can always provide a new __builtin_clear_padding_p ... > > Jakub > >
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
Am Di., 2. Nov. 2021 um 02:26 Uhr schrieb Thomas Rodgers via Libstdc++ : > > This should address Jonathan's feedback and adds support for atomic_ref > I'm wondering why __clear_padding doesn't refer to the computed __ptr value in the case where __builtin_clear_padding is used? Thanks, - Daniel
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
On Mon, Nov 01, 2021 at 06:25:45PM -0700, Thomas Rodgers via Gcc-patches wrote: > +template > + constexpr bool > + __maybe_has_padding() > + { > +#if __has_builtin(__has_unique_object_representations) > + return !__has_unique_object_representations(_Tp) > + && !is_floating_point<_Tp>::value; > +#else > + return true; > +#endif I'm not sure I understand the && !is_floating_point<_Tp>::value. Yes, float and double will never have padding, but long double often will, e.g. on x86 or ia64 (but e.g. not on ppc, s390x, etc.). So, unless we want to play with numeric_limits, it should be either just return !__has_unique_object_representations(_Tp); or return !__has_unique_object_representations(_Tp) && (!is_floating_point<_Tp>::value || is_same<__remove_cv_t<_Tp>,long double>::value); or, with numeric_limits test numeric_limits<_Tp>::digits == 64 (but I'm sure Jonathan will not want including such a header dependency unless it is already included). Or I can always provide a new __builtin_clear_padding_p ... Jakub
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
This should address Jonathan's feedback and adds support for atomic_ref On Wed, Sep 29, 2021 at 5:14 AM Jonathan Wakely wrote: > On Mon, 27 Sept 2021 at 15:11, Thomas Rodgers > wrote: > > > > From: Thomas Rodgers > > > > Now with checks for __has_builtin(__builtin_clear_padding) > > > > 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 > > before they are used in comparisons. This alrequires 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. > > Don't we also need this for std::atomic_ref, i.e. for the > __atomic_impl free functions in ? > > There we don't have any distinction between atomic_ref > and atomic_ref, they both use the same > implementations. But I think that's OK, as I think the built-in is > smart enough to be a no-op for types with no padding. > > > * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New > > test. > > --- > > libstdc++-v3/include/std/atomic | 41 +- > > .../atomic/compare_exchange_padding.cc| 42 +++ > > 2 files changed, 81 insertions(+), 2 deletions(-) > > create mode 100644 > libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc > > > > diff --git a/libstdc++-v3/include/std/atomic > b/libstdc++-v3/include/std/atomic > > index 936dd50ba1c..4ac9ccdc1ab 100644 > > --- a/libstdc++-v3/include/std/atomic > > +++ b/libstdc++-v3/include/std/atomic > > @@ -228,7 +228,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >atomic& operator=(const atomic&) = delete; > >atomic& operator=(const atomic&) volatile = delete; > > > > - constexpr atomic(_Tp __i) noexcept : _M_i(__i) { } > > +#if __cplusplus > 201703L && __has_builtin(__builtin_clear_padding) > > + constexpr atomic(_Tp __i) noexcept : _M_i(__i) > > + { __builtin_clear_padding(std::__addressof(_M_i)); } > > +#else > > + constexpr atomic(_Tp __i) noexcept : _M_i(__i) > > + { } > > +#endif > > Please write this as a single function with the preprocessor > conditions in the body: > > constexpr atomic(_Tp __i) noexcept : _M_i(__i) > { > #if __cplusplus > 201703L && __has_builtin(__builtin_clear_padding) > __builtin_clear_padding(std::__addressof(_M_i)); } > #endif > } > > This not only avoids duplication of the identical parts, but it avoids > warnings from ld.gold if you use --detect-odr-violations. Otherwise, > the linker can see a definition of that constructor on two different > lines (233 and 236), and so warns about possible ODR violations, > something like "warning: while linking foo: symbol > 'std::atomic::atomic(int)' defined in multiple places (possible > ODR violation): ...atomic:233 ... atomic:236" > > Can't we clear the padding for >= 201402L instead of only C++20? Only > C++11 has a problem with the built-in in a constexpr function, right? > So we can DTRT for C++14 upwards. > > > > > >operator _Tp() const noexcept > >{ return load(); } > > @@ -268,12 +274,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >void > >store(_Tp __i, memory_order __m = memory_order_seq_cst) noexcept > >{ > > +#if __has_builtin(__builtin_clear_padding) > > + __builtin_clear_padding(std::__addressof(__i)); > > +#endif > > We repeat this *a lot*. When I started work on this I defined a > non-member function in the __atomic_impl namespace: > > template > _GLIBCXX_ALWAYS_INLINE void > __clear_padding(_Tp& __val) noexcept > { > #if __has_builtin(__builtin_clear_padding) >__builtin_clear_padding(std::__addressof(__val)); > #endif > } > > Then you can just use that everywhere (except the constexpr > constructor), without all the #if checks. > > > > > __atomic_store(std::__addressof(_M_i), std::__addressof(__i), > int(__m)); > >} > > > >void > >store(_Tp __i, memory_order __m = memory_order_seq_cst) volatile > noexcept > >{ > > +#if __has_builtin(__builtin_clear_padding) > > + __builtin_clear_padding(std::__addressof(__i)); > > +#endif > > __atomic_store(std::__addressof(_M_i), std::__addressof(__i), > int(__m)); > >} > > > > @@ -300,6 +312,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >{ > > alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; > > _Tp* __ptr = reinterpret_cast<_Tp*>(__buf); > > +#if __has_builtin(__builtin_clear_padding) > > + __builtin_clear_padding(std::__addressof(__i)); > > +#endi
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
On Wed, Sep 29, 2021 at 11:22:29AM -0700, Thomas Rodgers via Gcc-patches wrote: > > The MSVC implementation uses !__has_unique_object_representations(_Tp) > > && !is_floating_point<_Tp>::value here, which is better than mine > > above (FP types don't have unique object reps, but also don't have > > padding bits). Except the 80-bit long double, that one has padding bits (similarly _Complex long double). As I said earlier, if you want a builtin using the __builtin_clear_padding infrastructure that will return bool if __builtin_clear_padding expands to something or not, it can be easily added. GCC already uses something like that internally. Jakub
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
On Wed, Sep 29, 2021 at 5:14 AM Jonathan Wakely wrote: > On Mon, 27 Sept 2021 at 15:11, Thomas Rodgers > wrote: > > > > From: Thomas Rodgers > > > > Now with checks for __has_builtin(__builtin_clear_padding) > > > > 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 > > before they are used in comparisons. This alrequires 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. > > Don't we also need this for std::atomic_ref, i.e. for the > __atomic_impl free functions in ? > > There we don't have any distinction between atomic_ref > and atomic_ref, they both use the same > implementations. But I think that's OK, as I think the built-in is > smart enough to be a no-op for types with no padding. > > > * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New > > test. > > --- > > libstdc++-v3/include/std/atomic | 41 +- > > .../atomic/compare_exchange_padding.cc| 42 +++ > > 2 files changed, 81 insertions(+), 2 deletions(-) > > create mode 100644 > libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc > > > > diff --git a/libstdc++-v3/include/std/atomic > b/libstdc++-v3/include/std/atomic > > index 936dd50ba1c..4ac9ccdc1ab 100644 > > --- a/libstdc++-v3/include/std/atomic > > +++ b/libstdc++-v3/include/std/atomic > > @@ -228,7 +228,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >atomic& operator=(const atomic&) = delete; > >atomic& operator=(const atomic&) volatile = delete; > > > > - constexpr atomic(_Tp __i) noexcept : _M_i(__i) { } > > +#if __cplusplus > 201703L && __has_builtin(__builtin_clear_padding) > > + constexpr atomic(_Tp __i) noexcept : _M_i(__i) > > + { __builtin_clear_padding(std::__addressof(_M_i)); } > > +#else > > + constexpr atomic(_Tp __i) noexcept : _M_i(__i) > > + { } > > +#endif > > Please write this as a single function with the preprocessor > conditions in the body: > > constexpr atomic(_Tp __i) noexcept : _M_i(__i) > { > #if __cplusplus > 201703L && __has_builtin(__builtin_clear_padding) > __builtin_clear_padding(std::__addressof(_M_i)); } > #endif > } > > This not only avoids duplication of the identical parts, but it avoids > warnings from ld.gold if you use --detect-odr-violations. Otherwise, > the linker can see a definition of that constructor on two different > lines (233 and 236), and so warns about possible ODR violations, > something like "warning: while linking foo: symbol > 'std::atomic::atomic(int)' defined in multiple places (possible > ODR violation): ...atomic:233 ... atomic:236" > > Can't we clear the padding for >= 201402L instead of only C++20? Only > C++11 has a problem with the built-in in a constexpr function, right? > So we can DTRT for C++14 upwards. > > We can, I was being conservative expecting guiding elvish feedback :) > > > > >operator _Tp() const noexcept > >{ return load(); } > > @@ -268,12 +274,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >void > >store(_Tp __i, memory_order __m = memory_order_seq_cst) noexcept > >{ > > +#if __has_builtin(__builtin_clear_padding) > > + __builtin_clear_padding(std::__addressof(__i)); > > +#endif > > We repeat this *a lot*. When I started work on this I defined a > non-member function in the __atomic_impl namespace: > > template > _GLIBCXX_ALWAYS_INLINE void > __clear_padding(_Tp& __val) noexcept > { > #if __has_builtin(__builtin_clear_padding) >__builtin_clear_padding(std::__addressof(__val)); > #endif > } > > Then you can just use that everywhere (except the constexpr > constructor), without all the #if checks. > > > > > __atomic_store(std::__addressof(_M_i), std::__addressof(__i), > int(__m)); > >} > > > >void > >store(_Tp __i, memory_order __m = memory_order_seq_cst) volatile > noexcept > >{ > > +#if __has_builtin(__builtin_clear_padding) > > + __builtin_clear_padding(std::__addressof(__i)); > > +#endif > > __atomic_store(std::__addressof(_M_i), std::__addressof(__i), > int(__m)); > >} > > > > @@ -300,6 +312,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > >{ > > alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; > > _Tp* __ptr = reinterpret_cast<_Tp*>(__buf); > > +#if __has_builtin(__builtin_clear_padding) > > + __builtin_clear_padding(std::__addressof(__i)); > > +#end
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
On Wed, Sep 29, 2021 at 01:13:46PM +0100, Jonathan Wakely via Gcc-patches wrote: > But I think that's OK, as I think the built-in is > smart enough to be a no-op for types with no padding. Yes. The only effect it will have is that during the initial optimization passes the variable/parameter will be addressable where without the call and __addressof it wouldn't be; but as soon as __builtin_clear_padding is folded to nothing if there is no padding or something if there is (which happens quite early) and TODO_update_address_taken is done at the end of some pass, it will no longer be addressable (unless something different keeps taking its address). Jakub
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
On Wed, 29 Sept 2021 at 13:13, Jonathan Wakely wrote: > We repeat this *a lot*. When I started work on this I defined a > non-member function in the __atomic_impl namespace: I've attached my incomplete patch from when I was working on this. diff --cc libstdc++-v3/include/bits/atomic_base.h index 71e1de078b5,35023ad30a8..000 --- a/libstdc++-v3/include/bits/atomic_base.h +++ b/libstdc++-v3/include/bits/atomic_base.h @@@ -944,6 -952,28 +944,27 @@@ _GLIBCXX_BEGIN_NAMESPACE_VERSIO template using _Val = remove_volatile_t<_Tp>; + template + constexpr bool + __maybe_has_padding() + { -#if _GLIBCXX_HAVE_BUILTIN_HAS_UNIQ_OBJ_REP - return !__has_unique_object_representations(_Tp); ++#if __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 void + __clear_padding(_Tp& __val) noexcept + { - auto* __ptr = std::__addressof(__val); + #if __has_builtin(__builtin_clear_padding) - __builtin_clear_padding(__ptr); ++ __builtin_clear_padding(std::__addressof(__val)); + #endif - return __ptr; + } + // As above, but for difference_type arguments. template using _Diff = conditional_t, ptrdiff_t, _Val<_Tp>>; @@@ -984,13 -1015,26 +1006,26 @@@ template _GLIBCXX_ALWAYS_INLINE bool compare_exchange_weak(_Tp* __ptr, _Val<_Tp>& __expected, - _Val<_Tp> __desired, memory_order __success, - memory_order __failure) noexcept + _Val<_Tp> __desired, + memory_order __success, memory_order __failure, + bool __weak = true) noexcept { + __glibcxx_assert(__is_valid_cmpexch_failure_order(__failure)); - + #if __has_builtin(__builtin_clear_padding) + if _GLIBCXX_CONSTEXPR17 (__maybe_has_padding<_Tp>()) + { + _Val<_Tp> __expected0 = __expected; + auto* __exp = __atomic_impl::__clear_padding(__expected0); + auto* __des = __atomic_impl::__clear_padding(__desired); + if (__atomic_compare_exchange(__ptr, __exp, __des, __weak, + int(__success), int(__failure))) + return true; + __builtin_memcpy(std::__addressof(__expected), __exp, sizeof(_Tp)); + return false; + } - else + #endif return __atomic_compare_exchange(__ptr, std::__addressof(__expected), -std::__addressof(__desired), true, +std::__addressof(__desired), __weak, int(__success), int(__failure)); } @@@ -1000,11 -1044,8 +1035,9 @@@ _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 compare_exchange_weak(__ptr, __expected, __desired, __success, +__failure, false); } #if __cpp_lib_atomic_wait
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
On Mon, 27 Sept 2021 at 15:11, Thomas Rodgers wrote: > > From: Thomas Rodgers > > Now with checks for __has_builtin(__builtin_clear_padding) > > 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 > before they are used in comparisons. This alrequires 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. Don't we also need this for std::atomic_ref, i.e. for the __atomic_impl free functions in ? There we don't have any distinction between atomic_ref and atomic_ref, they both use the same implementations. But I think that's OK, as I think the built-in is smart enough to be a no-op for types with no padding. > * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New > test. > --- > libstdc++-v3/include/std/atomic | 41 +- > .../atomic/compare_exchange_padding.cc| 42 +++ > 2 files changed, 81 insertions(+), 2 deletions(-) > create mode 100644 > libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc > > diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic > index 936dd50ba1c..4ac9ccdc1ab 100644 > --- a/libstdc++-v3/include/std/atomic > +++ b/libstdc++-v3/include/std/atomic > @@ -228,7 +228,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >atomic& operator=(const atomic&) = delete; >atomic& operator=(const atomic&) volatile = delete; > > - constexpr atomic(_Tp __i) noexcept : _M_i(__i) { } > +#if __cplusplus > 201703L && __has_builtin(__builtin_clear_padding) > + constexpr atomic(_Tp __i) noexcept : _M_i(__i) > + { __builtin_clear_padding(std::__addressof(_M_i)); } > +#else > + constexpr atomic(_Tp __i) noexcept : _M_i(__i) > + { } > +#endif Please write this as a single function with the preprocessor conditions in the body: constexpr atomic(_Tp __i) noexcept : _M_i(__i) { #if __cplusplus > 201703L && __has_builtin(__builtin_clear_padding) __builtin_clear_padding(std::__addressof(_M_i)); } #endif } This not only avoids duplication of the identical parts, but it avoids warnings from ld.gold if you use --detect-odr-violations. Otherwise, the linker can see a definition of that constructor on two different lines (233 and 236), and so warns about possible ODR violations, something like "warning: while linking foo: symbol 'std::atomic::atomic(int)' defined in multiple places (possible ODR violation): ...atomic:233 ... atomic:236" Can't we clear the padding for >= 201402L instead of only C++20? Only C++11 has a problem with the built-in in a constexpr function, right? So we can DTRT for C++14 upwards. > >operator _Tp() const noexcept >{ return load(); } > @@ -268,12 +274,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >void >store(_Tp __i, memory_order __m = memory_order_seq_cst) noexcept >{ > +#if __has_builtin(__builtin_clear_padding) > + __builtin_clear_padding(std::__addressof(__i)); > +#endif We repeat this *a lot*. When I started work on this I defined a non-member function in the __atomic_impl namespace: template _GLIBCXX_ALWAYS_INLINE void __clear_padding(_Tp& __val) noexcept { #if __has_builtin(__builtin_clear_padding) __builtin_clear_padding(std::__addressof(__val)); #endif } Then you can just use that everywhere (except the constexpr constructor), without all the #if checks. > __atomic_store(std::__addressof(_M_i), std::__addressof(__i), > int(__m)); >} > >void >store(_Tp __i, memory_order __m = memory_order_seq_cst) volatile > noexcept >{ > +#if __has_builtin(__builtin_clear_padding) > + __builtin_clear_padding(std::__addressof(__i)); > +#endif > __atomic_store(std::__addressof(_M_i), std::__addressof(__i), > int(__m)); >} > > @@ -300,6 +312,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >{ > alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; > _Tp* __ptr = reinterpret_cast<_Tp*>(__buf); > +#if __has_builtin(__builtin_clear_padding) > + __builtin_clear_padding(std::__addressof(__i)); > +#endif > __atomic_exchange(std::__addressof(_M_i), std::__addressof(__i), > __ptr, int(__m)); > return *__ptr; > @@ -311,6 +326,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION >{ > alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; > _Tp* __ptr = reinterpret_cast<_Tp*>(__buf); > +#if __has_builtin(__builtin_clear_padd
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
On Thu, 23 Sep 2021, 20:07 Jakub Jelinek via Libstdc++, < libstd...@gcc.gnu.org> wrote: > On Thu, Sep 23, 2021 at 11:08:37AM -0700, Thomas Rodgers wrote: > > From: Thomas Rodgers > > > > This change implements P0528 which requires that padding bits not > > participate in atomic compare exchange operations. All arguments to the > > Thanks for working on this. > > > generic template are 'sanitized' by the __builtin_clear_padding intrinsic > > before they are used in atomic compare_exchange. This alrequires that any > > stores also sanitize the incoming value. > > Not a review, just random nit. > Shouldn't the __builtin_clear_padding calls be guarded with > #if __has_builtin(__builtin_clear_padding) > or #ifdef _GLIBCXX_HAVE_BUILTIN_CLEAR_PADDING defined based on that? > Yes. It can just use __has_builtin directly. All the compilers we care about support that now. I think clang doesn't support it (yet?), and it doesn't support the MSVC > __builtin_zero_non_value_bits (with very similar, but slightly different, > behavior). > > > 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. > > * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New > > test. > > Jakub > >
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
Agreed, I'll revise the patch to do so. On Thu, Sep 23, 2021 at 12:07 PM Jakub Jelinek wrote: > On Thu, Sep 23, 2021 at 11:08:37AM -0700, Thomas Rodgers wrote: > > From: Thomas Rodgers > > > > This change implements P0528 which requires that padding bits not > > participate in atomic compare exchange operations. All arguments to the > > Thanks for working on this. > > > generic template are 'sanitized' by the __builtin_clear_padding intrinsic > > before they are used in atomic compare_exchange. This alrequires that any > > stores also sanitize the incoming value. > > Not a review, just random nit. > Shouldn't the __builtin_clear_padding calls be guarded with > #if __has_builtin(__builtin_clear_padding) > or #ifdef _GLIBCXX_HAVE_BUILTIN_CLEAR_PADDING defined based on that? > I think clang doesn't support it (yet?), and it doesn't support the MSVC > __builtin_zero_non_value_bits (with very similar, but slightly different, > behavior). > > > 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. > > * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New > > test. > > Jakub > >
Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange
On Thu, Sep 23, 2021 at 11:08:37AM -0700, Thomas Rodgers wrote: > From: Thomas Rodgers > > This change implements P0528 which requires that padding bits not > participate in atomic compare exchange operations. All arguments to the Thanks for working on this. > generic template are 'sanitized' by the __builtin_clear_padding intrinsic > before they are used in atomic compare_exchange. This alrequires that any > stores also sanitize the incoming value. Not a review, just random nit. Shouldn't the __builtin_clear_padding calls be guarded with #if __has_builtin(__builtin_clear_padding) or #ifdef _GLIBCXX_HAVE_BUILTIN_CLEAR_PADDING defined based on that? I think clang doesn't support it (yet?), and it doesn't support the MSVC __builtin_zero_non_value_bits (with very similar, but slightly different, behavior). > 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. > * testsuite/29_atomics/atomic/compare_exchange_padding.cc: New > test. Jakub