public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Thomas Rodgers <trodgers@redhat.com>
To: Jakub Jelinek <jakub@redhat.com>
Cc: Jonathan Wakely <jwakely@redhat.com>,
	gcc Patches <gcc-patches@gcc.gnu.org>,
	 "libstdc++" <libstdc++@gcc.gnu.org>,
	Thomas Rodgers <rodgert@twrodgers.com>
Subject: Re: Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange)
Date: Thu, 1 Sep 2022 15:57:50 -0700	[thread overview]
Message-ID: <CAMmuTO9C9544kDQe=NuJOOrAJGzv8D24bYdMB-2MGV+syC+XJA@mail.gmail.com> (raw)
In-Reply-To: <YwdK3BX3Wdg7ymS4@tucnak>

[-- Attachment #1: Type: text/plain, Size: 11545 bytes --]

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 <jakub@redhat.com> 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 <trodgers@redhat.com> wrote:
> >
> > > This should address Jonathan's feedback and adds support for
> atomic_ref<T>
> > >
> >
> >
> > >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 <trodgers@redhat.com>
>
> libstdc++-v3/ChangeLog:
>
>         * include/std/atomic (atomic<T>::atomic(_Tp)): Clear padding for
>         __cplusplus > 201703L.
>         (atomic<T>::store()): Clear padding.
>         (atomic<T>::exchange()): Likewise.
>         (atomic<T>::compare_exchange_weak()): Likewise.
>         (atomic<T>::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<typename _Tp>
> +      _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<typename _Tp>
> +      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<typename _Tp>
> +      _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<typename _Tp>
> +      _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<typename _Tp>
>        _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<typename _Tp>
>        _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<typename _Tp>
> @@ -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 <atomic>
> +
> +#include <testsuite_hooks.h>
> +
> +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<S> 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
>
>

  reply	other threads:[~2022-09-01 22:58 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-23 18:08 [PATCH] libstdc++: Clear padding bits in atomic compare_exchange Thomas Rodgers
2021-09-23 19:07 ` Jakub Jelinek
2021-09-23 20:15   ` Thomas Rodgers
2021-09-23 20:15   ` Jonathan Wakely
2021-09-27 14:10 ` Thomas Rodgers
2021-09-29 12:13   ` Jonathan Wakely
2021-09-29 12:18     ` Jonathan Wakely
2021-09-29 12:28     ` Jakub Jelinek
2021-09-29 18:22     ` Thomas Rodgers
2021-09-29 18:29       ` Jakub Jelinek
2021-11-02  1:25     ` Thomas Rodgers
2021-11-02  7:49       ` Jakub Jelinek
2021-11-03  3:06         ` Thomas Rodgers
2021-11-02  8:49       ` Daniel Krügler
2022-01-18 21:48       ` Jonathan Wakely
2022-08-25 10:11         ` Patch ping (was Re: [PATCH] libstdc++: Clear padding bits in atomic compare_exchange) Jakub Jelinek
2022-09-01 22:57           ` Thomas Rodgers [this message]
2022-09-07 11:56             ` Jonathan Wakely
2022-09-07 22:06               ` Thomas Rodgers
2022-09-09 18:36               ` Rainer Orth
2022-09-09 18:46                 ` Iain Sandoe
2022-09-09 19:01                   ` Thomas Rodgers
2022-09-09 20:14                     ` Jonathan Wakely

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMmuTO9C9544kDQe=NuJOOrAJGzv8D24bYdMB-2MGV+syC+XJA@mail.gmail.com' \
    --to=trodgers@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jakub@redhat.com \
    --cc=jwakely@redhat.com \
    --cc=libstdc++@gcc.gnu.org \
    --cc=rodgert@twrodgers.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).