[compiler-rt][builtins] Switch libatomic locks to pthread_mutex_t.

[Logikable]

When an uninstrumented libatomic is used with a TSan instrumented
memcpy, TSan may report a data race in circumstances where writes are
arguably safe.

This occurs because __atomic_compare_exchange won't be instrumented in
an uninstrumented libatomic, so TSan doesn't know that the subsequent
memcpy is race-free.

On the other hand, pthread_mutex_(un)lock will be intercepted by TSan,
meaning an uninstrumented libatomic will not report this false-positive.

pthread_mutexes also may try a number of different strategies to acquire
the lock, which may bound the amount of time a thread has to wait for a
lock during contention.

While pthread_mutex_lock has a larger overhead (due to the function
call and some dispatching), a dispatch to libatomic already predicates
a lack of performance guarantees.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[MaskRay]

LGTM. It's perhaps worth mentioning that GCC libatomic also uses pthread_mutex_t. So this change would bring three advantages:

• The uninstrumented atomic.c will work seemlessly with tsan (thanks to the pthread_mutex_lock interceptor). We don't need a separate tsan version of libatomic
• It would prevent potential issues with high thread competition for the spin lock.
• It aligns with GCC libatomic.

It seems that glibc's pthread_mutex_lock implementation has slightly larger overhead (a few instructions before and after lock cmpxchg %edx,(%rdi) including __libc_single_threaded test which is always 0 for non-trivial google3 code) while other implementations seem to optimize the PTHREAD_MUTEX_NORMAL common scenarios better, e.g. https://git.musl-libc.org/cgit/musl/tree/src/thread/pthread_mutex_lock.c

[MaskRay]

It seems that we forgot PTHREAD_MUTEX_INITIALIZER (benign issue since it is all zeroes in glibc and musl). Perhaps we should avoid false sharing as well:
#94374

[pirama-arumuga-nainar]

This PR broke the armv8-a builtins build in Android toolchain - which can lower _Atomic but the baremetal environment doesn't have <pthread.h>. Can you specialize this change to fall back to _Atomic when COMPILER_RT_HAS_LIBPTHREAD is off?

[mstorsjo]

This also broke builds for Windows: https://github.com/mstorsjo/llvm-mingw/actions/runs/9376329007/job/25816083980

Can you specialize this change to fall back to _Atomic when COMPILER_RT_HAS_LIBPTHREAD is off?

This sounds like a good idea, but I'd also like to make it dependent on platform. E.g. on Windows, libpthread may exist - there are third party libraries that provide the pthreads API, but we don't want to use it here even if it happens to exist while building compiler-rt.

[appujee]

This also broke builds for Windows: https://github.com/mstorsjo/llvm-mingw/actions/runs/9376329007/job/25816083980

Can you specialize this change to fall back to _Atomic when COMPILER_RT_HAS_LIBPTHREAD is off?

This sounds like a good idea, but I'd also like to make it dependent on platform. E.g. on Windows, libpthread may exist - there are third party libraries that provide the pthreads API, but we don't want to use it here even if it happens to exist while building compiler-rt.

Yeah, i agree that users should have a choice to opt-in to pthreads. As it seems like there are a few things to clarify, should this patch be reverted?

[mstorsjo]

This also broke builds for Windows: https://github.com/mstorsjo/llvm-mingw/actions/runs/9376329007/job/25816083980

Can you specialize this change to fall back to _Atomic when COMPILER_RT_HAS_LIBPTHREAD is off?

This sounds like a good idea, but I'd also like to make it dependent on platform. E.g. on Windows, libpthread may exist - there are third party libraries that provide the pthreads API, but we don't want to use it here even if it happens to exist while building compiler-rt.

Yeah, i agree that users should have a choice to opt-in to pthreads. As it seems like there are a few things to clarify, should this patch be reverted?

Yes, this should be reverted for now. I can push a revert shortly.

[mstorsjo]

This also broke builds for Windows: https://github.com/mstorsjo/llvm-mingw/actions/runs/9376329007/job/25816083980

Can you specialize this change to fall back to _Atomic when COMPILER_RT_HAS_LIBPTHREAD is off?

This sounds like a good idea, but I'd also like to make it dependent on platform. E.g. on Windows, libpthread may exist - there are third party libraries that provide the pthreads API, but we don't want to use it here even if it happens to exist while building compiler-rt.

Yeah, i agree that users should have a choice to opt-in to pthreads. As it seems like there are a few things to clarify, should this patch be reverted?

Yes, this should be reverted for now. I can push a revert shortly.

Reverted now in a4b32c2.

[MaskRay]

Perhaps we can keep the current spin locks for #if defined(__ANDROID__) || defined(_WIN32)

[pirama-arumuga-nainar]

Perhaps we can keep the current spin locks for #if defined(__ANDROID__) || defined(_WIN32)

The breakage in the Android toolchain is not when targeting Android but a baremetal configuration needed in Android. So the above will not work. This target does set COMPILER_RT_BAREMETAL_BUILD=ON - if you want to key off of that. But !COMPILER_RT_HAS_LIBPTHREAD || defined(_WIN32) is probably the correct check here.