[2/5] Atomic __platform_wait: accept any 32-bit type, not just int

The kernel doesn't care what we store in those 32 bits, only that they are
comparable. This commit adds:
 * pointers and long on 32-bit architectures
 * unsigned
 * untyped enums and typed enums on int & unsigned int
 * float

We're not using FUTEX_OP anywhere today. The kernel reserves 4 bits for
this field but has only used 6 values so far, so it can be extended to
unsigned compares in the future, if needed.
---
 libstdc++-v3/include/bits/atomic_wait.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

On 26/02/21 07:59 -0800, Thiago Macieira via Libstdc++ wrote:
>The kernel doesn't care what we store in those 32 bits, only that they are
>comparable. This commit adds:
> * pointers and long on 32-bit architectures
> * unsigned
> * untyped enums and typed enums on int & unsigned int
> * float
>
>We're not using FUTEX_OP anywhere today. The kernel reserves 4 bits for
>this field but has only used 6 values so far, so it can be extended to
>unsigned compares in the future, if needed.

And this one for GCC 11 too.

> libstdc++-v3/include/bits/atomic_wait.h | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h
>index 424fccbe4c5..1c6bda2e2b6 100644
>--- a/libstdc++-v3/include/bits/atomic_wait.h
>+++ b/libstdc++-v3/include/bits/atomic_wait.h
>@@ -65,7 +65,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     template<typename _Tp>
>       inline constexpr bool __platform_wait_uses_type
> #ifdef _GLIBCXX_HAVE_LINUX_FUTEX
>-	= is_same_v<remove_cv_t<_Tp>, __platform_wait_t>;
>+	= is_scalar_v<remove_cv_t<_Tp>> && sizeof(_Tp) == sizeof(__platform_wait_t)
>+	  && alignof(_Tp) >= alignof(__platform_wait_t);
> #else
> 	= false;
> #endif
>@@ -91,13 +92,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
>     template<typename _Tp>
>       void
>-      __platform_wait(const _Tp* __addr, __platform_wait_t __val) noexcept
>+      __platform_wait(const _Tp* __addr, _Tp __val) noexcept
>       {
> 	for(;;)
> 	  {
> 	    auto __e = syscall (SYS_futex, static_cast<const void*>(__addr),
> 				  static_cast<int>(__futex_wait_flags::__wait_private),
>-				    __val, nullptr);
>+				  static_cast<__platform_wait_t>(__val), nullptr);
> 	    if (!__e || errno == EAGAIN)
> 	      break;
> 	    else if (errno != EINTR)
>-- 
>2.30.1
>

On 03/03/21 14:34 +0000, Jonathan Wakely wrote:
>On 26/02/21 07:59 -0800, Thiago Macieira via Libstdc++ wrote:
>>The kernel doesn't care what we store in those 32 bits, only that they are
>>comparable. This commit adds:
>>* pointers and long on 32-bit architectures
>>* unsigned
>>* untyped enums and typed enums on int & unsigned int
>>* float
>>
>>We're not using FUTEX_OP anywhere today. The kernel reserves 4 bits for
>>this field but has only used 6 values so far, so it can be extended to
>>unsigned compares in the future, if needed.
>
>And this one for GCC 11 too.
>
>>libstdc++-v3/include/bits/atomic_wait.h | 7 ++++---
>>1 file changed, 4 insertions(+), 3 deletions(-)
>>
>>diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h
>>index 424fccbe4c5..1c6bda2e2b6 100644
>>--- a/libstdc++-v3/include/bits/atomic_wait.h
>>+++ b/libstdc++-v3/include/bits/atomic_wait.h
>>@@ -65,7 +65,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>    template<typename _Tp>
>>      inline constexpr bool __platform_wait_uses_type
>>#ifdef _GLIBCXX_HAVE_LINUX_FUTEX
>>-	= is_same_v<remove_cv_t<_Tp>, __platform_wait_t>;
>>+	= is_scalar_v<remove_cv_t<_Tp>> && sizeof(_Tp) == sizeof(__platform_wait_t)

Oh, except that is_scalar is surprisingly expensive to instantiate
(its defined in a really expensive way) and since we control all uses
of this constant, I don't think we need it. It's only ever used from
atomic waiting functions which are only defined for scalar types.

>>+	  && alignof(_Tp) >= alignof(__platform_wait_t);
>>#else
>>	= false;
>>#endif
>>@@ -91,13 +92,13 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>>
>>    template<typename _Tp>
>>      void
>>-      __platform_wait(const _Tp* __addr, __platform_wait_t __val) noexcept
>>+      __platform_wait(const _Tp* __addr, _Tp __val) noexcept
>>      {
>>	for(;;)
>>	  {
>>	    auto __e = syscall (SYS_futex, static_cast<const void*>(__addr),
>>				  static_cast<int>(__futex_wait_flags::__wait_private),
>>-				    __val, nullptr);
>>+				  static_cast<__platform_wait_t>(__val), nullptr);
>>	    if (!__e || errno == EAGAIN)
>>	      break;
>>	    else if (errno != EINTR)
>>-- 
>>2.30.1
>>

On Wednesday, 3 March 2021 08:21:51 PST Jonathan Wakely wrote:
> >>-       = is_same_v<remove_cv_t<_Tp>, __platform_wait_t>;
> >>+       = is_scalar_v<remove_cv_t<_Tp>> && sizeof(_Tp) ==
> >>sizeof(__platform_wait_t)
> Oh, except that is_scalar is surprisingly expensive to instantiate
> (its defined in a really expensive way) and since we control all uses
> of this constant, I don't think we need it. It's only ever used from
> atomic waiting functions which are only defined for scalar types.

Thanks. Will update and re-submit.

On Wed, 3 Mar 2021 at 19:25, Jonathan Wakely via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
> Oh, except that is_scalar is surprisingly expensive to instantiate
> (its defined in a really expensive way) and since we control all uses

I'll be more than happy to write you an __is_scalar for GCC 12. :P

On 03/03/21 19:34 +0200, Ville Voutilainen wrote:
>On Wed, 3 Mar 2021 at 19:25, Jonathan Wakely via Libstdc++
><libstdc++@gcc.gnu.org> wrote:
>> Oh, except that is_scalar is surprisingly expensive to instantiate
>> (its defined in a really expensive way) and since we control all uses
>
>I'll be more than happy to write you an __is_scalar for GCC 12. :P

That would help with https://gcc.gnu.org/PR96710 too. The current
implementation is inefficient and arguably wrong.