Re: [Xen-devel] [PATCH for-4.9 v3 2/3] x86/atomic: fix clang build

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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