On Wed, Oct 3, 2012 at 8:54 AM, Tijl Coosemans <span dir="ltr"><<a href="mailto:tijl@coosemans.org" target="_blank">tijl@coosemans.org</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">>> On Tue, Sep 18, 2012 at 6:47 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>>> On Mon, Sep 17, 2012 at 4:11 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>> wrote:<br>
>>>> On Sat, Sep 15, 2012 at 1:05 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>>>> wrote:<br>
>>>>> On Fri, Sep 14, 2012 at 8:17 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>><br>
>>>>> wrote:<br>
>>>>>> On Fri, Sep 14, 2012 at 7:48 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a><br>
>>>>>> wrote:<br>
</div><div><div class="h5">>>>>>>> The attached patch adds an implementation of<br>
>>>>>>> <stdatomic.h> to the set of headers provided by Clang.<br>
>>>>>>> Since this header is so compiler-dependent, it seems that<br>
>>>>>>> we are the most rational component to be providing this<br>
>>>>>>> header (even though, for instance, some flavors of BSD<br>
>>>>>>> already provide their own).<br>
>>>>>>> Please review!<br>
>>>>>><br>
>>>>>> +// Clang allows memory_order_consume ordering for __c11_atomic_store,<br>
>>>>>> +// even though C11 doesn't allow it for atomic_store.<br>
>>>>>><br>
>>>>>> That looks like a bug...<br>
>>>>><br>
>>>>> Possibly it's a bug in the specification for atomic_flag_clear?<br>
>>>>> memory_order_consume doesn't seem to have any meaning for a store<br>
>>>>> operation.<br>
>>>>><br>
>>>>>> Please put the new warning in a separate commit.<br>
>>>>><br>
>>>>> r163964.<br>
>>>>><br>
>>>>>> It looks like standard requires that we expose functions named<br>
>>>>>> atomic_thread_fence, atomic_signal_fence, atomic_flag_test_and_set,<br>
>>>>>> atomic_flag_test_and_set_explicit, and atomic_flag_clear; your version<br>
>>>>>> of stdatomic.h doesn't include declarations for these functions (which<br>
>>>>>> is required by C11 7.1.4p1).<br>
>>>>><br>
>>>>> Ugh. And C11 7.1.2/6 requires them to have external linkage.<br>
>>>>> I don't want these functions to require linking to a library.<br>
>>>>> We could emit them weak and inline, but then we'll get a weak<br>
>>>>> copy in every TU which includes this header, which seems<br>
>>>>> fairly egregious. Is there currently any way to emit a<br>
>>>>> function as linkonce_odr from C? Do you have any suggestions<br>
>>>>> as to how to proceed?<br>
>>>><br>
>>>> There isn't any way to get linkonce_odr from C at the moment; patches<br>
>>>> welcome.  I don't see any issues with that from the standpoint of the<br>
>>>> standard; I'm a little worried about ABI-compat issues, though.<br>
>>>> (Specifically, if the system provides the header, having our own<br>
>>>> linkonce_odr version could cause strange linker errors.)<br>
>>>><br>
>>>> We could put it into compiler-rt, and say that if someone tries to use<br>
>>>> the function instead of the macro without linking in compiler-rt,<br>
>>>> that's an error.  Not particularly satisfying either, but somewhat<br>
>>>> simpler.<br>
>>><br>
>>> After some discussion with Chandler, we think the best approach is to say<br>
>>> that the definition of these functions belongs in libc, and to provide only<br>
>>> declarations of them. A patch for that approach is attached.<br>
<br>
</div></div>If the compiler provides stdatomic.h but libc implements the functions<br>
then all compilers need to have a binary compatible definition of<br>
memory_order and atomic_flag types.<br></blockquote><div><br></div><div>[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).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The patch implements atomic_flag on top of atomic_bool, but that means<br>
atomic_flag f = ATOMIC_FLAG_INIT is an atomic store.</blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Also instead of including stdint.h and stddef.h, maybe you can use<br>
preprocessor macros like __INT8_TYPE__.<br>
</blockquote></div><div><br></div>Much of the work in stdint.h goes into defining the int_least*_t and int_fast*_t types correctly, and it doesn't make sense to duplicate that when defining atomic_int_least*_t etc. I don't think removing these dependencies is particularly worthwhile.