[PATCH] host/include/generic/host/atomic128: Fix compilation problem with Clang 17

[PATCH] host/include/generic/host/atomic128: Fix compilation problem with Clang 17

[Thomas Huth]

When compiling QEMU with Clang 17 on a s390x, the compilation fails:

In file included from ../accel/tcg/cputlb.c:32:
In file included from /root/qemu/include/exec/helper-proto-common.h:10:
In file included from /root/qemu/include/qemu/atomic128.h:62:
/root/qemu/host/include/generic/host/atomic128-ldst.h:68:15: error:
 __sync builtin operation MUST have natural alignment (consider using __
atomic). [-Werror,-Wsync-alignment]
   68 |     } while (!__sync_bool_compare_and_swap_16(ptr_align, old, new.i));
      |               ^
In file included from ../accel/tcg/cputlb.c:32:
In file included from /root/qemu/include/exec/helper-proto-common.h:10:
In file included from /root/qemu/include/qemu/atomic128.h:61:
/root/qemu/host/include/generic/host/atomic128-cas.h:36:11: error:
 __sync builtin operation MUST have natural alignment (consider using __a
tomic). [-Werror,-Wsync-alignment]
   36 |     r.i = __sync_val_compare_and_swap_16(ptr_align, c.i, n.i);
      |           ^
2 errors generated.

It's arguably a bug in Clang since we already use __builtin_assume_aligned()
to tell the compiler that the pointer is properly aligned. But according to
https://github.com/llvm/llvm-project/issues/69146 it seems like the Clang
folks don't see an easy fix on their side and recommend to use a type
declared with __attribute__((aligned(16))) to work around this problem.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1934
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 host/include/generic/host/atomic128-cas.h  | 2 +-
 host/include/generic/host/atomic128-ldst.h | 2 +-
 include/qemu/int128.h                      | 1 +
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/host/include/generic/host/atomic128-cas.h b/host/include/generic/host/atomic128-cas.h
index 991d3da082..6b40cc2271 100644
--- a/host/include/generic/host/atomic128-cas.h
+++ b/host/include/generic/host/atomic128-cas.h
@@ -28,7 +28,7 @@ atomic16_cmpxchg(Int128 *ptr, Int128 cmp, Int128 new)
 static inline Int128 ATTRIBUTE_ATOMIC128_OPT
 atomic16_cmpxchg(Int128 *ptr, Int128 cmp, Int128 new)
 {
-    __int128_t *ptr_align = __builtin_assume_aligned(ptr, 16);
+    Int128Aligned *ptr_align = __builtin_assume_aligned(ptr, 16);
     Int128Alias r, c, n;
 
     c.s = cmp;
diff --git a/host/include/generic/host/atomic128-ldst.h b/host/include/generic/host/atomic128-ldst.h
index 80fff0643a..691e6a8531 100644
--- a/host/include/generic/host/atomic128-ldst.h
+++ b/host/include/generic/host/atomic128-ldst.h
@@ -58,7 +58,7 @@ atomic16_read_rw(Int128 *ptr)
 static inline void ATTRIBUTE_ATOMIC128_OPT
 atomic16_set(Int128 *ptr, Int128 val)
 {
-    __int128_t *ptr_align = __builtin_assume_aligned(ptr, 16);
+    Int128Aligned *ptr_align = __builtin_assume_aligned(ptr, 16);
     __int128_t old;
     Int128Alias new;
 
diff --git a/include/qemu/int128.h b/include/qemu/int128.h
index 73624e8be7..44530d3e10 100644
--- a/include/qemu/int128.h
+++ b/include/qemu/int128.h
@@ -10,6 +10,7 @@
  */
 #if defined(CONFIG_INT128) && !defined(CONFIG_TCG_INTERPRETER)
 typedef __int128_t Int128;
+typedef __int128_t __attribute__((aligned(16))) Int128Aligned;
 
 static inline Int128 int128_make64(uint64_t a)
 {
-- 
2.41.0

Re: [PATCH] host/include/generic/host/atomic128: Fix compilation problem with Clang 17

[Philippe Mathieu-Daudé]

On 8/11/23 09:59, Thomas Huth wrote:
> When compiling QEMU with Clang 17 on a s390x, the compilation fails:
> 
> In file included from ../accel/tcg/cputlb.c:32:
> In file included from /root/qemu/include/exec/helper-proto-common.h:10:
> In file included from /root/qemu/include/qemu/atomic128.h:62:
> /root/qemu/host/include/generic/host/atomic128-ldst.h:68:15: error:
>   __sync builtin operation MUST have natural alignment (consider using __
> atomic). [-Werror,-Wsync-alignment]
>     68 |     } while (!__sync_bool_compare_and_swap_16(ptr_align, old, new.i));
>        |               ^
> In file included from ../accel/tcg/cputlb.c:32:
> In file included from /root/qemu/include/exec/helper-proto-common.h:10:
> In file included from /root/qemu/include/qemu/atomic128.h:61:
> /root/qemu/host/include/generic/host/atomic128-cas.h:36:11: error:
>   __sync builtin operation MUST have natural alignment (consider using __a
> tomic). [-Werror,-Wsync-alignment]
>     36 |     r.i = __sync_val_compare_and_swap_16(ptr_align, c.i, n.i);
>        |           ^
> 2 errors generated.
> 
> It's arguably a bug in Clang since we already use __builtin_assume_aligned()
> to tell the compiler that the pointer is properly aligned. But according to
> https://github.com/llvm/llvm-project/issues/69146 it seems like the Clang
> folks don't see an easy fix on their side and recommend to use a type
> declared with __attribute__((aligned(16))) to work around this problem.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1934
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   host/include/generic/host/atomic128-cas.h  | 2 +-
>   host/include/generic/host/atomic128-ldst.h | 2 +-
>   include/qemu/int128.h                      | 1 +
>   3 files changed, 3 insertions(+), 2 deletions(-)

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

Re: [PATCH] host/include/generic/host/atomic128: Fix compilation problem with Clang 17

[Richard Henderson]

On 11/8/23 00:59, Thomas Huth wrote:
> When compiling QEMU with Clang 17 on a s390x, the compilation fails:
> 
> In file included from ../accel/tcg/cputlb.c:32:
> In file included from /root/qemu/include/exec/helper-proto-common.h:10:
> In file included from /root/qemu/include/qemu/atomic128.h:62:
> /root/qemu/host/include/generic/host/atomic128-ldst.h:68:15: error:
>   __sync builtin operation MUST have natural alignment (consider using __
> atomic). [-Werror,-Wsync-alignment]
>     68 |     } while (!__sync_bool_compare_and_swap_16(ptr_align, old, new.i));
>        |               ^
> In file included from ../accel/tcg/cputlb.c:32:
> In file included from /root/qemu/include/exec/helper-proto-common.h:10:
> In file included from /root/qemu/include/qemu/atomic128.h:61:
> /root/qemu/host/include/generic/host/atomic128-cas.h:36:11: error:
>   __sync builtin operation MUST have natural alignment (consider using __a
> tomic). [-Werror,-Wsync-alignment]
>     36 |     r.i = __sync_val_compare_and_swap_16(ptr_align, c.i, n.i);
>        |           ^
> 2 errors generated.
> 
> It's arguably a bug in Clang since we already use __builtin_assume_aligned()
> to tell the compiler that the pointer is properly aligned. But according to
> https://github.com/llvm/llvm-project/issues/69146 it seems like the Clang
> folks don't see an easy fix on their side and recommend to use a type
> declared with __attribute__((aligned(16))) to work around this problem.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1934
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   host/include/generic/host/atomic128-cas.h  | 2 +-
>   host/include/generic/host/atomic128-ldst.h | 2 +-
>   include/qemu/int128.h                      | 1 +
>   3 files changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH] host/include/generic/host/atomic128: Fix compilation problem with Clang 17

[Thomas Huth]

On 08/11/2023 18.57, Richard Henderson wrote:
> On 11/8/23 00:59, Thomas Huth wrote:
>> When compiling QEMU with Clang 17 on a s390x, the compilation fails:
>>
>> In file included from ../accel/tcg/cputlb.c:32:
>> In file included from /root/qemu/include/exec/helper-proto-common.h:10:
>> In file included from /root/qemu/include/qemu/atomic128.h:62:
>> /root/qemu/host/include/generic/host/atomic128-ldst.h:68:15: error:
>>   __sync builtin operation MUST have natural alignment (consider using __
>> atomic). [-Werror,-Wsync-alignment]
>>     68 |     } while (!__sync_bool_compare_and_swap_16(ptr_align, old, 
>> new.i));
>>        |               ^
>> In file included from ../accel/tcg/cputlb.c:32:
>> In file included from /root/qemu/include/exec/helper-proto-common.h:10:
>> In file included from /root/qemu/include/qemu/atomic128.h:61:
>> /root/qemu/host/include/generic/host/atomic128-cas.h:36:11: error:
>>   __sync builtin operation MUST have natural alignment (consider using __a
>> tomic). [-Werror,-Wsync-alignment]
>>     36 |     r.i = __sync_val_compare_and_swap_16(ptr_align, c.i, n.i);
>>        |           ^
>> 2 errors generated.
>>
>> It's arguably a bug in Clang since we already use __builtin_assume_aligned()
>> to tell the compiler that the pointer is properly aligned. But according to
>> https://github.com/llvm/llvm-project/issues/69146 it seems like the Clang
>> folks don't see an easy fix on their side and recommend to use a type
>> declared with __attribute__((aligned(16))) to work around this problem.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1934
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   host/include/generic/host/atomic128-cas.h  | 2 +-
>>   host/include/generic/host/atomic128-ldst.h | 2 +-
>>   include/qemu/int128.h                      | 1 +
>>   3 files changed, 3 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

I just noticed that this new type needs to be declared for the #else part, 
too, otherwise the compilation breaks with --enable-tcg-interpreter :

diff --git a/include/qemu/int128.h b/include/qemu/int128.h
--- a/include/qemu/int128.h
+++ b/include/qemu/int128.h
@@ -225,6 +225,7 @@ static inline Int128 int128_rems(Int128 a, Int128 b)
  #else /* !CONFIG_INT128 */

  typedef struct Int128 Int128;
+typedef struct Int128 __attribute__((aligned(16))) Int128Aligned;

  /*
   * We guarantee that the in-memory byte representation of an

I'll add that when picking up the patch.

  Thomas

Re: [PATCH] host/include/generic/host/atomic128: Fix compilation problem with Clang 17

[Philippe Mathieu-Daudé]

On 13/11/23 11:34, Thomas Huth wrote:
> On 08/11/2023 18.57, Richard Henderson wrote:
>> On 11/8/23 00:59, Thomas Huth wrote:
>>> When compiling QEMU with Clang 17 on a s390x, the compilation fails:
>>>
>>> In file included from ../accel/tcg/cputlb.c:32:
>>> In file included from /root/qemu/include/exec/helper-proto-common.h:10:
>>> In file included from /root/qemu/include/qemu/atomic128.h:62:
>>> /root/qemu/host/include/generic/host/atomic128-ldst.h:68:15: error:
>>>   __sync builtin operation MUST have natural alignment (consider 
>>> using __
>>> atomic). [-Werror,-Wsync-alignment]
>>>     68 |     } while (!__sync_bool_compare_and_swap_16(ptr_align, 
>>> old, new.i));
>>>        |               ^
>>> In file included from ../accel/tcg/cputlb.c:32:
>>> In file included from /root/qemu/include/exec/helper-proto-common.h:10:
>>> In file included from /root/qemu/include/qemu/atomic128.h:61:
>>> /root/qemu/host/include/generic/host/atomic128-cas.h:36:11: error:
>>>   __sync builtin operation MUST have natural alignment (consider 
>>> using __a
>>> tomic). [-Werror,-Wsync-alignment]
>>>     36 |     r.i = __sync_val_compare_and_swap_16(ptr_align, c.i, n.i);
>>>        |           ^
>>> 2 errors generated.
>>>
>>> It's arguably a bug in Clang since we already use 
>>> __builtin_assume_aligned()
>>> to tell the compiler that the pointer is properly aligned. But 
>>> according to
>>> https://github.com/llvm/llvm-project/issues/69146 it seems like the 
>>> Clang
>>> folks don't see an easy fix on their side and recommend to use a type
>>> declared with __attribute__((aligned(16))) to work around this problem.
>>>
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1934
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   host/include/generic/host/atomic128-cas.h  | 2 +-
>>>   host/include/generic/host/atomic128-ldst.h | 2 +-
>>>   include/qemu/int128.h                      | 1 +
>>>   3 files changed, 3 insertions(+), 2 deletions(-)
>>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> I just noticed that this new type needs to be declared for the #else 
> part, too, otherwise the compilation breaks with --enable-tcg-interpreter :
> 
> diff --git a/include/qemu/int128.h b/include/qemu/int128.h
> --- a/include/qemu/int128.h
> +++ b/include/qemu/int128.h
> @@ -225,6 +225,7 @@ static inline Int128 int128_rems(Int128 a, Int128 b)
>   #else /* !CONFIG_INT128 */
> 
>   typedef struct Int128 Int128;
> +typedef struct Int128 __attribute__((aligned(16))) Int128Aligned;

Indeed.

> 
>   /*
>    * We guarantee that the in-memory byte representation of an
> 
> I'll add that when picking up the patch.

LGTM.