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

> For `semaphore` case, I don't think we have to reuse `try_acquire` in `acquire` as I think they are a bit different.
> 
>     1. We don't need a loop in `try_acquire`. can we just change it to oneoff thing?
> 
>     2. we need `compare_exchange_strong` in `try_acquire` and `compare_exchange_weak` in `acquire`

Yes, you don't need a loop in `try_acquire()`. But then, you don't need `compare_exchange_strong` either, as `weak` will do. Imagine this sequence where a consumer calls this version of `try_acquire()` (which doesn't use a loop and uses `compare_exchange_strong`), and there is a concurrent producer:

```
producer         consumer         sem
-------------------------------------
                                   42
-------------------------------------
                  old=42           42
-------------------------------------
release(1)                         43
-------------------------------------
                  tries
                  strong           43
                  cas: fails!
-------------------------------------
                  returns false
```

...so you might then as well have used `compare_exchange_weak` as this is allowed to spuriously fail.

So you _can_ implement `acquire()` even in terms of this "weaker" `try_acquire()` -- you just have to make sure that any waiting happens based on the value "0" of the semaphore like this:

```c++
struct semaphore {
    std::atomic<std::ptrdiff_t> a_{};

    bool try_acquire_impl(std::ptrdiff_t &value) {
        // if `try_acquire_impl` returns `false`, `value` must be 0!
        return (value != 0 &&
                a_.compare_exchange_weak(value, value - 1,  //
                                         std::memory_order_acquire,
                                         std::memory_order_relaxed)) ||
               (value = 0, false);
    }

    static constexpr auto acquire_initial_load_mo = std::memory_order_relaxed;

    bool try_acquire() {
        auto old = a_.load(acquire_initial_load_mo);
        return try_acquire_impl(old);
    }

    void acquire() {
        using namespace std::placeholders;
        cxx_atomic_wait(&a_, std::bind(&semaphore::try_acquire_impl, this, _1),
                        acquire_initial_load_mo);
    }
};
```

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