[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.9 v3 2/3] x86/atomic: fix clang build
>>> On 10.04.17 at 15:34, <roger.pau@xxxxxxxxxx> wrote: > --- a/xen/include/asm-x86/atomic.h > +++ b/xen/include/asm-x86/atomic.h > @@ -41,20 +41,42 @@ build_add_sized(add_u64_sized, "q", uint64_t, "ri") > #undef build_write_atomic > #undef build_add_sized > > -void __bad_atomic_size(void); > +/* > + * NB: read_atomic needs to be a static inline function because clang doesn't > + * like breaks inside of expressions, even when there's an inner switch where > + * those breaks should apply, and complains with "'break' is bound to loop, > GCC > + * binds it to switch", so the following code: > + * > + * while ( read_atomic(&foo) ) { ... } > + * > + * Doesn't work if read_atomic is a macro with an inner switch. > + */ > +static inline unsigned long readatomic(const void *p, size_t s) > +{ > + switch ( s ) > + { > + case 1: > + return read_u8_atomic((uint8_t *)p); > + case 2: > + return read_u16_atomic((uint16_t *)p); > + case 4: > + return read_u32_atomic((uint32_t *)p); > + case 8: > + return read_u64_atomic((uint64_t *)p); By going though void as the function's parameter type I don't think you need the bogus casts here anymore. > + default: > + ASSERT_UNREACHABLE(); > + return 0; > + } > +} > > -#define read_atomic(p) ({ \ > - unsigned long x_; \ > - switch ( sizeof(*(p)) ) { \ > - case 1: x_ = read_u8_atomic((uint8_t *)(p)); break; \ > - case 2: x_ = read_u16_atomic((uint16_t *)(p)); break; \ > - case 4: x_ = read_u32_atomic((uint32_t *)(p)); break; \ > - case 8: x_ = read_u64_atomic((uint64_t *)(p)); break; \ > - default: x_ = 0; __bad_atomic_size(); break; \ > - } \ > - (typeof(*(p)))x_; \ > +#define read_atomic(p) ({ \ > + BUILD_BUG_ON(sizeof(*(p)) != 1 && sizeof(*(p)) != 2 && \ > + sizeof(*(p)) != 4 && sizeof(*(p)) != 8); \ > + (typeof(*(p)))readatomic(p, sizeof(*(p))); \ > }) So did you take a look at whether / how much generated code changes? In any event, while this is better than dealing with it at the use site(s) of the macro, I still don't think this is really acceptable, mainly because it still doesn't scale: What if tomorrow I use write_atomic() in a context that clang doesn't like? And perhaps we have a few more such constructs, or may be gaining them at any time going forward. I'm honestly not convinced of the usefulness of keeping our code clang compliant, if they have such fundamental issues with the understanding of the language spec. Bottom line - I currently can't see myself ack-ing ugliness like this, but I also think I don't want to stand in the way of someone else (read: Andrew) doing so if this is really deemed an appropriate solution by everyone else. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |