From 8baf83839e9a22fff98d3b906a63063f62934d16 Mon Sep 17 00:00:00 2001 From: Eric Fiselier Date: Sun, 7 Jul 2019 01:20:54 +0000 Subject: [PATCH] Fix PR27658 - Make ~mutex trivial when possible. Currently std::mutex has a constexpr constructor, but a non-trivial destruction. The constexpr constructor is required to ensure the construction of a mutex with static storage duration happens at compile time, during constant initialization, and not during dynamic initialization. This means that static mutex's are always initialized and can be used safely during dynamic initialization without the "static initialization order fiasco". A trivial destructor is important for similar reasons. If a mutex is used during dynamic initialization it might also be used during program termination. If a static mutex has a non-trivial destructor it will be invoked during termination. This can introduce the "static deinitialization order fiasco". Additionally, function-local statics emit a guard variable around non-trivially destructible types. This results in horrible codegen and adds a runtime cost to every call to that function. non-local static's also result in slightly worse codegen but it's not as big of a problem. Example codegen can be found here: https://goo.gl/3CSzbM Note: This optimization is not safe with every pthread implementation. Some implementations allocate on the first call to pthread_mutex_lock and free the allocation in pthread_mutex_destroy. Also, changing the triviality of the destructor is not an ABI break. At least to the best of my knowledge :-) llvm-svn: 365273 --- libcxx/include/__config | 8 +++ libcxx/include/__mutex_base | 22 ++++---- libcxx/src/CMakeLists.txt | 1 + libcxx/src/mutex.cpp | 5 +- libcxx/src/mutex_destructor.cpp | 51 +++++++++++++++++++ .../thread.mutex.class/default.pass.cpp | 6 +-- 6 files changed, 73 insertions(+), 20 deletions(-) create mode 100644 libcxx/src/mutex_destructor.cpp diff --git a/libcxx/include/__config b/libcxx/include/__config index 51ac16cc572285..1ed51699932cc4 100644 --- a/libcxx/include/__config +++ b/libcxx/include/__config @@ -1097,6 +1097,14 @@ _LIBCPP_FUNC_VIS extern "C" void __sanitizer_annotate_contiguous_container( _LIBCPP_HAS_NO_THREADS is defined. #endif +// The Apple, glibc, and Bionic implementation of pthreads implements +// pthread_mutex_destroy as nop for regular mutexes. +// TODO(EricWF): Enable this optimization on Apple and Bionic platforms after +// speaking to their respective stakeholders. +#if defined(_LIBCPP_HAS_THREAD_API_PTHREAD) && defined(__GLIBC__) +# define _LIBCPP_HAS_TRIVIAL_MUTEX_DESTRUCTION +#endif + // Systems that use capability-based security (FreeBSD with Capsicum, // Nuxi CloudABI) may only provide local filesystem access (using *at()). // Functions like open(), rename(), unlink() and stat() should not be diff --git a/libcxx/include/__mutex_base b/libcxx/include/__mutex_base index 0c34a3bf7095ef..61abae979ac3f1 100644 --- a/libcxx/include/__mutex_base +++ b/libcxx/include/__mutex_base @@ -36,28 +36,24 @@ _LIBCPP_BEGIN_NAMESPACE_STD # endif #endif // _LIBCPP_THREAD_SAFETY_ANNOTATION + class _LIBCPP_TYPE_VIS _LIBCPP_THREAD_SAFETY_ANNOTATION(capability("mutex")) mutex { -#ifndef _LIBCPP_CXX03_LANG __libcpp_mutex_t __m_ = _LIBCPP_MUTEX_INITIALIZER; -#else - __libcpp_mutex_t __m_; -#endif public: _LIBCPP_INLINE_VISIBILITY -#ifndef _LIBCPP_CXX03_LANG - constexpr mutex() = default; + _LIBCPP_CONSTEXPR mutex() = default; + + mutex(const mutex&) = delete; + mutex& operator=(const mutex&) = delete; + +#if defined(_LIBCPP_HAS_TRIVIAL_MUTEX_DESTRUCTION) + ~mutex() = default; #else - mutex() _NOEXCEPT {__m_ = (__libcpp_mutex_t)_LIBCPP_MUTEX_INITIALIZER;} -#endif ~mutex() _NOEXCEPT; +#endif -private: - mutex(const mutex&);// = delete; - mutex& operator=(const mutex&);// = delete; - -public: void lock() _LIBCPP_THREAD_SAFETY_ANNOTATION(acquire_capability()); bool try_lock() _NOEXCEPT _LIBCPP_THREAD_SAFETY_ANNOTATION(try_acquire_capability(true)); void unlock() _NOEXCEPT _LIBCPP_THREAD_SAFETY_ANNOTATION(release_capability()); diff --git a/libcxx/src/CMakeLists.txt b/libcxx/src/CMakeLists.txt index 2a8ff2c2d89b7f..56d7b9888e3a70 100644 --- a/libcxx/src/CMakeLists.txt +++ b/libcxx/src/CMakeLists.txt @@ -22,6 +22,7 @@ set(LIBCXX_SOURCES locale.cpp memory.cpp mutex.cpp + mutex_destructor.cpp new.cpp optional.cpp random.cpp diff --git a/libcxx/src/mutex.cpp b/libcxx/src/mutex.cpp index 33a8197dadf849..0d69d7cacfa1af 100644 --- a/libcxx/src/mutex.cpp +++ b/libcxx/src/mutex.cpp @@ -25,10 +25,7 @@ const defer_lock_t defer_lock = {}; const try_to_lock_t try_to_lock = {}; const adopt_lock_t adopt_lock = {}; -mutex::~mutex() _NOEXCEPT -{ - __libcpp_mutex_destroy(&__m_); -} +// ~mutex is defined elsewhere void mutex::lock() diff --git a/libcxx/src/mutex_destructor.cpp b/libcxx/src/mutex_destructor.cpp new file mode 100644 index 00000000000000..3a6b6b55eeb776 --- /dev/null +++ b/libcxx/src/mutex_destructor.cpp @@ -0,0 +1,51 @@ +//===--------------------- mutex_destructor.cpp ---------------------------===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +// Define ~mutex. +// +// On some platforms ~mutex has been made trivial and the definition is only +// provided for ABI compatibility. +// +// In order to avoid ODR violations within libc++ itself, we need to ensure +// that *nothing* sees the non-trivial mutex declaration. For this reason +// we re-declare the entire class in this file instead of using +// _LIBCPP_BUILDING_LIBRARY to change the definition in the headers. + +#include "__config" +#include "__threading_support" + +#if !defined(_LIBCPP_HAS_NO_THREADS) +#if _LIBCPP_ABI_VERSION == 1 || !defined(_LIBCPP_HAS_TRIVIAL_MUTEX_DESTRUCTION) +#define NEEDS_MUTEX_DESTRUCTOR +#endif +#endif + +_LIBCPP_BEGIN_NAMESPACE_STD + +#ifdef NEEDS_MUTEX_DESTRUCTOR +class _LIBCPP_TYPE_VIS mutex +{ + __libcpp_mutex_t __m_ = _LIBCPP_MUTEX_INITIALIZER; + +public: + _LIBCPP_ALWAYS_INLINE _LIBCPP_INLINE_VISIBILITY + constexpr mutex() = default; + mutex(const mutex&) = delete; + mutex& operator=(const mutex&) = delete; + ~mutex() noexcept; +}; + + +mutex::~mutex() _NOEXCEPT +{ + __libcpp_mutex_destroy(&__m_); +} + +#endif // !_LIBCPP_HAS_NO_THREADS +_LIBCPP_END_NAMESPACE_STD + diff --git a/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp b/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp index e2fd416b6e43d2..f754e7b7031d39 100644 --- a/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp +++ b/libcxx/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp @@ -21,8 +21,8 @@ int main(int, char**) { - static_assert(std::is_nothrow_default_constructible::value, ""); - std::mutex m; - + static_assert(std::is_nothrow_default_constructible::value, ""); + std::mutex m; + ((void)m); return 0; }