[libcxx-commits] [PATCH] D114119: [libcxx] Fix potential lost wake-up in counting semaphore

[Fabian Wolff via Phabricator via libcxx-commits]

fwolff added a comment.
Herald added a project: All.

Thanks a lot for your analysis @jiixyj! Your analysis of the ABA problem makes sense to me.

To summarize, the current `acquire()` implementation does this:

  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);

where `__cxx_atomic_wait` does this:

  auto const __monitor = __libcpp_atomic_monitor(__a);
  if(__test_fn())
      return true;
  __libcpp_atomic_wait(__a, __monitor);

i.e. `__cxx_atomic_wait` loads the old value, and then the test function //also// loads the old value, which might have changed in between, so the test function tests a different value than `__cxx_atomic_wait` then waits on. In particular, `__cxx_atomic_wait` might read 1, `__test_fn` might read zero and therefore return `false`, and then the value of `__a.__a_` might concurrently be updated to 1 before `__cxx_atomic_wait` enters the wait with an old value of 1, hence causing a deadlock/lost wakeup.

So I think the main problem here is that `__cxx_atomic_wait` loads the value, and then `__test_fn` has to load it again. The fix proposed by @jiixyj, namely to have `__cxx_atomic_wait` pass the value of `__monitor` as a reference into `__test_fn`, appears sensible to me; what do you think, @ldionne?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114119/new/

https://reviews.llvm.org/D114119