From 24922f3d2746534b1b4365aa6a8d92fc356f5b37 Mon Sep 17 00:00:00 2001 From: cpplearner Date: Mon, 12 Jun 2023 19:50:13 +0800 Subject: [PATCH 01/12] `mutex.cpp`, `cond.cpp`: Use static dispatch --- stl/src/cond.cpp | 4 ++-- stl/src/mutex.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/stl/src/cond.cpp b/stl/src/cond.cpp index 7545c6805c..ed334bd626 100644 --- a/stl/src/cond.cpp +++ b/stl/src/cond.cpp @@ -15,9 +15,9 @@ struct _Cnd_internal_imp_t { // condition variable implementation for ConcRT typename std::_Aligned_storage::type cv; - [[nodiscard]] Concurrency::details::stl_condition_variable_interface* _get_cv() noexcept { + [[nodiscard]] Concurrency::details::stl_condition_variable_win7* _get_cv() noexcept { // get pointer to implementation - return reinterpret_cast(&cv); + return reinterpret_cast(&cv); } }; diff --git a/stl/src/mutex.cpp b/stl/src/mutex.cpp index a58090ebef..07827bce60 100644 --- a/stl/src/mutex.cpp +++ b/stl/src/mutex.cpp @@ -42,8 +42,8 @@ struct _Mtx_internal_imp_t { // ConcRT mutex Concurrency::details::stl_critical_section_max_alignment>::type cs; long thread_id; int count; - Concurrency::details::stl_critical_section_interface* _get_cs() { // get pointer to implementation - return reinterpret_cast(&cs); + [[nodiscard]] Concurrency::details::stl_critical_section_win7* _get_cs() { // get pointer to implementation + return reinterpret_cast(&cs); } }; From e952e5ae0ea9dc8c38a534a17f32608976cc4ee6 Mon Sep 17 00:00:00 2001 From: cpplearner Date: Wed, 14 Jun 2023 14:10:54 +0800 Subject: [PATCH 02/12] Create non-polymorphic classes --- stl/src/primitives.hpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/stl/src/primitives.hpp b/stl/src/primitives.hpp index f3e2d99920..bc5dc006ac 100644 --- a/stl/src/primitives.hpp +++ b/stl/src/primitives.hpp @@ -100,12 +100,22 @@ namespace Concurrency { CONDITION_VARIABLE m_condition_variable; }; + struct stl_critical_section { + void* unused = nullptr; + SRWLOCK m_srw_lock = SRWLOCK_INIT; + }; + inline void create_stl_critical_section(stl_critical_section_interface* p) { - new (p) stl_critical_section_win7; + new (p) stl_critical_section; } + struct stl_condition_variable { + void* unused = nullptr; + CONDITION_VARIABLE m_condition_variable = CONDITION_VARIABLE_INIT; + }; + inline void create_stl_condition_variable(stl_condition_variable_interface* p) { - new (p) stl_condition_variable_win7; + new (p) stl_condition_variable; } #ifdef _WIN64 From c3114d1876eba6eb6b066f335f3067c9cae41a7d Mon Sep 17 00:00:00 2001 From: cpplearner Date: Thu, 15 Jun 2023 11:03:16 +0800 Subject: [PATCH 03/12] Alternative approach --- stl/src/cond.cpp | 4 +-- stl/src/primitives.hpp | 72 ++++++++++++------------------------------ 2 files changed, 23 insertions(+), 53 deletions(-) diff --git a/stl/src/cond.cpp b/stl/src/cond.cpp index ed334bd626..7c9b41385c 100644 --- a/stl/src/cond.cpp +++ b/stl/src/cond.cpp @@ -53,7 +53,7 @@ void _Cnd_destroy(const _Cnd_t cond) { // clean up } int _Cnd_wait(const _Cnd_t cond, const _Mtx_t mtx) { // wait until signaled - const auto cs = static_cast(_Mtx_getconcrtcs(mtx)); + const auto cs = static_cast(_Mtx_getconcrtcs(mtx)); _Mtx_clear_owner(mtx); cond->_get_cv()->wait(cs); _Mtx_reset_owner(mtx); @@ -63,7 +63,7 @@ int _Cnd_wait(const _Cnd_t cond, const _Mtx_t mtx) { // wait until signaled // wait until signaled or timeout int _Cnd_timedwait(const _Cnd_t cond, const _Mtx_t mtx, const _timespec64* const target) { int res = _Thrd_success; - const auto cs = static_cast(_Mtx_getconcrtcs(mtx)); + const auto cs = static_cast(_Mtx_getconcrtcs(mtx)); if (target == nullptr) { // no target time specified, wait on mutex _Mtx_clear_owner(mtx); cond->_get_cv()->wait(cs); diff --git a/stl/src/primitives.hpp b/stl/src/primitives.hpp index bc5dc006ac..1f648dc447 100644 --- a/stl/src/primitives.hpp +++ b/stl/src/primitives.hpp @@ -10,25 +10,7 @@ namespace Concurrency { namespace details { - class __declspec(novtable) stl_critical_section_interface { - public: - virtual void lock() = 0; - virtual bool try_lock() = 0; - virtual bool try_lock_for(unsigned int) = 0; - virtual void unlock() = 0; - virtual void destroy() = 0; - }; - - class __declspec(novtable) stl_condition_variable_interface { - public: - virtual void wait(stl_critical_section_interface*) = 0; - virtual bool wait_for(stl_critical_section_interface*, unsigned int) = 0; - virtual void notify_one() = 0; - virtual void notify_all() = 0; - virtual void destroy() = 0; - }; - - class stl_critical_section_win7 final : public stl_critical_section_interface { + class stl_critical_section_win7 { public: stl_critical_section_win7() = default; @@ -36,22 +18,22 @@ namespace Concurrency { stl_critical_section_win7(const stl_critical_section_win7&) = delete; stl_critical_section_win7& operator=(const stl_critical_section_win7&) = delete; - void destroy() override {} + void destroy() {} - void lock() override { + void lock() { AcquireSRWLockExclusive(&m_srw_lock); } - bool try_lock() override { + bool try_lock() { return TryAcquireSRWLockExclusive(&m_srw_lock) != 0; } - bool try_lock_for(unsigned int) override { + bool try_lock_for(unsigned int) { // STL will call try_lock_for once again if this call will not succeed return stl_critical_section_win7::try_lock(); } - void unlock() override { + void unlock() { _Analysis_assume_lock_held_(m_srw_lock); ReleaseSRWLockExclusive(&m_srw_lock); } @@ -61,61 +43,49 @@ namespace Concurrency { } private: + void* unused = nullptr; // TRANSITON, ABI: was the vptr SRWLOCK m_srw_lock = SRWLOCK_INIT; }; - class stl_condition_variable_win7 final : public stl_condition_variable_interface { + class stl_condition_variable_win7 { public: - stl_condition_variable_win7() { - InitializeConditionVariable(&m_condition_variable); - } + stl_condition_variable_win7() = default; ~stl_condition_variable_win7() = delete; stl_condition_variable_win7(const stl_condition_variable_win7&) = delete; stl_condition_variable_win7& operator=(const stl_condition_variable_win7&) = delete; - void destroy() override {} + void destroy() {} - void wait(stl_critical_section_interface* lock) override { + void wait(stl_critical_section_win7* lock) { if (!stl_condition_variable_win7::wait_for(lock, INFINITE)) { std::terminate(); } } - bool wait_for(stl_critical_section_interface* lock, unsigned int timeout) override { - return SleepConditionVariableSRW(&m_condition_variable, - static_cast(lock)->native_handle(), timeout, 0) - != 0; + bool wait_for(stl_critical_section_win7* lock, unsigned int timeout) { + return SleepConditionVariableSRW(&m_condition_variable, lock->native_handle(), timeout, 0) != 0; } - void notify_one() override { + void notify_one() { WakeConditionVariable(&m_condition_variable); } - void notify_all() override { + void notify_all() { WakeAllConditionVariable(&m_condition_variable); } private: - CONDITION_VARIABLE m_condition_variable; - }; - - struct stl_critical_section { - void* unused = nullptr; - SRWLOCK m_srw_lock = SRWLOCK_INIT; + void* unused = nullptr; // TRANSITON, ABI: was the vptr + CONDITION_VARIABLE m_condition_variable = CONDITION_VARIABLE_INIT; }; - inline void create_stl_critical_section(stl_critical_section_interface* p) { - new (p) stl_critical_section; + inline void create_stl_critical_section(stl_critical_section_win7* p) { + new (p) stl_critical_section_win7; } - struct stl_condition_variable { - void* unused = nullptr; - CONDITION_VARIABLE m_condition_variable = CONDITION_VARIABLE_INIT; - }; - - inline void create_stl_condition_variable(stl_condition_variable_interface* p) { - new (p) stl_condition_variable; + inline void create_stl_condition_variable(stl_condition_variable_win7* p) { + new (p) stl_condition_variable_win7; } #ifdef _WIN64 From 13f90f1fc88d2b4a7ed7b305725a2481f708de0a Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 16 Jun 2023 15:28:39 -0700 Subject: [PATCH 04/12] `TRANSITON` => `TRANSITION` --- stl/src/primitives.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stl/src/primitives.hpp b/stl/src/primitives.hpp index acfb362061..2642a07e80 100644 --- a/stl/src/primitives.hpp +++ b/stl/src/primitives.hpp @@ -43,7 +43,7 @@ namespace Concurrency { } private: - void* unused = nullptr; // TRANSITON, ABI: was the vptr + void* unused = nullptr; // TRANSITION, ABI: was the vptr SRWLOCK m_srw_lock = SRWLOCK_INIT; }; @@ -76,7 +76,7 @@ namespace Concurrency { } private: - void* unused = nullptr; // TRANSITON, ABI: was the vptr + void* unused = nullptr; // TRANSITION, ABI: was the vptr CONDITION_VARIABLE m_condition_variable = CONDITION_VARIABLE_INIT; }; From fd49a2f42293bda72ebff74d40284b1a961d6ae7 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 16 Jun 2023 15:38:08 -0700 Subject: [PATCH 05/12] When `wait` calls `wait_for`, we don't need to qualify it anymore. --- stl/src/primitives.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/src/primitives.hpp b/stl/src/primitives.hpp index 2642a07e80..491594d2e5 100644 --- a/stl/src/primitives.hpp +++ b/stl/src/primitives.hpp @@ -58,7 +58,7 @@ namespace Concurrency { void destroy() {} void wait(stl_critical_section_win7* lock) { - if (!stl_condition_variable_win7::wait_for(lock, INFINITE)) { + if (!wait_for(lock, INFINITE)) { std::terminate(); } } From 36987850750a24c458cc8af407bb9a44b9ecd7f1 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 16 Jun 2023 15:58:49 -0700 Subject: [PATCH 06/12] Remove `destroy` for `stl_critical_section_win7` and `stl_condition_variable_win7`. --- stl/src/cond.cpp | 4 +--- stl/src/mutex.cpp | 2 +- stl/src/primitives.hpp | 4 ---- 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/stl/src/cond.cpp b/stl/src/cond.cpp index 4160441da3..153b59974e 100644 --- a/stl/src/cond.cpp +++ b/stl/src/cond.cpp @@ -28,9 +28,7 @@ void _Cnd_init_in_situ(const _Cnd_t cond) { // initialize condition variable in Concurrency::details::create_stl_condition_variable(cond->_get_cv()); } -void _Cnd_destroy_in_situ(const _Cnd_t cond) { // destroy condition variable in situ - cond->_get_cv()->destroy(); -} +void _Cnd_destroy_in_situ(_Cnd_t) {} // destroy condition variable in situ int _Cnd_init(_Cnd_t* const pcond) { // initialize *pcond = nullptr; diff --git a/stl/src/mutex.cpp b/stl/src/mutex.cpp index 15dc1e075f..fea8d4eccf 100644 --- a/stl/src/mutex.cpp +++ b/stl/src/mutex.cpp @@ -65,7 +65,7 @@ void _Mtx_init_in_situ(_Mtx_t mtx, int type) { // initialize mutex in situ void _Mtx_destroy_in_situ(_Mtx_t mtx) { // destroy mutex in situ _THREAD_ASSERT(mtx->count == 0, "mutex destroyed while busy"); - mtx->_get_cs()->destroy(); + (void) mtx; } int _Mtx_init(_Mtx_t* mtx, int type) { // initialize mutex diff --git a/stl/src/primitives.hpp b/stl/src/primitives.hpp index 491594d2e5..b1873251c3 100644 --- a/stl/src/primitives.hpp +++ b/stl/src/primitives.hpp @@ -18,8 +18,6 @@ namespace Concurrency { stl_critical_section_win7(const stl_critical_section_win7&) = delete; stl_critical_section_win7& operator=(const stl_critical_section_win7&) = delete; - void destroy() {} - void lock() { AcquireSRWLockExclusive(&m_srw_lock); } @@ -55,8 +53,6 @@ namespace Concurrency { stl_condition_variable_win7(const stl_condition_variable_win7&) = delete; stl_condition_variable_win7& operator=(const stl_condition_variable_win7&) = delete; - void destroy() {} - void wait(stl_critical_section_win7* lock) { if (!wait_for(lock, INFINITE)) { std::terminate(); From 53656321b0a51b69e972be57110039fc462ed111 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 16 Jun 2023 16:08:18 -0700 Subject: [PATCH 07/12] Drop `stl_critical_section_win7::try_lock_for`, directly call `try_lock`. --- stl/src/mutex.cpp | 2 +- stl/src/primitives.hpp | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/stl/src/mutex.cpp b/stl/src/mutex.cpp index fea8d4eccf..6deb0e8ebd 100644 --- a/stl/src/mutex.cpp +++ b/stl/src/mutex.cpp @@ -126,7 +126,7 @@ static int mtx_do_lock(_Mtx_t mtx, const _timespec64* target) { // lock mutex while (now.tv_sec < target->tv_sec || now.tv_sec == target->tv_sec && now.tv_nsec < target->tv_nsec) { // time has not expired if (mtx->thread_id == static_cast(GetCurrentThreadId()) - || mtx->_get_cs()->try_lock_for(_Xtime_diff_to_millis2(target, &now))) { // stop waiting + || mtx->_get_cs()->try_lock()) { // stop waiting res = WAIT_OBJECT_0; break; } else { diff --git a/stl/src/primitives.hpp b/stl/src/primitives.hpp index b1873251c3..b3d2db3236 100644 --- a/stl/src/primitives.hpp +++ b/stl/src/primitives.hpp @@ -26,11 +26,6 @@ namespace Concurrency { return TryAcquireSRWLockExclusive(&m_srw_lock) != 0; } - bool try_lock_for(unsigned int) { - // STL will call try_lock_for once again if this call will not succeed - return stl_critical_section_win7::try_lock(); - } - void unlock() { _Analysis_assume_lock_held_(m_srw_lock); ReleaseSRWLockExclusive(&m_srw_lock); From 589f8d71c8f904bb295df804081579c9e799ba32 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 16 Jun 2023 16:43:43 -0700 Subject: [PATCH 08/12] `=delete` `native_handle()` for the `mutex` family and `condition_variable`. We never provided user-visible declarations for what they returned. It's extremely unlikely that users were successfully calling them in any useful way, so the bincompat risk of nulling out the vptrs is minimal. I am opting to `=delete` these functions, instead of simply not declaring them, so that attempted calls will have a slightly more comprehensible error message. As the TRANSITION comments note, in theory we could provide implementations that return the underlying Win32 machinery which actually would be possible to document. I am also changing `condition_variable`'s `native_handle_type` from `_Cnd_t` (an alias provided by `` for `_Cnd_internal_imp_t*`, which is forward-declared and never defined for users) to `void*` and grouping it with the function, which follows the Standard's order. --- stl/inc/mutex | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/stl/inc/mutex b/stl/inc/mutex index 1d4faf58c4..5063824bd9 100644 --- a/stl/inc/mutex +++ b/stl/inc/mutex @@ -95,9 +95,8 @@ public: using native_handle_type = void*; - _NODISCARD native_handle_type native_handle() noexcept /* strengthened */ { - return _Mtx_getconcrtcs(_Mymtx()); - } + // TRANSITION, native_handle() could return a pointer to the underlying SRWLOCK + _NODISCARD native_handle_type native_handle() noexcept /* strengthened */ = delete; protected: _NODISCARD_TRY_CHANGE_STATE bool _Verify_ownership_levels() noexcept { @@ -636,8 +635,6 @@ _EXPORT_STD enum class cv_status { // names for wait returns _EXPORT_STD class condition_variable { // class for waiting for conditions public: - using native_handle_type = _Cnd_t; - condition_variable() noexcept /* strengthened */ { _Cnd_init_in_situ(_Mycnd()); } @@ -725,9 +722,10 @@ public: return _Wait_until1(_Lck, _Abs_time, _Pred); } - _NODISCARD native_handle_type native_handle() noexcept /* strengthened */ { - return _Mycnd(); - } + using native_handle_type = void*; + + // TRANSITION, native_handle() could return a pointer to the underlying CONDITION_VARIABLE + _NODISCARD native_handle_type native_handle() noexcept /* strengthened */ = delete; void _Register(unique_lock& _Lck, int* _Ready) noexcept { // register this object for release at thread exit _Cnd_register_at_thread_exit(_Mycnd(), _Lck.release()->_Mymtx(), _Ready); From 0578610035f2fde390652fe3996994f92f7599a6 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 16 Jun 2023 19:53:31 -0700 Subject: [PATCH 09/12] Remove test coverage for deleted native_handle(). --- tests/std/tests/Dev11_1150223_shared_mutex/test.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/std/tests/Dev11_1150223_shared_mutex/test.cpp b/tests/std/tests/Dev11_1150223_shared_mutex/test.cpp index 44ca1f2b6f..4a5ecdf278 100644 --- a/tests/std/tests/Dev11_1150223_shared_mutex/test.cpp +++ b/tests/std/tests/Dev11_1150223_shared_mutex/test.cpp @@ -58,10 +58,7 @@ STATIC_ASSERT(noexcept(declval().native_handle())); #if _HAS_CXX20 STATIC_ASSERT(noexcept(declval().native_handle())); #endif // _HAS_CXX20 -STATIC_ASSERT(noexcept(declval().native_handle())); -STATIC_ASSERT(noexcept(declval().native_handle())); STATIC_ASSERT(noexcept(declval().native_handle())); -STATIC_ASSERT(noexcept(declval().native_handle())); // Also test mandatory and strengthened exception specification for try_lock(). STATIC_ASSERT(noexcept(declval().try_lock())); // strengthened From 34fdc1716b58f5845b5848dc3f6297423a23d604 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Tue, 20 Jun 2023 19:48:31 -0700 Subject: [PATCH 10/12] We don't actually need vptr placeholders. --- stl/src/primitives.hpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/stl/src/primitives.hpp b/stl/src/primitives.hpp index b3d2db3236..4d347dbbea 100644 --- a/stl/src/primitives.hpp +++ b/stl/src/primitives.hpp @@ -36,7 +36,6 @@ namespace Concurrency { } private: - void* unused = nullptr; // TRANSITION, ABI: was the vptr SRWLOCK m_srw_lock = SRWLOCK_INIT; }; @@ -67,7 +66,6 @@ namespace Concurrency { } private: - void* unused = nullptr; // TRANSITION, ABI: was the vptr CONDITION_VARIABLE m_condition_variable = CONDITION_VARIABLE_INIT; }; From a62223f3d77066228af09e1051c675823d7e5683 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Wed, 21 Jun 2023 11:21:10 -0700 Subject: [PATCH 11/12] Revert "We don't actually need vptr placeholders." This reverts commit 34fdc1716b58f5845b5848dc3f6297423a23d604. --- stl/src/primitives.hpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/stl/src/primitives.hpp b/stl/src/primitives.hpp index 4d347dbbea..b3d2db3236 100644 --- a/stl/src/primitives.hpp +++ b/stl/src/primitives.hpp @@ -36,6 +36,7 @@ namespace Concurrency { } private: + void* unused = nullptr; // TRANSITION, ABI: was the vptr SRWLOCK m_srw_lock = SRWLOCK_INIT; }; @@ -66,6 +67,7 @@ namespace Concurrency { } private: + void* unused = nullptr; // TRANSITION, ABI: was the vptr CONDITION_VARIABLE m_condition_variable = CONDITION_VARIABLE_INIT; }; From f3499b7290b68379fd7bf353909dccf75a0a38ad Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Wed, 21 Jun 2023 18:24:11 -0700 Subject: [PATCH 12/12] Remove native_handle_type and native_handle(). --- stl/inc/mutex | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/stl/inc/mutex b/stl/inc/mutex index 5063824bd9..ed72ac0834 100644 --- a/stl/inc/mutex +++ b/stl/inc/mutex @@ -93,10 +93,7 @@ public: _Mtx_unlock(_Mymtx()); } - using native_handle_type = void*; - - // TRANSITION, native_handle() could return a pointer to the underlying SRWLOCK - _NODISCARD native_handle_type native_handle() noexcept /* strengthened */ = delete; + // native_handle_type and native_handle() have intentionally been removed. See GH-3820. protected: _NODISCARD_TRY_CHANGE_STATE bool _Verify_ownership_levels() noexcept { @@ -722,10 +719,7 @@ public: return _Wait_until1(_Lck, _Abs_time, _Pred); } - using native_handle_type = void*; - - // TRANSITION, native_handle() could return a pointer to the underlying CONDITION_VARIABLE - _NODISCARD native_handle_type native_handle() noexcept /* strengthened */ = delete; + // native_handle_type and native_handle() have intentionally been removed. See GH-3820. void _Register(unique_lock& _Lck, int* _Ready) noexcept { // register this object for release at thread exit _Cnd_register_at_thread_exit(_Mycnd(), _Lck.release()->_Mymtx(), _Ready);