[cfe-commits] C11 <stdatomic.h>

[Tijl Coosemans]

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.

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