[cfe-commits] C11 <stdatomic.h>

[Jeffrey Yasskin]

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.

> On the other hand, storing a single byte is always atomic so
> atomic_flag_clear_explicit(&f,memory_order_relaxed) is exactly
> the same as f = ATOMIC_FLAG_INIT, so you might be right.

It's likely that you won't see any difference in performance or
possibly even the instructions emitted between
atomic_store_explicit(&an_atomic_char, 0, memory_order_relaxed) and
atomic_init(&an_atomic_char, 0), yes. The semantics are somewhat
different since atomic_init() can cause data races, while
atomic_store_explicit cannot. Any requests for new features in this
area should come with benchmarks.

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

Jeffrey