On Sat, 23 Oct 2021 at 16:50, Willy Tarreau <w...@1wt.eu> wrote:
>
> 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 <devne...@gmail.com>
> > 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.
Note this works ok with gcc but not clang. But clang is the main
compiler, gcc is secondary on this platform (less well supported).
I tried to convert to llvm asm-ism but seems applying direct
contiguous data values (ie cmp) to registers seems unsupported and
requires more instructions/operands thus
it is safer to just fallback to the builtin which just does that
anyway as you can see :
`
0x100087a3c <+28>: mov x3, x5
0x100087a40 <+32>: casp x2, x3, x6, x7, [x8]
0x100087a44 <+36>: eor x8, x3, x5
0x100087a48 <+40>: eor x9, x2, x4
0x100087a4c <+44>: orr x8, x9, x8
0x100087a50 <+48>: cbz x8, 0x100087a58 ; <+56> at
atomic.h:757:2
`
>
> > /* 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).
mac m1 supports rightfully the 128 bits type.
`
#define __SIZEOF_INT128__ 16
`
I just recall on arm64 __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16 is not even
present and indeed when I look up on both on my raspberry and my mac
m1 that is confirmed.
>
> > /* 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
