[PATCH for-4.9 v3 1/3] xsm: fix clang 3.5 build after c47d1d

[PATCH for-4.9 v3 1/3] xsm: fix clang 3.5 build after c47d1d

[Roger Pau Monne]

The changes introduced on c47d1d broke the clang build due to undefined
references to __xsm_action_mismatch_detected, because clang hasn't optimized
the code properly. The following patch allows the clang build to work again,
while keeping the same functionality.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Tamas K Lengyel <tamas.lengyel@zentific.com>
---
Changes since v2:
 - Use an "if" like v1.

Changes since v1:
 - Remove unused "break".
 - Remove if condition.

NB: this fixes travis build: https://travis-ci.org/royger/xen/builds/219697038
---
 xen/include/xsm/dummy.h | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 56a8814d82..62fcea6f04 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -557,25 +557,21 @@ static XSM_INLINE int xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d)
 
 static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, uint64_t mode, uint32_t op)
 {
-    xsm_default_t a;
     XSM_ASSERT_ACTION(XSM_OTHER);
 
     switch ( mode )
     {
     case XEN_ALTP2M_mixed:
-        a = XSM_TARGET;
-        break;
+        return xsm_default_action(XSM_TARGET, current->domain, d);
     case XEN_ALTP2M_external:
-        a = XSM_DM_PRIV;
-        break;
+        return xsm_default_action(XSM_DM_PRIV, current->domain, d);
     case XEN_ALTP2M_limited:
-        a = (HVMOP_altp2m_vcpu_enable_notify == op) ? XSM_TARGET : XSM_DM_PRIV;
-        break;
+        if ( HVMOP_altp2m_vcpu_enable_notify == op )
+            return xsm_default_action(XSM_TARGET, current->domain, d);
+        return xsm_default_action(XSM_DM_PRIV, current->domain, d);
     default:
         return -EPERM;
-    };
-
-    return xsm_default_action(a, current->domain, d);
+    }
 }
 
 static XSM_INLINE int xsm_vm_event_control(XSM_DEFAULT_ARG struct domain *d, int mode, int op)
-- 
2.11.0 (Apple Git-81)


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

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

[Roger Pau Monne]

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

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;                \
-- 
2.11.0 (Apple Git-81)


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

[PATCH for-4.9 v3 3/3] x86/atomic: fix cmpxchg16b inline assembly to work with clang

[Roger Pau Monne]

clang doesn't understand the "=A" register constrain when used with 64bits
assembly and spits out an internal error:

fatal error: error in backend: Cannot select: 0x7f9fb89c9390: i64 = build_pair 0x7f9fb89c92b0,
      0x7f9fb89c9320
  0x7f9fb89c92b0: i32,ch,glue = CopyFromReg 0x7f9fb89c9240, Register:i32 %EAX, 0x7f9fb89c9240:1
    0x7f9fb89c8c20: i32 = Register %EAX
    0x7f9fb89c9240: ch,glue = inlineasm 0x7f9fb89c90f0,
TargetExternalSymbol:i64'lock; cmpxchg16b $1', MDNode:ch<0x7f9fb8476c38>,
TargetConstant:i64<25>, TargetConstant:i32<18>, Register:i32 %EAX, Register:i32
%EDX, TargetConstant:i32<196622>, 0x7f9fb89c87c0, TargetConstant:i32<9>,
Register:i64 %RCX, TargetConstant:i32<9>, Register:i64 %RBX,
TargetConstant:i32<9>, Register:i64 %RDX, TargetConstant:i32<9>, Register:i64
%RAX, TargetConstant:i32<196622>, 0x7f9fb89c87c0, TargetConstant:i32<12>,
Register:i32 %EFLAGS, 0x7f9fb89c90f0:1
      0x7f9fb89c8a60: i64 = TargetExternalSymbol'lock; cmpxchg16b $1'
      0x7f9fb89c8b40: i64 = TargetConstant<25>
      0x7f9fb89c8bb0: i32 = TargetConstant<18>
      0x7f9fb89c8c20: i32 = Register %EAX
      0x7f9fb89c8c90: i32 = Register %EDX
      0x7f9fb89c8d00: i32 = TargetConstant<196622>
      0x7f9fb89c87c0: i64,ch = load<LD8[%4]> 0x7f9fb9053da0, FrameIndex:i64<1>, undef:i64
        0x7f9fb9053a90: i64 = FrameIndex<1>
        0x7f9fb9053e80: i64 = undef
      0x7f9fb89c8e50: i32 = TargetConstant<9>
      0x7f9fb89c8d70: i64 = Register %RCX
      0x7f9fb89c8e50: i32 = TargetConstant<9>
      0x7f9fb89c8ec0: i64 = Register %RBX
      0x7f9fb89c8e50: i32 = TargetConstant<9>
      0x7f9fb89c8fa0: i64 = Register %RDX
      0x7f9fb89c8e50: i32 = TargetConstant<9>
      0x7f9fb89c9080: i64 = Register %RAX
[...]

Fix this by specifying "rdx:rax" manually using the "d" and "a" constraints.

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:
 - New in this version.

---
NB: this is the only usage of "=A" on 64bit assembly in Xen. I will send a bug
report upstream to get this fixed, so that clang properly understands "=A" for
64bit assembly as "RDX:RAX", but in the meantime I would like to get this
patch accepted so the clang build can be functional again.

Upstream bug report can be found at: http://bugs.llvm.org/show_bug.cgi?id=32594
---
 xen/include/asm-x86/x86_64/system.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/include/asm-x86/x86_64/system.h b/xen/include/asm-x86/x86_64/system.h
index 7026c05a25..0a438cd9d1 100644
--- a/xen/include/asm-x86/x86_64/system.h
+++ b/xen/include/asm-x86/x86_64/system.h
@@ -14,20 +14,20 @@
  */
 
 static always_inline __uint128_t __cmpxchg16b(
-    volatile void *ptr, const __uint128_t *old, const __uint128_t *new)
+    volatile void *ptr, const __uint128_t *oldp, const __uint128_t *newp)
 {
-    __uint128_t prev;
-    uint64_t new_high = *new >> 64;
-    uint64_t new_low = *new;
+    union {
+        struct { uint64_t lo, hi; };
+        __uint128_t raw;
+    } new = { .raw = *newp }, old = { .raw = *oldp }, prev;
 
     ASSERT(cpu_has_cx16);
 
-    asm volatile ( "lock; cmpxchg16b %1"
-                   : "=A" (prev), "+m" (*__xg(ptr))
-                   : "c" (new_high), "b" (new_low),
-                     "0" (*old) );
+    asm volatile ( "lock; cmpxchg16b %2"
+                   : "=d" (prev.hi), "=a" (prev.lo), "+m" (*__xg(ptr))
+                   : "c" (new.hi), "b" (new.lo), "0" (old.hi), "1" (old.lo) );
 
-    return prev;
+    return prev.raw;
 }
 
 #define cmpxchg16b(ptr, o, n) ({                           \
-- 
2.11.0 (Apple Git-81)


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

Re: [PATCH for-4.9 v3 1/3] xsm: fix clang 3.5 build after c47d1d

[Daniel De Graaf]

On 04/10/2017 09:34 AM, Roger Pau Monne wrote:
> The changes introduced on c47d1d broke the clang build due to undefined
> references to __xsm_action_mismatch_detected, because clang hasn't optimized
> the code properly. The following patch allows the clang build to work again,
> while keeping the same functionality.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>

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

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

[Jan Beulich]

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


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

Re: [PATCH for-4.9 v3 3/3] x86/atomic: fix cmpxchg16b inline assembly to work with clang

[Jan Beulich]

>>> On 10.04.17 at 15:34, <roger.pau@citrix.com> wrote:
> clang doesn't understand the "=A" register constrain when used with 64bits
> assembly and spits out an internal error:
> 
> fatal error: error in backend: Cannot select: 0x7f9fb89c9390: i64 = 
> build_pair 0x7f9fb89c92b0,
>       0x7f9fb89c9320
>   0x7f9fb89c92b0: i32,ch,glue = CopyFromReg 0x7f9fb89c9240, Register:i32 
> %EAX, 0x7f9fb89c9240:1
>     0x7f9fb89c8c20: i32 = Register %EAX
>     0x7f9fb89c9240: ch,glue = inlineasm 0x7f9fb89c90f0,
> TargetExternalSymbol:i64'lock; cmpxchg16b $1', MDNode:ch<0x7f9fb8476c38>,
> TargetConstant:i64<25>, TargetConstant:i32<18>, Register:i32 %EAX, Register:i32
> %EDX, TargetConstant:i32<196622>, 0x7f9fb89c87c0, TargetConstant:i32<9>,
> Register:i64 %RCX, TargetConstant:i32<9>, Register:i64 %RBX,
> TargetConstant:i32<9>, Register:i64 %RDX, TargetConstant:i32<9>, Register:i64
> %RAX, TargetConstant:i32<196622>, 0x7f9fb89c87c0, TargetConstant:i32<12>,
> Register:i32 %EFLAGS, 0x7f9fb89c90f0:1
>       0x7f9fb89c8a60: i64 = TargetExternalSymbol'lock; cmpxchg16b $1'
>       0x7f9fb89c8b40: i64 = TargetConstant<25>
>       0x7f9fb89c8bb0: i32 = TargetConstant<18>
>       0x7f9fb89c8c20: i32 = Register %EAX
>       0x7f9fb89c8c90: i32 = Register %EDX
>       0x7f9fb89c8d00: i32 = TargetConstant<196622>
>       0x7f9fb89c87c0: i64,ch = load<LD8[%4]> 0x7f9fb9053da0, FrameIndex:i64<1>, 
> undef:i64
>         0x7f9fb9053a90: i64 = FrameIndex<1>
>         0x7f9fb9053e80: i64 = undef
>       0x7f9fb89c8e50: i32 = TargetConstant<9>
>       0x7f9fb89c8d70: i64 = Register %RCX
>       0x7f9fb89c8e50: i32 = TargetConstant<9>
>       0x7f9fb89c8ec0: i64 = Register %RBX
>       0x7f9fb89c8e50: i32 = TargetConstant<9>
>       0x7f9fb89c8fa0: i64 = Register %RDX
>       0x7f9fb89c8e50: i32 = TargetConstant<9>
>       0x7f9fb89c9080: i64 = Register %RAX
> [...]
> 
> Fix this by specifying "rdx:rax" manually using the "d" and "a" constraints.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Albeit it would have been a good idea to add a code comment, so
there won't be someone wanting to undo this as it can be done
with fewer operands (and hence in a more legible way).

Jan

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

Re: [PATCH for-4.9 v3 1/3] xsm: fix clang 3.5 build after c47d1d

[Julien Grall]

Hi Roger,

On 10/04/17 14:34, Roger Pau Monne wrote:
> The changes introduced on c47d1d broke the clang build due to undefined
> references to __xsm_action_mismatch_detected, because clang hasn't optimized
> the code properly. The following patch allows the clang build to work again,
> while keeping the same functionality.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Tested-by: Julien Grall <julien.grall@arm.com>

Can someone commit this patch today? I'd like to cut an RC as soon as 
osstest pushed to staging.

Cheers,

> ---
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Tamas K Lengyel <tamas.lengyel@zentific.com>
> ---
> Changes since v2:
>  - Use an "if" like v1.
>
> Changes since v1:
>  - Remove unused "break".
>  - Remove if condition.
>
> NB: this fixes travis build: https://travis-ci.org/royger/xen/builds/219697038
> ---
>  xen/include/xsm/dummy.h | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 56a8814d82..62fcea6f04 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -557,25 +557,21 @@ static XSM_INLINE int xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d)
>
>  static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, uint64_t mode, uint32_t op)
>  {
> -    xsm_default_t a;
>      XSM_ASSERT_ACTION(XSM_OTHER);
>
>      switch ( mode )
>      {
>      case XEN_ALTP2M_mixed:
> -        a = XSM_TARGET;
> -        break;
> +        return xsm_default_action(XSM_TARGET, current->domain, d);
>      case XEN_ALTP2M_external:
> -        a = XSM_DM_PRIV;
> -        break;
> +        return xsm_default_action(XSM_DM_PRIV, current->domain, d);
>      case XEN_ALTP2M_limited:
> -        a = (HVMOP_altp2m_vcpu_enable_notify == op) ? XSM_TARGET : XSM_DM_PRIV;
> -        break;
> +        if ( HVMOP_altp2m_vcpu_enable_notify == op )
> +            return xsm_default_action(XSM_TARGET, current->domain, d);
> +        return xsm_default_action(XSM_DM_PRIV, current->domain, d);
>      default:
>          return -EPERM;
> -    };
> -
> -    return xsm_default_action(a, current->domain, d);
> +    }
>  }
>
>  static XSM_INLINE int xsm_vm_event_control(XSM_DEFAULT_ARG struct domain *d, int mode, int op)
>

-- 
Julien Grall

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

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

[Roger Pau Monne]

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

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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 10.04.17 at 18:46, <roger.pau@citrix.com> wrote:
> 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.

Personally I like this much better as a workaround, provided the
omitted warning isn't just cosmetic (i.e. the generated code then
also matches gcc's rather than doing what the warning suggests
clang does). I do think, however, that the comment would better
go next to the #define.

Jan


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

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

[Roger Pau Monne]

On Tue, Apr 11, 2017 at 12:21:23AM -0600, Jan Beulich wrote:
> >>> On 10.04.17 at 18:46, <roger.pau@citrix.com> wrote:
> > 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.
> 
> Personally I like this much better as a workaround, provided the
> omitted warning isn't just cosmetic (i.e. the generated code then
> also matches gcc's rather than doing what the warning suggests
> clang does). I do think, however, that the comment would better
> go next to the #define.

Yes, the generated code matches gcc behavior. I've been told that what might
behave differently is something like:

switch ( foo )
{
case 1:
	while ( ({ if ( bar ) { x = 1; break; } x; }) )
	{
		...

But this is not the case clearly, and the clang warning is simply a bug.

OK, so let me move the comment with the defines and submit the patch properly,
thanks.

Roger.

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

Re: [PATCH for-4.9 v3 3/3] x86/atomic: fix cmpxchg16b inline assembly to work with clang

[Roger Pau Monne]

On Mon, Apr 10, 2017 at 02:34:35PM +0100, Roger Pau Monne wrote:
> clang doesn't understand the "=A" register constrain when used with 64bits
> assembly and spits out an internal error:
> 
> fatal error: error in backend: Cannot select: 0x7f9fb89c9390: i64 = build_pair 0x7f9fb89c92b0,
>       0x7f9fb89c9320
>   0x7f9fb89c92b0: i32,ch,glue = CopyFromReg 0x7f9fb89c9240, Register:i32 %EAX, 0x7f9fb89c9240:1
>     0x7f9fb89c8c20: i32 = Register %EAX
>     0x7f9fb89c9240: ch,glue = inlineasm 0x7f9fb89c90f0,
> TargetExternalSymbol:i64'lock; cmpxchg16b $1', MDNode:ch<0x7f9fb8476c38>,
> TargetConstant:i64<25>, TargetConstant:i32<18>, Register:i32 %EAX, Register:i32
> %EDX, TargetConstant:i32<196622>, 0x7f9fb89c87c0, TargetConstant:i32<9>,
> Register:i64 %RCX, TargetConstant:i32<9>, Register:i64 %RBX,
> TargetConstant:i32<9>, Register:i64 %RDX, TargetConstant:i32<9>, Register:i64
> %RAX, TargetConstant:i32<196622>, 0x7f9fb89c87c0, TargetConstant:i32<12>,
> Register:i32 %EFLAGS, 0x7f9fb89c90f0:1
>       0x7f9fb89c8a60: i64 = TargetExternalSymbol'lock; cmpxchg16b $1'
>       0x7f9fb89c8b40: i64 = TargetConstant<25>
>       0x7f9fb89c8bb0: i32 = TargetConstant<18>
>       0x7f9fb89c8c20: i32 = Register %EAX
>       0x7f9fb89c8c90: i32 = Register %EDX
>       0x7f9fb89c8d00: i32 = TargetConstant<196622>
>       0x7f9fb89c87c0: i64,ch = load<LD8[%4]> 0x7f9fb9053da0, FrameIndex:i64<1>, undef:i64
>         0x7f9fb9053a90: i64 = FrameIndex<1>
>         0x7f9fb9053e80: i64 = undef
>       0x7f9fb89c8e50: i32 = TargetConstant<9>
>       0x7f9fb89c8d70: i64 = Register %RCX
>       0x7f9fb89c8e50: i32 = TargetConstant<9>
>       0x7f9fb89c8ec0: i64 = Register %RBX
>       0x7f9fb89c8e50: i32 = TargetConstant<9>
>       0x7f9fb89c8fa0: i64 = Register %RDX
>       0x7f9fb89c8e50: i32 = TargetConstant<9>
>       0x7f9fb89c9080: i64 = Register %RAX
> [...]
> 
> Fix this by specifying "rdx:rax" manually using the "d" and "a" constraints.
> 
> 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:
>  - New in this version.
> 
> ---
> NB: this is the only usage of "=A" on 64bit assembly in Xen. I will send a bug
> report upstream to get this fixed, so that clang properly understands "=A" for
> 64bit assembly as "RDX:RAX", but in the meantime I would like to get this
> patch accepted so the clang build can be functional again.
> 
> Upstream bug report can be found at: http://bugs.llvm.org/show_bug.cgi?id=32594

And this has now been fixed upstream, for the record:

http://llvm.org/viewvc/llvm-project?view=revision&revision=300404

Roger.

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