[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 22:29:04 PDT 2019
Older versions (before the patch) of libc++ exported it, I just
checked a few older builds of libc++ DSO and they had the destructor
exported, my ugly compatibility hack just involved exporting the
mangled version of it with C linkage that was a no-op. I think
technically this can be worked around by rebuilding everything with
new headers and new libc++ (with this change), removing the need for
the symbol. And I'm not complaining since I'm aware that ABIv2 is
unstable, just wanted to mention that I've had this issue as well, and
thought one compatibility symbol wouldn't hurt to smoothen the
otherwise-ABI-breaking change. I think it would be nice to have it
upstream, at least for the time being but considering the status of
ABIv2, I'm not sure how other libcxx maintainers feel about that. And
to answer your question, the platform in question is ELF based and
I'll also note that newer binaries built against libc++ with this
patch no longer link against the destructor symbol and everything
works great.
Thanks.
On Wed, Jul 10, 2019 at 6:02 AM Eric Fiselier <eric at efcs.ca> wrote:
>
> Wait... What platform is this breaking on?
> The symbol shouldn't be referenced, though that might be happening on Windows.
>
> I think I incorrectly assumed this was a breakage caused by ABI stability. I'll investigate
> early tomorrow.
>
> Apologies,
>
> /Eric
>
> On Wed, Jul 10, 2019 at 12:54 AM Eric Fiselier <eric at efcs.ca> wrote:
>>
>>
>> ABI v2 isnt stable yet. Its just a staging area.
>> It appears we need to communicate this more clearly.
>>
>> /Eric
>>
>> On Wed., Jul. 10, 2019, 12:48 a.m. Kristina Brooks, <notstina at gmail.com> wrote:
>>>
>>> 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