https://gcc.gnu.org/g:05ac72feea409f96c2b690a6f064171e274c8f91
commit r17-5118-g05ac72feea409f96c2b690a6f064171e274c8f91 Author: Jonathan Wakely <[email protected]> Date: Wed Oct 7 17:54:54 2026 +0100 libstdc++: Fix missed atomic notifying operations [PR127757] As described in the PR we can get missed wakes for atomic wait/notify operations, due to the notifier thread thinking there are no waiters and eliding the futex wake syscall. Without the full fence added by this commit the notifying thread can load _M_waiters == 0 and the waiting thread can load the old value of the atomic object, so the waiter blocks but the notifier doesn't notify. The full fence with seq_cst ordering ensures that either the waiter thread sees the new value of the atomic object or the notifier sees a non-zero count of waiters. An LLM was used to modify the original testcase so that it uses less memory and completes faster. Assisted-by: LLM libstdc++-v3/ChangeLog: PR libstdc++/127757 * src/c++20/atomic.cc (__waitable_state::_M_enter_wait): Add comment. (__waitable_state::_M_waiting): Add seq_cst fence and comments. * testsuite/29_atomics/atomic/wait_notify/127757.cc: New test. Reviewed-by: Tomasz Kamiński <[email protected]> Diff: --- libstdc++-v3/src/c++20/atomic.cc | 15 ++- .../29_atomics/atomic/wait_notify/127757.cc | 129 +++++++++++++++++++++ 2 files changed, 143 insertions(+), 1 deletion(-) diff --git a/libstdc++-v3/src/c++20/atomic.cc b/libstdc++-v3/src/c++20/atomic.cc index 572f0898167a..ee6dc04ffcc2 100644 --- a/libstdc++-v3/src/c++20/atomic.cc +++ b/libstdc++-v3/src/c++20/atomic.cc @@ -325,15 +325,28 @@ namespace void _M_enter_wait() noexcept - { __atomic_fetch_add(&_M_waiters, 1, __ATOMIC_SEQ_CST); } + { + // This seq_cst RMW and the seq_cst fence in _M_waiting() are both in + // the single total order on seq_cst operations. + __atomic_fetch_add(&_M_waiters, 1, __ATOMIC_SEQ_CST); + } void _M_leave_wait() noexcept { __atomic_fetch_sub(&_M_waiters, 1, __ATOMIC_RELEASE); } + // Called by notifying thread to decide if it needs to do the notification + // (which requires an expensive wake syscall or condvar signal). bool _M_waiting() const noexcept { + // This thread modifies the atomic (or the proxy _M_ver) at memory + // location A, then loads _M_waiters here. A waiter thread calls + // _M_enter_wait() to increment _M_waiters, then loads A. Without this + // fence the two loads can both read stale values due to store buffering + // and if the notifier sees `_M_waiters == 0` the notification would be + // skipped, meaning the waiter would not get unblocked. + __atomic_thread_fence(__ATOMIC_SEQ_CST); __platform_wait_t __res; __atomic_load(&_M_waiters, &__res, __ATOMIC_SEQ_CST); return __res != 0; diff --git a/libstdc++-v3/testsuite/29_atomics/atomic/wait_notify/127757.cc b/libstdc++-v3/testsuite/29_atomics/atomic/wait_notify/127757.cc new file mode 100644 index 000000000000..a5c8dd11e6db --- /dev/null +++ b/libstdc++-v3/testsuite/29_atomics/atomic/wait_notify/127757.cc @@ -0,0 +1,129 @@ +// { dg-do run { target c++20 } } +// { dg-require-gthreads "" } +// { dg-additional-options "-pthread" { target pthread } } +// { dg-add-options libatomic } + +// A notify_all() call must not be lost when it follows the store that a +// waiter is waiting for. The library decides whether to issue the +// FUTEX_WAKE by checking a per-address waiter count; that check has to be +// ordered after the caller's store, otherwise the notifier can observe +// "no waiters" (store-buffering reorder) while the waiter observes the old +// value and blocks forever. +// +// This exercises the direct-futex path (a 4-byte scalar on Linux). A +// watchdog thread detects a waiter still blocked after the value already +// changed, recovers with an extra notify_all(), and records the failure. +// The reproduction is timing-dependent (it needs real hardware store +// buffering, seen on x86-64), but on a correct library it always completes +// quickly with no lost wakeups, so the test never produces a false failure. + +#include <atomic> +#include <chrono> +#include <thread> +#include <vector> +#include <cstddef> +#include <testsuite_hooks.h> + +int +main () +{ + using namespace std::chrono; + + constexpr unsigned iterations = 100000; + constexpr unsigned cold_stores = 64; // delay visibility of the store + constexpr unsigned jitter_mod = 4000; // place the notify near the + // waiter's spin -> block window + // Walked by the cold stores below so that each store misses at least the + // L2 cache on common hardware; that is what keeps the store to x pending + // long enough to expose the missing ordering. A fixed size cannot exceed + // every possible last-level cache, so reproduction is best-effort, but the + // test always passes quickly on a correct library. + std::vector<char> buffer (std::size_t (32) << 20); + + alignas(64) std::atomic<unsigned> x{0}; + alignas(64) std::atomic<unsigned> go{0}; + alignas(64) std::atomic<unsigned> done{0}; + std::atomic<bool> stop{false}; + std::atomic<bool> failed{false}; + + std::thread waiter([&] { + for (unsigned i = 0; i < iterations; ++i) + { + if (failed.load(std::memory_order_acquire)) + break; + go.store(i + 1, std::memory_order_release); + x.wait(i, std::memory_order_acquire); + done.store(i + 1, std::memory_order_release); + } + }); + + // Keep x's cache line bouncing between cores, which keeps the notifier's + // store to x in its store buffer a little longer and widens the window. + std::thread stirrer([&] { + while (!stop.load(std::memory_order_relaxed)) + (void) x.load(std::memory_order_relaxed); + }); + + // Watchdog: if the waiter has not made progress for a while although x + // already holds the value it is waiting to leave, its wakeup was lost. + std::thread watchdog([&] { + unsigned last = 0; + auto last_progress = steady_clock::now(); + while (done.load(std::memory_order_acquire) != iterations + && !failed.load(std::memory_order_acquire)) + { + std::this_thread::sleep_for(milliseconds(5)); + const unsigned d = done.load(std::memory_order_acquire); + if (d != last) + { + last = d; + last_progress = steady_clock::now(); + } + else if (steady_clock::now() - last_progress > milliseconds(500) + && x.load(std::memory_order_acquire) == d + 1) + { + failed.store(true, std::memory_order_release); + x.notify_all(); // release the stuck waiter + } + } + }); + + unsigned seed = 1; + std::size_t pos = 0; + volatile unsigned spin_sink = 0; + for (unsigned i = 0; i < iterations; ++i) + { + while (go.load(std::memory_order_acquire) != i + 1) + if (failed.load(std::memory_order_acquire)) + break; + if (failed.load(std::memory_order_acquire)) + break; + + seed = seed * 1103515245 + 12345; + for (unsigned d = (seed >> 16) % jitter_mod; d != 0; --d) + spin_sink = spin_sink + 1; + for (unsigned k = 0; k < cold_stores; ++k) + { + buffer[pos] = static_cast<char>(i); + pos = (pos + 4160) % buffer.size(); + } + + x.store(i + 1, std::memory_order_release); + x.notify_all(); + + while (done.load(std::memory_order_acquire) != i + 1) + if (failed.load(std::memory_order_acquire)) + break; + } + + stop.store(true, std::memory_order_relaxed); + watchdog.join(); + stirrer.join(); + // The waiter may be parked in its final wait; wake it so it can exit. + x.store(iterations + 1, std::memory_order_release); + x.notify_all(); + waiter.join(); + + VERIFY( !failed.load() ); + return 0; +}
