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

[Iain Sandoe <iain@sandoe.co.uk>]

> 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