[PATCH 0/4] atomic: Friendlier assertions, avoidance of __sync

[PATCH 0/4] atomic: Friendlier assertions, avoidance of __sync

[Richard Henderson]

The current use of _Static_assert, via QEMU_BUILD_BUG_ON, requires
the user have #if conditionals to avoid the statement from appearing
in the preprocessed file at all.  Introduce a new primitive that
allows normal C conditionals and dead-code elimination.

Remove all use of __sync* builtins in favor of __atomic*.
We have required them since 47345e71247, last year, and
should have removed these at that point.  My bad.


r~


Richard Henderson (4):
  include/qemu/osdep: Add qemu_build_assert
  include/qemu/atomic: Use qemu_build_assert
  include/qemu/thread: Use qatomic_* functions
  include/qemu/atomic128: Avoid __sync_val_compare_and_swap_16

 include/qemu/atomic.h    | 16 ++++++++--------
 include/qemu/atomic128.h |  8 +-------
 include/qemu/osdep.h     |  8 ++++++++
 include/qemu/thread.h    |  8 ++++----
 meson.build              |  3 ++-
 5 files changed, 23 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH 1/4] include/qemu/osdep: Add qemu_build_assert

[Richard Henderson]

This differs from assert, in that with optimization enabled it
triggers at build-time.  It differs from QEMU_BUILD_BUG_ON,
aka _Static_assert, in that it is sensitive to control flow
and is subject to dead-code elimination.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/osdep.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index b1c161c035..2276094729 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -186,6 +186,14 @@ void QEMU_ERROR("code path is reachable")
 #define qemu_build_not_reached()  g_assert_not_reached()
 #endif
 
+/**
+ * qemu_build_assert()
+ *
+ * The compiler, during optimization, is expected to prove that the
+ * assertion is true.
+ */
+#define qemu_build_assert(test)  while (!(test)) qemu_build_not_reached()
+
 /*
  * According to waitpid man page:
  * WCOREDUMP
-- 
2.34.1

Re: [PATCH 1/4] include/qemu/osdep: Add qemu_build_assert

[Philippe Mathieu-Daudé]

On 25/10/22 01:24, Richard Henderson wrote:
> This differs from assert, in that with optimization enabled it
> triggers at build-time.  It differs from QEMU_BUILD_BUG_ON,
> aka _Static_assert, in that it is sensitive to control flow
> and is subject to dead-code elimination.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/qemu/osdep.h | 8 ++++++++
>   1 file changed, 8 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 2/4] include/qemu/atomic: Use qemu_build_assert

[Richard Henderson]

Change from QEMU_BUILD_BUG_ON, which requires ifdefs to avoid
problematic code, to qemu_build_assert, which can use C ifs.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/atomic.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 7e8fc8e7cd..874134fd19 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -133,7 +133,7 @@
 
 #define qatomic_read(ptr)                              \
     ({                                                 \
-    QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE); \
+    qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
     qatomic_read__nocheck(ptr);                        \
     })
 
@@ -141,7 +141,7 @@
     __atomic_store_n(ptr, i, __ATOMIC_RELAXED)
 
 #define qatomic_set(ptr, i)  do {                      \
-    QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE); \
+    qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
     qatomic_set__nocheck(ptr, i);                      \
 } while(0)
 
@@ -159,27 +159,27 @@
 
 #define qatomic_rcu_read(ptr)                          \
     ({                                                 \
-    QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE); \
+    qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
     typeof_strip_qual(*ptr) _val;                      \
     qatomic_rcu_read__nocheck(ptr, &_val);             \
     _val;                                              \
     })
 
 #define qatomic_rcu_set(ptr, i) do {                   \
-    QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE); \
+    qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
     __atomic_store_n(ptr, i, __ATOMIC_RELEASE);        \
 } while(0)
 
 #define qatomic_load_acquire(ptr)                       \
     ({                                                  \
-    QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE);  \
+    qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
     typeof_strip_qual(*ptr) _val;                       \
     __atomic_load(ptr, &_val, __ATOMIC_ACQUIRE);        \
     _val;                                               \
     })
 
 #define qatomic_store_release(ptr, i)  do {             \
-    QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE);  \
+    qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE); \
     __atomic_store_n(ptr, i, __ATOMIC_RELEASE);         \
 } while(0)
 
@@ -191,7 +191,7 @@
 })
 
 #define qatomic_xchg(ptr, i)    ({                          \
-    QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE);      \
+    qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE);     \
     qatomic_xchg__nocheck(ptr, i);                          \
 })
 
@@ -204,7 +204,7 @@
 })
 
 #define qatomic_cmpxchg(ptr, old, new)    ({                            \
-    QEMU_BUILD_BUG_ON(sizeof(*ptr) > ATOMIC_REG_SIZE);                  \
+    qemu_build_assert(sizeof(*ptr) <= ATOMIC_REG_SIZE);                 \
     qatomic_cmpxchg__nocheck(ptr, old, new);                            \
 })
 
-- 
2.34.1

Re: [PATCH 2/4] include/qemu/atomic: Use qemu_build_assert

[Philippe Mathieu-Daudé]

On 25/10/22 01:24, Richard Henderson wrote:
> Change from QEMU_BUILD_BUG_ON, which requires ifdefs to avoid
> problematic code, to qemu_build_assert, which can use C ifs.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/qemu/atomic.h | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 3/4] include/qemu/thread: Use qatomic_* functions

[Richard Henderson]

Use qatomic_*, which expands to __atomic_* in preference
to the "legacy" __sync_* functions.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/thread.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index af19f2b3fc..976e1ab995 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -227,7 +227,7 @@ struct QemuSpin {
 
 static inline void qemu_spin_init(QemuSpin *spin)
 {
-    __sync_lock_release(&spin->value);
+    qatomic_set(&spin->value, 0);
 #ifdef CONFIG_TSAN
     __tsan_mutex_create(spin, __tsan_mutex_not_static);
 #endif
@@ -246,7 +246,7 @@ static inline void qemu_spin_lock(QemuSpin *spin)
 #ifdef CONFIG_TSAN
     __tsan_mutex_pre_lock(spin, 0);
 #endif
-    while (unlikely(__sync_lock_test_and_set(&spin->value, true))) {
+    while (unlikely(qatomic_xchg(&spin->value, true))) {
         while (qatomic_read(&spin->value)) {
             cpu_relax();
         }
@@ -261,7 +261,7 @@ static inline bool qemu_spin_trylock(QemuSpin *spin)
 #ifdef CONFIG_TSAN
     __tsan_mutex_pre_lock(spin, __tsan_mutex_try_lock);
 #endif
-    bool busy = __sync_lock_test_and_set(&spin->value, true);
+    bool busy = qatomic_xchg(&spin->value, true);
 #ifdef CONFIG_TSAN
     unsigned flags = __tsan_mutex_try_lock;
     flags |= busy ? __tsan_mutex_try_lock_failed : 0;
@@ -280,7 +280,7 @@ static inline void qemu_spin_unlock(QemuSpin *spin)
 #ifdef CONFIG_TSAN
     __tsan_mutex_pre_unlock(spin, 0);
 #endif
-    __sync_lock_release(&spin->value);
+    qatomic_store_release(&spin->value, 0);
 #ifdef CONFIG_TSAN
     __tsan_mutex_post_unlock(spin, 0);
 #endif
-- 
2.34.1

Re: [PATCH 3/4] include/qemu/thread: Use qatomic_* functions

[Claudio Fontana]

On 10/25/22 01:24, Richard Henderson wrote:
> Use qatomic_*, which expands to __atomic_* in preference
> to the "legacy" __sync_* functions.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/qemu/thread.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> index af19f2b3fc..976e1ab995 100644
> --- a/include/qemu/thread.h
> +++ b/include/qemu/thread.h
> @@ -227,7 +227,7 @@ struct QemuSpin {
>  
>  static inline void qemu_spin_init(QemuSpin *spin)
>  {
> -    __sync_lock_release(&spin->value);
> +    qatomic_set(&spin->value, 0);

Here an integer literal is used, which makes sense, spin->value is int.

>  #ifdef CONFIG_TSAN
>      __tsan_mutex_create(spin, __tsan_mutex_not_static);
>  #endif
> @@ -246,7 +246,7 @@ static inline void qemu_spin_lock(QemuSpin *spin)
>  #ifdef CONFIG_TSAN
>      __tsan_mutex_pre_lock(spin, 0);
>  #endif
> -    while (unlikely(__sync_lock_test_and_set(&spin->value, true))) {
> +    while (unlikely(qatomic_xchg(&spin->value, true))) {

nit: here 'true' is used. Maybe "1" instead of "true" for consistency? 

>          while (qatomic_read(&spin->value)) {
>              cpu_relax();
>          }
> @@ -261,7 +261,7 @@ static inline bool qemu_spin_trylock(QemuSpin *spin)
>  #ifdef CONFIG_TSAN
>      __tsan_mutex_pre_lock(spin, __tsan_mutex_try_lock);
>  #endif
> -    bool busy = __sync_lock_test_and_set(&spin->value, true);
> +    bool busy = qatomic_xchg(&spin->value, true);
>  #ifdef CONFIG_TSAN
>      unsigned flags = __tsan_mutex_try_lock;
>      flags |= busy ? __tsan_mutex_try_lock_failed : 0;
> @@ -280,7 +280,7 @@ static inline void qemu_spin_unlock(QemuSpin *spin)
>  #ifdef CONFIG_TSAN
>      __tsan_mutex_pre_unlock(spin, 0);
>  #endif
> -    __sync_lock_release(&spin->value);
> +    qatomic_store_release(&spin->value, 0);
>  #ifdef CONFIG_TSAN
>      __tsan_mutex_post_unlock(spin, 0);
>  #endif

Thanks,

C

Re: [PATCH 3/4] include/qemu/thread: Use qatomic_* functions

[Richard Henderson]

On 10/25/22 23:47, Claudio Fontana wrote:
> On 10/25/22 01:24, Richard Henderson wrote:
>> Use qatomic_*, which expands to __atomic_* in preference
>> to the "legacy" __sync_* functions.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   include/qemu/thread.h | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
>> index af19f2b3fc..976e1ab995 100644
>> --- a/include/qemu/thread.h
>> +++ b/include/qemu/thread.h
>> @@ -227,7 +227,7 @@ struct QemuSpin {
>>   
>>   static inline void qemu_spin_init(QemuSpin *spin)
>>   {
>> -    __sync_lock_release(&spin->value);
>> +    qatomic_set(&spin->value, 0);
> 
> Here an integer literal is used, which makes sense, spin->value is int.
> 
>>   #ifdef CONFIG_TSAN
>>       __tsan_mutex_create(spin, __tsan_mutex_not_static);
>>   #endif
>> @@ -246,7 +246,7 @@ static inline void qemu_spin_lock(QemuSpin *spin)
>>   #ifdef CONFIG_TSAN
>>       __tsan_mutex_pre_lock(spin, 0);
>>   #endif
>> -    while (unlikely(__sync_lock_test_and_set(&spin->value, true))) {
>> +    while (unlikely(qatomic_xchg(&spin->value, true))) {
> 
> nit: here 'true' is used. Maybe "1" instead of "true" for consistency?

Fair enough.


r~

[PATCH 4/4] include/qemu/atomic128: Avoid __sync_val_compare_and_swap_16

[Richard Henderson]

Merge the CONFIG_ATOMIC128 and CONFIG_CMPXCHG128 cases
with respect to atomic16_cmpxchg and use
__atomic_compare_exchange_nomic (via qatomic_cmpxchg)
instead of the "legacy" __sync_val_compare_and_swap_16.

Update the meson has_cmpxchg128 test to match.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/atomic128.h | 8 +-------
 meson.build              | 3 ++-
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/include/qemu/atomic128.h b/include/qemu/atomic128.h
index adb9a1a260..ec45754515 100644
--- a/include/qemu/atomic128.h
+++ b/include/qemu/atomic128.h
@@ -41,18 +41,12 @@
  * Therefore, special case each platform.
  */
 
-#if defined(CONFIG_ATOMIC128)
+#if defined(CONFIG_ATOMIC128) || defined(CONFIG_CMPXCHG128)
 static inline Int128 atomic16_cmpxchg(Int128 *ptr, Int128 cmp, Int128 new)
 {
     return qatomic_cmpxchg__nocheck(ptr, cmp, new);
 }
 # define HAVE_CMPXCHG128 1
-#elif defined(CONFIG_CMPXCHG128)
-static inline Int128 atomic16_cmpxchg(Int128 *ptr, Int128 cmp, Int128 new)
-{
-    return __sync_val_compare_and_swap_16(ptr, cmp, new);
-}
-# define HAVE_CMPXCHG128 1
 #elif defined(__aarch64__)
 /* Through gcc 8, aarch64 has no support for 128-bit at all.  */
 static inline Int128 atomic16_cmpxchg(Int128 *ptr, Int128 cmp, Int128 new)
diff --git a/meson.build b/meson.build
index 1ec3f72edc..d8c4e76e7b 100644
--- a/meson.build
+++ b/meson.build
@@ -2224,7 +2224,8 @@ if has_int128
       int main(void)
       {
         unsigned __int128 x = 0, y = 0;
-        __sync_val_compare_and_swap_16(&x, y, x);
+        __atomic_compare_exchange_n(&x, &y, x, 0,
+            __ATOMIC_RELAXED, __ATOMIC_RELAXED);
         return 0;
       }
     ''')
-- 
2.34.1

Re: [PATCH 4/4] include/qemu/atomic128: Avoid __sync_val_compare_and_swap_16

[Richard Henderson]

On 10/25/22 09:24, Richard Henderson wrote:
> Merge the CONFIG_ATOMIC128 and CONFIG_CMPXCHG128 cases
> with respect to atomic16_cmpxchg and use
> __atomic_compare_exchange_nomic (via qatomic_cmpxchg)
> instead of the "legacy" __sync_val_compare_and_swap_16.
> 
> Update the meson has_cmpxchg128 test to match.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/qemu/atomic128.h | 8 +-------
>   meson.build              | 3 ++-
>   2 files changed, 3 insertions(+), 8 deletions(-)

Ho hum.  Must drop this one since for reasons that I cannot fathom, x86_64 does not 
implement the __atomic version.


r~

Re: [PATCH 0/4] atomic: Friendlier assertions, avoidance of __sync

[Paolo Bonzini]

On 10/25/22 01:24, Richard Henderson wrote:
> The current use of _Static_assert, via QEMU_BUILD_BUG_ON, requires
> the user have #if conditionals to avoid the statement from appearing
> in the preprocessed file at all.  Introduce a new primitive that
> allows normal C conditionals and dead-code elimination.
> 
> Remove all use of __sync* builtins in favor of __atomic*.
> We have required them since 47345e71247, last year, and
> should have removed these at that point.  My bad.
> 
> 
> r~
> 
> 
> Richard Henderson (4):
>    include/qemu/osdep: Add qemu_build_assert
>    include/qemu/atomic: Use qemu_build_assert
>    include/qemu/thread: Use qatomic_* functions
>    include/qemu/atomic128: Avoid __sync_val_compare_and_swap_16
> 
>   include/qemu/atomic.h    | 16 ++++++++--------
>   include/qemu/atomic128.h |  8 +-------
>   include/qemu/osdep.h     |  8 ++++++++
>   include/qemu/thread.h    |  8 ++++----
>   meson.build              |  3 ++-
>   5 files changed, 23 insertions(+), 20 deletions(-)
> 

For patches 1-3,

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [PATCH 0/4] atomic: Friendlier assertions, avoidance of __sync

[Richard Henderson]

On 10/25/22 22:43, Paolo Bonzini wrote:
> On 10/25/22 01:24, Richard Henderson wrote:
>> The current use of _Static_assert, via QEMU_BUILD_BUG_ON, requires
>> the user have #if conditionals to avoid the statement from appearing
>> in the preprocessed file at all.  Introduce a new primitive that
>> allows normal C conditionals and dead-code elimination.
>>
>> Remove all use of __sync* builtins in favor of __atomic*.
>> We have required them since 47345e71247, last year, and
>> should have removed these at that point.  My bad.
>>
>>
>> r~
>>
>>
>> Richard Henderson (4):
>>    include/qemu/osdep: Add qemu_build_assert
>>    include/qemu/atomic: Use qemu_build_assert
>>    include/qemu/thread: Use qatomic_* functions
>>    include/qemu/atomic128: Avoid __sync_val_compare_and_swap_16
>>
>>   include/qemu/atomic.h    | 16 ++++++++--------
>>   include/qemu/atomic128.h |  8 +-------
>>   include/qemu/osdep.h     |  8 ++++++++
>>   include/qemu/thread.h    |  8 ++++----
>>   meson.build              |  3 ++-
>>   5 files changed, 23 insertions(+), 20 deletions(-)
>>
> 
> For patches 1-3,
> 
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Queuing patches 1-3 to tcg-next.


r~