[darwin] switch blocking mutex from osspinlock to os_unfair_lock

[aralisza]

Summary

OSSpinLock is deprecated, so we are switching to os_unfair_lock. However, os_unfair_lock isn't available on older OSs, so we keep OSSpinLock as fallback.

Also change runtime assumption check to static since they only ever check constant values.

rdar://69588111

[aralisza]

make owner check static as well

[yln]

LGTM with nits!

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

515

I think this "not locked" (no owner) assert got lost accidentally.

518

525–529

Good catch for spotting that these can be static asserts!

Let's move them out of the function to show that they don't execute when it is called and make the function easier to read.
https://stackoverflow.com/questions/3718910/can-i-get-the-size-of-a-struct-field-w-o-creating-an-instance-of-the-struct

[delcypher]

LGTM apart from the owner_ check. Turning as many of the runtime checks into compile time checks seems like a good idea :)

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

518

Note:

Availability on this API is

#define OS_UNFAIR_LOCK_AVAILABILITY \
		__API_AVAILABLE(macos(10.12), ios(10.0), tvos(10.0), watchos(3.0))

but our minimum deployment targets are

macOS: 10.10
iOS: 9.0
tvOS: 9.0
watchOS: 2.0

which makes me sad because it means we have to guard use of the API like this.

@yln You've done more of these conditional API checks recently. Does this look the right thing to do?

525–529

I'm not sure this should really be a static check. At this point in the code we don't know which constructor was used for initialization. This static assert is actually only checking the constexpr constructor but it's not checking the other constructor (because it can't).

We're not actually using the owner_ field AFAICT so I'm not really sure if us checking the value of the owner_ field actually really matters here. If there's agreement here we could drop the check. If not then we should probably keep CHECK_EQ(owner_, 0).

[aralisza]

move static checks and make owner check runtime check again

[aralisza]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

525–529

the owner_ field is private, so I can't access it outside of the method definitions. I moved it to the constructor, is that a good alternative? Or is there a better way?

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

525–529

@yln I wouldn't expect that static_asserts would ever be executed. But you're right that moving them elsewhere might help clarity.

525–529

There are terrible alternatives (like adding a "friend" function) but let's not do that. I think that moving the static_asserts that we can't move to global scope into the BlockingMutex constructor is reasonable.

[hans]

This broke Chromium's AddressSanitizer build on Mac: https://crbug.com/1184481

Based on D97821 and D97880 it looks like you're planning on reverting this, so I've gone ahead and pushed 840a16d3c4cbb193dde4e1f41f9137a2a234d0f8 to do that.

compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

 #include <errno.h>
 #include <fcntl.h>
 #include <libkern/OSAtomic.h>
 #include <mach-o/dyld.h>
 #include <mach/mach.h>
 #include <mach/mach_time.h>
 #include <mach/vm_statistics.h>
 #include <malloc/malloc.h>
+#include <os/lock.h>
 #include <os/log.h>
 #include <pthread.h>
 #include <sched.h>
 #include <signal.h>
 #include <spawn.h>
 #include <stdlib.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>
if (malloc_num_zones > 1 && malloc_zones[0] != &sanitizer_zone) {
           break;
         }
       }
     }
   }
 }
 
 BlockingMutex::BlockingMutex() {
+  // Initialize all member variables to 0
   internal_memset(this, 0, sizeof(*this));
+  static_assert(sizeof(os_unfair_lock_t) <= sizeof(opaque_storage_),
+                "Not enough space in opaque storage to use os_unfair_lock");
+  static_assert(sizeof(OSSpinLock) <= sizeof(opaque_storage_),
+                "Not enough space in opaque storage to use OSSpinLock");
 }
 
+static bool UnfairLockAvailable() { return bool(os_unfair_lock_lock); }
+static_assert(OS_UNFAIR_LOCK_INIT._os_unfair_lock_opaque == 0,
+              "os_unfair_lock does not initialize to 0");
+static_assert(OS_SPINLOCK_INIT == 0, "OSSpinLock does not initialize to 0");
+
 void BlockingMutex::Lock() {
-  CHECK(sizeof(OSSpinLock) <= sizeof(opaque_storage_));
-  CHECK_EQ(OS_SPINLOCK_INIT, 0);
   CHECK_EQ(owner_, 0);
-  OSSpinLockLock((OSSpinLock*)&opaque_storage_);
+  if (UnfairLockAvailable()) {
+    os_unfair_lock_lock((os_unfair_lock *)&opaque_storage_);
+  } else {
+    OSSpinLockLock((OSSpinLock *)&opaque_storage_);
   }
+}
 
 void BlockingMutex::Unlock() {
-  OSSpinLockUnlock((OSSpinLock*)&opaque_storage_);
+  if (UnfairLockAvailable()) {
+    os_unfair_lock_unlock((os_unfair_lock *)&opaque_storage_);
+  } else {
+    OSSpinLockUnlock((OSSpinLock *)&opaque_storage_);
   }
+}
 
 void BlockingMutex::CheckLocked() {
-  CHECK_NE(*(OSSpinLock*)&opaque_storage_, 0);
+  if (UnfairLockAvailable()) {
+    CHECK_NE((*(os_unfair_lock *)&opaque_storage_)._os_unfair_lock_opaque, 0);
+  } else {
+    CHECK_NE(*(OSSpinLock *)&opaque_storage_, 0);
   }
+}
 
 u64 NanoTime() {
   timeval tv;
   internal_memset(&tv, 0, sizeof(tv));
   gettimeofday(&tv, 0);
   return (u64)tv.tv_sec * 1000*1000*1000 + tv.tv_usec * 1000;
 }