[libc++] fix atomic::wait memory order

[huixie90]

Fixes #109290

See the GH issue for the details. In conclusion, we have two issues in the atomic<T>::wait when T does not match our contention_t:

• We don't have a total single order which can leads to missed notification based on the Herd7 simulation on relaxed architectural like PowerPC
• We assumed the platform wait (futex_wait/__ulock_wait) has seq_cst barrier internally but there is no such guarantee

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Hui (huixie90)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/146267.diff

1 Files Affected:

• (modified) libcxx/src/atomic.cpp (+3-2)

diff --git a/libcxx/src/atomic.cpp b/libcxx/src/atomic.cpp
index c1af8d6f95aae..585fd1f538e46 100644
--- a/libcxx/src/atomic.cpp
+++ b/libcxx/src/atomic.cpp
@@ -151,8 +151,9 @@ __libcpp_contention_monitor_for_wait(__cxx_atomic_contention_t volatile* /*__con
 static void __libcpp_contention_wait(__cxx_atomic_contention_t volatile* __contention_state,
                                      __cxx_atomic_contention_t const volatile* __platform_state,
                                      __cxx_contention_t __old_value) {
-  __cxx_atomic_fetch_add(__contention_state, __cxx_contention_t(1), memory_order_seq_cst);
+  __cxx_atomic_fetch_add(__contention_state, __cxx_contention_t(1), memory_order_relaxed);
   // We sleep as long as the monitored value hasn't changed.
+  __cxx_atomic_thread_fence(memory_order_seq_cst);
   __libcpp_platform_wait_on_address(__platform_state, __old_value);
   __cxx_atomic_fetch_sub(__contention_state, __cxx_contention_t(1), memory_order_release);
 }
@@ -163,7 +164,7 @@ static void __libcpp_contention_wait(__cxx_atomic_contention_t volatile* __conte
 static void __libcpp_atomic_notify(void const volatile* __location) {
   auto const __entry = __libcpp_contention_state(__location);
   // The value sequence laundering happens on the next line below.
-  __cxx_atomic_fetch_add(&__entry->__platform_state, __cxx_contention_t(1), memory_order_release);
+  __cxx_atomic_fetch_add(&__entry->__platform_state, __cxx_contention_t(1), memory_order_seq_cst);
   __libcpp_contention_notify(
       &__entry->__contention_state,
       &__entry->__platform_state,

[aharrison24]

Thanks for taking this issue on! This looks like a correct implementation of "Option 1a" from the original issue I raised. That's been verified by three separate people using Herd, Dartagnan and GenMC respectively, so we're relatively sure it's doing the right thing at this point!

[ldionne]

LGTM, but +1 about adding a comment.

[gonzalobg]

@huixie90 why seq_cst? Could we add a comment explaining that? e.g. why doesn't acquire or acq_rel suffice.

[gonzalobg]

Similar as above, why is seq_cst required? e.g. why doesn't release or acq_rel suffice.