[cfe-commits] C11 <stdatomic.h>

[Tijl Coosemans]

On 08-10-2012 01:34, Jeffrey Yasskin wrote:
> 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.

Indeterminate means set, clear or trap representation.

3.19.2 indeterminate value: either unspecified or a trap representation
3.19.3 unspecified value: any valid value may be chosen in every instance
3.19.4 trap representation: a value that need not be valid for this type
3.19.5 perform a trap: interrupt execution of program such that no further
operations are performed. Note that fetching a trap representation might
perform a trap but is not required to.

So the question is if atomic_flag_clear is guaranteed to work with a
flag in an invalid state. I think hardware support for this type is
allowed to assume the flag is in a valid state before any atomic
operations are used on it. But even if it does work, initialisation
doesn't require atomicity and shouldn't for performance reasons.

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

A program might link with libraries compiled by different compilers,
so ABI does matter even in this case. There are other cases though:

7.17.8 §2 operations on atomic_flag are lock-free
7.17.5 §2 lock-free implies address-free: atomic operations on the same
memory location via different addresses communicate atomically. This
restriction enables communication via memory mapped into a process more
than once and memory shared between two processes.

So atomic_flag may also be shared by two threads in different processes.

(And the standard doesn't really talk about this, but there are also
heterogeneous systems where one thread can run on a CPU and another on
a GPU or other types of processors.)

Anyway I don't find this particular point important enough any more to
keep discussing it. Just make sure your implementations of
atomic_flag_test_and_set and atomic_flag_clear are compatible with gcc
built-ins __atomic_test_and_set and __atomic_clear.



I found an old C++ example implementation using __sync built-ins:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2427.html

It implements atomic_flag like this:

typedef struct { bool __f__; } atomic_flag;
#define ATOMIC_FLAG_INIT { false }

I also found a document about uninitialised atomics:
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1482.htm (issue 3)

Summarised: There are 3 types of variables in C: static, auto and
dynamically allocated. This requires two mechanisms to initialise
atomic variables: one for static variables (ATOMIC_VAR_INIT) and
one for dynamically allocated variables (atomic_init). Auto variables
can use both.

The document doesn't talk about atomic_flag, but the same reasoning
applies to it. There must be a way to initialise a dynamically
allocated atomic_flag. With the above example implementation you can
do:

atomic_flag *f = malloc(sizeof(*f));
*f = (atomic_flag)ATOMIC_FLAG_INIT;

But it would be nice if the cast was part of the definition of
ATOMIC_FLAG_INIT.

7.17.8 §4 ATOMIC_FLAG may be used to initialise an atomic_flag.

*An* atomic_flag includes a dynamically allocated one.