[cfe-commits] C11 <stdatomic.h>

[Richard Smith]

On Sun, Oct 7, 2012 at 10:53 AM, Tijl Coosemans <tijl at coosemans.org> wrote:

> On 05-10-2012 20:36, Jeffrey Yasskin wrote:
> > On Fri, Oct 5, 2012 at 11:27 AM, Tijl Coosemans <tijl at coosemans.org>
> wrote:
> >> On 04-10-2012 23:04, Richard Smith wrote:
> >>> On Thu, Oct 4, 2012 at 12:23 PM, Richard Smith <richard at metafoo.co.uk>
> >>> wrote:
> >>>> On Thu, Oct 4, 2012 at 5:18 AM, Tijl Coosemans <tijl at coosemans.org>
> >>>> wrote:
> >>>>>>> The patch implements atomic_flag on top of atomic_bool, but that
> means
> >>>>>>> atomic_flag f = ATOMIC_FLAG_INIT is an atomic store.
> >>>>>>
> >>>>>> Not true. There is no need for the initialization of an
> >>>>>> _Atomic variable to use an atomic write, and the code Clang
> >>>>>> emits does not perform one.
> >>>>>
> >>>>> Ok, but reinitialisation like f = ATOMIC_FLAG_INIT then.
> >>>>
> >>>> As far as I can see, that is not a valid use of ATOMIC_FLAG_INIT.
> >>
> >> I think it's valid, because the other atomic types can be
> >> reinitialised using atomic_init and there's no such function
> >> for atomic_flag.
> >
> > That's a feature request for the C or C++ standard, not something
> > clang should implement on its own. Remember that Richard is
> > implementing a spec that's already written, not trying to invent what
> > might be useful.
>
> Maybe I shouldn't have used the word reinitialisation. It isn't
> something special. It's what you do when you need to reset some
> state to recover from an error, e.g. in a device driver if the
> device crashes you reset the device and reinitialise any state
> kept by the driver. For normal types you use simple assignment
> for that, for _Atomic types you can use atomic_init and for
> atomic_flag (which is not an atomic type) you should be able to
> assign ATOMIC_FLAG_INIT.
>

'should' here sounds like your own opinion. Can you point to somewhere in
the C11 standard which justifies this? Why not just use atomic_clear with
memory_order_relaxed?

Also, consider this (admittedly rather questionable) code:

atomic_flag a, b;
// ...
a = b;

With your approach, that will perform a non-atomic load and a non-atomic
store. Is that really what you'd want?

And this, assuming it even compiles:

// thread 1
a = ATOMIC_FLAG_INIT;

// thread 2
a = ATOMIC_FLAG_INIT;

With your approach, that code has a data race.


> >>>>> In any case you need to use char instead of bool, because the
> >>>>> flag can be shared with another piece of hardware that should be
> >>>>> allowed to store other values than 1. When testing whether the
> >>>>> flag is set the compiler should never assume it can simply
> >>>>> compare with 1.
> >>>>
> >>>> Can you cite a source for this claim? C11 7.17.8 says "It has two
> >>>> states, set and clear." It seems to me that if you want more than
> >>>> two states and you use atomic flag, you're doing it wrong; use an
> >>>> appropriate atomic type for the data you are representing.
> >>
> >> No, I meant for any non-zero value to be interpreted as set. The
> >> x86 asm for test-and-set is currently something like "movb $1,%al \
> >> xchgb %al,(%ecx) \ andb $1,%al". If you use char instead of bool that
> >> "andb $1,%al" becomes "testb %al,%al" which is less restrictive.
> >
> > If you want to interact with hardware using C++ atomic types, you're
> > going to need a spec from the hardware maker which clang (and other
> > compilers) could attempt to make implementable. Most likely, the
> > compiler makers would say to implement the hardware's requirements
> > using a non-flag atomic, but we can't be sure without seeing the
> > requirements. Can you link to such a spec, or are you just ...
> > speculating?
>
> Hmm, this is more about the atomic_flag ABI than compiler support
> for any special semantics. The ABI has the following components as
> far as I can see:
>
> a) sizeof atomic_flag
> b) value stored by ATOMIC_FLAG_INIT and atomic_flag_clear
> c) value stored by atomic_flag_test_and_set
> d) conversion of value loaded by atomic_flag_test_and_set to bool.
>
> The current implementation has:
>
> a) sizeof atomic_bool which is >= 1
> b) 0
> c) 1
> d) if 0 return false; if 1 return true; otherwise undefined
>
> This is only ABI compatible with itself. My suggestion would be:
>
> a) sizeof unsigned char which is == 1 in every C implementation
> b) 0
> c) 1
> d) if 0 return false; otherwise return true
>
> This allows for variation in c) which I 'speculate' is the point
> you'll see most variation across different implementations.
>

Your argument applies equally well to _Bool as it does to atomic_flag, but
_Bool only supports two values. Can you explain why you think this case is
different?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121007/40a38d89/attachment.html>