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

> edit: Scratch the next part -- I'm not 100% sure, but it could well be that the condition for the `cxx_atomic_wait` has to be "strong". So if one has a "weak" `try_acquire()`, one has to implement the `acquire()` seperately, just as you wrote. I have to think about this some more :)

I think I got it! You basically can implement `try_acquire()` in three different ways, and the way you would implement `acquire()` in terms of that differs significantly:

1. "strong" `try_acquire()`: this only ever returns `false` if it has actually observed the semaphore value to be "0". You can implement `acquire()` in terms of that if you notify the eventcount if `old == 0` in `release()`.
2. "weak" `try_acquire()` with strong CAS (`return value != 0 && cas_strong(...);`): You could implement `acquire()` in terms of that, but then you would have to notify in any case, not only if `old == 0`! Example execution:

```
producer    consumer         sem        evc (eventcount)
-------------------------------------------
                              42          0
-------------------------------------------
             monitor=0        42          0
-------------------------------------------
             old=42           42          0
-------------------------------------------
release(1)                    43          0
-------------------------------------------
             strong
             CAS(42->41)      43          0
             fails
-------------------------------------------
evc.notify()                  43          1
^
this is
important!
-------------------------------------------
             evc.wait()       43          1
             ends
             as monitor
             doesn't
             match (0!=1)
-------------------------------------------
             loop restarts
             and strong CAS   42          1
             will succeed
             this time
```

3. "weak" `try_acquire()`, with weak CAS (`return value != 0 && cas_weak(...);`): You cannot implement `acquire()` in terms of that, because the weak CAS could just randomly decide to spuriously fail, and any notifications would get lost.

Sorry for the confused rambling, but I hope this kinda makes sense...

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