[cfe-commits] C11 <stdatomic.h>

[Tijl Coosemans]

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:
>>>>> On 03-10-2012 23:02, Richard Smith wrote:
>>>>>> On Wed, Oct 3, 2012 at 8:54 AM, Tijl Coosemans <tijl at coosemans.org>
>>>>>> wrote:
>>>>>>>>> On Tue, Sep 18, 2012 at 6:47 PM, Richard Smith
>>>>>>>>> <richard at metafoo.co.uk> wrote:
>>>>>>>>>> On Mon, Sep 17, 2012 at 4:11 PM, Eli Friedman
>>>>>>>>>> <eli.friedman at gmail.com> wrote:
>>>>>>>>>>> On Sat, Sep 15, 2012 at 1:05 AM, Richard Smith
>>>>>>>>>>> <richard at metafoo.co.uk> wrote:
>>>>>>>>>>>> On Fri, Sep 14, 2012 at 8:17 PM, Eli Friedman
>>>>>>>>>>>> <eli.friedman at gmail.com> wrote:
>>>>>>>>>>>>> It looks like standard requires that we expose
>>>>>>>>>>>>> functions named atomic_thread_fence,
>>>>>>>>>>>>> atomic_signal_fence, atomic_flag_test_and_set,
>>>>>>>>>>>>> atomic_flag_test_and_set_explicit, and
>>>>>>>>>>>>> atomic_flag_clear; your version of stdatomic.h
>>>>>>>>>>>>> doesn't include declarations for these functions
>>>>>>>>>>>>> (which is required by C11 7.1.4p1).
>>>>>>>>>>>>
>>>>>>>>>>>> Ugh. And C11 7.1.2/6 requires them to have external linkage.
>>>>>>>>>>>> I don't want these functions to require linking to a library.
>>>>>>>>>>>> We could emit them weak and inline, but then we'll get a weak
>>>>>>>>>>>> copy in every TU which includes this header, which seems
>>>>>>>>>>>> fairly egregious. Is there currently any way to emit a
>>>>>>>>>>>> function as linkonce_odr from C? Do you have any suggestions
>>>>>>>>>>>> as to how to proceed?
>>>>>>>>>>>
>>>>>>>>>>> There isn't any way to get linkonce_odr from C at the
>>>>>>>>>>> moment; patches welcome.  I don't see any issues with
>>>>>>>>>>> that from the standpoint of the standard; I'm a
>>>>>>>>>>> little worried about ABI-compat issues, though.
>>>>>>>>>>> (Specifically, if the system provides the header,
>>>>>>>>>>> having our own linkonce_odr version could cause
>>>>>>>>>>> strange linker errors.)
>>>>>>>>>>>
>>>>>>>>>>> We could put it into compiler-rt, and say that if
>>>>>>>>>>> someone tries to use the function instead of the
>>>>>>>>>>> macro without linking in compiler-rt, that's an
>>>>>>>>>>> error. Not particularly satisfying either, but
>>>>>>>>>>> somewhat simpler.
>>>>>>>>>>
>>>>>>>>>> After some discussion with Chandler, we think the best
>>>>>>>>>> approach is to say that the definition of these
>>>>>>>>>> functions belongs in libc, and to provide only
>>>>>>>>>> declarations of them. A patch for that approach is
>>>>>>>>>> attached.
>>>>>>>
>>>>>>> If the compiler provides stdatomic.h but libc implements the functions
>>>>>>> then all compilers need to have a binary compatible definition of
>>>>>>> memory_order and atomic_flag types.
>>>>>>
>>>>>> [On the assumption that these atomic operations, and
>>>>>> stdatomic.h itself, should be provided by the compiler rather
>>>>>> than by libc...] The definition of atomic_flag needs to be part
>>>>>> of the ABI anyway, if we want to have any hope of compiler
>>>>>> interoperability with (structs containing) atomic types, so I
>>>>>> view that part as a non-issue. The memory_order part is
>>>>>> unfortunate, though a libc could choose to ignore that
>>>>>> parameter and always treat it as seq_cst (since this only
>>>>>> affects situations where the macro definition isn't being used,
>>>>>> it should be very rare).
>>>>>>
>>>>>>> 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.

It's also valid to do:

atomic_flag f;
atomic_flag g = ATOMIC_FLAG_INIT;
f = g;

This is valid because atomic_flag is a structure type. If it were
meant to be invalid it would have been an array type (like jmp_buf).
There shouldn't be any atomic loads and stores here either because
atomic_flag is not an _Atomic type.

Another common practice is to declare variables at the start of a
function but initialise them later with simple assignment. There's
no reason for this practice to be invalid for atomic_flag.

I'm looking into implementing atomic_flag for FreeBSD's stdatomic.h
and I currently have:

typedef struct { volatile unsigned char __atomic_flag; } atomic_flag;
#define ATOMIC_FLAG_INIT ((atomic_flag){0})

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