std::mutex should be trivially destructible

[Quuxplusone]

Bugzilla Link	PR27658
Status	CONFIRMED
Importance	P normal
Reported by	Eric Fiselier (eric@efcs.ca)
Reported on	2016-05-05 13:25:26 -0700
Last modified on	2019-07-07 10:58:05 -0700
Version	unspecified
Hardware	PC All
CC	compnerd@compnerd.org, hfinkel@anl.gov, llvm-bugs@lists.llvm.org, mclow.lists@gmail.com, nicolasweber@gmx.de
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

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

For these reasons I believe we should change 'std::mutex' to be trivially
destructible (when possible). This means *NOT* invoking
"pthread_mutex_destroy(...)" in the destructor.

I believe is a safe change on some pthread implementations. The main purpose of
"pthread_mutex_destroy" is to set the lock to an invalid value, allowing use-
after-free to be diagnosed. AFAIK mutex's initialized with
"PTHREAD_MUTEX_INITIALIZER" own no resources and so omitting the call will not
cause leaks.

On other pthread implementations this change will not be possible.

Note that std::mutex::~mutex is defined in the dylib, so such a change would
have to ensure we continue to define it there.

[Quuxplusone]

Here's a survey of what implementations require pthread_mutex_destroy.

FreeBSD
-------

std::mutex requires a destructor on this platform.

On FreeBSD pthread_mutex_t is a typedef for a pointer type, which must be
deallocated using pthread_mutex_destroy. PTHREAD_MUTEX_INITIALIZER is defined
as "NULL", and null mutexs are initialized by the first call to
pthread_mutex_lock.

https://github.com/freebsd/freebsd/blob/e472ae2aec6936a875835e91508cefe7a262a853/lib/libthr/thread/thr_mutex.c#L608

NPTL (GLIBC)
------------

std::mutex should not require a destructor on this platform.

Here pthread_mutex_t is a struct type, which does not hold allocated resources.
The definition of pthread_mutex_destroy seems to be OK to emit.

https://github.com/lattera/glibc/blob/master/nptl/pthread_mutex_destroy.c#L26

Apple
-----

std::mutex should not require a destructor on this platform.

pthread_mutex_t is a struct type that does not contain allocated resources.
Calls to pthread_mutex_t may be omitted.

https://opensource.apple.com/source/Libc/Libc-391.2.3/pthreads/pthread_mutex.c

[Quuxplusone]

This also looks safe on Android/Bionic, but I'll check with each relevant vendor before enabling this change on their platform.

https://android.googlesource.com/platform/bionic/+/master/libc/bionic/pthread_mutex.cpp#1008

[Quuxplusone]

What about libc++/Windows?

[Quuxplusone]

This optimization doesn't work with mingw threads, but it looks safe for Windows native threads seeing as our implementation of libcpp_mutex_destroy is already a NOP [1]. libcpp_condvar_destroy is similarly a NOP on Windows.

I'll commit the relevant changes shortly.

[1] https://github.com/llvm-mirror/libcxx/blob/master/src/support/win32/thread_win32.cpp#L87-L91

[Quuxplusone]

r365281 for the Windows changes, and I mailed D64298 and D64299 to do the same thing on Apple and Android.