On Sun, Oct 7, 2012 at 10:53 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="HOEnZb"><div class="h5">On 05-10-2012 20:36, Jeffrey Yasskin wrote:<br>
> On Fri, Oct 5, 2012 at 11:27 AM, Tijl Coosemans <<a href="mailto:tijl@coosemans.org">tijl@coosemans.org</a>> wrote:<br>
>> On 04-10-2012 23:04, Richard Smith wrote:<br>
>>> On Thu, Oct 4, 2012 at 12:23 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>>> wrote:<br>
>>>> On Thu, Oct 4, 2012 at 5:18 AM, Tijl Coosemans <<a href="mailto:tijl@coosemans.org">tijl@coosemans.org</a>><br>
>>>> wrote:<br>>>>>>>> The patch implements atomic_flag on top of atomic_bool, but that means<br>
>>>>>>> atomic_flag f = ATOMIC_FLAG_INIT is an atomic store.<br>
>>>>>><br>
>>>>>> Not true. There is no need for the initialization of an<br>
>>>>>> _Atomic variable to use an atomic write, and the code Clang<br>
>>>>>> emits does not perform one.<br>
>>>>><br>
>>>>> Ok, but reinitialisation like f = ATOMIC_FLAG_INIT then.<br>
>>>><br>
>>>> As far as I can see, that is not a valid use of ATOMIC_FLAG_INIT.<br>
>><br>
>> I think it's valid, because the other atomic types can be<br>
>> reinitialised using atomic_init and there's no such function<br>
>> for atomic_flag.<br>
><br>
> That's a feature request for the C or C++ standard, not something<br>
> clang should implement on its own. Remember that Richard is<br>
> implementing a spec that's already written, not trying to invent what<br>
> might be useful.<br>
<br>
</div></div>Maybe I shouldn't have used the word reinitialisation. It isn't<br>
something special. It's what you do when you need to reset some<br>
state to recover from an error, e.g. in a device driver if the<br>
device crashes you reset the device and reinitialise any state<br>
kept by the driver. For normal types you use simple assignment<br>
for that, for _Atomic types you can use atomic_init and for<br>
atomic_flag (which is not an atomic type) you should be able to<br>
assign ATOMIC_FLAG_INIT.<br></blockquote><div><br></div><div>'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?</div>
<div><br></div><div>Also, consider this (admittedly rather questionable) code:</div><div><br></div><div>atomic_flag a, b;</div><div>// ...</div><div>a = b;</div><div><br></div><div>With your approach, that will perform a non-atomic load and a non-atomic store. Is that really what you'd want?</div>
<div><br></div><div>And this, assuming it even compiles:</div><div><br></div><div>// thread 1</div><div>a = ATOMIC_FLAG_INIT;</div><div><br></div><div>// thread 2</div><div>a = ATOMIC_FLAG_INIT;</div><div><br></div><div>With your approach, that code has a data race.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">>>>>> In any case you need to use char instead of bool, because the<br>
>>>>> flag can be shared with another piece of hardware that should be<br>
>>>>> allowed to store other values than 1. When testing whether the<br>
>>>>> flag is set the compiler should never assume it can simply<br>
>>>>> compare with 1.<br>
>>>><br>
>>>> Can you cite a source for this claim? C11 7.17.8 says "It has two<br>
>>>> states, set and clear." It seems to me that if you want more than<br>
>>>> two states and you use atomic flag, you're doing it wrong; use an<br>
>>>> appropriate atomic type for the data you are representing.<br>
>><br>
>> No, I meant for any non-zero value to be interpreted as set. The<br>
>> x86 asm for test-and-set is currently something like "movb $1,%al \<br>
>> xchgb %al,(%ecx) \ andb $1,%al". If you use char instead of bool that<br>
>> "andb $1,%al" becomes "testb %al,%al" which is less restrictive.<br>
><br>
> If you want to interact with hardware using C++ atomic types, you're<br>
> going to need a spec from the hardware maker which clang (and other<br>
> compilers) could attempt to make implementable. Most likely, the<br>
> compiler makers would say to implement the hardware's requirements<br>
> using a non-flag atomic, but we can't be sure without seeing the<br>
> requirements. Can you link to such a spec, or are you just ...<br>
> speculating?<br>
<br>
</div>Hmm, this is more about the atomic_flag ABI than compiler support<br>
for any special semantics. The ABI has the following components as<br>
far as I can see:<br>
<br>
a) sizeof atomic_flag<br>
b) value stored by ATOMIC_FLAG_INIT and atomic_flag_clear<br>
c) value stored by atomic_flag_test_and_set<br>
d) conversion of value loaded by atomic_flag_test_and_set to bool.<br>
<br>
The current implementation has:<br>
<br>
a) sizeof atomic_bool which is >= 1<br>
b) 0<br>
c) 1<br>
d) if 0 return false; if 1 return true; otherwise undefined<br>
<br>
This is only ABI compatible with itself. My suggestion would be:<br>
<br>
a) sizeof unsigned char which is == 1 in every C implementation<br>
b) 0<br>
c) 1<br>
d) if 0 return false; otherwise return true<br>
<br>
This allows for variation in c) which I 'speculate' is the point<br>
you'll see most variation across different implementations.<br>
</blockquote></div><br><div>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?</div>