From dd2fef5791d7a3a487ba0d7e6b92d89c45e6f0f9 Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Wed, 15 Feb 2023 11:25:28 +0800 Subject: [PATCH 1/4] Implement LWG-2309 --- stl/inc/mutex | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/stl/inc/mutex b/stl/inc/mutex index 0b2c363967d..140142d694a 100644 --- a/stl/inc/mutex +++ b/stl/inc/mutex @@ -45,11 +45,30 @@ public: _Mutex_base& operator=(const _Mutex_base&) = delete; void lock() { - _Check_C_return(_Mtx_lock(_Mymtx())); + if (_Mtx_lock(_Mymtx()) != _Thrd_success) { + // undefined behavior, only occurs for plain mutexes (N4928 [thread.mutex.requirements.mutex.general]/6) + _STD _Throw_Cpp_error(_RESOURCE_DEADLOCK_WOULD_OCCUR); + } + if (_Ownership_levels() == INT_MAX) { + // only occurs for recursive mutexes (N4928 [thread.mutex.recursive]/3) + --_Ownership_levels(); + // POSIX specifies EAGAIN in the corresponding situation: + // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_lock.html + _STD _Throw_Cpp_error(_RESOURCE_UNAVAILABLE_TRY_AGAIN); + } } _NODISCARD_TRY_CHANGE_STATE bool try_lock() noexcept /* strengthened */ { - return _Mtx_trylock(_Mymtx()) == _Thrd_success; + if (_Mtx_trylock(_Mymtx()) != _Thrd_success) { + // undefined behavior, only occurs for plain mutexes (N4928 [thread.mutex.requirements.mutex.general]/6) + return false; + } + if (_Ownership_levels() == INT_MAX) { + // only occurs for recursive mutexes (N4928 [thread.mutex.recursive]/3) + --_Ownership_levels(); + return false; + } + return true; } void unlock() noexcept /* strengthened */ { @@ -71,6 +90,10 @@ private: _Mtx_t _Mymtx() noexcept { // get pointer to _Mtx_internal_imp_t inside _Mtx_storage return reinterpret_cast<_Mtx_t>(&_Mtx_storage); } + + int& _Ownership_levels() noexcept { // the count is denoted by an int subobject at the end of _Mtx_storage + return *reinterpret_cast(reinterpret_cast(&_Mtx_storage + 1) - sizeof(int)); + } }; _EXPORT_STD class mutex : public _Mutex_base { // class for mutual exclusion @@ -852,7 +875,9 @@ public: if (_My_locked < UINT_MAX) { ++_My_locked; } else { - _Throw_system_error(errc::device_or_resource_busy); + // POSIX specifies EAGAIN in the corresponding situation: + // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_lock.html + _STD _Throw_system_error(errc::resource_unavailable_try_again); } } else { while (_My_locked != 0) { From b2d678e2c35888bd4caaf5a7c1ddf77ef27a0a76 Mon Sep 17 00:00:00 2001 From: "Stephan T. Lavavej" Date: Wed, 15 Feb 2023 11:45:28 -0800 Subject: [PATCH 2/4] Code review feedback. --- stl/inc/mutex | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/stl/inc/mutex b/stl/inc/mutex index 140142d694a..821ceddd30f 100644 --- a/stl/inc/mutex +++ b/stl/inc/mutex @@ -49,6 +49,7 @@ public: // undefined behavior, only occurs for plain mutexes (N4928 [thread.mutex.requirements.mutex.general]/6) _STD _Throw_Cpp_error(_RESOURCE_DEADLOCK_WOULD_OCCUR); } + if (_Ownership_levels() == INT_MAX) { // only occurs for recursive mutexes (N4928 [thread.mutex.recursive]/3) --_Ownership_levels(); @@ -63,11 +64,13 @@ public: // undefined behavior, only occurs for plain mutexes (N4928 [thread.mutex.requirements.mutex.general]/6) return false; } + if (_Ownership_levels() == INT_MAX) { // only occurs for recursive mutexes (N4928 [thread.mutex.recursive]/3) --_Ownership_levels(); return false; } + return true; } @@ -92,7 +95,7 @@ private: } int& _Ownership_levels() noexcept { // the count is denoted by an int subobject at the end of _Mtx_storage - return *reinterpret_cast(reinterpret_cast(&_Mtx_storage + 1) - sizeof(int)); + return reinterpret_cast(&_Mtx_storage + 1)[-1]; } }; From c1b114a8f2d96775d708f64a88bfaa9775f3468b Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Thu, 16 Feb 2023 07:19:47 +0800 Subject: [PATCH 3/4] Failures of `try_lock` for recursive mutexes are well-defined --- stl/inc/mutex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stl/inc/mutex b/stl/inc/mutex index 821ceddd30f..ed64da6620d 100644 --- a/stl/inc/mutex +++ b/stl/inc/mutex @@ -61,7 +61,7 @@ public: _NODISCARD_TRY_CHANGE_STATE bool try_lock() noexcept /* strengthened */ { if (_Mtx_trylock(_Mymtx()) != _Thrd_success) { - // undefined behavior, only occurs for plain mutexes (N4928 [thread.mutex.requirements.mutex.general]/6) + // undefined behavior for plain mutexes (N4928 [thread.mutex.requirements.mutex.general]/6) return false; } From fbfc328d9893e0a87125db04e7840df7f08685df Mon Sep 17 00:00:00 2001 From: "A. Jiang" Date: Thu, 16 Feb 2023 17:20:58 +0800 Subject: [PATCH 4/4] Address @barcharcraz --- stl/inc/mutex | 70 ++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 50 insertions(+), 20 deletions(-) diff --git a/stl/inc/mutex b/stl/inc/mutex index ed64da6620d..46a75447559 100644 --- a/stl/inc/mutex +++ b/stl/inc/mutex @@ -31,6 +31,34 @@ _STD_BEGIN _EXPORT_STD class condition_variable; _EXPORT_STD class condition_variable_any; +struct _Mtx_internal_imp_mirror { +#ifdef _CRT_WINDOWS +#ifdef _WIN64 + static constexpr size_t _Critical_section_size = 8; +#else // _WIN64 + static constexpr size_t _Critical_section_size = 4; +#endif // _WIN64 +#else // _CRT_WINDOWS +#ifdef _WIN64 + static constexpr size_t _Critical_section_size = 56; +#else // _WIN64 + static constexpr size_t _Critical_section_size = 32; +#endif // _WIN64 +#endif // _CRT_WINDOWS + + int _Type; + const void* _Vptr; + union { + void* _Srw_lock_placeholder; + unsigned char _Padding[_Critical_section_size]; + }; + long _Thread_id; + int _Count; +}; + +static_assert(sizeof(_Mtx_internal_imp_mirror) == _Mtx_internal_imp_size, "inconsistent size for mutex"); +static_assert(alignof(_Mtx_internal_imp_mirror) == _Mtx_internal_imp_alignment, "inconsistent alignment for mutex"); + class _Mutex_base { // base class for all mutex types public: _Mutex_base(int _Flags = 0) noexcept { @@ -50,9 +78,8 @@ public: _STD _Throw_Cpp_error(_RESOURCE_DEADLOCK_WOULD_OCCUR); } - if (_Ownership_levels() == INT_MAX) { + if (!_Verify_ownership_levels()) { // only occurs for recursive mutexes (N4928 [thread.mutex.recursive]/3) - --_Ownership_levels(); // POSIX specifies EAGAIN in the corresponding situation: // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_lock.html _STD _Throw_Cpp_error(_RESOURCE_UNAVAILABLE_TRY_AGAIN); @@ -60,18 +87,8 @@ public: } _NODISCARD_TRY_CHANGE_STATE bool try_lock() noexcept /* strengthened */ { - if (_Mtx_trylock(_Mymtx()) != _Thrd_success) { - // undefined behavior for plain mutexes (N4928 [thread.mutex.requirements.mutex.general]/6) - return false; - } - - if (_Ownership_levels() == INT_MAX) { - // only occurs for recursive mutexes (N4928 [thread.mutex.recursive]/3) - --_Ownership_levels(); - return false; - } - - return true; + // false may be from undefined behavior for plain mutexes (N4928 [thread.mutex.requirements.mutex.general]/6) + return _Mtx_trylock(_Mymtx()) == _Thrd_success; } void unlock() noexcept /* strengthened */ { @@ -84,21 +101,34 @@ public: return _Mtx_getconcrtcs(_Mymtx()); } +protected: + _NODISCARD_TRY_CHANGE_STATE bool _Verify_ownership_levels() noexcept { + if (_Mtx_storage_mirror._Count == INT_MAX) { + // only occurs for recursive mutexes (N4928 [thread.mutex.recursive]/3) + --_Mtx_storage_mirror._Count; + return false; + } + + return true; + } + private: friend condition_variable; friend condition_variable_any; - _Aligned_storage_t<_Mtx_internal_imp_size, _Mtx_internal_imp_alignment> _Mtx_storage; + union { + _Aligned_storage_t<_Mtx_internal_imp_size, _Mtx_internal_imp_alignment> _Mtx_storage; + _Mtx_internal_imp_mirror _Mtx_storage_mirror; + }; _Mtx_t _Mymtx() noexcept { // get pointer to _Mtx_internal_imp_t inside _Mtx_storage return reinterpret_cast<_Mtx_t>(&_Mtx_storage); } - - int& _Ownership_levels() noexcept { // the count is denoted by an int subobject at the end of _Mtx_storage - return reinterpret_cast(&_Mtx_storage + 1)[-1]; - } }; +static_assert(sizeof(_Mutex_base) == _Mtx_internal_imp_size, "inconsistent size for mutex"); +static_assert(alignof(_Mutex_base) == _Mtx_internal_imp_alignment, "inconsistent alignment for mutex"); + _EXPORT_STD class mutex : public _Mutex_base { // class for mutual exclusion public: /* constexpr */ mutex() noexcept // TRANSITION, ABI @@ -114,7 +144,7 @@ public: : _Mutex_base(_Mtx_recursive) {} _NODISCARD_TRY_CHANGE_STATE bool try_lock() noexcept { - return _Mutex_base::try_lock(); + return _Mutex_base::try_lock() && _Verify_ownership_levels(); } recursive_mutex(const recursive_mutex&) = delete;