From 77a33b306548343716a3b223f8b57cae9a23e6d4 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sun, 9 Jun 2024 04:01:25 -0700 Subject: [PATCH 01/13] mutex.cpp doesn't use anything from primitives.hpp. --- stl/src/mutex.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/stl/src/mutex.cpp b/stl/src/mutex.cpp index 0d8c4fa387..8d7e26f5c4 100644 --- a/stl/src/mutex.cpp +++ b/stl/src/mutex.cpp @@ -10,8 +10,6 @@ #include #include -#include "primitives.hpp" - extern "C" { // TRANSITION, ABI: exported only for ABI compat From 5c52c087ac4e7551a4017b3fc836a82e1bed857f Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sun, 9 Jun 2024 07:07:32 -0700 Subject: [PATCH 02/13] primitives.hpp doesn't need ``. --- stl/src/primitives.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/stl/src/primitives.hpp b/stl/src/primitives.hpp index ff9b593725..4d222c6277 100644 --- a/stl/src/primitives.hpp +++ b/stl/src/primitives.hpp @@ -5,7 +5,6 @@ #include <__msvc_threads_core.hpp> #include -#include #include From fdbd37ea8eb3aac1fc4384356e95c079be3e18db Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sun, 9 Jun 2024 04:11:40 -0700 Subject: [PATCH 03/13] Stop calling no-op `_Cnd_destroy_in_situ()`. `~condition_variable()` can be defaulted, making it trivially destructible. `~condition_variable_any()` can also be defaulted. It's destroying a `shared_ptr`, so it's not trivially destructible. Stop declaring `_Cnd_destroy_in_situ()` in xthreads.h. Note that this doesn't affect the dllexport surface, as we previously ensured that `_CRTIMP2_PURE` always appears on both declarations and definitions. Comment the definition of `_Cnd_destroy_in_situ()` in cond.cpp as preserved for bincompat. --- stl/inc/condition_variable | 4 +--- stl/inc/mutex | 4 +--- stl/inc/xthreads.h | 1 - stl/src/cond.cpp | 2 +- 4 files changed, 3 insertions(+), 8 deletions(-) diff --git a/stl/inc/condition_variable b/stl/inc/condition_variable index 29bd6f50fc..72d10ae804 100644 --- a/stl/inc/condition_variable +++ b/stl/inc/condition_variable @@ -55,9 +55,7 @@ public: _Cnd_init_in_situ(_Mycnd()); } - ~condition_variable_any() noexcept { - _Cnd_destroy_in_situ(_Mycnd()); - } + ~condition_variable_any() noexcept = default; condition_variable_any(const condition_variable_any&) = delete; condition_variable_any& operator=(const condition_variable_any&) = delete; diff --git a/stl/inc/mutex b/stl/inc/mutex index 3fdf82f199..0e4fdf3a6f 100644 --- a/stl/inc/mutex +++ b/stl/inc/mutex @@ -535,9 +535,7 @@ public: _Cnd_init_in_situ(_Mycnd()); } - ~condition_variable() noexcept { - _Cnd_destroy_in_situ(_Mycnd()); - } + ~condition_variable() noexcept = default; condition_variable(const condition_variable&) = delete; condition_variable& operator=(const condition_variable&) = delete; diff --git a/stl/inc/xthreads.h b/stl/inc/xthreads.h index 8681f40d08..65f936294d 100644 --- a/stl/inc/xthreads.h +++ b/stl/inc/xthreads.h @@ -60,7 +60,6 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_init(_Cnd_t*) noexcept; _CRTIMP2_PURE void __cdecl _Cnd_destroy(_Cnd_t) noexcept; #endif // _CRTBLD _CRTIMP2_PURE void __cdecl _Cnd_init_in_situ(_Cnd_t) noexcept; -_CRTIMP2_PURE void __cdecl _Cnd_destroy_in_situ(_Cnd_t) noexcept; _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_wait(_Cnd_t, _Mtx_t) noexcept; // TRANSITION, ABI: Always succeeds _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_broadcast(_Cnd_t) noexcept; // TRANSITION, ABI: Always succeeds _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_signal(_Cnd_t) noexcept; // TRANSITION, ABI: Always succeeds diff --git a/stl/src/cond.cpp b/stl/src/cond.cpp index 4cb255335f..e06749e28f 100644 --- a/stl/src/cond.cpp +++ b/stl/src/cond.cpp @@ -17,6 +17,7 @@ _CRTIMP2_PURE void __cdecl _Cnd_init_in_situ(const _Cnd_t cond) noexcept { // in new (Concurrency::details::_Get_cond_var(cond)) Concurrency::details::stl_condition_variable_win7; } +// TRANSITION, ABI: preserved for binary compatibility _CRTIMP2_PURE void __cdecl _Cnd_destroy_in_situ(_Cnd_t) noexcept {} // destroy condition variable in situ _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_init(_Cnd_t* const pcond) noexcept { // initialize @@ -34,7 +35,6 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_init(_Cnd_t* const pcond) noexcept { // _CRTIMP2_PURE void __cdecl _Cnd_destroy(const _Cnd_t cond) noexcept { // clean up if (cond) { // something to do, do it - _Cnd_destroy_in_situ(cond); _free_crt(cond); } } From f8cc4cfa937c60ec84b21aa79d1b5f066d918dba Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sun, 9 Jun 2024 05:11:13 -0700 Subject: [PATCH 04/13] Statically initialize `_Cnd_internal_imp_t`. This follows the example of `_Mtx_internal_imp_t` and `_Stl_critical_section` above. Define `_Stl_condition_variable` mirroring `Concurrency::details::stl_condition_variable_win7`. We can mirror `CONDITION_VARIABLE m_condition_variable = CONDITION_VARIABLE_INIT;` with `void* _Win_cv = nullptr;` because `CONDITION_VARIABLE` is just a struct wrapping `void*` (so it has the same size and alignment) and `CONDITION_VARIABLE_INIT` just initializes it to null. Then, use the `union` pattern to keep the size and alignment of `_Cnd_internal_imp_t` unchanged, while initializing `_Stl_condition_variable _Stl_cv{};` with a default member initializer, so its two pointers will be null. (The size is unchanged because `_Cv_storage` is at least `2 * sizeof(void*)`. The alignment is unchanged because everything is `void*`-aligned here.) Finally, add empty braces when we have `_Cnd_internal_imp_t _Cnd_storage{};` data members. These aren't necessary, but they serve as a reminder that we're getting null instead of garbage, and this is consistent with how we have `_Mtx_internal_imp_t _Mtx_storage{};`. Note that I chose these names to avoid confusion between the different layers: * `_Stl_cv` stores two pointers, including the unused vptr * `_Win_cv` directly mirrors `CONDITION_VARIABLE` --- stl/inc/__msvc_threads_core.hpp | 10 +++++++++- stl/inc/condition_variable | 2 +- stl/inc/mutex | 2 +- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/stl/inc/__msvc_threads_core.hpp b/stl/inc/__msvc_threads_core.hpp index 94cc734613..2c4174123e 100644 --- a/stl/inc/__msvc_threads_core.hpp +++ b/stl/inc/__msvc_threads_core.hpp @@ -52,6 +52,11 @@ struct _Mtx_internal_imp_t { using _Mtx_t = _Mtx_internal_imp_t*; +struct _Stl_condition_variable { + void* _Unused = nullptr; // TRANSITION, ABI: was the vptr + void* _Win_cv = nullptr; +}; + struct _Cnd_internal_imp_t { #if defined(_CRT_WINDOWS) // for Windows-internal code static constexpr size_t _Cnd_internal_imp_size = 2 * sizeof(void*); @@ -61,7 +66,10 @@ struct _Cnd_internal_imp_t { static constexpr size_t _Cnd_internal_imp_size = 40; #endif // ^^^ ordinary 32-bit code ^^^ - _STD _Aligned_storage_t<_Cnd_internal_imp_size, alignof(void*)> _Cv_storage; + union { + _Stl_condition_variable _Stl_cv{}; + _STD _Aligned_storage_t<_Cnd_internal_imp_size, alignof(void*)> _Cv_storage; + }; }; using _Cnd_t = _Cnd_internal_imp_t*; diff --git a/stl/inc/condition_variable b/stl/inc/condition_variable index 72d10ae804..be04880cf8 100644 --- a/stl/inc/condition_variable +++ b/stl/inc/condition_variable @@ -216,7 +216,7 @@ public: private: shared_ptr _Myptr; - _Cnd_internal_imp_t _Cnd_storage; + _Cnd_internal_imp_t _Cnd_storage{}; _NODISCARD _Cnd_t _Mycnd() noexcept { return &_Cnd_storage; diff --git a/stl/inc/mutex b/stl/inc/mutex index 0e4fdf3a6f..07a35b717b 100644 --- a/stl/inc/mutex +++ b/stl/inc/mutex @@ -627,7 +627,7 @@ public: } private: - _Cnd_internal_imp_t _Cnd_storage; + _Cnd_internal_imp_t _Cnd_storage{}; _Cnd_t _Mycnd() noexcept { return &_Cnd_storage; From 07c1152fd127e166582fbf1a70a80196673fdcbb Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sun, 9 Jun 2024 06:01:36 -0700 Subject: [PATCH 05/13] Now we can stop calling `_Cnd_init_in_situ()`, with `_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR` as the escape hatch. We just changed `condition_variable_any` and `condition_variable` to statically initialize `_Cnd_internal_imp_t`. `condition_variable`'s default constructor can now be defaulted. In xthreads.h, declare `_Cnd_init_in_situ()` only when needed for the escape hatch. (Again, this doesn't affect bincompat, the definition still has `_CRTIMP2_PURE`.) Comment `_Cnd_init_in_situ()` as preserved for bincompat and the escape hatch. Its only caller is `_Cnd_init()` immediately below, which is only called by `_Thrd_create()`, which is preserved for bincompat. We need the escape hatch for the same reason that `mutex` does. When a user hasn't followed our documented restrictions for binary compatibility, and mixes new headers with a very old STL DLL (older than VS 2022 17.8, which was the release that shipped GH 3770), the headers will initialize the vptr to null, but the old STL DLL will want to actually use the vptr. --- stl/inc/condition_variable | 2 ++ stl/inc/mutex | 4 ++++ stl/inc/xthreads.h | 2 ++ stl/src/cond.cpp | 2 +- 4 files changed, 9 insertions(+), 1 deletion(-) diff --git a/stl/inc/condition_variable b/stl/inc/condition_variable index be04880cf8..c72cabc8cb 100644 --- a/stl/inc/condition_variable +++ b/stl/inc/condition_variable @@ -52,7 +52,9 @@ private: _EXPORT_STD class condition_variable_any { // class for waiting for conditions with any kind of mutex public: condition_variable_any() : _Myptr{_STD make_shared()} { +#ifdef _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR _Cnd_init_in_situ(_Mycnd()); +#endif // ^^^ defined(_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) ^^^ } ~condition_variable_any() noexcept = default; diff --git a/stl/inc/mutex b/stl/inc/mutex index 07a35b717b..3bf8b71ce6 100644 --- a/stl/inc/mutex +++ b/stl/inc/mutex @@ -531,9 +531,13 @@ _EXPORT_STD enum class cv_status { // names for wait returns _EXPORT_STD class condition_variable { // class for waiting for conditions public: +#ifdef _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR condition_variable() noexcept /* strengthened */ { _Cnd_init_in_situ(_Mycnd()); } +#else // ^^^ defined(_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) / !defined(_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) vvv + condition_variable() noexcept /* strengthened */ = default; +#endif // ^^^ !defined(_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) ^^^ ~condition_variable() noexcept = default; diff --git a/stl/inc/xthreads.h b/stl/inc/xthreads.h index 65f936294d..6f2989f60b 100644 --- a/stl/inc/xthreads.h +++ b/stl/inc/xthreads.h @@ -59,7 +59,9 @@ void __cdecl _Smtx_unlock_shared(_Smtx_t*) noexcept; _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_init(_Cnd_t*) noexcept; _CRTIMP2_PURE void __cdecl _Cnd_destroy(_Cnd_t) noexcept; #endif // _CRTBLD +#ifdef _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR _CRTIMP2_PURE void __cdecl _Cnd_init_in_situ(_Cnd_t) noexcept; +#endif // ^^^ defined(_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) ^^^ _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_wait(_Cnd_t, _Mtx_t) noexcept; // TRANSITION, ABI: Always succeeds _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_broadcast(_Cnd_t) noexcept; // TRANSITION, ABI: Always succeeds _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_signal(_Cnd_t) noexcept; // TRANSITION, ABI: Always succeeds diff --git a/stl/src/cond.cpp b/stl/src/cond.cpp index e06749e28f..3dbe665c92 100644 --- a/stl/src/cond.cpp +++ b/stl/src/cond.cpp @@ -12,7 +12,7 @@ extern "C" { - +// TRANSITION, ABI: preserved for binary compatibility (and _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) _CRTIMP2_PURE void __cdecl _Cnd_init_in_situ(const _Cnd_t cond) noexcept { // initialize condition variable in situ new (Concurrency::details::_Get_cond_var(cond)) Concurrency::details::stl_condition_variable_win7; } From 5546cda11a3241cf379c24c0f82c34424733559d Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sun, 9 Jun 2024 07:53:45 -0700 Subject: [PATCH 06/13] Simplify the definition of `_Cnd_init_in_situ()`. Constructing `_Cnd_internal_imp_t` will now perform the initialization that we need. --- stl/src/cond.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/src/cond.cpp b/stl/src/cond.cpp index 3dbe665c92..68699b7643 100644 --- a/stl/src/cond.cpp +++ b/stl/src/cond.cpp @@ -14,7 +14,7 @@ extern "C" { // TRANSITION, ABI: preserved for binary compatibility (and _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) _CRTIMP2_PURE void __cdecl _Cnd_init_in_situ(const _Cnd_t cond) noexcept { // initialize condition variable in situ - new (Concurrency::details::_Get_cond_var(cond)) Concurrency::details::stl_condition_variable_win7; + new (cond) _Cnd_internal_imp_t; } // TRANSITION, ABI: preserved for binary compatibility From 7ed5c6a78e95958ac852ea9a496547aa2cc7c808 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sun, 9 Jun 2024 06:26:54 -0700 Subject: [PATCH 07/13] Stop calling `_Cnd_init()` and `_Cnd_destroy()`. We declared them in xthreads.h (guarded by `_CRTBLD`) because they were defined by cond.cpp and used by cthread.cpp. As usual, dropping these declarations won't affect bincompat, as the definitions are consistently marked `_CRTIMP2_PURE`. Mark the definitions as preserved for bincompat. In cthread.cpp `_Thrd_create()` (itself preserved for bincompat), we don't need to dynamically allocate and deallocate a condition variable. Locally initializing `_Cnd_internal_imp_t cond_var{};` and taking its address is sufficient. --- stl/inc/xthreads.h | 4 ---- stl/src/cond.cpp | 2 ++ stl/src/cthread.cpp | 5 ++--- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/stl/inc/xthreads.h b/stl/inc/xthreads.h index 6f2989f60b..c5cf4fee26 100644 --- a/stl/inc/xthreads.h +++ b/stl/inc/xthreads.h @@ -55,10 +55,6 @@ void __cdecl _Smtx_unlock_exclusive(_Smtx_t*) noexcept; void __cdecl _Smtx_unlock_shared(_Smtx_t*) noexcept; // condition variables -#ifdef _CRTBLD -_CRTIMP2_PURE _Thrd_result __cdecl _Cnd_init(_Cnd_t*) noexcept; -_CRTIMP2_PURE void __cdecl _Cnd_destroy(_Cnd_t) noexcept; -#endif // _CRTBLD #ifdef _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR _CRTIMP2_PURE void __cdecl _Cnd_init_in_situ(_Cnd_t) noexcept; #endif // ^^^ defined(_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) ^^^ diff --git a/stl/src/cond.cpp b/stl/src/cond.cpp index 68699b7643..a10ff9b705 100644 --- a/stl/src/cond.cpp +++ b/stl/src/cond.cpp @@ -20,6 +20,7 @@ _CRTIMP2_PURE void __cdecl _Cnd_init_in_situ(const _Cnd_t cond) noexcept { // in // TRANSITION, ABI: preserved for binary compatibility _CRTIMP2_PURE void __cdecl _Cnd_destroy_in_situ(_Cnd_t) noexcept {} // destroy condition variable in situ +// TRANSITION, ABI: preserved for binary compatibility _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_init(_Cnd_t* const pcond) noexcept { // initialize *pcond = nullptr; @@ -33,6 +34,7 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_init(_Cnd_t* const pcond) noexcept { // return _Thrd_result::_Success; } +// TRANSITION, ABI: preserved for binary compatibility _CRTIMP2_PURE void __cdecl _Cnd_destroy(const _Cnd_t cond) noexcept { // clean up if (cond) { // something to do, do it _free_crt(cond); diff --git a/stl/src/cthread.cpp b/stl/src/cthread.cpp index 141fbe9d4a..58c86b8c93 100644 --- a/stl/src/cthread.cpp +++ b/stl/src/cthread.cpp @@ -113,9 +113,9 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Thrd_create(_Thrd_t* thr, _Thrd_start_t func _Thrd_result res; _Thrd_binder b; int started = 0; - _Cnd_t cond; + _Cnd_internal_imp_t cond_var{}; + _Cnd_t cond = &cond_var; _Mtx_t mtx; - _Cnd_init(&cond); _Mtx_init(&mtx, _Mtx_plain); b.func = func; b.data = d; @@ -129,7 +129,6 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Thrd_create(_Thrd_t* thr, _Thrd_start_t func } } _Mtx_unlock(mtx); - _Cnd_destroy(cond); _Mtx_destroy(mtx); return res; } From a63cf477d9505b259abc8a98ba9ce67dc65455bb Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sun, 9 Jun 2024 06:47:18 -0700 Subject: [PATCH 08/13] Stop calling `_Mtx_init()` and `_Mtx_destroy()`. We declared them in xthreads.h (guarded by `_CRTBLD`) because they were defined by mutex.cpp and used by cthread.cpp. As usual, dropping these declarations won't affect bincompat, as the definitions are consistently marked `_CRTIMP2_PURE`. Mark the definition of `_Mtx_init()` as preserved for bincompat. `_Mtx_destroy()` was already marked. In cthread.cpp `_Thrd_create()` (itself preserved for bincompat), we don't need to dynamically allocate and deallocate a mutex. Locally initializing `_Mtx_internal_imp_t mtx_var{};`, taking its address, and calling `_Mtx_init_in_situ()` is sufficient. We don't need to call `_Mtx_destroy_in_situ()` (which isn't declared). It's a no-op except for a debug check "mutex destroyed while busy". `std::mutex` doesn't bother with that anymore, and we're definitely calling `_Mtx_unlock()` so we're fine. --- stl/inc/xthreads.h | 4 ---- stl/src/cthread.cpp | 6 +++--- stl/src/mutex.cpp | 1 + 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/stl/inc/xthreads.h b/stl/inc/xthreads.h index c5cf4fee26..8ff2682262 100644 --- a/stl/inc/xthreads.h +++ b/stl/inc/xthreads.h @@ -35,10 +35,6 @@ enum { // mutex types _Mtx_recursive = 0x100 }; -#ifdef _CRTBLD -_CRTIMP2_PURE _Thrd_result __cdecl _Mtx_init(_Mtx_t*, int) noexcept; -_CRTIMP2_PURE void __cdecl _Mtx_destroy(_Mtx_t) noexcept; -#endif // _CRTBLD _CRTIMP2_PURE void __cdecl _Mtx_init_in_situ(_Mtx_t, int) noexcept; _CRTIMP2_PURE int __cdecl _Mtx_current_owns(_Mtx_t) noexcept; _CRTIMP2_PURE _Thrd_result __cdecl _Mtx_lock(_Mtx_t) noexcept; diff --git a/stl/src/cthread.cpp b/stl/src/cthread.cpp index 58c86b8c93..731f9296cb 100644 --- a/stl/src/cthread.cpp +++ b/stl/src/cthread.cpp @@ -115,8 +115,9 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Thrd_create(_Thrd_t* thr, _Thrd_start_t func int started = 0; _Cnd_internal_imp_t cond_var{}; _Cnd_t cond = &cond_var; - _Mtx_t mtx; - _Mtx_init(&mtx, _Mtx_plain); + _Mtx_internal_imp_t mtx_var{}; + _Mtx_t mtx = &mtx_var; + _Mtx_init_in_situ(mtx, _Mtx_plain); b.func = func; b.data = d; b.cond = &cond; @@ -129,7 +130,6 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Thrd_create(_Thrd_t* thr, _Thrd_start_t func } } _Mtx_unlock(mtx); - _Mtx_destroy(mtx); return res; } diff --git a/stl/src/mutex.cpp b/stl/src/mutex.cpp index 8d7e26f5c4..424997321d 100644 --- a/stl/src/mutex.cpp +++ b/stl/src/mutex.cpp @@ -49,6 +49,7 @@ _CRTIMP2_PURE void __cdecl _Mtx_destroy_in_situ(_Mtx_t mtx) noexcept { // destro (void) mtx; } +// TRANSITION, ABI: preserved for binary compatibility _CRTIMP2_PURE _Thrd_result __cdecl _Mtx_init(_Mtx_t* mtx, int type) noexcept { // initialize mutex *mtx = nullptr; From f8ba7efaa60c53718b3ca9ed8438fe9bdd9a3bc7 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sun, 9 Jun 2024 11:15:57 -0700 Subject: [PATCH 09/13] Declare `_Mtx_init_in_situ()` only when needed. And update its comment accordingly. --- stl/inc/xthreads.h | 2 ++ stl/src/mutex.cpp | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/stl/inc/xthreads.h b/stl/inc/xthreads.h index 8ff2682262..09dad03f55 100644 --- a/stl/inc/xthreads.h +++ b/stl/inc/xthreads.h @@ -35,7 +35,9 @@ enum { // mutex types _Mtx_recursive = 0x100 }; +#if defined(_CRTBLD) || defined(_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) _CRTIMP2_PURE void __cdecl _Mtx_init_in_situ(_Mtx_t, int) noexcept; +#endif // ^^^ defined(_CRTBLD) || defined(_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) ^^^ _CRTIMP2_PURE int __cdecl _Mtx_current_owns(_Mtx_t) noexcept; _CRTIMP2_PURE _Thrd_result __cdecl _Mtx_lock(_Mtx_t) noexcept; _CRTIMP2_PURE _Thrd_result __cdecl _Mtx_trylock(_Mtx_t) noexcept; diff --git a/stl/src/mutex.cpp b/stl/src/mutex.cpp index 424997321d..cd44a0044a 100644 --- a/stl/src/mutex.cpp +++ b/stl/src/mutex.cpp @@ -35,7 +35,7 @@ _CRTIMP2 void __cdecl __set_stl_sync_api_mode(__stl_sync_api_modes_enum) noexcep return reinterpret_cast(&mtx->_Critical_section._M_srw_lock); } -// TRANSITION, only used when constexpr mutex constructor is not enabled +// TRANSITION, ABI: preserved for binary compatibility (and _DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) _CRTIMP2_PURE void __cdecl _Mtx_init_in_situ(_Mtx_t mtx, int type) noexcept { // initialize mutex in situ new (&mtx->_Critical_section) _Stl_critical_section; mtx->_Thread_id = -1; From 4cabe128afdfe417422923c30502ecb26a0f5a9a Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sun, 9 Jun 2024 08:38:17 -0700 Subject: [PATCH 10/13] Replace `Concurrency::details::stl_condition_variable_win7` with non-member functions. This wasn't dllexported, and the changes are limited to stl/src, so there's no bincompat impact. I chose the naming scheme `_Primitive_wait` to avoid confusion with `_Cnd_wait` which does more work. The functions don't need to be extern "C", because they don't interact with user code. (They do need to be ugly to avoid conflicts during static linking.) They have to be `inline`. I also marked them as `noexcept`, although that wasn't necessary. Because they aren't member functions anymore, I have to define `_Primitive_wait_for()` before `_Primitive_wait()` can call it. --- stl/src/cond.cpp | 11 ++++---- stl/src/primitives.hpp | 61 ++++++++++++++--------------------------- stl/src/sharedmutex.cpp | 2 +- 3 files changed, 27 insertions(+), 47 deletions(-) diff --git a/stl/src/cond.cpp b/stl/src/cond.cpp index a10ff9b705..46c85025ad 100644 --- a/stl/src/cond.cpp +++ b/stl/src/cond.cpp @@ -56,7 +56,7 @@ _CRTIMP2_PURE void __cdecl _Mtx_reset_owner(_Mtx_t mtx) noexcept { // set owner _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_wait(const _Cnd_t cond, const _Mtx_t mtx) noexcept { // wait until signaled const auto cs = &mtx->_Critical_section; _Mtx_clear_owner(mtx); - Concurrency::details::_Get_cond_var(cond)->wait(cs); + _Primitive_wait(cond, cs); _Mtx_reset_owner(mtx); return _Thrd_result::_Success; // TRANSITION, ABI: Always succeeds } @@ -68,14 +68,13 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_timedwait( const auto cs = &mtx->_Critical_section; if (target == nullptr) { // no target time specified, wait on mutex _Mtx_clear_owner(mtx); - Concurrency::details::_Get_cond_var(cond)->wait(cs); + _Primitive_wait(cond, cs); _Mtx_reset_owner(mtx); } else { // target time specified, wait for it _timespec64 now; _Timespec64_get_sys(&now); _Mtx_clear_owner(mtx); - if (!Concurrency::details::_Get_cond_var(cond)->wait_for( - cs, _Xtime_diff_to_millis2(target, &now))) { // report timeout + if (!_Primitive_wait_for(cond, cs, _Xtime_diff_to_millis2(target, &now))) { // report timeout _Timespec64_get_sys(&now); if (_Xtime_diff_to_millis2(target, &now) == 0) { res = _Thrd_result::_Timedout; @@ -87,12 +86,12 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_timedwait( } _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_signal(const _Cnd_t cond) noexcept { // release one waiting thread - Concurrency::details::_Get_cond_var(cond)->notify_one(); + _Primitive_notify_one(cond); return _Thrd_result::_Success; // TRANSITION, ABI: Always succeeds } _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_broadcast(const _Cnd_t cond) noexcept { // release all waiting threads - Concurrency::details::_Get_cond_var(cond)->notify_all(); + _Primitive_notify_all(cond); return _Thrd_result::_Success; // TRANSITION, ABI: Always succeeds } diff --git a/stl/src/primitives.hpp b/stl/src/primitives.hpp index 4d222c6277..bf357b2093 100644 --- a/stl/src/primitives.hpp +++ b/stl/src/primitives.hpp @@ -8,43 +8,24 @@ #include -namespace Concurrency { - namespace details { - class stl_condition_variable_win7 { - public: - 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 wait(_Stl_critical_section* lock) { - if (!wait_for(lock, INFINITE)) { - _CSTD abort(); - } - } - - bool wait_for(_Stl_critical_section* lock, unsigned int timeout) { - return SleepConditionVariableSRW( - &m_condition_variable, reinterpret_cast(&lock->_M_srw_lock), timeout, 0) - != 0; - } - - void notify_one() { - WakeConditionVariable(&m_condition_variable); - } - - void notify_all() { - WakeAllConditionVariable(&m_condition_variable); - } - - private: - void* unused = nullptr; // TRANSITION, ABI: was the vptr - CONDITION_VARIABLE m_condition_variable = CONDITION_VARIABLE_INIT; - }; - - [[nodiscard]] inline stl_condition_variable_win7* _Get_cond_var(::_Cnd_internal_imp_t* _Cond) noexcept { - return reinterpret_cast(&_Cond->_Cv_storage); - } - } // namespace details -} // namespace Concurrency +inline bool _Primitive_wait_for(const _Cnd_t cond, _Stl_critical_section* lock, unsigned int timeout) noexcept { + const auto pcv = reinterpret_cast(&cond->_Stl_cv._Win_cv); + const auto psrw = reinterpret_cast(&lock->_M_srw_lock); + return SleepConditionVariableSRW(pcv, psrw, timeout, 0) != 0; +} + +inline void _Primitive_wait(const _Cnd_t cond, _Stl_critical_section* lock) noexcept { + if (!_Primitive_wait_for(cond, lock, INFINITE)) { + _CSTD abort(); + } +} + +inline void _Primitive_notify_one(const _Cnd_t cond) noexcept { + const auto pcv = reinterpret_cast(&cond->_Stl_cv._Win_cv); + WakeConditionVariable(pcv); +} + +inline void _Primitive_notify_all(const _Cnd_t cond) noexcept { + const auto pcv = reinterpret_cast(&cond->_Stl_cv._Win_cv); + WakeAllConditionVariable(pcv); +} diff --git a/stl/src/sharedmutex.cpp b/stl/src/sharedmutex.cpp index ef5e53f745..5d55df643d 100644 --- a/stl/src/sharedmutex.cpp +++ b/stl/src/sharedmutex.cpp @@ -50,7 +50,7 @@ _Thrd_result __stdcall _Cnd_timedwait_for(const _Cnd_t cond, const _Mtx_t mtx, c mtx->_Thread_id = -1; --mtx->_Count; - if (!Concurrency::details::_Get_cond_var(cond)->wait_for(cs, target_ms)) { // report timeout + if (!_Primitive_wait_for(cond, cs, target_ms)) { // report timeout const auto end_ms = GetTickCount64(); if (end_ms - start_ms >= target_ms) { res = _Thrd_result::_Timedout; From 748773d5478add7bd9a32e17aa21f8405d383428 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sun, 9 Jun 2024 09:14:55 -0700 Subject: [PATCH 11/13] Simplify `_Primitive_wait[_for]()` by taking `_Mtx_t mtx`. Now that the originally-ConcRT-derived layer has been eradicated, we don't need to make every caller reach into `_Mtx_t` internals. This also increases consistency with the `_Cnd_t cond` parameter. --- stl/src/cond.cpp | 8 +++----- stl/src/primitives.hpp | 8 ++++---- stl/src/sharedmutex.cpp | 3 +-- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/stl/src/cond.cpp b/stl/src/cond.cpp index 46c85025ad..040e696b43 100644 --- a/stl/src/cond.cpp +++ b/stl/src/cond.cpp @@ -54,9 +54,8 @@ _CRTIMP2_PURE void __cdecl _Mtx_reset_owner(_Mtx_t mtx) noexcept { // set owner } _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_wait(const _Cnd_t cond, const _Mtx_t mtx) noexcept { // wait until signaled - const auto cs = &mtx->_Critical_section; _Mtx_clear_owner(mtx); - _Primitive_wait(cond, cs); + _Primitive_wait(cond, mtx); _Mtx_reset_owner(mtx); return _Thrd_result::_Success; // TRANSITION, ABI: Always succeeds } @@ -65,16 +64,15 @@ _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_wait(const _Cnd_t cond, const _Mtx_t mtx _CRTIMP2_PURE _Thrd_result __cdecl _Cnd_timedwait( const _Cnd_t cond, const _Mtx_t mtx, const _timespec64* const target) noexcept { _Thrd_result res = _Thrd_result::_Success; - const auto cs = &mtx->_Critical_section; if (target == nullptr) { // no target time specified, wait on mutex _Mtx_clear_owner(mtx); - _Primitive_wait(cond, cs); + _Primitive_wait(cond, mtx); _Mtx_reset_owner(mtx); } else { // target time specified, wait for it _timespec64 now; _Timespec64_get_sys(&now); _Mtx_clear_owner(mtx); - if (!_Primitive_wait_for(cond, cs, _Xtime_diff_to_millis2(target, &now))) { // report timeout + if (!_Primitive_wait_for(cond, mtx, _Xtime_diff_to_millis2(target, &now))) { // report timeout _Timespec64_get_sys(&now); if (_Xtime_diff_to_millis2(target, &now) == 0) { res = _Thrd_result::_Timedout; diff --git a/stl/src/primitives.hpp b/stl/src/primitives.hpp index bf357b2093..d9572d4a00 100644 --- a/stl/src/primitives.hpp +++ b/stl/src/primitives.hpp @@ -8,14 +8,14 @@ #include -inline bool _Primitive_wait_for(const _Cnd_t cond, _Stl_critical_section* lock, unsigned int timeout) noexcept { +inline bool _Primitive_wait_for(const _Cnd_t cond, const _Mtx_t mtx, unsigned int timeout) noexcept { const auto pcv = reinterpret_cast(&cond->_Stl_cv._Win_cv); - const auto psrw = reinterpret_cast(&lock->_M_srw_lock); + const auto psrw = reinterpret_cast(&mtx->_Critical_section._M_srw_lock); return SleepConditionVariableSRW(pcv, psrw, timeout, 0) != 0; } -inline void _Primitive_wait(const _Cnd_t cond, _Stl_critical_section* lock) noexcept { - if (!_Primitive_wait_for(cond, lock, INFINITE)) { +inline void _Primitive_wait(const _Cnd_t cond, const _Mtx_t mtx) noexcept { + if (!_Primitive_wait_for(cond, mtx, INFINITE)) { _CSTD abort(); } } diff --git a/stl/src/sharedmutex.cpp b/stl/src/sharedmutex.cpp index 5d55df643d..ecb73c754c 100644 --- a/stl/src/sharedmutex.cpp +++ b/stl/src/sharedmutex.cpp @@ -43,14 +43,13 @@ void __stdcall _Thrd_sleep_for(const unsigned long ms) noexcept { // suspend cur _Thrd_result __stdcall _Cnd_timedwait_for(const _Cnd_t cond, const _Mtx_t mtx, const unsigned int target_ms) noexcept { _Thrd_result res = _Thrd_result::_Success; - const auto cs = &mtx->_Critical_section; const auto start_ms = GetTickCount64(); // TRANSITION: replace with _Mtx_clear_owner(mtx); mtx->_Thread_id = -1; --mtx->_Count; - if (!_Primitive_wait_for(cond, cs, target_ms)) { // report timeout + if (!_Primitive_wait_for(cond, mtx, target_ms)) { // report timeout const auto end_ms = GetTickCount64(); if (end_ms - start_ms >= target_ms) { res = _Thrd_result::_Timedout; From 391239b8e59b0596ac16f81c9b8b9f13429bd458 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Sun, 9 Jun 2024 10:48:22 -0700 Subject: [PATCH 12/13] Skip 3 libcxx tests for Clang until LLVM-94907 is merged. --- tests/libcxx/expected_results.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/libcxx/expected_results.txt b/tests/libcxx/expected_results.txt index 699e522f93..8b9e4709f0 100644 --- a/tests/libcxx/expected_results.txt +++ b/tests/libcxx/expected_results.txt @@ -26,6 +26,11 @@ std/time/time.syn/formatter.year_month_weekday.pass.cpp:1 FAIL std/utilities/memory/specialized.algorithms/uninitialized.copy/uninitialized_copy.pass.cpp FAIL std/utilities/memory/specialized.algorithms/uninitialized.move/uninitialized_move.pass.cpp FAIL +# LLVM-94907: [libc++][test] Avoid -Wunused-variable warnings in mutex tests +std/thread/thread.mutex/thread.mutex.requirements/thread.sharedtimedmutex.requirements/thread.sharedtimedmutex.class/default.pass.cpp:2 FAIL +std/thread/thread.mutex/thread.mutex.requirements/thread.timedmutex.requirements/thread.timedmutex.class/default.pass.cpp:2 FAIL +std/thread/thread.mutex/thread.mutex.requirements/thread.timedmutex.requirements/thread.timedmutex.recursive/default.pass.cpp:2 FAIL + # Non-Standard regex behavior. # "It seems likely that the test is still non-conforming due to how libc++ handles the 'w' character class." std/re/re.traits/lookup_classname.pass.cpp FAIL From 48d01c284160c4d615661740a14b1301eb8899fe Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Fri, 14 Jun 2024 12:53:06 -0700 Subject: [PATCH 13/13] Silence false positive warning C26495. --- stl/inc/__msvc_threads_core.hpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/stl/inc/__msvc_threads_core.hpp b/stl/inc/__msvc_threads_core.hpp index 2c4174123e..4afe27a5a2 100644 --- a/stl/inc/__msvc_threads_core.hpp +++ b/stl/inc/__msvc_threads_core.hpp @@ -57,6 +57,8 @@ struct _Stl_condition_variable { void* _Win_cv = nullptr; }; +#pragma warning(push) +#pragma warning(disable : 26495) // Variable 'meow' is uninitialized. Always initialize a member variable (type.6). struct _Cnd_internal_imp_t { #if defined(_CRT_WINDOWS) // for Windows-internal code static constexpr size_t _Cnd_internal_imp_size = 2 * sizeof(void*); @@ -71,6 +73,7 @@ struct _Cnd_internal_imp_t { _STD _Aligned_storage_t<_Cnd_internal_imp_size, alignof(void*)> _Cv_storage; }; }; +#pragma warning(pop) using _Cnd_t = _Cnd_internal_imp_t*; } // extern "C"