RE: [PATCH 2/2][AArch32] Fix 128-bit sequential consistency atomic operations.

[Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>]

> -----Original Message-----
> From: Tamar Christina <Tamar.Christina@arm.com>
> Sent: Wednesday, June 8, 2022 3:50 PM
> To: gcc-patches@gcc.gnu.org
> Cc: nd <nd@arm.com>; Ramana Radhakrishnan
> <Ramana.Radhakrishnan@arm.com>; Richard Earnshaw
> <Richard.Earnshaw@arm.com>; nickc@redhat.com; Kyrylo Tkachov
> <Kyrylo.Tkachov@arm.com>
> Subject: [PATCH 2/2][AArch32] Fix 128-bit sequential consistency atomic
> operations.
> 
> Hi All,
> 
> Similar to AArch64 the Arm implementation of 128-bit atomics is broken.
> 
> For 128-bit atomics we rely on pthread barriers to correct guard the address
> in the pointer to get correct memory ordering.  However for 128-bit atomics
> the
> address under the lock is different from the original pointer.
> 
> This means that one of the values under the atomic operation is not
> protected
> properly and so we fail during when the user has requested sequential
> consistency as there's no barrier to enforce this requirement.
> 
> As such users have resorted to adding an
> 
> #ifdef GCC
> <emit barrier>
> #endif
> 
> around the use of these atomics.
> 
> This corrects the issue by issuing a barrier only when __ATOMIC_SEQ_CST
> was
> requested.  I have hand verified that the barriers are inserted
> for atomic seq cst.
> 
> 
> Bootstrapped Regtested on arm-none-linux-gnueabihf and no issues.
> 
> Ok for master? and for backporting to GCC 12, 11 and 10?

Ok, with backports after a couple weeks on master.
Thanks,
Kyrill

> 
> Thanks,
> Tamar
> 
> libatomic/ChangeLog:
> 
> 	PR target/102218
> 	* config/arm/host-config.h (pre_seq_barrier, post_seq_barrier,
> 	pre_post_seq_barrier): Require barrier on __ATOMIC_SEQ_CST.
> 
> --- inline copy of patch --
> diff --git a/libatomic/config/arm/host-config.h b/libatomic/config/arm/host-
> config.h
> index
> bbf4a3f84c3f3ae21fb2162aab68bdedf3fbdcb4..ef16fad2a35ec9055e918849e
> 69a1a0e23b62838 100644
> --- a/libatomic/config/arm/host-config.h
> +++ b/libatomic/config/arm/host-config.h
> @@ -1,4 +1,23 @@
>  /* Avoiding the DMB (or kernel helper) can be a good thing.  */
>  #define WANT_SPECIALCASE_RELAXED
> 
> +/* Glibc, at least, uses acq_rel in its pthread mutex
> +   implementation.  If the user is asking for seq_cst,
> +   this is insufficient.  */
> +
> +static inline void __attribute__((always_inline, artificial))
> +pre_seq_barrier(int model)
> +{
> +  if (model == __ATOMIC_SEQ_CST)
> +    __atomic_thread_fence (__ATOMIC_SEQ_CST);
> +}
> +
> +static inline void __attribute__((always_inline, artificial))
> +post_seq_barrier(int model)
> +{
> +  pre_seq_barrier(model);
> +}
> +
> +#define pre_post_seq_barrier 1
> +
>  #include_next <host-config.h>
> 
> 
> 
> 
> --