Hi David, On Sat, Oct 23, 2021 at 02:51:59PM +0100, David CARLIER wrote: > Hi, > Hopefully not too late for the 2.5 release :-)
No worries, and fixes can be merged later anyway. I have some questions below. > From b9c083252bdabf2d0bbfffa1383453cdfd94ab13 Mon Sep 17 00:00:00 2001 > From: David CARLIER <[email protected]> > Date: Sat, 23 Oct 2021 14:43:42 +0100 > Subject: [PATCH] BUILD/MINOR: atomics mac arm64 build fix > > the inline assembly is invalid for this platform thus falling back > to the builtin instead. > > --- > include/haproxy/atomic.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/haproxy/atomic.h b/include/haproxy/atomic.h > index 3198b381a..29a06c57b 100644 > --- a/include/haproxy/atomic.h > +++ b/include/haproxy/atomic.h > @@ -698,7 +698,7 @@ __ha_barrier_atomic_full(void) > */ > #define __ha_cpu_relax() ({ asm volatile("isb" ::: "memory"); 1; }) > > -#if defined(__ARM_FEATURE_ATOMICS) // ARMv8.1-A atomics > +#if defined(__ARM_FEATURE_ATOMICS) && !defined(__APPLE__) // ARMv8.1-A > atomics Hmmm that's not expected at all, what error are you getting ? We're not doing anything special, so if the __ARM_FEATURE_ATOMICS macro is defined, we ought to have them. > /* returns 0 on failure, non-zero on success */ > static forceinline int __ha_cas_dw(void *target, void *compare, const void > *set) > @@ -738,7 +738,7 @@ static forceinline int __ha_cas_dw(void *target, void > *compare, const void *set) > return ret; > } > > -#elif defined(__SIZEOF_INT128__) && > defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_16) // no ARMv8.1-A atomics but > 128-bit atomics > +#elif defined(__SIZEOF_INT128__) && > (defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_16) || defined(__APPLE__)) // no > ARMv8.1-A atomics but 128-bit atomics I feel uncomfortable with adding this "||" in the expression, as we ought to condition this instructions to this. Are you sure this was needed ? Did you check the resulting code to make sure a CASP instruction was emitted ? Something that would be nice is to dump all defines: gcc -dM -E - < /dev/null (or use clang instead of gcc). > /* According to > https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html > * we can use atomics on __int128. The availability of CAS is defined there: > @@ -752,7 +752,7 @@ static forceinline int __ha_cas_dw(void *target, void > *compare, const void *set) > /* returns 0 on failure, non-zero on success */ > static __inline int __ha_cas_dw(void *target, void *compare, const void *set) > { > - return __atomic_compare_exchange((__int128*)target, (__int128*)compare, > (const __int128*)set, > + return __atomic_compare_exchange((__int128*)target, (__int128*)compare, > (__int128*)set, It's not correct to remove the const there, first because this value is not expected to be changed by the emitted instructions, and second because we're violating the function's contract which promises not to modify this argument. Are you sure it's not just a leftover of your debugging session ? Thanks, Willy
