From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp001-out.apm-internet.net (smtp001-out.apm-internet.net [85.119.248.222]) by sourceware.org (Postfix) with ESMTPS id A8FE33858D20 for ; Thu, 29 Dec 2022 15:56:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A8FE33858D20 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=sandoe.co.uk Authentication-Results: sourceware.org; spf=none smtp.mailfrom=sandoe.co.uk Received: (qmail 60083 invoked from network); 29 Dec 2022 15:56:12 -0000 X-APM-Out-ID: 16723293726008 X-APM-Authkey: 257869/1(257869/1) 8 Received: from unknown (HELO smtpclient.apple) (81.138.1.83) by smtp001.apm-internet.net with SMTP; 29 Dec 2022 15:56:12 -0000 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.120.41.1.1\)) Subject: Re: [RFA] choosing __platform_wait_t on targets without lock-free 64 atomics From: Iain Sandoe In-Reply-To: Date: Thu, 29 Dec 2022 15:56:11 +0000 Cc: libstdc++ , Thomas Rodgers Content-Transfer-Encoding: quoted-printable Message-Id: References: <6640F26B-F267-40E0-9223-2F0F45462176@sandoe.co.uk> <05A9C1EF-7EC9-4276-BA11-8922A66B3D3A@sandoe.co.uk> To: Jonathan Wakely X-Mailer: Apple Mail (2.3696.120.41.1.1) X-Spam-Status: No, score=-14.1 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_COUK,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: > On 29 Dec 2022, at 15:44, Jonathan Wakely = wrote: >=20 >=20 >=20 > On Thu, 29 Dec 2022, 15:30 Iain Sandoe, wrote: > Hi Folks, >=20 > > On 29 Dec 2022, at 12:09, Jonathan Wakely = wrote: >=20 > > On Thu, 29 Dec 2022, 11:29 Iain Sandoe, wrote: >=20 > >> The recent addition of the tz handling has pulled in a dependency = on > >>=20 > >> This currently specifies __platform_wait_t as a 64bit quatity on = platforms without _GLIBCXX_HAVE_LINUX_FUTEX. > >>=20 > >> 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). > >>=20 > >> 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.) > >>=20 > > 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. >=20 > Definitely, that=E2=80=99s probably a recipe for some subtle race = condition .. nested locks etc. >=20 > I didn't see any nested cases from a quick look, but it would still be = better to avoid two locks. >=20 >=20 > >> Advice on the right way to fix this welcome =E2=80=94 as a = work-around to allow bootstrap to complete > >> I applied the patch below - but that seems unlikely to be the right = thing generically . > >>=20 > > 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. >=20 > OK.. that makes sense here=E2=80=99s a proposed patch (pending = subsequent input from Tom). >=20 > I am using =E2=80=9Clock free always=E2=80=9D as the criterion, = =E2=80=9Csometimes=E2=80=9D does not seem useful here. >=20 > Agreed. >=20 >=20 > 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. >=20 > Yep, but do we need the size checks at all? >=20 > 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=E2=80=99s fine by me - I was just copying what was there :) In this patch I made it so that a target without a =E2=80=98suitable' = lock-free size would fail to compile the source, which seems better than a link fail later =E2=80=94 = I could make it more specific (e.g. # fail clause) or we could test for smaller lock-free = entities=E2=80=A6 Iain >=20 > 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). >=20 > .. wider testing will follow, smoke tested only ... >=20 > cheers > Iain >=20 > =3D=3D=3D=3D=3D=3D=3D=3D >=20 > 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__ =3D=3D 8 && ATOMIC_LONG_LOCK_FREE =3D=3D 2) || = \ > + (__SIZEOF_LONG_LONG__ =3D=3D 8 && ATOMIC_LLONG_LOCK_FREE =3D=3D = 2) > using __platform_wait_t =3D uint64_t; > +# elif (__SIZEOF_INT__ =3D=3D 4 && ATOMIC_INT_LOCK_FREE =3D=3D 2) || = \ > + (__SIZEOF_LONG__ =3D=3D 4 && ATOMIC_LONG_LOCK_FREE =3D=3D 2) > + using __platform_wait_t =3D uint32_t; > +# elif (__SIZEOF_INT__ =3D=3D 2 && ATOMIC_INT_LOCK_FREE =3D=3D 2) > + using __platform_wait_t =3D uint16_t; > +# endif > inline constexpr size_t __platform_wait_alignment > =3D __alignof__(__platform_wait_t); > #endif