From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2181) id 3382238582AF; Thu, 8 Sep 2022 18:30:44 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3382238582AF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1662661844; bh=j9YX12LuAlePTRPUGe17mPaUPCVd9L8u/C3kje5UDi4=; h=From:To:Subject:Date:From; b=eSZfw72zt0jDGh/2JwZv6g6eVSM7re+iMBndYqeuCmlAtUu8Ojj7YIu8sjimQJkO6 lFv5CAu+1bfGQXQs9URgLYm4Oxtddx/AKCNG6s7hw4dchtvWQ40bQggC7OUNU0WeCg DQar6Gs7lgbY6rhqzmJH28JeKyI0+lWLbZpeMM0g= MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" From: Jonathan Wakely To: gcc-cvs@gcc.gnu.org, libstdc++-cvs@gcc.gnu.org Subject: [gcc r13-2548] libstdc++: Clear padding bits in atomic compare_exchange X-Act-Checkin: gcc X-Git-Author: Thomas Rodgers X-Git-Refname: refs/heads/master X-Git-Oldrev: d3883dc77b1426984c0edea6081f57ed2305c9f2 X-Git-Newrev: 157236dbd621644b3cec50b6cf38811959f3e78c Message-Id: <20220908183044.3382238582AF@sourceware.org> Date: Thu, 8 Sep 2022 18:30:44 +0000 (GMT) List-Id: https://gcc.gnu.org/g:157236dbd621644b3cec50b6cf38811959f3e78c commit r13-2548-g157236dbd621644b3cec50b6cf38811959f3e78c Author: Thomas Rodgers Date: Thu Aug 25 12:11:40 2022 +0200 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: --- libstdc++-v3/include/bits/atomic_base.h | 97 +++++++++++++++++----- libstdc++-v3/include/std/atomic | 58 ++++++------- .../29_atomics/atomic/compare_exchange_padding.cc | 42 ++++++++++ .../atomic_ref/compare_exchange_padding.cc | 43 ++++++++++ 4 files changed, 188 insertions(+), 52 deletions(-) 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 - -#if __cplusplus > 201703L - /// @cond undocumented - - // Implementation details of atomic_ref and atomic. 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 = remove_volatile_t<_Tp>; + 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 - // As above, but for difference_type arguments. +#if __cplusplus > 201703L + // Implementation details of atomic_ref and atomic. + namespace __atomic_impl + { + // Like _Val above, but for difference_type arguments. template using _Diff = __conditional_t, ptrdiff_t, _Val<_Tp>>; @@ -979,7 +1037,9 @@ _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)); } + { + __atomic_store(__ptr, __atomic_impl::__clear_padding(__t), int(__m)); + } template _GLIBCXX_ALWAYS_INLINE _Val<_Tp> @@ -997,7 +1057,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; auto* __dest = reinterpret_cast<_Val<_Tp>*>(__buf); - __atomic_exchange(__ptr, std::__addressof(__desired), __dest, int(__m)); + __atomic_exchange(__ptr, __atomic_impl::__clear_padding(__desired), + __dest, int(__m)); return *__dest; } @@ -1007,11 +1068,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 +1078,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 @@ -1955,9 +2010,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _Tp** _M_ptr; }; +#endif // C++2a /// @endcond -#endif // C++2a /// @} group atomics diff --git a/libstdc++-v3/include/std/atomic b/libstdc++-v3/include/std/atomic index 70055b8fa83..b913960336d 100644 --- a/libstdc++-v3/include/std/atomic +++ b/libstdc++-v3/include/std/atomic @@ -230,7 +230,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION atomic& operator=(const atomic&) = delete; atomic& operator=(const atomic&) volatile = delete; - constexpr atomic(_Tp __i) noexcept : _M_i(__i) { } + constexpr atomic(_Tp __i) noexcept : _M_i(__i) + { +#if __cplusplus >= 201402L && __has_builtin(__builtin_clear_padding) + if _GLIBCXX17_CONSTEXPR (__atomic_impl::__maybe_has_padding<_Tp>()) + __builtin_clear_padding(std::__addressof(_M_i)); +#endif + } operator _Tp() const noexcept { return load(); } @@ -270,13 +276,17 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION void store(_Tp __i, memory_order __m = memory_order_seq_cst) noexcept { - __atomic_store(std::__addressof(_M_i), std::__addressof(__i), int(__m)); + __atomic_store(std::__addressof(_M_i), + __atomic_impl::__clear_padding(__i), + int(__m)); } void store(_Tp __i, memory_order __m = memory_order_seq_cst) volatile noexcept { - __atomic_store(std::__addressof(_M_i), std::__addressof(__i), int(__m)); + __atomic_store(std::__addressof(_M_i), + __atomic_impl::__clear_padding(__i), + int(__m)); } _Tp @@ -302,7 +312,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; _Tp* __ptr = reinterpret_cast<_Tp*>(__buf); - __atomic_exchange(std::__addressof(_M_i), std::__addressof(__i), + __atomic_exchange(std::__addressof(_M_i), + __atomic_impl::__clear_padding(__i), __ptr, int(__m)); return *__ptr; } @@ -313,7 +324,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { alignas(_Tp) unsigned char __buf[sizeof(_Tp)]; _Tp* __ptr = reinterpret_cast<_Tp*>(__buf); - __atomic_exchange(std::__addressof(_M_i), std::__addressof(__i), + __atomic_exchange(std::__addressof(_M_i), + __atomic_impl::__clear_padding(__i), __ptr, int(__m)); return *__ptr; } @@ -322,24 +334,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION compare_exchange_weak(_Tp& __e, _Tp __i, memory_order __s, memory_order __f) noexcept { - __glibcxx_assert(__is_valid_cmpexch_failure_order(__f)); - - return __atomic_compare_exchange(std::__addressof(_M_i), - std::__addressof(__e), - std::__addressof(__i), - true, int(__s), int(__f)); + return __atomic_impl::__compare_exchange(_M_i, __e, __i, true, + __s, __f); } bool compare_exchange_weak(_Tp& __e, _Tp __i, memory_order __s, memory_order __f) volatile noexcept { - __glibcxx_assert(__is_valid_cmpexch_failure_order(__f)); - - return __atomic_compare_exchange(std::__addressof(_M_i), - std::__addressof(__e), - std::__addressof(__i), - true, int(__s), int(__f)); + return __atomic_impl::__compare_exchange(_M_i, __e, __i, true, + __s, __f); } bool @@ -358,24 +362,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION compare_exchange_strong(_Tp& __e, _Tp __i, memory_order __s, memory_order __f) noexcept { - __glibcxx_assert(__is_valid_cmpexch_failure_order(__f)); - - return __atomic_compare_exchange(std::__addressof(_M_i), - std::__addressof(__e), - std::__addressof(__i), - false, int(__s), int(__f)); + return __atomic_impl::__compare_exchange(_M_i, __e, __i, false, + __s, __f); } bool compare_exchange_strong(_Tp& __e, _Tp __i, memory_order __s, memory_order __f) volatile noexcept { - __glibcxx_assert(__is_valid_cmpexch_failure_order(__f)); - - return __atomic_compare_exchange(std::__addressof(_M_i), - std::__addressof(__e), - std::__addressof(__i), - false, int(__s), int(__f)); + return __atomic_impl::__compare_exchange(_M_i, __e, __i, false, + __s, __f); } bool @@ -390,7 +386,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION { return compare_exchange_strong(__e, __i, __m, __cmpexch_failure_order(__m)); } -#if __cpp_lib_atomic_wait +#if __cpp_lib_atomic_wait void wait(_Tp __old, memory_order __m = memory_order_seq_cst) const noexcept { @@ -407,7 +403,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION void notify_all() noexcept { std::__atomic_notify_address(&_M_i, true); } -#endif // __cpp_lib_atomic_wait +#endif // __cpp_lib_atomic_wait }; #undef _GLIBCXX20_INIT diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc b/libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc new file mode 100644 index 00000000000..c4ab876db2a --- /dev/null +++ b/libstdc++-v3/testsuite/29_atomics/atomic/compare_exchange_padding.cc @@ -0,0 +1,42 @@ +// { 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; + + std::atomic as{ s }; + auto ts = as.load(); + VERIFY( !compare_struct(s, ts) ); // padding cleared on construction + as.exchange(s); + 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; +} 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 new file mode 100644 index 00000000000..1b1a12dddda --- /dev/null +++ b/libstdc++-v3/testsuite/29_atomics/atomic_ref/compare_exchange_padding.cc @@ -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; +}