diff mbox

[for-4.9,v3,2/3] x86/atomic: fix clang build

Message ID 20170410133435.51728-2-roger.pau@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

Roger Pau Monné April 10, 2017, 1:34 p.m. UTC
The current code in dm_op breaks clang build with:

dm.c:411:21: error: 'break' is bound to loop, GCC binds it to switch [-Werror,-Wgcc-compat]
            while ( read_atomic(&p2m->ioreq.entry_count) &&
                    ^
xen/include/asm/atomic.h:53:43: note: expanded from macro 'read_atomic'
    default: x_ = 0; __bad_atomic_size(); break;          \
                                          ^

Introduce a readatomic static inline helper function in order to solve this.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
 - Introduce a readatomic static inline helper in order to prevent moving the
   read_atomic check.

Changes since v1:
 - New in this version.
---
 xen/include/asm-x86/atomic.h | 44 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 11 deletions(-)

Comments

Jan Beulich April 10, 2017, 3:19 p.m. UTC | #1
>>> On 10.04.17 at 15:34, <roger.pau@citrix.com> 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
diff mbox

Patch

diff --git a/xen/include/asm-x86/atomic.h b/xen/include/asm-x86/atomic.h
index 2fbe705518..ae87a0dcf5 100644
--- 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);
+    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)));             \
 })
 
+void __bad_atomic_size(void);
+
 #define write_atomic(p, x) ({                             \
     typeof(*(p)) __x = (x);                               \
     unsigned long x_ = (unsigned long)__x;                \