Re: Deduplicating atomics written in terms of CAS

[Rich Felker <dalias@libc.org>]

On Sun, May 17, 2015 at 08:49:04AM +0200, Jens Gustedt wrote:
> 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 ?

Hm? I don't understand what you're asking.

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

These are misnomers. They're only used/needed as atomic bit-set and
bit-clear. It would be nice to eliminate them completely, but malloc
is using them right now. It would be easy to put the above logic
directly in malloc and have the bitmasks be kept as a union of
uint64_t and int[], but that's mildly ugly too I think.

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

The cast itself doesn't break aliasing rules. Only accessing the
memory as int does that. The intent was that a_or would only access
the object via asm, so the C type rules would not apply -- that's how
things originally worked when we only had i386 and x86_64. But now
that a_or is a C wrapper for a_cas on many/most archs, we do have an
aliasing problem, I think. That makes me more eagar to remove these.

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

I'm not so sure about that. See this question on SO, which has two
conflicting and both reasonable-sounding answers:

http://stackoverflow.com/questions/28654418/requirements-for-behavior-of-pointer-to-volatile-pointing-to-non-volatile-object

In any case, all objects used with atomics in musl are declared
volatile now, or that is the intent anyway. If I missed any please let
me know.

Rich