[libcxx-commits] [libcxx] [libc++] fix `counting_semaphore` lost wakeups (PR #79265)

[Jan Kokemüller via libcxx-commits]

================
@@ -94,20 +95,25 @@ public:
     auto __old = __a_.fetch_add(__update, memory_order_release);
     _LIBCPP_ASSERT_ARGUMENT_WITHIN_DOMAIN(
         __update <= _LIBCPP_SEMAPHORE_MAX - __old, "update is greater than the expected value");
-
-    if (__old > 0) {
-      // Nothing to do
-    } else if (__update > 1)
+    (void)__old;
+    if (__update > 1) {
       __a_.notify_all();
-    else
+    } else {
       __a_.notify_one();
+    }
   }
   _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void acquire() {
     auto const __test_fn = [this]() -> bool {
       auto __old = __a_.load(memory_order_relaxed);
       return (__old != 0) && __a_.compare_exchange_strong(__old, __old - 1, memory_order_acquire, memory_order_relaxed);
     };
-    __cxx_atomic_wait(&__a_.__a_, __test_fn);
+    auto const __test_with_old = [this](__cxx_contention_t& __monitor) -> bool {
----------------
jiixyj wrote:

> w.r.t to the `latch`, do you agree that in the current form, there is no issues like the one in `semaphore`? All the modification of a `latch` is going one direction so there should not be ABA problem IIUC

Ah yes, you are right! The latch is a downward counter, so the `__test_fn` could only "err" in the direction of not-blocking. So if the monitor read "1" for example, the `__test_fn`  could only read either "1" as well (and then the algorithm would proceed to block), or read "0" and exit the loop.

I still think it's worth to refactor it so that the `__test_fn` always takes the current value of the atomic as an in-out-argument, as this would just make this class of ABA issue go away completely. More synchronization primitives might be implemented in terms of `__cxx_atomic_wait` in the future.

https://github.com/llvm/llvm-project/pull/79265