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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Mon, Apr 10, 2017 at 09:19:52AM -0600, Jan Beulich wrote:
> >>> 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?

No, I haven't looked.

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

Well, I think clang has proven useful in the past for detecting issues that gcc
didn't catch. Those should all be considered bugs, and IMHO clang is quite good
at solving them. I never had issues sending bug reports upstream, and getting
them fixed. I cannot sadly say the same about gcc.

In any case, I don't think it's reasonable to expect no bugs (like Xen also has
bugs). I understand your reluctance to merge this because it pollutes the code
just to fix a bug that's not even ours, but I don't see any other way to solve
this.

I have a (I think) less intrusive fix, which relies on using _Pragma, pasted
below. Let me know what you think, and I can formally submit it.

---8<---
diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h
index 2fbe705518..d24e30c3df 100644
--- a/xen/include/asm-x86/atomic.h
+++ b/xen/include/asm-x86/atomic.h
@@ -43,8 +43,23 @@ build_add_sized(add_u64_sized, "q", uint64_t, "ri")
 
 void __bad_atomic_size(void);
 
+/*
+ * NB: we need to disable the gcc-compat warnings for clang in read_atomic or
+ * else it will complain with: "'break' is bound to loop, GCC binds it to
+ * switch" when read_atomic is used inside of a while expression inside of a
+ * switch statement, ie:
+ *
+ * switch (...)
+ * {
+ * case ...:
+ *      while ( read_atomic(&foo) ) { ... }
+ *
+ * This has already been reported upstream:
+ * http://bugs.llvm.org/show_bug.cgi?id=32595
+ */
 #define read_atomic(p) ({                                 \
     unsigned long x_;                                     \
+    CLANG_DISABLE_WARN_GCC_COMPAT_START                   \
     switch ( sizeof(*(p)) ) {                             \
     case 1: x_ = read_u8_atomic((uint8_t *)(p)); break;   \
     case 2: x_ = read_u16_atomic((uint16_t *)(p)); break; \
@@ -52,6 +67,7 @@ void __bad_atomic_size(void);
     case 8: x_ = read_u64_atomic((uint64_t *)(p)); break; \
     default: x_ = 0; __bad_atomic_size(); break;          \
     }                                                     \
+    CLANG_DISABLE_WARN_GCC_COMPAT_END                     \
     (typeof(*(p)))x_;                                     \
 })
 
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 16aeeea7f1..569dcb70d6 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -100,4 +100,15 @@
 # define ASM_FLAG_OUT(yes, no) no
 #endif
 
+#ifdef __clang__
+# define CLANG_DISABLE_WARN_GCC_COMPAT_START                    \
+    _Pragma("clang diagnostic push")                            \
+    _Pragma("clang diagnostic ignored \"-Wgcc-compat\"")
+# define CLANG_DISABLE_WARN_GCC_COMPAT_END                      \
+    _Pragma("clang diagnostic pop")
+#else
+# define CLANG_DISABLE_WARN_GCC_COMPAT_START
+# define CLANG_DISABLE_WARN_GCC_COMPAT_END
+#endif
+
 #endif /* __LINUX_COMPILER_H */


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel