Page MenuHomePhabricator

Make ~mutex and ~condition_variable trivial with Bionic pthreads
AcceptedPublic

Authored by EricWF on Jul 7 2019, 10:55 AM.

Details

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

Diff Detail

Event Timeline

EricWF created this revision.Jul 7 2019, 10:55 AM

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

enh added a comment.Jul 16 2019, 9:30 AM

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.

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

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 accepted this revision.Jul 16 2019, 9:58 PM

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).

This revision is now accepted and ready to land.Jul 16 2019, 9:58 PM

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

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?

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?

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.

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.