Re: [dpdk-dev] [PATCH v1] eal/arm: fix gcc build for 128-bit atomic compare exchange

[Ruifeng Wang <Ruifeng.Wang@arm.com>]

> -----Original Message-----
> From: Joyce Kong <joyce.kong@arm.com>
> Sent: Friday, January 15, 2021 5:58 PM
> To: jerinj@marvell.com; david.marchand@redhat.com; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>
> Cc: dev@dpdk.org; nd <nd@arm.com>; stable@dpdk.org
> Subject: [PATCH v1] eal/arm: fix gcc build for 128-bit atomic compare
> exchange
> 
> Compiling with "meson build -Dbuildtype=debug --cross-file
> config/arm/arm64_thunderx2_linux_gcc" shows the warnings "function

Issue can be reproduced with the posted command. But it is not specific to ThunderX2 platform.
It is reproducible on any platform that has LSE extension when building with 'buildtype=debug'.

Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>

> returns an aggregate [-Waggregate-return]":
> ../../dpdk/lib/librte_eal/arm/include/rte_atomic_64.h: In function
> ‘__cas_128_relaxed’:
> ../../dpdk/lib/librte_eal/arm/include/rte_atomic_64.h:81:20:
> error: function returns an aggregate [-Werror=aggregate-return]
> __ATOMIC128_CAS_OP(__cas_128_relaxed, "casp")
>                     ^~~~~~~~~~~~~~~~~
> 
> Fix the compiling issue by defining __ATOMIC128_CAS_OP as a void function
> and passing the address pointer into it.
> 
> Fixes: 7e2c3e17fe2c ("eal/arm64: add 128-bit atomic compare exchange")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> ---
>  lib/librte_eal/arm/include/rte_atomic_64.h | 28 +++++++++++-----------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/librte_eal/arm/include/rte_atomic_64.h
> b/lib/librte_eal/arm/include/rte_atomic_64.h
> index 467d32a45..fa6f334c0 100644
> --- a/lib/librte_eal/arm/include/rte_atomic_64.h
> +++ b/lib/librte_eal/arm/include/rte_atomic_64.h
> @@ -53,15 +53,15 @@ rte_atomic_thread_fence(int memorder)  #endif
> 
>  #define __ATOMIC128_CAS_OP(cas_op_name, op_string)                          \
> -static __rte_noinline rte_int128_t                                          \
> -cas_op_name(rte_int128_t *dst, rte_int128_t old, rte_int128_t updated)
> \
> +static __rte_noinline void                                                  \
> +cas_op_name(rte_int128_t *dst, rte_int128_t *old, rte_int128_t updated)
> \
>  {                                                                           \
>  	/* caspX instructions register pair must start from even-numbered
>  	 * register at operand 1.
>  	 * So, specify registers for local variables here.
>  	 */                                                                 \
> -	register uint64_t x0 __asm("x0") = (uint64_t)old.val[0];            \
> -	register uint64_t x1 __asm("x1") = (uint64_t)old.val[1];            \
> +	register uint64_t x0 __asm("x0") = (uint64_t)old->val[0];           \
> +	register uint64_t x1 __asm("x1") = (uint64_t)old->val[1];           \
>  	register uint64_t x2 __asm("x2") = (uint64_t)updated.val[0];        \
>  	register uint64_t x3 __asm("x3") = (uint64_t)updated.val[1];        \
>  	asm volatile(                                                       \
> @@ -73,9 +73,8 @@ cas_op_name(rte_int128_t *dst, rte_int128_t old,
> rte_int128_t updated)      \
>  		[upd1] "r" (x3),                                            \
>  		[dst] "r" (dst)                                             \
>  		: "memory");                                                \
> -	old.val[0] = x0;                                                    \
> -	old.val[1] = x1;                                                    \
> -	return old;                                                         \
> +	old->val[0] = x0;                                                   \
> +	old->val[1] = x1;                                                   \
>  }
> 
>  __ATOMIC128_CAS_OP(__cas_128_relaxed, "casp") @@ -113,13 +112,14
> @@ rte_atomic128_cmp_exchange(rte_int128_t *dst, rte_int128_t *exp,
> 
>  #if defined(__ARM_FEATURE_ATOMICS) ||
> defined(RTE_ARM_FEATURE_ATOMICS)
>  	if (success == __ATOMIC_RELAXED)
> -		old = __cas_128_relaxed(dst, expected, desired);
> +		__cas_128_relaxed(dst, exp, desired);
>  	else if (success == __ATOMIC_ACQUIRE)
> -		old = __cas_128_acquire(dst, expected, desired);
> +		__cas_128_acquire(dst, exp, desired);
>  	else if (success == __ATOMIC_RELEASE)
> -		old = __cas_128_release(dst, expected, desired);
> +		__cas_128_release(dst, exp, desired);
>  	else
> -		old = __cas_128_acq_rel(dst, expected, desired);
> +		__cas_128_acq_rel(dst, exp, desired);
> +	old = *exp;
>  #else
>  #define __HAS_ACQ(mo) ((mo) != __ATOMIC_RELAXED && (mo) !=
> __ATOMIC_RELEASE)  #define __HAS_RLS(mo) ((mo) ==
> __ATOMIC_RELEASE || (mo) == __ATOMIC_ACQ_REL || \ @@ -183,12
> +183,12 @@ rte_atomic128_cmp_exchange(rte_int128_t *dst, rte_int128_t
> *exp,  #undef __STORE_128
> 
>  	} while (unlikely(ret));
> -#endif
> 
> -	/* Unconditionally updating expected removes an 'if' statement.
> -	 * expected should already be in register if not in the cache.
> +	/* Unconditionally updating the value of exp removes an 'if'
> statement.
> +	 * The value of exp should already be in register if not in the cache.
>  	 */
>  	*exp = old;
> +#endif
> 
>  	return (old.int128 == expected.int128);  }
> --
> 2.30.0