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: 4766 bytes --]

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

>
>
> > On 29 Dec 2022, at 15:44, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> >
> >
> >
> > 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.
>
> That’s fine by me - I was just copying what was there :)
>
> In this patch I made it so that a target without a ‘suitable' lock-free
> size would fail to
> compile the source, which seems better than a link fail later — I could
> make it more
> specific (e.g. # fail clause) or we could test for smaller lock-free
> entities…
>

I think we can just eschew atomics altogether in that case, and just use
the mutex for all accesses. I can do that after the break when I'm back
online.



> Iain
>
> >
> > 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
>
>