[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1431845344.11628.1.camel@inria.fr>
Date: Sun, 17 May 2015 08:49:04 +0200
From: Jens Gustedt <jens.gustedt@...ia.fr>
To: musl@...ts.openwall.com
Subject: Re: Deduplicating atomics written in terms of CAS
Hello,
Am Sonntag, den 17.05.2015, 00:55 -0400 schrieb Rich Felker:
> Lots of archs define most or all of their atomics except a_cas in
> terms of a_cas. The attached atomic.h is a proposed replacement for
> arch-specific atomic.h that would live in src/internal. The arch
> atomic.h files would be replaced with atomic_arch.h, which could opt
> to define nothing but a_cas, or could define more primitives itself if
> it can do so more efficiently.
I like the approach
> The second attachment, atomic_generic.h, is an implementation of the
> atomics (and other non-atomic ops we've traditionally had in atomic.h)
> using GNU C builtins. This file can be used as-is for any new archs
> that satisfy the following conditions:
>
> - They're not supported by compilers too old to have the __sync_*
> builtins.
>
> - They don't need runtime switching/detection of atomic
> implementations.
>
> - GCC doesn't generate pathologically bad code for the builtins.
shouldn't this file then define or macros such as a_swap, too ?
On quick inspection I found issues with the two 64 bit functions:
#ifndef a_and_64
static inline void a_and_64(volatile uint64_t *p, uint64_t v)
{
union { uint64_t v; uint32_t r[2]; } u = { v };
if (u.r[0]+1) a_and((int *)p, u.r[0]);
if (u.r[1]+1) a_and((int *)p+1, u.r[1]);
}
#endif
#ifndef a_or_64
static inline void a_or_64(volatile uint64_t *p, uint64_t v)
{
union { uint64_t v; uint32_t r[2]; } u = { v };
if (u.r[0]) a_or((int *)p, u.r[0]);
if (u.r[1]) a_or((int *)p+1, u.r[1]);
}
#endif
First I don't get it how we can expect these to be be atomic. It looks
to me that the two 32 bit words can be updated with quite a laps of
time between them if the thread is delayed. I didn't check this, do we
really need 64bit atomics?
Then, the mix of uint32_t and int is unfortunate. This code is in
header files and thus visible to all compilation units, especially
user code that might use any optimization option that the compiler
offers. The cast to int* breaks aliasing rules, so compilers that are
used with aggressive optimization may produce wrong executables, in
pretending that *p didn't change.
I only recently learned that even cast to volatile doesn't help in
cases where the original object to which p points is not declared
volatile. The C standard states that only volatile *declared* objects
are subject to the rules of volatile. Accessing through a volatile
pointer doesn't help.
Jens
--
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536 ::
:: :::::::::::::::::::::: gsm France : +33 651400183 ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::
Download attachment "signature.asc" of type "application/pgp-signature" (182 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
