On Sun, Oct 7, 2012 at 3:42 PM, Tijl Coosemans <[email protected]> wrote: > On 07-10-2012 20:53, Richard Smith wrote: >> On Sun, Oct 7, 2012 at 10:53 AM, Tijl Coosemans <[email protected]> wrote: >> >>> On 05-10-2012 20:36, Jeffrey Yasskin wrote: >>>> On Fri, Oct 5, 2012 at 11:27 AM, Tijl Coosemans <[email protected]> >>>> wrote: >>>>> On 04-10-2012 23:04, Richard Smith wrote: >>>>>> On Thu, Oct 4, 2012 at 12:23 PM, Richard Smith <[email protected]> >>>>>> wrote: >>>>>>> On Thu, Oct 4, 2012 at 5:18 AM, Tijl Coosemans <[email protected]> >>>>>>> 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. >> 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. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
