[cfe-commits] C11 <stdatomic.h>

[Jeffrey Yasskin]

On Sun, Oct 7, 2012 at 3:42 PM, Tijl Coosemans <tijl at coosemans.org> wrote:
> On 07-10-2012 20:53, Richard Smith wrote:
>> 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?
>
> Well you should be able to do it because there's no alternative.
> atomic_clear performs an atomic operation and initialisation
> shouldn't require atomicity.
>
> Perhaps a better example is:
>
> atomic_flag *f = malloc(sizeof(*f));
> *f = ATOMIC_FLAG_INIT;
>
> If assigning ATOMIC_FLAG_INIT isn't valid you cannot initialise this
> flag at all.

7.17.8 (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf)
says, "An atomic_flag that is not explicitly initialized with
ATOMIC_FLAG_INIT is initially in an indeterminate state." That is,
it's either set or clear, not undefined, so you can put it into a
known state by calling atomic_flag_clear().

That does mean that atomic_flag needs to be known to the compiler
since it's the only type (or one of very few) that doesn't cause
undefined behavior when it's uninitialized.

>> 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?
>
> Yes, 6.2.5§20 defines atomic types as those designated by
> _Atomic(typename). atomic_flag isn't like that. None of the
> operator overloading for _Atomic types applies to it.
>
>> 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.
>
> Yes, just like atomic_init for _Atomic types.
>
>>>>>>>> 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?
>
> _Bool is defined as a type large enough to store the values 0 and 1,
> so it's more clearly defined. It makes sense to store 0 and 1 in
> memory as 0 and 1. The clear and set states of atomic_flag aren't
> defined at all. Using 0 and 1 makes most sense though and it's
> quite likely that most implementations will do that, so my point d)
> is just a suggestion. I think it keeps some options open at
> virtually no cost though.
>
> Also, as far as I'm aware _Bool is rarely used for a variable stored
> in memory (except in bitfields) and it's rarely used for communication
> between two processes so its binary encoding is less important.
> atomic_flag on the other hand is always in memory and always used for
> communication between two processes.

atomic_flag can be used for two threads within the same process, which
doesn't need any special support for ABI compatibility.