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

[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 {
----------------
huixie90 wrote:

I think we are coming close to an agreement now.  yes we both agree to work on the higher level abstraction instead of trying fighting the backoff functions. yes having another function that takes a predicate can be useful too. maybe call it `wait_if_not`   (like `find` vs `find_if`). 

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`

something like
```
bool try_acquire() {
    auto __old = __a_.load(memory_order_acquire);
    return __old != 0 && __a_.compare_exchange_strong(__old, __old - 1, memory_order_acquire, memory_order_relaxed);
}

void acquire() {
    auto __old = __a_.load(std::memory_order_relaxed);
    while (true) {
      if (__old == 0) {
        __a_.wait(__old, std::memory_order_relaxed);
        __old = __a_.load(std::memory_order_relaxed);
      } else if (__a_.compare_exchange_weak(__old, __old - 1, std::memory_order_acquire, std::memory_order_relaxed)) {
        break;
      }
}
```

here is just a small refactoring of `semaphore` `latch` `barrier` to just use existing `atomic`'s API
https://github.com/huixie90/llvm-project/commit/8a4b98a2bdaab954c13a35bbd9b07e1c44ca69fe

Going back to the original bug. I don't think the refactoring of all this solved anything. The original lost wake up bug was not in `semaphore`, or `latch` etc...  It in the `atomic::wait`'s `backoff` function:
```
auto const __monitor = std::__libcpp_atomic_monitor(__a);
if (__test_fn())
      return true;
std::__libcpp_atomic_wait(__a, __monitor);
```
when this function is called, it is possible that the `atomic` variable has already been changed and `__monitor` is the new value. so we are waiting for a different value now. (and also there is no happens-before so __test_fn could be evaluated before taking the monitor). so basically we need to fix this function.
(also we need to careful about `__libcpp_atomic_monitor` returns different thing for `atomic<int64t>` and everything else

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