[ObjC] Use os_unfair_lock instead of dispatch_semaphore_t.

The minOS version is high enough it can be used, and it avoids the long standing
issue for priority inversion dependent on what callers did with
threading/queues.

objectivec/GPBMessage.m

@@ -32,6 +32,7 @@
 
 #import <objc/message.h>
 #import <objc/runtime.h>
+#import <os/lock.h>
 #import <stdatomic.h>
 
 #import "GPBArray_PackagePrivate.h"
@@ -70,8 +71,7 @@ static NSString *const kGPBDataCoderKey = @"GPBData";
  @package
   GPBUnknownFieldSet *unknownFields_;
   NSMutableDictionary *extensionMap_;
-  // Readonly access to autocreatedExtensionMap_ is protected via
-  // readOnlySemaphore_.
+  // Readonly access to autocreatedExtensionMap_ is protected via readOnlyLock_.
   NSMutableDictionary *autocreatedExtensionMap_;
 
   // If the object was autocreated, we remember the creator so that if we get
@@ -80,19 +80,22 @@ static NSString *const kGPBDataCoderKey = @"GPBData";
   GPBFieldDescriptor *autocreatorField_;
   GPBExtensionDescriptor *autocreatorExtension_;
 
-  // Message can only be mutated from one thread. But some *readonly* operations
-  // modify internal state because they autocreate things. The
-  // autocreatedExtensionMap_ is one such structure. Access during readonly
-  // operations is protected via this semaphore.
-  // NOTE: OSSpinLock may seem like a good fit here but Apple engineers have
-  // pointed out that they are vulnerable to live locking on iOS in cases of
-  // priority inversion:
+  // Message can only be mutated from one thread. But some *readonly* operations modify internal
+  // state because they autocreate things. The autocreatedExtensionMap_ is one such structure.
+  // Access during readonly operations is protected via this semaphore.
+  //
+  // Long ago, this was a OSSpinLock, but then it came to light that there were issues for that on
+  // iOS:
   //   http://mjtsai.com/blog/2015/12/16/osspinlock-is-unsafe/
   //   https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151214/000372.html
-  // Use of readOnlySemaphore_ must be prefaced by a call to
-  // GPBPrepareReadOnlySemaphore to ensure it has been created. This allows
-  // readOnlySemaphore_ to be only created when actually needed.
-  _Atomic(dispatch_semaphore_t) readOnlySemaphore_;
+  // So it was a dispatch_semaphore_t, but that has issues with priority inversion if developer code
+  // uses queue for different things. But the minOS versions are now high enough, os_unfair_lock and
+  // be used, and should provide all the support needed. For more information in the
+  // concurrence/locking space see:
+  //   https://gist.github.com/tclementdev/6af616354912b0347cdf6db159c37057
+  //   https://developer.apple.com/library/archive/documentation/Performance/Conceptual/EnergyGuide-iOS/PrioritizeWorkWithQoS.html
+  //   https://developer.apple.com/videos/play/wwdc2017/706/
+  os_unfair_lock readOnlyLock_;
 }
 @end
 
@@ -746,33 +749,6 @@ void GPBClearMessageAutocreator(GPBMessage *self) {
   self->autocreatorExtension_ = nil;
 }
 
-// Call this before using the readOnlySemaphore_. This ensures it is created only once.
-void GPBPrepareReadOnlySemaphore(GPBMessage *self) {
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wdirect-ivar-access"
-
-  // Create the semaphore on demand (rather than init) as developers might not cause them
-  // to be needed, and the heap usage can add up.  The atomic swap is used to avoid needing
-  // another lock around creating it.
-  if (self->readOnlySemaphore_ == nil) {
-    dispatch_semaphore_t worker = dispatch_semaphore_create(1);
-    dispatch_semaphore_t expected = nil;
-    if (!atomic_compare_exchange_strong(&self->readOnlySemaphore_, &expected, worker)) {
-      dispatch_release(worker);
-    }
-#if defined(__clang_analyzer__)
-    // The static analyzer thinks worker is leaked (doesn't seem to know about
-    // atomic_compare_exchange_strong); so just for the analyzer, let it think
-    // worker is also released in this case.
-    else {
-      dispatch_release(worker);
-    }
-#endif
-  }
-
-#pragma clang diagnostic pop
-}
-
 static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self) {
   if (!self->unknownFields_) {
     self->unknownFields_ = [[GPBUnknownFieldSet alloc] init];
@@ -845,6 +821,7 @@ static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self) {
   if ((self = [super init])) {
     messageStorage_ =
         (GPBMessage_StoragePtr)(((uint8_t *)self) + class_getInstanceSize([self class]));
+    readOnlyLock_ = OS_UNFAIR_LOCK_INIT;
   }
 
   return self;
@@ -915,9 +892,6 @@ static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self) {
 - (void)dealloc {
   [self internalClear:NO];
   NSCAssert(!autocreator_, @"Autocreator was not cleared before dealloc.");
-  if (readOnlySemaphore_) {
-    dispatch_release(readOnlySemaphore_);
-  }
   [super dealloc];
 }
 
@@ -1741,8 +1715,7 @@ static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self) {
   }
 
   // Check for an autocreated value.
-  GPBPrepareReadOnlySemaphore(self);
-  dispatch_semaphore_wait(readOnlySemaphore_, DISPATCH_TIME_FOREVER);
+  os_unfair_lock_lock(&readOnlyLock_);
   value = [autocreatedExtensionMap_ objectForKey:extension];
   if (!value) {
     // Auto create the message extensions to match normal fields.
@@ -1759,7 +1732,7 @@ static GPBUnknownFieldSet *GetOrMakeUnknownFields(GPBMessage *self) {
     [value release];
   }
 
-  dispatch_semaphore_signal(readOnlySemaphore_);
+  os_unfair_lock_unlock(&readOnlyLock_);
   return value;
 }
 

objectivec/GPBRootObject.m

@@ -31,6 +31,7 @@
 #import "GPBRootObject_PackagePrivate.h"
 
 #import <objc/runtime.h>
+#import <os/lock.h>
 
 #import <CoreFoundation/CoreFoundation.h>
 
@@ -96,19 +97,24 @@ static CFHashCode GPBRootExtensionKeyHash(const void *value) {
   return jenkins_one_at_a_time_hash(key);
 }
 
-// NOTE: OSSpinLock may seem like a good fit here but Apple engineers have
-// pointed out that they are vulnerable to live locking on iOS in cases of
-// priority inversion:
+// Long ago, this was a OSSpinLock, but then it came to light that there were issues for that on
+// iOS:
 //   http://mjtsai.com/blog/2015/12/16/osspinlock-is-unsafe/
 //   https://lists.swift.org/pipermail/swift-dev/Week-of-Mon-20151214/000372.html
-static dispatch_semaphore_t gExtensionSingletonDictionarySemaphore;
+// So it was a dispatch_semaphore_t, but that has issues with priority inversion if developer code
+// uses queue for different things. But the minOS versions are now high enough, os_unfair_lock and
+// be used, and should provide all the support needed. For more information in the
+// concurrence/locking space see:
+//   https://gist.github.com/tclementdev/6af616354912b0347cdf6db159c37057
+//   https://developer.apple.com/library/archive/documentation/Performance/Conceptual/EnergyGuide-iOS/PrioritizeWorkWithQoS.html
+//   https://developer.apple.com/videos/play/wwdc2017/706/
+static os_unfair_lock gExtensionSingletonDictionaryLock = OS_UNFAIR_LOCK_INIT;
 static CFMutableDictionaryRef gExtensionSingletonDictionary = NULL;
 static GPBExtensionRegistry *gDefaultExtensionRegistry = NULL;
 
 + (void)initialize {
   // Ensure the global is started up.
   if (!gExtensionSingletonDictionary) {
-    gExtensionSingletonDictionarySemaphore = dispatch_semaphore_create(1);
     CFDictionaryKeyCallBacks keyCallBacks = {
         // See description above for reason for using custom dictionary.
         0,
@@ -139,9 +145,9 @@ static GPBExtensionRegistry *gDefaultExtensionRegistry = NULL;
 
 + (void)globallyRegisterExtension:(GPBExtensionDescriptor *)field {
   const char *key = [field singletonNameC];
-  dispatch_semaphore_wait(gExtensionSingletonDictionarySemaphore, DISPATCH_TIME_FOREVER);
+  os_unfair_lock_lock(&gExtensionSingletonDictionaryLock);
   CFDictionarySetValue(gExtensionSingletonDictionary, key, field);
-  dispatch_semaphore_signal(gExtensionSingletonDictionarySemaphore);
+  os_unfair_lock_unlock(&gExtensionSingletonDictionaryLock);
 }
 
 static id ExtensionForName(id self, SEL _cmd) {
@@ -173,20 +179,20 @@ static id ExtensionForName(id self, SEL _cmd) {
   key[classNameLen + 1 + selNameLen] = '\0';
 
   // NOTE: Even though this method is called from another C function,
-  // gExtensionSingletonDictionarySemaphore and gExtensionSingletonDictionary
+  // gExtensionSingletonDictionaryLock and gExtensionSingletonDictionary
   // will always be initialized. This is because this call flow is just to
   // lookup the Extension, meaning the code is calling an Extension class
   // message on a Message or Root class. This guarantees that the class was
   // initialized and Message classes ensure their Root was also initialized.
   NSAssert(gExtensionSingletonDictionary, @"Startup order broken!");
 
-  dispatch_semaphore_wait(gExtensionSingletonDictionarySemaphore, DISPATCH_TIME_FOREVER);
+  os_unfair_lock_lock(&gExtensionSingletonDictionaryLock);
   id extension = (id)CFDictionaryGetValue(gExtensionSingletonDictionary, key);
   // We can't remove the key from the dictionary here (as an optimization),
   // two threads could have gone into +resolveClassMethod: for the same method,
   // and ended up here; there's no way to ensure both return YES without letting
   // both try to wire in the method.
-  dispatch_semaphore_signal(gExtensionSingletonDictionarySemaphore);
+  os_unfair_lock_unlock(&gExtensionSingletonDictionaryLock);
   return extension;
 }