Re: [RFA] choosing __platform_wait_t on targets without lock-free 64 atomics

[Jonathan Wakely <jwakely.gcc@gmail.com>]

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

On Thu, 29 Dec 2022, 15:30 Iain Sandoe, <iain@sandoe.co.uk> wrote:

> Hi Folks,
>
> > On 29 Dec 2022, at 12:09, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> > On Thu, 29 Dec 2022, 11:29 Iain Sandoe, <iain@sandoe.co.uk> wrote:
>
> >> The recent addition of the tz handling has pulled in a dependency on
> </bits/atomic_wait.h>
> >>
> >> This currently specifies __platform_wait_t as a 64bit quatity on
> platforms without _GLIBCXX_HAVE_LINUX_FUTEX.
> >>
> >> PowerPC does not have a 64b atomic without library support - so that
> this causes a bootstrap
> >> fail on powerpc-darwin (and I guess any other 32b powerpc non-futex
> target).
> >>
> >> Rather than contrive to build and add libatomic (which is not at
> present available at the point
> >> that libstdc++ is built), I wonder if there is any specific reason that
> __platform_wait_t needs
> >> to be 64 bits on these platforms? (Especially since the futex case uses
> an int.)
> >>
> > I think we do want the generic case's _M_wait atomic variable to be lock
> free, otherwise we use two locks for every operation, the one in libatomic
> and the waiter mutex. That's more important than it being any specific
> width.
>
> Definitely, that’s probably a recipe for some subtle race condition ..
> nested locks etc.
>

I didn't see any nested cases from a quick look, but it would still be
better to avoid two locks.


> >> Advice on the right way to fix this welcome — as a work-around to allow
> bootstrap to complete
> >> I applied the patch below - but that seems unlikely to be the right
> thing generically .
> >>
> > Rather than __lp64__ I think we should check the ATOMIC_LONG_LOCK_FREE
> macro and use long if it's lock free and int otherwise. But Tom needs to
> confirm that. That would be approximately the same as your patch in
> practice.
>
> OK.. that makes sense here’s a proposed patch (pending subsequent input
> from Tom).
>
> I am using “lock free always” as the criterion, “sometimes” does not seem
> useful here.
>

Agreed.


> Although we normally build libstdc++ with the just-built GCC...
> .. AFAIK the __SIZEOF_* are available from any version of GCC or clang
> that would
> be capable of building the sources.
>

Yep, but do we need the size checks at all?

I was thinking we could just use 'unsigned long' or 'unsigned int'
directly, instead of a uintN_t typedef. Using the typedef just seems to
complicate things.


> The contortion in the first #elif is attempting to cater for the
> (supposed, possible) case
> that there could be a 16b int target with a lock free 32b long (that might
> be a stretch
> so feel free to delete the second half).
>
> .. wider testing will follow, smoke tested only ...
>
> cheers
> Iain
>
> ========
>
> diff --git a/libstdc++-v3/include/bits/atomic_wait.h
> b/libstdc++-v3/include/bits/atomic_wait.h
> index bd1ed56..3ef0e92 100644
> --- a/libstdc++-v3/include/bits/atomic_wait.h
> +++ b/libstdc++-v3/include/bits/atomic_wait.h
> @@ -64,7 +64,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>  // and __platform_notify() if there is a more efficient primitive
> supported
>  // by the platform (e.g. __ulock_wait()/__ulock_wake()) which is better
> than
>  // a mutex/condvar based wait.
> +# if (__SIZEOF_LONG__ == 8 && ATOMIC_LONG_LOCK_FREE == 2) || \
> +     (__SIZEOF_LONG_LONG__ == 8 && ATOMIC_LLONG_LOCK_FREE == 2)
>      using __platform_wait_t = uint64_t;
> +# elif (__SIZEOF_INT__ == 4 && ATOMIC_INT_LOCK_FREE == 2) || \
> +       (__SIZEOF_LONG__ == 4 && ATOMIC_LONG_LOCK_FREE == 2)
> +    using __platform_wait_t = uint32_t;
> +# elif (__SIZEOF_INT__ == 2 && ATOMIC_INT_LOCK_FREE == 2)
> +    using __platform_wait_t = uint16_t;
> +# endif
>      inline constexpr size_t __platform_wait_alignment
>        = __alignof__(__platform_wait_t);
>  #endif
>
>