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

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

[Iain Sandoe]

Hi,

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.)

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 .

thanks
Iain

----


diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h
index bd1ed56..2f67180 100644
--- a/libstdc++-v3/include/bits/atomic_wait.h
+++ b/libstdc++-v3/include/bits/atomic_wait.h
@@ -64,7 +64,11 @@ _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 __LP64__
     using __platform_wait_t = uint64_t;
+#else
+    using __platform_wait_t = uint32_t;
+#endif
     inline constexpr size_t __platform_wait_alignment
       = __alignof__(__platform_wait_t);

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

[Jonathan Wakely]

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

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

> Hi,
>
> 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.


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






> thanks
> Iain
>
> ----
>
>
> diff --git a/libstdc++-v3/include/bits/atomic_wait.h
> b/libstdc++-v3/include/bits/atomic_wait.h
> index bd1ed56..2f67180 100644
> --- a/libstdc++-v3/include/bits/atomic_wait.h
> +++ b/libstdc++-v3/include/bits/atomic_wait.h
> @@ -64,7 +64,11 @@ _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 __LP64__
>      using __platform_wait_t = uint64_t;
> +#else
> +    using __platform_wait_t = uint32_t;
> +#endif
>      inline constexpr size_t __platform_wait_alignment
>        = __alignof__(__platform_wait_t);
>
>

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

[Iain Sandoe]

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.

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

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.

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

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

[Jonathan Wakely]

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

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

[Iain Sandoe]

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

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

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

[Jonathan Wakely]

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

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

[Iain Sandoe]

> On 29 Dec 2022, at 17:02, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> 
> 
> 
> 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.

Great, thanks!
cheers
Iain

I’m using this locally in the meantime:

# if  ATOMIC_LONG_LOCK_FREE == 2
    using __platform_wait_t = long;
# elif ATOMIC_INT_LOCK_FREE == 2
    using __platform_wait_t = int;
# elif ATOMIC_SHORT_LOCK_FREE == 2
    using __platform_wait_t = short;
# elif ATOMIC_CHAR_LOCK_FREE == 2
    using __platform_wait_t = char;
# else
# fail No suitable lock-free type found.
# endif

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

[Thomas Rodgers]

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

__platform_wait_t should be whatever the platform supports lock free
natively. The use of a 64 bit int there in the fall through was copied from
Olivier's original implementation for libc++, which uses __ulock_wait/wake
on Darwin which takes a unit64_t, because I had intended to add support
Darwin, but haven't done so yet.

On Fri, Dec 30, 2022 at 2:51 AM Iain Sandoe <iain@sandoe.co.uk> wrote:

>
>
> > On 29 Dec 2022, at 17:02, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> >
> >
> >
> > 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.
>
> Great, thanks!
> cheers
> Iain
>
> I’m using this locally in the meantime:
>
> # if  ATOMIC_LONG_LOCK_FREE == 2
>     using __platform_wait_t = long;
> # elif ATOMIC_INT_LOCK_FREE == 2
>     using __platform_wait_t = int;
> # elif ATOMIC_SHORT_LOCK_FREE == 2
>     using __platform_wait_t = short;
> # elif ATOMIC_CHAR_LOCK_FREE == 2
>     using __platform_wait_t = char;
> # else
> # fail No suitable lock-free type found.
> # endif
>
>

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

[Iain Sandoe]

> On 2 Jan 2023, at 00:53, Thomas Rodgers <trodgers@redhat.com> wrote:
> 
> __platform_wait_t should be whatever the platform supports lock free natively. The use of a 64 bit int there in the fall through was copied from Olivier's original implementation for libc++, which uses __ulock_wait/wake on Darwin which takes a unit64_t, because I had intended to add support Darwin, but haven't done so yet.

In general, libc++ supports fewer versions of Darwin than GCC does (I don’t know right now which versions/archs we support have the __ulock_wait/wake).  Of course, I would very much like to see an efficient solution for Darwin - so please let me know/share patches with me when you get to it - I can test on older supported versions.

However, the issue here is not really Darwin-specific - it will effect any target that does not have either a futex or a 64b lock-free atomic.

thanks
Iain

> 
> On Fri, Dec 30, 2022 at 2:51 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
> 
> 
> > On 29 Dec 2022, at 17:02, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> > 
> > 
> > 
> > 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.
> 
> Great, thanks!
> cheers
> Iain
> 
> I’m using this locally in the meantime:
> 
> # if  ATOMIC_LONG_LOCK_FREE == 2
>     using __platform_wait_t = long;
> # elif ATOMIC_INT_LOCK_FREE == 2
>     using __platform_wait_t = int;
> # elif ATOMIC_SHORT_LOCK_FREE == 2
>     using __platform_wait_t = short;
> # elif ATOMIC_CHAR_LOCK_FREE == 2
>     using __platform_wait_t = char;
> # else
> # fail No suitable lock-free type found.
> # endif
> 

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

[Thomas Rodgers]

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

On Sun, Jan 1, 2023 at 11:47 PM Iain Sandoe <iain@sandoe.co.uk> wrote:

>
>
> > On 2 Jan 2023, at 00:53, Thomas Rodgers <trodgers@redhat.com> wrote:
> >
> > __platform_wait_t should be whatever the platform supports lock free
> natively. The use of a 64 bit int there in the fall through was copied from
> Olivier's original implementation for libc++, which uses __ulock_wait/wake
> on Darwin which takes a unit64_t, because I had intended to add support
> Darwin, but haven't done so yet.
>
> In general, libc++ supports fewer versions of Darwin than GCC does (I
> don’t know right now which versions/archs we support have the
> __ulock_wait/wake).  Of course, I would very much like to see an efficient
> solution for Darwin - so please let me know/share patches with me when you
> get to it - I can test on older supported versions.
>

I *think* it's supported for 10.12 onward, for anything older the
mutex/condvar implementation would have to be used.


>
> However, the issue here is not really Darwin-specific - it will effect any
> target that does not have either a futex or a 64b lock-free atomic.
>
>
I agree. I was just relating the back-story of why it was 64b in the
not-Linux case.


> thanks
> Iain
>
> >
> > On Fri, Dec 30, 2022 at 2:51 AM Iain Sandoe <iain@sandoe.co.uk> wrote:
> >
> >
> > > On 29 Dec 2022, at 17:02, Jonathan Wakely <jwakely.gcc@gmail.com>
> wrote:
> > >
> > >
> > >
> > > 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.
> >
> > Great, thanks!
> > cheers
> > Iain
> >
> > I’m using this locally in the meantime:
> >
> > # if  ATOMIC_LONG_LOCK_FREE == 2
> >     using __platform_wait_t = long;
> > # elif ATOMIC_INT_LOCK_FREE == 2
> >     using __platform_wait_t = int;
> > # elif ATOMIC_SHORT_LOCK_FREE == 2
> >     using __platform_wait_t = short;
> > # elif ATOMIC_CHAR_LOCK_FREE == 2
> >     using __platform_wait_t = char;
> > # else
> > # fail No suitable lock-free type found.
> > # endif
> >
>
>

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

[Jonathan Wakely]

How about this?

I don't think we should worry about targets without atomic int, so don't
bother using types smaller than int.


-- >8 --

For non-futex targets the __platform_wait_t type is currently uint64_t,
but that requires a lock in libatomic for some 32-bit targets. We don't
really need a 64-bit type, so use unsigned long if that is lock-free,
and int otherwise. This should mean it's lock-free on a wider set of
targets.

libstdc++-v3/ChangeLog:

	* include/bits/atomic_wait.h (__detail::__platform_wait_t):
	Define as unsigned long if always lock-free, and unsigned int
	otherwise.
---
 libstdc++-v3/include/bits/atomic_wait.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h
index bd1ed56d157..46f39f10cbc 100644
--- a/libstdc++-v3/include/bits/atomic_wait.h
+++ b/libstdc++-v3/include/bits/atomic_wait.h
@@ -64,7 +64,11 @@ _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.
-    using __platform_wait_t = uint64_t;
+# if  ATOMIC_LONG_LOCK_FREE == 2
+    using __platform_wait_t = unsigned long;
+# else
+    using __platform_wait_t = unsigned int;
+# endif
     inline constexpr size_t __platform_wait_alignment
       = __alignof__(__platform_wait_t);
 #endif
-- 
2.39.0

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

[Thomas Rodgers]

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

I agree with this change.

On Thu, Jan 5, 2023 at 4:22 PM Jonathan Wakely <jwakely@redhat.com> wrote:

> How about this?
>
> I don't think we should worry about targets without atomic int, so don't
> bother using types smaller than int.
>
>
> -- >8 --
>
> For non-futex targets the __platform_wait_t type is currently uint64_t,
> but that requires a lock in libatomic for some 32-bit targets. We don't
> really need a 64-bit type, so use unsigned long if that is lock-free,
> and int otherwise. This should mean it's lock-free on a wider set of
> targets.
>
> libstdc++-v3/ChangeLog:
>
>         * include/bits/atomic_wait.h (__detail::__platform_wait_t):
>         Define as unsigned long if always lock-free, and unsigned int
>         otherwise.
> ---
>  libstdc++-v3/include/bits/atomic_wait.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/libstdc++-v3/include/bits/atomic_wait.h
> b/libstdc++-v3/include/bits/atomic_wait.h
> index bd1ed56d157..46f39f10cbc 100644
> --- a/libstdc++-v3/include/bits/atomic_wait.h
> +++ b/libstdc++-v3/include/bits/atomic_wait.h
> @@ -64,7 +64,11 @@ _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.
> -    using __platform_wait_t = uint64_t;
> +# if  ATOMIC_LONG_LOCK_FREE == 2
> +    using __platform_wait_t = unsigned long;
> +# else
> +    using __platform_wait_t = unsigned int;
> +# endif
>      inline constexpr size_t __platform_wait_alignment
>        = __alignof__(__platform_wait_t);
>  #endif
> --
> 2.39.0
>
>

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

[Jonathan Wakely]

On Thu, 12 Jan 2023 at 01:27, Thomas Rodgers wrote:
>
> I agree with this change.

Thanks, pushed to trunk.

>
> On Thu, Jan 5, 2023 at 4:22 PM Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>> How about this?
>>
>> I don't think we should worry about targets without atomic int, so don't
>> bother using types smaller than int.
>>
>>
>> -- >8 --
>>
>> For non-futex targets the __platform_wait_t type is currently uint64_t,
>> but that requires a lock in libatomic for some 32-bit targets. We don't
>> really need a 64-bit type, so use unsigned long if that is lock-free,
>> and int otherwise. This should mean it's lock-free on a wider set of
>> targets.
>>
>> libstdc++-v3/ChangeLog:
>>
>>         * include/bits/atomic_wait.h (__detail::__platform_wait_t):
>>         Define as unsigned long if always lock-free, and unsigned int
>>         otherwise.
>> ---
>>  libstdc++-v3/include/bits/atomic_wait.h | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/libstdc++-v3/include/bits/atomic_wait.h b/libstdc++-v3/include/bits/atomic_wait.h
>> index bd1ed56d157..46f39f10cbc 100644
>> --- a/libstdc++-v3/include/bits/atomic_wait.h
>> +++ b/libstdc++-v3/include/bits/atomic_wait.h
>> @@ -64,7 +64,11 @@ _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.
>> -    using __platform_wait_t = uint64_t;
>> +# if  ATOMIC_LONG_LOCK_FREE == 2
>> +    using __platform_wait_t = unsigned long;
>> +# else
>> +    using __platform_wait_t = unsigned int;
>> +# endif
>>      inline constexpr size_t __platform_wait_alignment
>>        = __alignof__(__platform_wait_t);
>>  #endif
>> --
>> 2.39.0
>>