Remove strict tid checks from the mac implementation of BlockingMutex

[fjricci]

Summary

This patch unifies the behavior of BlockingMutex on linux and mac,
resolving problems that can arise when BlockingMutex is used in
code shared by the two platforms but has different behavior depending
on the platform.

No longer requires that the calling thread own the mutex for
CheckLocked calls to pass.

[fjricci]

Resolves issues discussed in D29588.

[dvyukov]

Since we spent so much time discussion this, I guess you need to leave some traces in comments for future generations. Document what's CheckLocked contract and why it is that way.

[fjricci]

Add comment explaining CheckLocked behavior

[dvyukov]

Do you have commit access? Or you want me to commit after the comment move?

lib/sanitizer_common/sanitizer_mac.cc

359

Sorry I meant comment on _declaration of the function.
This implementation is now not different from other implementations, so it does not make sense to have it here. Also a linux, or windows developers does not necessary read comments in _mac files, so he/she can change, say, windows implementation tomorrow to do the stricter check.

[dvyukov]

Kuba, this is a prerequisite for lsan on mac.

[fjricci]

I do have commit access, thanks for checking! I'll move the comment and commit.

[dvyukov]

LGTM if the comment is moved

lib/sanitizer_common/sanitizer_mac.cc

 
 BlockingMutex::BlockingMutex() {
   internal_memset(this, 0, sizeof(*this));
 }
 
 void BlockingMutex::Lock() {
   CHECK(sizeof(OSSpinLock) <= sizeof(opaque_storage_));
   CHECK_EQ(OS_SPINLOCK_INIT, 0);
-  CHECK_NE(owner_, (uptr)pthread_self());
+  CHECK_EQ(owner_, 0);
   OSSpinLockLock((OSSpinLock*)&opaque_storage_);
-  CHECK(!owner_);
-  owner_ = (uptr)pthread_self();
 }
 
 void BlockingMutex::Unlock() {
-  CHECK(owner_ == (uptr)pthread_self());
-  owner_ = 0;
   OSSpinLockUnlock((OSSpinLock*)&opaque_storage_);
 }
 
+// This function explicitly avoids checking whether the mutex is owned by
+// the calling thread. This behavior, while more strictly correct, causes
+// problems in cases like StopTheWorld, where a parent thread owns the mutex
+// but a child checks that it is locked. Rather than maintaining complex state
+// to work around those situations, the check only checks that the mutex is
+// owned, and assumes callers to be generally well-behaved.
 void BlockingMutex::CheckLocked() {
-  CHECK_EQ((uptr)pthread_self(), owner_);
+  CHECK_NE(*(OSSpinLock*)&opaque_storage_, 0);
 }
 
 u64 NanoTime() {
   return 0;
 }
 
 uptr GetTlsSize() {
   return 0;