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 ↗

(On Diff #87822)

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

compiler-rt/trunk/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_);
 }
 
 void BlockingMutex::CheckLocked() {
-  CHECK_EQ((uptr)pthread_self(), owner_);
+  CHECK_NE(*(OSSpinLock*)&opaque_storage_, 0);
 }
 
 u64 NanoTime() {
   return 0;
 }
 
 uptr GetTlsSize() {
   return 0;

compiler-rt/trunk/lib/sanitizer_common/sanitizer_mutex.h

#if SANITIZER_WINDOWS
   explicit BlockingMutex(LinkerInitialized);
 #else
   explicit constexpr BlockingMutex(LinkerInitialized)
       : opaque_storage_ {0, }, owner_(0) {}
 #endif
   BlockingMutex();
   void Lock();
   void Unlock();
+
+  // This function does not guarantee an explicit check that the calling thread
+  // is the thread which owns the mutex. 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 CheckLocked();
  private:
   uptr opaque_storage_[10];
   uptr owner_;  // for debugging
 };
 
 // Reader-writer spin mutex.
 class RWMutex {