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. > 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. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
