* [PATCH 0/3] Atomic cleanup + clang-12 build fix
@ 2021-07-12 15:59 Richard Henderson
2021-07-12 15:59 ` [PATCH 1/3] qemu/atomic: Remove pre-C11 atomic fallbacks Richard Henderson
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Richard Henderson @ 2021-07-12 15:59 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, pbonzini, crobinso
The first two patches are not strictly required, but they
were useful in tracking down the root problem here.
I understand the logic behind the clang-12 warning, but I think
it's a clear mistake that it should be enabled by default for a
target where alignment is not enforced by default.
I found over a dozen places where we would have to manually add
QEMU_ALIGNED(8) to uint64_t declarations in order to suppress
all of the instances. IMO there's no point fighting this.
r~
Richard Henderson (3):
qemu/atomic: Remove pre-C11 atomic fallbacks
qemu/atomic: Use macros for CONFIG_ATOMIC64
configure: Conditionally disable clang-12 -Watomic-alignment
configure | 23 +++--
include/qemu/atomic.h | 229 +++---------------------------------------
2 files changed, 31 insertions(+), 221 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] qemu/atomic: Remove pre-C11 atomic fallbacks 2021-07-12 15:59 [PATCH 0/3] Atomic cleanup + clang-12 build fix Richard Henderson @ 2021-07-12 15:59 ` Richard Henderson 2021-07-12 15:59 ` [PATCH 2/3] qemu/atomic: Use macros for CONFIG_ATOMIC64 Richard Henderson ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Richard Henderson @ 2021-07-12 15:59 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, pbonzini, crobinso We now require c11, so the fallbacks are now dead code Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- configure | 7 -- include/qemu/atomic.h | 204 +++--------------------------------------- 2 files changed, 10 insertions(+), 201 deletions(-) diff --git a/configure b/configure index 85db248ac1..95e0a08938 100755 --- a/configure +++ b/configure @@ -3915,18 +3915,11 @@ cat > $TMPC << EOF int main(void) { uint64_t x = 0, y = 0; -#ifdef __ATOMIC_RELAXED y = __atomic_load_n(&x, __ATOMIC_RELAXED); __atomic_store_n(&x, y, __ATOMIC_RELAXED); __atomic_compare_exchange_n(&x, &y, x, 0, __ATOMIC_RELAXED, __ATOMIC_RELAXED); __atomic_exchange_n(&x, y, __ATOMIC_RELAXED); __atomic_fetch_add(&x, y, __ATOMIC_RELAXED); -#else - typedef char is_host64[sizeof(void *) >= sizeof(uint64_t) ? 1 : -1]; - __sync_lock_test_and_set(&x, y); - __sync_val_compare_and_swap(&x, y, 0); - __sync_fetch_and_add(&x, y); -#endif return 0; } EOF diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h index 3ccf84fd46..c5d6df6bf8 100644 --- a/include/qemu/atomic.h +++ b/include/qemu/atomic.h @@ -60,8 +60,9 @@ (unsigned short)1, \ (expr)+0)))))) -#ifdef __ATOMIC_RELAXED -/* For C11 atomic ops */ +#ifndef __ATOMIC_RELAXED +#error "Expecting C11 atomic ops" +#endif /* Manual memory barriers * @@ -239,193 +240,8 @@ #define qatomic_xor(ptr, n) \ ((void) __atomic_fetch_xor(ptr, n, __ATOMIC_SEQ_CST)) -#else /* __ATOMIC_RELAXED */ - -#ifdef __alpha__ -#define smp_read_barrier_depends() asm volatile("mb":::"memory") -#endif - -#if defined(__i386__) || defined(__x86_64__) || defined(__s390x__) - -/* - * Because of the strongly ordered storage model, wmb() and rmb() are nops - * here (a compiler barrier only). QEMU doesn't do accesses to write-combining - * qemu memory or non-temporal load/stores from C code. - */ -#define smp_mb_release() barrier() -#define smp_mb_acquire() barrier() - -/* - * __sync_lock_test_and_set() is documented to be an acquire barrier only, - * but it is a full barrier at the hardware level. Add a compiler barrier - * to make it a full barrier also at the compiler level. - */ -#define qatomic_xchg(ptr, i) (barrier(), __sync_lock_test_and_set(ptr, i)) - -#elif defined(_ARCH_PPC) - -/* - * We use an eieio() for wmb() on powerpc. This assumes we don't - * need to order cacheable and non-cacheable stores with respect to - * each other. - * - * smp_mb has the same problem as on x86 for not-very-new GCC - * (http://patchwork.ozlabs.org/patch/126184/, Nov 2011). - */ -#define smp_wmb() ({ asm volatile("eieio" ::: "memory"); (void)0; }) -#if defined(__powerpc64__) -#define smp_mb_release() ({ asm volatile("lwsync" ::: "memory"); (void)0; }) -#define smp_mb_acquire() ({ asm volatile("lwsync" ::: "memory"); (void)0; }) -#else -#define smp_mb_release() ({ asm volatile("sync" ::: "memory"); (void)0; }) -#define smp_mb_acquire() ({ asm volatile("sync" ::: "memory"); (void)0; }) -#endif -#define smp_mb() ({ asm volatile("sync" ::: "memory"); (void)0; }) - -#endif /* _ARCH_PPC */ - -/* - * For (host) platforms we don't have explicit barrier definitions - * for, we use the gcc __sync_synchronize() primitive to generate a - * full barrier. This should be safe on all platforms, though it may - * be overkill for smp_mb_acquire() and smp_mb_release(). - */ -#ifndef smp_mb -#define smp_mb() __sync_synchronize() -#endif - -#ifndef smp_mb_acquire -#define smp_mb_acquire() __sync_synchronize() -#endif - -#ifndef smp_mb_release -#define smp_mb_release() __sync_synchronize() -#endif - -#ifndef smp_read_barrier_depends -#define smp_read_barrier_depends() barrier() -#endif - -#ifndef signal_barrier -#define signal_barrier() barrier() -#endif - -/* These will only be atomic if the processor does the fetch or store - * in a single issue memory operation - */ -#define qatomic_read__nocheck(p) (*(__typeof__(*(p)) volatile*) (p)) -#define qatomic_set__nocheck(p, i) ((*(__typeof__(*(p)) volatile*) (p)) = (i)) - -#define qatomic_read(ptr) qatomic_read__nocheck(ptr) -#define qatomic_set(ptr, i) qatomic_set__nocheck(ptr,i) - -/** - * qatomic_rcu_read - reads a RCU-protected pointer to a local variable - * into a RCU read-side critical section. The pointer can later be safely - * dereferenced within the critical section. - * - * This ensures that the pointer copy is invariant thorough the whole critical - * section. - * - * Inserts memory barriers on architectures that require them (currently only - * Alpha) and documents which pointers are protected by RCU. - * - * qatomic_rcu_read also includes a compiler barrier to ensure that - * value-speculative optimizations (e.g. VSS: Value Speculation - * Scheduling) does not perform the data read before the pointer read - * by speculating the value of the pointer. - * - * Should match qatomic_rcu_set(), qatomic_xchg(), qatomic_cmpxchg(). - */ -#define qatomic_rcu_read(ptr) ({ \ - typeof(*ptr) _val = qatomic_read(ptr); \ - smp_read_barrier_depends(); \ - _val; \ -}) - -/** - * qatomic_rcu_set - assigns (publicizes) a pointer to a new data structure - * meant to be read by RCU read-side critical sections. - * - * Documents which pointers will be dereferenced by RCU read-side critical - * sections and adds the required memory barriers on architectures requiring - * them. It also makes sure the compiler does not reorder code initializing the - * data structure before its publication. - * - * Should match qatomic_rcu_read(). - */ -#define qatomic_rcu_set(ptr, i) do { \ - smp_wmb(); \ - qatomic_set(ptr, i); \ -} while (0) - -#define qatomic_load_acquire(ptr) ({ \ - typeof(*ptr) _val = qatomic_read(ptr); \ - smp_mb_acquire(); \ - _val; \ -}) - -#define qatomic_store_release(ptr, i) do { \ - smp_mb_release(); \ - qatomic_set(ptr, i); \ -} while (0) - -#ifndef qatomic_xchg -#if defined(__clang__) -#define qatomic_xchg(ptr, i) __sync_swap(ptr, i) -#else -/* __sync_lock_test_and_set() is documented to be an acquire barrier only. */ -#define qatomic_xchg(ptr, i) (smp_mb(), __sync_lock_test_and_set(ptr, i)) -#endif -#endif -#define qatomic_xchg__nocheck qatomic_xchg - -/* Provide shorter names for GCC atomic builtins. */ -#define qatomic_fetch_inc(ptr) __sync_fetch_and_add(ptr, 1) -#define qatomic_fetch_dec(ptr) __sync_fetch_and_add(ptr, -1) - -#define qatomic_fetch_add(ptr, n) __sync_fetch_and_add(ptr, n) -#define qatomic_fetch_sub(ptr, n) __sync_fetch_and_sub(ptr, n) -#define qatomic_fetch_and(ptr, n) __sync_fetch_and_and(ptr, n) -#define qatomic_fetch_or(ptr, n) __sync_fetch_and_or(ptr, n) -#define qatomic_fetch_xor(ptr, n) __sync_fetch_and_xor(ptr, n) - -#define qatomic_inc_fetch(ptr) __sync_add_and_fetch(ptr, 1) -#define qatomic_dec_fetch(ptr) __sync_add_and_fetch(ptr, -1) -#define qatomic_add_fetch(ptr, n) __sync_add_and_fetch(ptr, n) -#define qatomic_sub_fetch(ptr, n) __sync_sub_and_fetch(ptr, n) -#define qatomic_and_fetch(ptr, n) __sync_and_and_fetch(ptr, n) -#define qatomic_or_fetch(ptr, n) __sync_or_and_fetch(ptr, n) -#define qatomic_xor_fetch(ptr, n) __sync_xor_and_fetch(ptr, n) - -#define qatomic_cmpxchg(ptr, old, new) \ - __sync_val_compare_and_swap(ptr, old, new) -#define qatomic_cmpxchg__nocheck(ptr, old, new) qatomic_cmpxchg(ptr, old, new) - -/* And even shorter names that return void. */ -#define qatomic_inc(ptr) ((void) __sync_fetch_and_add(ptr, 1)) -#define qatomic_dec(ptr) ((void) __sync_fetch_and_add(ptr, -1)) -#define qatomic_add(ptr, n) ((void) __sync_fetch_and_add(ptr, n)) -#define qatomic_sub(ptr, n) ((void) __sync_fetch_and_sub(ptr, n)) -#define qatomic_and(ptr, n) ((void) __sync_fetch_and_and(ptr, n)) -#define qatomic_or(ptr, n) ((void) __sync_fetch_and_or(ptr, n)) -#define qatomic_xor(ptr, n) ((void) __sync_fetch_and_xor(ptr, n)) - -#endif /* __ATOMIC_RELAXED */ - -#ifndef smp_wmb #define smp_wmb() smp_mb_release() -#endif -#ifndef smp_rmb #define smp_rmb() smp_mb_acquire() -#endif - -/* This is more efficient than a store plus a fence. */ -#if !defined(__SANITIZE_THREAD__) -#if defined(__i386__) || defined(__x86_64__) || defined(__s390x__) -#define qatomic_mb_set(ptr, i) ((void)qatomic_xchg(ptr, i)) -#endif -#endif /* qatomic_mb_read/set semantics map Java volatile variables. They are * less expensive on some platforms (notably POWER) than fully @@ -435,16 +251,16 @@ * use. See docs/devel/atomics.rst for more discussion. */ -#ifndef qatomic_mb_read #define qatomic_mb_read(ptr) \ qatomic_load_acquire(ptr) -#endif -#ifndef qatomic_mb_set -#define qatomic_mb_set(ptr, i) do { \ - qatomic_store_release(ptr, i); \ - smp_mb(); \ -} while(0) +#if !defined(__SANITIZE_THREAD__) && \ + (defined(__i386__) || defined(__x86_64__) || defined(__s390x__)) +/* This is more efficient than a store plus a fence. */ +# define qatomic_mb_set(ptr, i) ((void)qatomic_xchg(ptr, i)) +#else +# define qatomic_mb_set(ptr, i) \ + ({ qatomic_store_release(ptr, i); smp_mb(); }) #endif #define qatomic_fetch_inc_nonzero(ptr) ({ \ -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] qemu/atomic: Use macros for CONFIG_ATOMIC64 2021-07-12 15:59 [PATCH 0/3] Atomic cleanup + clang-12 build fix Richard Henderson 2021-07-12 15:59 ` [PATCH 1/3] qemu/atomic: Remove pre-C11 atomic fallbacks Richard Henderson @ 2021-07-12 15:59 ` Richard Henderson 2021-07-12 16:29 ` Philippe Mathieu-Daudé 2021-07-12 15:59 ` [PATCH 3/3] configure: Conditionally disable clang-12 -Watomic-alignment Richard Henderson 2021-07-12 21:30 ` [PATCH 0/3] Atomic cleanup + clang-12 build fix Cole Robinson 3 siblings, 1 reply; 11+ messages in thread From: Richard Henderson @ 2021-07-12 15:59 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, pbonzini, crobinso Clang warnings about questionable atomic usage get localized to the inline function in atomic.h. By using a macro, we get the full traceback to the original use that caused the warning. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- include/qemu/atomic.h | 25 +++++-------------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h index c5d6df6bf8..bf89855209 100644 --- a/include/qemu/atomic.h +++ b/include/qemu/atomic.h @@ -273,26 +273,11 @@ /* Abstractions to access atomically (i.e. "once") i64/u64 variables */ #ifdef CONFIG_ATOMIC64 -static inline int64_t qatomic_read_i64(const int64_t *ptr) -{ - /* use __nocheck because sizeof(void *) might be < sizeof(u64) */ - return qatomic_read__nocheck(ptr); -} - -static inline uint64_t qatomic_read_u64(const uint64_t *ptr) -{ - return qatomic_read__nocheck(ptr); -} - -static inline void qatomic_set_i64(int64_t *ptr, int64_t val) -{ - qatomic_set__nocheck(ptr, val); -} - -static inline void qatomic_set_u64(uint64_t *ptr, uint64_t val) -{ - qatomic_set__nocheck(ptr, val); -} +/* Use __nocheck because sizeof(void *) might be < sizeof(u64) */ +#define qatomic_read_i64 qatomic_read__nocheck +#define qatomic_read_u64 qatomic_read__nocheck +#define qatomic_set_i64 qatomic_set__nocheck +#define qatomic_set_u64 qatomic_set__nocheck static inline void qatomic64_init(void) { -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] qemu/atomic: Use macros for CONFIG_ATOMIC64 2021-07-12 15:59 ` [PATCH 2/3] qemu/atomic: Use macros for CONFIG_ATOMIC64 Richard Henderson @ 2021-07-12 16:29 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 11+ messages in thread From: Philippe Mathieu-Daudé @ 2021-07-12 16:29 UTC (permalink / raw) To: Richard Henderson, qemu-devel; +Cc: peter.maydell, crobinso, pbonzini On 7/12/21 5:59 PM, Richard Henderson wrote: > Clang warnings about questionable atomic usage get localized > to the inline function in atomic.h. By using a macro, we get > the full traceback to the original use that caused the warning. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > include/qemu/atomic.h | 25 +++++-------------------- > 1 file changed, 5 insertions(+), 20 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] configure: Conditionally disable clang-12 -Watomic-alignment 2021-07-12 15:59 [PATCH 0/3] Atomic cleanup + clang-12 build fix Richard Henderson 2021-07-12 15:59 ` [PATCH 1/3] qemu/atomic: Remove pre-C11 atomic fallbacks Richard Henderson 2021-07-12 15:59 ` [PATCH 2/3] qemu/atomic: Use macros for CONFIG_ATOMIC64 Richard Henderson @ 2021-07-12 15:59 ` Richard Henderson 2021-07-12 21:30 ` [PATCH 0/3] Atomic cleanup + clang-12 build fix Cole Robinson 3 siblings, 0 replies; 11+ messages in thread From: Richard Henderson @ 2021-07-12 15:59 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, pbonzini, crobinso The i386 abi does not align uint64_t (or double) in structures to 8 bytes, only to 4 bytes. Furthermore, the hardware does not require 8 byte alignent for cmpxchg8b, so the warning is confusing and useless. Retain the warning for hosts (notably x86_64) where the ABI is sane, and one has to do something extraordinary to remove alignment. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- configure | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/configure b/configure index 95e0a08938..163b4edee2 100755 --- a/configure +++ b/configure @@ -3927,6 +3927,22 @@ if compile_prog "" "" ; then atomic64=yes fi +# Test for clang atomic misalignment warning vs ABI structure misalignment. +# E.g. i386 only aligns structures to 4 bytes by default. +if test "$atomic64" = "yes" ; then +cat > $TMPC << EOF +#include <stdint.h> +struct X { uint64_t x; }; +uint64_t foo(struct X *p) { return __atomic_exchange_n(&p->x, 0, 0); } +EOF + if ! compile_prog "-Werror" "" ; then + if cc_has_warning_flag "-Wno-atomic-alignment" ; then + glib_cflags="-Wno-atomic-alignment $glib_cflags" + CONFIGURE_CFLAGS="$CONFIGURE_CFLAGS -Wno-atomic-alignment" + fi + fi +fi + ######################################### # See if --dynamic-list is supported by the linker ld_dynamic_list="no" -- 2.25.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Atomic cleanup + clang-12 build fix 2021-07-12 15:59 [PATCH 0/3] Atomic cleanup + clang-12 build fix Richard Henderson ` (2 preceding siblings ...) 2021-07-12 15:59 ` [PATCH 3/3] configure: Conditionally disable clang-12 -Watomic-alignment Richard Henderson @ 2021-07-12 21:30 ` Cole Robinson 2021-07-13 0:37 ` Richard Henderson 3 siblings, 1 reply; 11+ messages in thread From: Cole Robinson @ 2021-07-12 21:30 UTC (permalink / raw) To: Richard Henderson, qemu-devel; +Cc: pbonzini, peter.maydell On 7/12/21 11:59 AM, Richard Henderson wrote: > The first two patches are not strictly required, but they > were useful in tracking down the root problem here. > > I understand the logic behind the clang-12 warning, but I think > it's a clear mistake that it should be enabled by default for a > target where alignment is not enforced by default. > > I found over a dozen places where we would have to manually add > QEMU_ALIGNED(8) to uint64_t declarations in order to suppress > all of the instances. IMO there's no point fighting this. > I tested your patches, they seem to get rid of the warnings. The errors persist. FWIW here's my reproduce starting from fedora 34 x86_64 host: $ sudo mock --root fedora-35-i386 --install dnf --install dnf-utils --install fedora-packager --install clang $ sudo mock --root fedora-35-i386 --shell --enable-network # dnf builddep -y qemu # git clone https://github.com/qemu/qemu # cd qemu # CC=clang CXX=clang++ ./configure --disable-werror # make V=1 Thanks, Cole ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Atomic cleanup + clang-12 build fix 2021-07-12 21:30 ` [PATCH 0/3] Atomic cleanup + clang-12 build fix Cole Robinson @ 2021-07-13 0:37 ` Richard Henderson 2021-07-13 14:43 ` Richard Henderson 0 siblings, 1 reply; 11+ messages in thread From: Richard Henderson @ 2021-07-13 0:37 UTC (permalink / raw) To: Cole Robinson, qemu-devel; +Cc: pbonzini, peter.maydell On 7/12/21 2:30 PM, Cole Robinson wrote: > On 7/12/21 11:59 AM, Richard Henderson wrote: >> The first two patches are not strictly required, but they >> were useful in tracking down the root problem here. >> >> I understand the logic behind the clang-12 warning, but I think >> it's a clear mistake that it should be enabled by default for a >> target where alignment is not enforced by default. >> >> I found over a dozen places where we would have to manually add >> QEMU_ALIGNED(8) to uint64_t declarations in order to suppress >> all of the instances. IMO there's no point fighting this. >> > > I tested your patches, they seem to get rid of the warnings. The errors > persist. > > FWIW here's my reproduce starting from fedora 34 x86_64 host: > > $ sudo mock --root fedora-35-i386 --install dnf --install dnf-utils > --install fedora-packager --install clang > $ sudo mock --root fedora-35-i386 --shell --enable-network > # dnf builddep -y qemu > # git clone https://github.com/qemu/qemu > # cd qemu > # CC=clang CXX=clang++ ./configure --disable-werror > # make V=1 Ho hum. So, the warnings are where clang has decided to insert calls to libatomic. So we either have to (1) work around all of the places, which, unless we set up an i386 clang-12 builder will quickly bitrot, or (2) write our own routines, compatible with libatomic, using cmpxchg8b directly. which requires no (extra) locking, and so is compatible with the tcg jit output, or (3) file a bug with clang, and document "use clang-11 and not clang-12". Thoughts? r~ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Atomic cleanup + clang-12 build fix 2021-07-13 0:37 ` Richard Henderson @ 2021-07-13 14:43 ` Richard Henderson 2021-07-13 15:18 ` Cole Robinson 0 siblings, 1 reply; 11+ messages in thread From: Richard Henderson @ 2021-07-13 14:43 UTC (permalink / raw) To: Cole Robinson, qemu-devel; +Cc: pbonzini, peter.maydell On 7/12/21 5:37 PM, Richard Henderson wrote: > On 7/12/21 2:30 PM, Cole Robinson wrote: >> On 7/12/21 11:59 AM, Richard Henderson wrote: >>> The first two patches are not strictly required, but they >>> were useful in tracking down the root problem here. >>> >>> I understand the logic behind the clang-12 warning, but I think >>> it's a clear mistake that it should be enabled by default for a >>> target where alignment is not enforced by default. >>> >>> I found over a dozen places where we would have to manually add >>> QEMU_ALIGNED(8) to uint64_t declarations in order to suppress >>> all of the instances. IMO there's no point fighting this. >>> >> >> I tested your patches, they seem to get rid of the warnings. The errors >> persist. >> >> FWIW here's my reproduce starting from fedora 34 x86_64 host: >> >> $ sudo mock --root fedora-35-i386 --install dnf --install dnf-utils >> --install fedora-packager --install clang >> $ sudo mock --root fedora-35-i386 --shell --enable-network >> # dnf builddep -y qemu >> # git clone https://github.com/qemu/qemu >> # cd qemu >> # CC=clang CXX=clang++ ./configure --disable-werror >> # make V=1 > > Ho hum. So, the warnings are where clang has decided to insert calls to libatomic. > > So we either have to > > (1) work around all of the places, which, unless we set up an i386 clang-12 builder will > quickly bitrot, or Update: (1) is out. There's a warning in cputlb.c vs a pointer that's known to be aligned, and it still fires. I have filed a bug: https://bugs.llvm.org/show_bug.cgi?id=51076 > > (2) write our own routines, compatible with libatomic, using cmpxchg8b directly. which > requires no (extra) locking, and so is compatible with the tcg jit output, or > > (3) file a bug with clang, and document "use clang-11 and not clang-12". So, Cole, with respect to (3), is this just general regression testing that discovered this (in which case, yay) or is there some other reason clang is required? Assuming that (3) isn't really viable long term, I guess (2) is the only viable option. Thoughts? r~ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Atomic cleanup + clang-12 build fix 2021-07-13 14:43 ` Richard Henderson @ 2021-07-13 15:18 ` Cole Robinson 2021-07-13 16:56 ` Richard Henderson 0 siblings, 1 reply; 11+ messages in thread From: Cole Robinson @ 2021-07-13 15:18 UTC (permalink / raw) To: Richard Henderson, qemu-devel; +Cc: pbonzini, peter.maydell On 7/13/21 10:43 AM, Richard Henderson wrote: > On 7/12/21 5:37 PM, Richard Henderson wrote: >> On 7/12/21 2:30 PM, Cole Robinson wrote: >>> On 7/12/21 11:59 AM, Richard Henderson wrote: >>>> The first two patches are not strictly required, but they >>>> were useful in tracking down the root problem here. >>>> >>>> I understand the logic behind the clang-12 warning, but I think >>>> it's a clear mistake that it should be enabled by default for a >>>> target where alignment is not enforced by default. >>>> >>>> I found over a dozen places where we would have to manually add >>>> QEMU_ALIGNED(8) to uint64_t declarations in order to suppress >>>> all of the instances. IMO there's no point fighting this. >>>> >>> >>> I tested your patches, they seem to get rid of the warnings. The errors >>> persist. >>> >>> FWIW here's my reproduce starting from fedora 34 x86_64 host: >>> >>> $ sudo mock --root fedora-35-i386 --install dnf --install dnf-utils >>> --install fedora-packager --install clang >>> $ sudo mock --root fedora-35-i386 --shell --enable-network >>> # dnf builddep -y qemu >>> # git clone https://github.com/qemu/qemu >>> # cd qemu >>> # CC=clang CXX=clang++ ./configure --disable-werror >>> # make V=1 >> >> Ho hum. So, the warnings are where clang has decided to insert calls >> to libatomic. >> >> So we either have to >> >> (1) work around all of the places, which, unless we set up an i386 >> clang-12 builder will quickly bitrot, or > > Update: (1) is out. There's a warning in cputlb.c vs a pointer that's > known to be aligned, and it still fires. I have filed a bug: > > https://bugs.llvm.org/show_bug.cgi?id=51076 > >> >> (2) write our own routines, compatible with libatomic, using cmpxchg8b >> directly. which requires no (extra) locking, and so is compatible >> with the tcg jit output, or >> >> (3) file a bug with clang, and document "use clang-11 and not clang-12". > > So, Cole, with respect to (3), is this just general regression testing > that discovered this (in which case, yay) or is there some other reason > clang is required? > > Assuming that (3) isn't really viable long term, I guess (2) is the only > viable option. > I never tested building qemu with clang prior to this so no idea if it's a regression. There's some interest in using clang (eventually with cfi) to build the Fedora qemu package, so I gave it a test run. If this case is problematic we could keep using gcc for it and clang for every other arch, in the short/medium term. Richard can you clarify, do you think the errors are a clang bug as well, or strictly a qemu issue? If it's clang maybe I can get Red Hat llvm devs to help Thanks, Cole ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Atomic cleanup + clang-12 build fix 2021-07-13 15:18 ` Cole Robinson @ 2021-07-13 16:56 ` Richard Henderson 2021-07-15 15:11 ` Cole Robinson 0 siblings, 1 reply; 11+ messages in thread From: Richard Henderson @ 2021-07-13 16:56 UTC (permalink / raw) To: Cole Robinson, qemu-devel; +Cc: pbonzini, peter.maydell On 7/13/21 8:18 AM, Cole Robinson wrote: >> https://bugs.llvm.org/show_bug.cgi?id=51076 ... > Richard can you clarify, do you think the errors are a clang bug as > well, or strictly a qemu issue? If it's clang maybe I can get Red Hat > llvm devs to help There are minor adjustments that can (and perhaps should be) be made to qemu, but the major portion seems to me to be a clang bug, reported above. Assistance with clang would be quite welcome. r~ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/3] Atomic cleanup + clang-12 build fix 2021-07-13 16:56 ` Richard Henderson @ 2021-07-15 15:11 ` Cole Robinson 0 siblings, 0 replies; 11+ messages in thread From: Cole Robinson @ 2021-07-15 15:11 UTC (permalink / raw) To: Richard Henderson, qemu-devel; +Cc: pbonzini, peter.maydell On 7/13/21 12:56 PM, Richard Henderson wrote: > On 7/13/21 8:18 AM, Cole Robinson wrote: >>> https://bugs.llvm.org/show_bug.cgi?id=51076 > ... >> Richard can you clarify, do you think the errors are a clang bug as >> well, or strictly a qemu issue? If it's clang maybe I can get Red Hat >> llvm devs to help > > There are minor adjustments that can (and perhaps should be) be made to > qemu, but the major portion seems to me to be a clang bug, reported > above. Assistance with clang would be quite welcome. > I tried to summarize the discussion and filed a bug against fedora rawhide clang: https://bugzilla.redhat.com/show_bug.cgi?id=1982740 Thanks, Cole ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-07-15 15:12 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-12 15:59 [PATCH 0/3] Atomic cleanup + clang-12 build fix Richard Henderson 2021-07-12 15:59 ` [PATCH 1/3] qemu/atomic: Remove pre-C11 atomic fallbacks Richard Henderson 2021-07-12 15:59 ` [PATCH 2/3] qemu/atomic: Use macros for CONFIG_ATOMIC64 Richard Henderson 2021-07-12 16:29 ` Philippe Mathieu-Daudé 2021-07-12 15:59 ` [PATCH 3/3] configure: Conditionally disable clang-12 -Watomic-alignment Richard Henderson 2021-07-12 21:30 ` [PATCH 0/3] Atomic cleanup + clang-12 build fix Cole Robinson 2021-07-13 0:37 ` Richard Henderson 2021-07-13 14:43 ` Richard Henderson 2021-07-13 15:18 ` Cole Robinson 2021-07-13 16:56 ` Richard Henderson 2021-07-15 15:11 ` Cole Robinson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.