From d03e1342a4c24206f4f2020e78a008e5bd9a7e0a Mon Sep 17 00:00:00 2001 From: Hui Date: Wed, 24 Jan 2024 08:38:00 +0000 Subject: [PATCH 1/4] [libc++] fix `counting_semaphore` lost wakeups --- libcxx/include/__atomic/atomic_sync.h | 16 +++-- libcxx/include/semaphore | 21 ++++-- .../thread.semaphore/lost_wakeup.pass.cpp | 65 +++++++++++++++++++ 3 files changed, 92 insertions(+), 10 deletions(-) create mode 100644 libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp diff --git a/libcxx/include/__atomic/atomic_sync.h b/libcxx/include/__atomic/atomic_sync.h index 93527958b2e1c..ba83396189edf 100644 --- a/libcxx/include/__atomic/atomic_sync.h +++ b/libcxx/include/__atomic/atomic_sync.h @@ -46,12 +46,12 @@ __libcpp_atomic_wait(__cxx_atomic_contention_t const volatile*, __cxx_contention template struct __libcpp_atomic_wait_backoff_impl { _Atp* __a; - _Fn __test_fn; + _Fn __test_with_old; _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool operator()(chrono::nanoseconds __elapsed) const { if (__elapsed > chrono::microseconds(64)) { - auto const __monitor = std::__libcpp_atomic_monitor(__a); - if (__test_fn()) + auto __monitor = std::__libcpp_atomic_monitor(__a); + if (__test_with_old(__monitor)) return true; std::__libcpp_atomic_wait(__a, __monitor); } else if (__elapsed > chrono::microseconds(4)) @@ -62,9 +62,17 @@ struct __libcpp_atomic_wait_backoff_impl { } }; +template +_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool +__cxx_atomic_wait(_Atp* __a, _Fn&& __test_fn, _Fn2&& __test_with_old) { + __libcpp_atomic_wait_backoff_impl<_Atp, __decay_t<_Fn2> > __backoff_fn = {__a, __test_with_old}; + return std::__libcpp_thread_poll_with_backoff(__test_fn, __backoff_fn); +} + template _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool __cxx_atomic_wait(_Atp* __a, _Fn&& __test_fn) { - __libcpp_atomic_wait_backoff_impl<_Atp, __decay_t<_Fn> > __backoff_fn = {__a, __test_fn}; + auto __test_with_old = [&](auto&) { return __test_fn(); }; + __libcpp_atomic_wait_backoff_impl<_Atp, decltype(__test_with_old)> __backoff_fn{__a, __test_with_old}; return std::__libcpp_thread_poll_with_backoff(__test_fn, __backoff_fn); } diff --git a/libcxx/include/semaphore b/libcxx/include/semaphore index 649705f45b049..c4d9ee1ab63e8 100644 --- a/libcxx/include/semaphore +++ b/libcxx/include/semaphore @@ -54,6 +54,7 @@ using binary_semaphore = counting_semaphore<1>; #include <__assert> // all public C++ headers provide the assertion handler #include <__atomic/atomic_base.h> #include <__atomic/atomic_sync.h> +#include <__atomic/contention_t.h> #include <__atomic/memory_order.h> #include <__availability> #include <__chrono/time_point.h> @@ -95,19 +96,22 @@ public: _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) + if (__old == 0) { __a_.notify_all(); - 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 { + ptrdiff_t __old = __monitor; + bool __r = __try_acquire_impl(__old); + __monitor = static_cast<__cxx_contention_t>(__old); + return __r; + }; + __cxx_atomic_wait(&__a_.__a_, __test_fn, __test_with_old); } template _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool @@ -119,6 +123,11 @@ public: } _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool try_acquire() { auto __old = __a_.load(memory_order_acquire); + return __try_acquire_impl(__old); + } + +private: + _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY bool __try_acquire_impl(ptrdiff_t& __old) { while (true) { if (__old == 0) return false; diff --git a/libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp b/libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp new file mode 100644 index 0000000000000..aa21e93221b31 --- /dev/null +++ b/libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp @@ -0,0 +1,65 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +// UNSUPPORTED: libcpp-has-no-threads +// UNSUPPORTED: c++03, c++11, c++14, c++17 + +// This test requires the dylib support introduced in D68480, which shipped in macOS 11.0. +// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14|15}} + +// This is a regression test for https://llvm.org/PR47013. + +// + +#include +#include +#include +#include + +#include "make_test_thread.h" + +static std::counting_semaphore<> s(0); +static std::barrier<> b(8 + 1); + +void acquire() { + for (int i = 0; i < 10'000; ++i) { + s.acquire(); + b.arrive_and_wait(); + } +} + +void release() { + for (int i = 0; i < 10'000; ++i) { + s.release(1); + s.release(1); + s.release(1); + s.release(1); + + s.release(1); + s.release(1); + s.release(1); + s.release(1); + + b.arrive_and_wait(); + } +} + +int main(int, char**) { + for (int run = 0; run < 20; ++run) { + std::vector threads; + for (int i = 0; i < 8; ++i) + threads.push_back(support::make_test_thread(acquire)); + + threads.push_back(support::make_test_thread(release)); + + for (auto& thread : threads) + thread.join(); + } + + return 0; +} From 7f731e892fe91751d8691556b32f261403284c3b Mon Sep 17 00:00:00 2001 From: Hui Date: Wed, 31 Jan 2024 16:17:28 +0000 Subject: [PATCH 2/4] address feedback --- libcxx/include/__atomic/atomic_sync.h | 38 ++++++++++++++++----------- libcxx/include/semaphore | 8 +++--- 2 files changed, 28 insertions(+), 18 deletions(-) diff --git a/libcxx/include/__atomic/atomic_sync.h b/libcxx/include/__atomic/atomic_sync.h index ba83396189edf..e1994ddde86c1 100644 --- a/libcxx/include/__atomic/atomic_sync.h +++ b/libcxx/include/__atomic/atomic_sync.h @@ -43,17 +43,17 @@ __libcpp_atomic_monitor(__cxx_atomic_contention_t const volatile*); _LIBCPP_AVAILABILITY_SYNC _LIBCPP_EXPORTED_FROM_ABI void __libcpp_atomic_wait(__cxx_atomic_contention_t const volatile*, __cxx_contention_t); -template +template struct __libcpp_atomic_wait_backoff_impl { - _Atp* __a; - _Fn __test_with_old; + _Atp* __a_; + _BackoffTest __backoff_test_; _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool operator()(chrono::nanoseconds __elapsed) const { if (__elapsed > chrono::microseconds(64)) { - auto __monitor = std::__libcpp_atomic_monitor(__a); - if (__test_with_old(__monitor)) + auto __monitor = std::__libcpp_atomic_monitor(__a_); + if (__backoff_test_(__monitor)) return true; - std::__libcpp_atomic_wait(__a, __monitor); + std::__libcpp_atomic_wait(__a_, __monitor); } else if (__elapsed > chrono::microseconds(4)) __libcpp_thread_yield(); else { @@ -62,18 +62,26 @@ struct __libcpp_atomic_wait_backoff_impl { } }; -template +template _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool -__cxx_atomic_wait(_Atp* __a, _Fn&& __test_fn, _Fn2&& __test_with_old) { - __libcpp_atomic_wait_backoff_impl<_Atp, __decay_t<_Fn2> > __backoff_fn = {__a, __test_with_old}; - return std::__libcpp_thread_poll_with_backoff(__test_fn, __backoff_fn); +__cxx_atomic_wait(_Atp* __a, _Poll&& __poll, _BackoffTest&& __backoff_test) { + __libcpp_atomic_wait_backoff_impl<_Atp, __decay_t<_BackoffTest> > __backoff_fn = {__a, __backoff_test}; + return std::__libcpp_thread_poll_with_backoff(__poll, __backoff_fn); } -template -_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool __cxx_atomic_wait(_Atp* __a, _Fn&& __test_fn) { - auto __test_with_old = [&](auto&) { return __test_fn(); }; - __libcpp_atomic_wait_backoff_impl<_Atp, decltype(__test_with_old)> __backoff_fn{__a, __test_with_old}; - return std::__libcpp_thread_poll_with_backoff(__test_fn, __backoff_fn); +template +struct __libcpp_atomic_wait_poll_as_backoff_test { + _Poll __poll_; + + _LIBCPP_AVAILABILITY_SYNC + _LIBCPP_HIDE_FROM_ABI bool operator()(__cxx_contention_t&) const { return __poll_(); } +}; + +template +_LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool __cxx_atomic_wait(_Atp* __a, _Poll&& __poll) { + __libcpp_atomic_wait_backoff_impl<_Atp, __libcpp_atomic_wait_poll_as_backoff_test<_Poll&> > __backoff_fn = { + __a, {__poll}}; + return std::__libcpp_thread_poll_with_backoff(__poll, __backoff_fn); } #else // _LIBCPP_HAS_NO_THREADS diff --git a/libcxx/include/semaphore b/libcxx/include/semaphore index c4d9ee1ab63e8..2b317daa7fead 100644 --- a/libcxx/include/semaphore +++ b/libcxx/include/semaphore @@ -95,9 +95,11 @@ 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) { + (void)__old; + if (__update > 1) { __a_.notify_all(); + } else { + __a_.notify_one(); } } _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void acquire() { @@ -127,7 +129,7 @@ public: } private: - _LIBCPP_AVAILABILITY_SYNC _LIBCPP_INLINE_VISIBILITY bool __try_acquire_impl(ptrdiff_t& __old) { + _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool __try_acquire_impl(ptrdiff_t& __old) { while (true) { if (__old == 0) return false; From 280b9002938bfc6407292ebc3f539b68a6676146 Mon Sep 17 00:00:00 2001 From: Hui Date: Wed, 31 Jan 2024 22:26:32 +0000 Subject: [PATCH 3/4] rename --- libcxx/include/semaphore | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/libcxx/include/semaphore b/libcxx/include/semaphore index 2b317daa7fead..5fd06ed09e4df 100644 --- a/libcxx/include/semaphore +++ b/libcxx/include/semaphore @@ -103,25 +103,25 @@ public: } } _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void acquire() { - auto const __test_fn = [this]() -> bool { + auto const __poll_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); }; - auto const __test_with_old = [this](__cxx_contention_t& __monitor) -> bool { + auto const __backoff_test = [this](__cxx_contention_t& __monitor) -> bool { ptrdiff_t __old = __monitor; bool __r = __try_acquire_impl(__old); __monitor = static_cast<__cxx_contention_t>(__old); return __r; }; - __cxx_atomic_wait(&__a_.__a_, __test_fn, __test_with_old); + __cxx_atomic_wait(&__a_.__a_, __poll_fn, __backoff_test); } template _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool try_acquire_for(chrono::duration<_Rep, _Period> const& __rel_time) { if (__rel_time == chrono::duration<_Rep, _Period>::zero()) return try_acquire(); - auto const __test_fn = [this]() { return try_acquire(); }; - return std::__libcpp_thread_poll_with_backoff(__test_fn, __libcpp_timed_backoff_policy(), __rel_time); + auto const __poll_fn = [this]() { return try_acquire(); }; + return std::__libcpp_thread_poll_with_backoff(__poll_fn, __libcpp_timed_backoff_policy(), __rel_time); } _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI bool try_acquire() { auto __old = __a_.load(memory_order_acquire); From 3dbe2326e3227f1ecb81baaad665eaee192b1fa5 Mon Sep 17 00:00:00 2001 From: Hui Date: Sat, 3 Feb 2024 13:36:50 +0000 Subject: [PATCH 4/4] CI --- libcxx/include/semaphore | 5 +---- libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp | 5 ++--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/libcxx/include/semaphore b/libcxx/include/semaphore index 5fd06ed09e4df..5235d720bf6fe 100644 --- a/libcxx/include/semaphore +++ b/libcxx/include/semaphore @@ -95,11 +95,8 @@ 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"); - (void)__old; - if (__update > 1) { + if (__old == 0) { __a_.notify_all(); - } else { - __a_.notify_one(); } } _LIBCPP_AVAILABILITY_SYNC _LIBCPP_HIDE_FROM_ABI void acquire() { diff --git a/libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp b/libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp index aa21e93221b31..dca3c01cf0103 100644 --- a/libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp +++ b/libcxx/test/std/thread/thread.semaphore/lost_wakeup.pass.cpp @@ -6,11 +6,10 @@ // //===----------------------------------------------------------------------===// -// UNSUPPORTED: libcpp-has-no-threads +// UNSUPPORTED: no-threads // UNSUPPORTED: c++03, c++11, c++14, c++17 -// This test requires the dylib support introduced in D68480, which shipped in macOS 11.0. -// XFAIL: use_system_cxx_lib && target={{.+}}-apple-macosx10.{{9|10|11|12|13|14|15}} +// XFAIL: availability-synchronization_library-missing // This is a regression test for https://llvm.org/PR47013.