[libcxx-commits] [libcxx] r365273 - Fix PR27658 - Make ~mutex trivial when possible.
Kristina Brooks via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Jul 9 21:48:30 PDT 2019
Had the same breakage (LIBCPP_ABI=2), currently using a hack to
provide backwards compatibility to anything that imported it (by just
exporting a dummy symbol so dynamic linking works). I think I
mentioned this on IRC to Eric, not sure if he noticed.
< kristina_> EricWF: for _LIBCPP_HAS_TRIVIAL_MUTEX_DESTRUCTION is it
possible to provide a symbol for backwards compat inside libc++ like
you did with condvar?
< kristina_> somehow this worked before though - #if
_LIBCPP_ABI_VERSION == 1 ||
!defined(_LIBCPP_HAS_TRIVIAL_MUTEX_DESTRUCTION)
< kristina_> at _LIBCPP_ABI_VERSION=2
It would be really nice to have it exported just for compatibility in
upstream libc++ without needing local patches, although I'm not sure
how libc++ maintainers feel about that.
Thank you.
On Tue, Jul 9, 2019 at 12:09 AM Petr Hosek via libcxx-commits
<libcxx-commits at lists.llvm.org> wrote:
>
> FYI we've seen ABI breakage in our build:
>
> ld.lld: error: undefined symbol: std::__2::mutex::~mutex()
> >>> referenced by ErrorHandling.cpp:60 (/b/s/w/ir/k/llvm-project/llvm/lib/Support/ErrorHandling.cpp:60)
> >>> ErrorHandling.cpp.o:(_GLOBAL__sub_I_ErrorHandling.cpp) in archive ../../garnet/third_party/llvm/lib/libLLVMSupport.a
>
> I'm not sure if it's related to our use of ABI v2? It's something we can handle by rolling out a new version of LLVM prebuilt, but I thought you may want to be aware.
>
> On Sat, Jul 6, 2019 at 6:21 PM Eric Fiselier via libcxx-commits <libcxx-commits at lists.llvm.org> wrote:
>>
>> Author: ericwf
>> Date: Sat Jul 6 18:20:54 2019
>> New Revision: 365273
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=365273&view=rev
>> Log:
>> 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 :-)
>>
>> Added:
>> libcxx/trunk/src/mutex_destructor.cpp
>> Modified:
>> libcxx/trunk/include/__config
>> libcxx/trunk/include/__mutex_base
>> libcxx/trunk/src/CMakeLists.txt
>> libcxx/trunk/src/mutex.cpp
>> libcxx/trunk/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp
>>
>> Modified: libcxx/trunk/include/__config
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__config?rev=365273&r1=365272&r2=365273&view=diff
>> ==============================================================================
>> --- libcxx/trunk/include/__config (original)
>> +++ libcxx/trunk/include/__config Sat Jul 6 18:20:54 2019
>> @@ -1097,6 +1097,14 @@ _LIBCPP_FUNC_VIS extern "C" void __sanit
>> _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
>>
>> Modified: libcxx/trunk/include/__mutex_base
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/include/__mutex_base?rev=365273&r1=365272&r2=365273&view=diff
>> ==============================================================================
>> --- libcxx/trunk/include/__mutex_base (original)
>> +++ libcxx/trunk/include/__mutex_base Sat Jul 6 18:20:54 2019
>> @@ -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());
>>
>> Modified: libcxx/trunk/src/CMakeLists.txt
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/CMakeLists.txt?rev=365273&r1=365272&r2=365273&view=diff
>> ==============================================================================
>> --- libcxx/trunk/src/CMakeLists.txt (original)
>> +++ libcxx/trunk/src/CMakeLists.txt Sat Jul 6 18:20:54 2019
>> @@ -22,6 +22,7 @@ set(LIBCXX_SOURCES
>> locale.cpp
>> memory.cpp
>> mutex.cpp
>> + mutex_destructor.cpp
>> new.cpp
>> optional.cpp
>> random.cpp
>>
>> Modified: libcxx/trunk/src/mutex.cpp
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/mutex.cpp?rev=365273&r1=365272&r2=365273&view=diff
>> ==============================================================================
>> --- libcxx/trunk/src/mutex.cpp (original)
>> +++ libcxx/trunk/src/mutex.cpp Sat Jul 6 18:20:54 2019
>> @@ -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()
>>
>> Added: libcxx/trunk/src/mutex_destructor.cpp
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/src/mutex_destructor.cpp?rev=365273&view=auto
>> ==============================================================================
>> --- libcxx/trunk/src/mutex_destructor.cpp (added)
>> +++ libcxx/trunk/src/mutex_destructor.cpp Sat Jul 6 18:20:54 2019
>> @@ -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
>> +
>>
>> Modified: libcxx/trunk/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp
>> URL: http://llvm.org/viewvc/llvm-project/libcxx/trunk/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp?rev=365273&r1=365272&r2=365273&view=diff
>> ==============================================================================
>> --- libcxx/trunk/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp (original)
>> +++ libcxx/trunk/test/std/thread/thread.mutex/thread.mutex.requirements/thread.mutex.requirements.mutex/thread.mutex.class/default.pass.cpp Sat Jul 6 18:20:54 2019
>> @@ -21,8 +21,8 @@
>>
>> int main(int, char**)
>> {
>> - static_assert(std::is_nothrow_default_constructible<std::mutex>::value, "");
>> - std::mutex m;
>> -
>> + static_assert(std::is_nothrow_default_constructible<std::mutex>::value, "");
>> + std::mutex m;
>> + ((void)m);
>> return 0;
>> }
>>
>>
>> _______________________________________________
>> libcxx-commits mailing list
>> libcxx-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/libcxx-commits
>
> _______________________________________________
> libcxx-commits mailing list
> libcxx-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/libcxx-commits
More information about the libcxx-commits
mailing list