Re: [PATCH 1/2] Optimize generic spinlock code and use C11 like atomic macros.

[Torvald Riegel <triegel@redhat.com>]

On Sun, 2017-02-19 at 10:20 +0100, Florian Weimer wrote:
> * Torvald Riegel:
> 
> >> On 12/16/2016 09:12 PM, Florian Weimer wrote:
> >> > That's undefined:
> >> >
> >> > “If an attempt is made to refer to an object defined with a
> >> > volatile-qualified type through use of an lvalue with
> >> > non-volatile-qualified type, the behavior is undefined.”
> >> >
> >> > But we cannot drop the volatile qualifier from the definition of
> >> > pthread_spinlock_t because it would change the C++ mangling of
> >> > pthread_spinlock_t * and similar types.
> >
> > Generally, I wouldn't agree with Florian's comment.  However, all we are
> > interested in is the storage behind the volatile-qualified type.  Users
> > aren't allowed the object through other means than the pthread_spin*
> > functions, so if we cast everywhere, we'll never have any
> > volatile-qualified accesses.
> 
> The spinlock is defined with the volatile qualifier.  This means that
> accesses without the qualifier are undefined.

The footnote for 6.7.3p6 (footnote 133, N1570) reads:
This applies to those objects that behave as if they were defined with
qualified types, even if they are
never actually defined as objects in the program (such as an object at a
memory-mapped input/output
address).

I don't remember whether footnotes are normative, but this doesn't apply
in our case.

> > I believe none of the architectures we
> > support makes weaker requirements on alignment for volatile-qualified
> > than for non-volatile-qualified types, so I can't see any problem in
> > practice with the cast.
> 
> We also need separate compilation or some other optimization barrier.

Also consider 2.14.2 in
https://www.cl.cam.ac.uk/~pes20/cerberus/notes30-full.pdf

This states that one can cast between a union and the pointers to
individual parts of a union.  If the spinlock's underlying type and the
volatile-qualified type both have the same size+alignment, then the
union will have the same size and alignment too.
In our virtual union, we only ever use the non-volatile member (users
are not allowed to acccess spinlocks other than through pthread_spin_*
functions, which don't use volatile-qualified accesses).  But we pass
the pointer to the volatile member around as handle to the spinlock.

Thus, I'm not concerned about the cast from volatile-qualified to
non-volatile-qualified in this particular case.

Maybe its best to just change pthread_spinlock_t from volatile int to
int.  The generic spinlock uses only atomic ops with Stefan's changes,
so that's okay for at least the archs that use the generic code; for all
others, we could either keep them as is or change it and confirm that
the custom code they use doesn't rely on the volatile.