[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:

> I am still not convinced. In my opinion, it is simply a choice of on which abstraction we should build our `counting_semaphore` and `latch` on. I think your suggestion is that we should build these things on top of the internal `__cxx_atomic_wait`, `contention_state`, `__libcpp_thread_poll_with_backoff`. In my opinion, we should simply build it on top of `atomic`.

My suggestion is to build on `atomic` and additionally on a fixed `__cxx_atomic_wait`. The higher level `semaphore` and `latch` shouldn't have to know about `contention_state` or `__libcpp_thread_poll_with_backoff`, I agree completely. You don't want to think about those internals when implementing high level logic.

`__cxx_atomic_wait` is the higher level abstraction you want, I believe. One can also think about it as the normal `.wait()`, but instead of a hardcoded condition (does `this->load(order)` compare equal to `old`?) you can put in any condition you want. So additionally to

```c++
void wait(T old, std::memory_order order = std::memory_order::seq_cst) const noexcept;
```

you have (`std::function` for clarity):

```c++
void __wait(std::function<bool(typename std::atomic<T>::value_type &old)> condition, std::memory_order order = std::memory_order::seq_cst) const noexcept; // this is basically (a fixed) __cxx_atomic_wait
```

This makes writing `semaphore` and `latch` easier, because you can just put in the `try_acquire()`s as the condition.

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