On Fri, Aug 23, 2019 at 11:21:50AM -0500, Eric Blake wrote: >On 8/19/19 9:08 AM, Wei Yang wrote: >> On Mon, Aug 19, 2019 at 12:26:32PM +0100, Dr. David Alan Gilbert wrote: >>> * Wei Yang (richardw.y...@linux.intel.com) wrote: > >Typo in the subject line: migrtion should be migration > >>>> No functional change. Add default case to fix warning. >>> >>> I think the problem with this is that migrate_set_state uses an >>> atomic_cmpxchg and so we have to be careful that the type we use >>> is compatible with that. >>> MigrationStatus is an enum and I think compilers are allowed to >>> choose the types of that; so I'm not sure we're guaranteed >>> that an enum is always OK for the atomic_cmpxchg, and if it is >> >> Took a look into the definition of atomic_cmpxchg, which finally calls >> >> * __atomic_compare_exchange_n for c++11 >> * __sync_val_compare_and_swap > >Those are compiler-defined macros, so you have to consult the compiler >documentation to see if they state what happens when invoked on an enum >type. You also have to check whether our macro >typeof_strip_qual(enum_type) produces 'int' or something else. > >C99 doesn't specify _Atomic at all (which is why we handrolled our own >atomic.h built on top of compiler primitives, instead of using ><stdatomic.h>). But reading C11, I see that 6.7.2.4 states that >_Atomic(type) is okay except for: > >"The type name in an atomic type specifier shall not refer to an array >type, a function type, an atomic type, or a qualified type." > >which does NOT preclude the use of _Atomic(enum_type), so presumably >compilers have to be prepared to handle an atomic enum type. Still, >it's rather shaky ground if you can't prove compilers handle it correctly. >
Sounds this is a dark area for all those compilers. I would keep the code untouched now. Thanks > >> >> Both of them take two pointers to compare and exchange its content. >> >> Per C99 standard, http://www.open-std.org/JTC1/SC22/WG14/www/docs/n1256.pdf, >> it mentioned: >> >> Each enumerated type shall be compatible with char, a signed integer type, >> or an unsigned integer type. The choice of type is implementation-defined, >> but shall be capable of representing the values of all the members of the >> enumeration. >> >> Based on this, I think atomic_cmpxchg should work fine with enum. > >What C99 says is rather weak; you really want to be basing your >decisions on atomics based on C11 or later. > > >-- >Eric Blake, Principal Software Engineer >Red Hat, Inc. +1-919-301-3226 >Virtualization: qemu.org | libvirt.org > -- Wei Yang Help you, Help me