Make ~mutex and ~condition_variable trivial with Bionic pthreads

[EricWF]

Summary

This optimization, as described in PR27658, is safe to perform with Bionic pthreads. The implementation of pthread_mutex_destroy and pthread_cond_destroy do nothing except poison the lock/condvar to aid in debugging [1][2]

If we choose to apply this patch we'll lose some amount of error checking, but I think it's probably worth it given the codegen improvements for global of function local static mutexes.

What do you think?

[1] https://android.googlesource.com/platform/bionic/+/10ce969/libc/bionic/pthread.c#1567
[2] https://android.googlesource.com/platform/bionic/+/10ce969/libc/bionic/pthread.c#1654

[danalbert]

I think for Android we'd rather have the debuggability, but +enh for a second opinion.

[enh]

i assume the idea here is "all libc++ mutexes are covered by RAII anyway, so there's no value to the error checking"?

ah, i see https://reviews.llvm.org/rL365273 has more motivation.

i think main concern would be that this would make it harder to change the pthread mutex implementation at a later date, but that seems unrealistic anyway given our existing ABI constraints, and i don't think anyone would want to add an allocation in there anyway. if i knew how to add yabinc and eugenis i'd ask their opinions too, but so far this actually seems reasonable (and the "trivial destructor" motivation in the other CL is a stronger argument than the "optimization" i was expecting).

lgtm.

[eugenis]

This feels like the right trade-off.

TSan should be fine with a mutex that is never destroyed, but it will not be able to catch lock-after-destroy bugs on such mutex obviously.
@dvyukov

[EricWF]

For sanitizers I wonder if we could continue to catch use after destrot without losing triviality. Doesn't MSAN poison the objects memory at the end of the destructor?

I considered adding the destructor back in debug modes, but this can break or deadlock code by introducing a potentially blocking region into the middle of a carfully considered locking algir

[danalbert]

Okay, SGTM.

MSan won't currently help us on Android (and I don't think there's a plan to support that any time soon, but eugenis would know better).

[dvyukov]

In D64299#1588251, @eugenis wrote:

This feels like the right trade-off.

TSan should be fine with a mutex that is never destroyed, but it will not be able to catch lock-after-destroy bugs on such mutex obviously.
@dvyukov

Effectively pthread_mutex_destroy is not trivial under tsan so, yes, there will be a number of negative consequences:

• no lock-after-destroy races
• no races between lock/unlock and a subsequent destory
• no racy use-after-free detection for destroy vs free
• incorrect stack trace for a mutex that happened to reuse the address later
• resource leaks for mutexes
• no reporting of unlock of locked mutex
• no auto-unlock for destroyed mutexes, which will look like the thread is taking infinite number of locks recursively, which will quickly check-fail in deadlock detector

[eugenis]

In D64299#1588931, @dvyukov wrote:

In D64299#1588251, @eugenis wrote:

This feels like the right trade-off.

TSan should be fine with a mutex that is never destroyed, but it will not be able to catch lock-after-destroy bugs on such mutex obviously.
@dvyukov

Effectively pthread_mutex_destroy is not trivial under tsan so, yes, there will be a number of negative consequences:

• no lock-after-destroy races
• no races between lock/unlock and a subsequent destory
• no racy use-after-free detection for destroy vs free
• incorrect stack trace for a mutex that happened to reuse the address later
• resource leaks for mutexes
• no reporting of unlock of locked mutex
• no auto-unlock for destroyed mutexes, which will look like the thread is taking infinite number of locks recursively, which will quickly check-fail in deadlock detector

Why is that not an issue for pthread mutexes with static initialization and no destruction?

[EricWF]

Thanks for bringing this up. Indeed this change really hurts TSAN.
I'll look for ways to work around this issue.

I think we could have clang provide an annotation for mutex types that will inject calls to the tsan runtime when a mutex gets created or destroyed.

On a separate note, what kinds of resources leaks are you imagining? And where are these resources created?

[dvyukov]

In D64299#1590038, @eugenis wrote:

In D64299#1588931, @dvyukov wrote:

In D64299#1588251, @eugenis wrote:

This feels like the right trade-off.

TSan should be fine with a mutex that is never destroyed, but it will not be able to catch lock-after-destroy bugs on such mutex obviously.
@dvyukov

Effectively pthread_mutex_destroy is not trivial under tsan so, yes, there will be a number of negative consequences:

• no lock-after-destroy races
• no races between lock/unlock and a subsequent destory
• no racy use-after-free detection for destroy vs free
• incorrect stack trace for a mutex that happened to reuse the address later
• resource leaks for mutexes
• no reporting of unlock of locked mutex
• no auto-unlock for destroyed mutexes, which will look like the thread is taking infinite number of locks recursively, which will quickly check-fail in deadlock detector

Why is that not an issue for pthread mutexes with static initialization and no destruction?

Because these are not destroyed at all. So any problems related to destruction are not problems.
Here we have mutexes that are destroyed, but destructor is not called.

[dvyukov]

In D64299#1590752, @EricWF wrote:

Thanks for bringing this up. Indeed this change really hurts TSAN.
I'll look for ways to work around this issue.

I think we could have clang provide an annotation for mutex types that will inject calls to the tsan runtime when a mutex gets created or destroyed.

On a separate note, what kinds of resources leaks are you imagining?

Memory.

And where are these resources created?

When mutex is created or first locked.

include/__config

 #if defined(_LIBCPP_HAS_NO_MONOTONIC_CLOCK) && !defined(_LIBCPP_HAS_NO_THREADS)
 #error _LIBCPP_HAS_NO_MONOTONIC_CLOCK may only be defined when \
        _LIBCPP_HAS_NO_THREADS is defined.
 #endif
 
 // The Apple, glibc, and Bionic implementation of pthreads implements
 // pthread_mutex_destroy as nop for regular mutexes. Additionally, Win32
 // mutexes have no destroy mechanism.
-// TODO(EricWF): Enable this optimization on Apple and Bionic platforms after
+// TODO(EricWF): Enable this optimization on Apple platforms after
 // speaking to their respective stakeholders.
-#if (defined(_LIBCPP_HAS_THREAD_API_PTHREAD) && defined(__GLIBC__)) \
+#if (defined(_LIBCPP_HAS_THREAD_API_PTHREAD) \
+     && (defined(__GLIBC__) || defined(__BIONIC__))) \
   || defined(_LIBCPP_HAS_THREAD_API_WIN32)
 # define _LIBCPP_HAS_TRIVIAL_MUTEX_DESTRUCTION
 #endif
 
-// Destroying a condvar is a nop on Windows.
-// TODO(EricWF): This is potentially true for some pthread implementations
-// as well.
-#if defined(_LIBCPP_HAS_THREAD_API_WIN32)
+// Destroying a condvar is a nop on Windows, as well as with the Bionic
+// implementations of pthreads
+#if (defined(_LIBCPP_HAS_THREAD_API_PTHREAD) && defined(__BIONIC__)) \
+  || defined(_LIBCPP_HAS_THREAD_API_WIN32)
 # define _LIBCPP_HAS_TRIVIAL_CONDVAR_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
 // used, as they attempt to access the global filesystem namespace.
 #ifdef __CloudABI__