[TSan] Enable detection of lock-order-inversions for Objective-C @synchronized

[yln]

Summary

@synchronized semantics can be synthesized by using existing mutex_[un]lock operations.

@synchronized(obj) {
  // ...
}

=> 
{
  mutex_lock(obj);
  // ...
  mutex_unlock(obj);
}

Let me know whether you think this a good idea.

[kubamracek]

LGTM with a nit. @dvyukov are we using the Mutex* calls correctly here?

compiler-rt/lib/tsan/rtl/tsan_interceptors_mac.cc

322 ↗

(On Diff #179167)

Can we just #include <objc-sync.h> instead?

[yln]

Added test for recursive @synchronized blocks, that is, nested @synchronized on the same object. This tests proves lock recursiveness.

Directly #include <objc/objc-sync.h>. Had to change parameter type from void *obj to id obj in the interceptors to avoid error: conflicting types for 'objc_sync_enter'.

[dvyukov]

compiler-rt/lib/tsan/rtl/tsan_interceptors_mac.cc

328 ↗

(On Diff #179192)

If we use these annotations, I think we need to fix SyncAddressForTaggedPointer in some way. Acquire/Release annotations can be arbitrary mixed on the same address; but Mutex annotations for different mutexes can't be mixed, e.g. 2 locks from different threads without unlock.

328 ↗

(On Diff #179192)

We don't check it today, but it can make sense to pass MutexFlagWriteReentrant because later we can start checking it.

330 ↗

(On Diff #179192)

We do not use assert in this code base. Use DCHECK_EQ, or better CHECK_EQ since this is not super performance-critical part.

[yln]

compiler-rt/lib/tsan/rtl/tsan_interceptors_mac.cc

328 ↗

(On Diff #179192)

Passing it on calls that could create the backing SyncVar is sufficient, correct?
That is, MutexPreLock, or MutexPostLock (when detect_deadlocks=0).

328 ↗

(On Diff #179192)

Very good point. I will add a test for synchronizing on a tagged pointer. Also let me investigate what this should do. When synchronizing on tagged pointers we are already getting into murky waters.

To make this work with the current scheme, we would need to map tagged pointers to dummy, but valid addresses, right?

[yln]

Addressed most comments and added failing test for @synchronize with tagged pointers.

[yln]

I would like to land this and fix @synchronized with tagged pointers in a follow-up since more discussion is required what we actually want it to do.

[yln]

Fix bug in test.

[dvyukov]

I would like to land this and fix @synchronized with tagged pointers in a follow-up since more discussion is required what we actually want it to do.

It's fine with me code-wise, but if we release this without follow up changes it will break real programs. If Kuba is OK with it, go ahead and commit.

[yln]

Create diff via arcanist

• Add failing test for deadlock detection for Objective-C @synchronize blocks
• Implement Obj-C @synchronized like full-fledged mutexes
• Activate test
• Refactor @synchronized interceptors
• Add test for @synchronized nesting with recursive locks
• #include <objc/objc-sync.h> directly
• Use CHECK_EQ instead of assert
• Pass MutexFlagWriteReentrant flag
• Add failing test for @synchronized with tagged pointer
• Fix broken test

[yln]

Trying to arc diff on git monorepo and arc patch && arc commit on svn working copy is painful, because path is longer on monorepo.

lib/tsan/rtl/tsan_interceptors_mac.cc

 #if SANITIZER_MAC
 
 #include "interception/interception.h"
 #include "tsan_interceptors.h"
 #include "tsan_interface.h"
 #include "tsan_interface_ann.h"
 
 #include <libkern/OSAtomic.h>
+#include <objc/objc-sync.h>
 
 #if defined(__has_include) && __has_include(<xpc/xpc.h>)
 #include <xpc/xpc.h>
 #endif  // #if defined(__has_include) && __has_include(<xpc/xpc.h>)
 
 typedef long long_t;  // NOLINT
 
 namespace __tsan {

 
 // Address on which we can synchronize for an Objective-C object. Supports
 // tagged pointers.
 static uptr SyncAddressForObjCObject(void *obj) {
   if (IsTaggedObjCPointer(obj)) return SyncAddressForTaggedPointer(obj);
   return (uptr)obj;
 }
 
-TSAN_INTERCEPTOR(int, objc_sync_enter, void *obj) {
+TSAN_INTERCEPTOR(int, objc_sync_enter, id obj) {
   SCOPED_TSAN_INTERCEPTOR(objc_sync_enter, obj);
+  if (!obj) return REAL(objc_sync_enter)(obj);
+  uptr addr = SyncAddressForObjCObject(obj);
+  MutexPreLock(thr, pc, addr, MutexFlagWriteReentrant);
   int result = REAL(objc_sync_enter)(obj);
-  if (obj) Acquire(thr, pc, SyncAddressForObjCObject(obj));
+  CHECK_EQ(result, OBJC_SYNC_SUCCESS);
+  MutexPostLock(thr, pc, addr, MutexFlagWriteReentrant);
   return result;
 }
 
-TSAN_INTERCEPTOR(int, objc_sync_exit, void *obj) {
+TSAN_INTERCEPTOR(int, objc_sync_exit, id obj) {
   SCOPED_TSAN_INTERCEPTOR(objc_sync_exit, obj);
-  if (obj) Release(thr, pc, SyncAddressForObjCObject(obj));
-  return REAL(objc_sync_exit)(obj);
+  if (!obj) return REAL(objc_sync_exit)(obj);
+  uptr addr = SyncAddressForObjCObject(obj);
+  MutexUnlock(thr, pc, addr);
+  int result = REAL(objc_sync_exit)(obj);
+  if (result != OBJC_SYNC_SUCCESS) MutexInvalidAccess(thr, pc, addr);
+  return result;
 }
 
 // On macOS, libc++ is always linked dynamically, so intercepting works the
 // usual way.
 #define STDCXX_INTERCEPTOR TSAN_INTERCEPTOR
 
 namespace {
 struct fake_shared_weak_count {

test/tsan/Darwin/objc-synchronize-cycle-tagged.mm

+// RUN: %clangxx_tsan %s -o %t -framework Foundation -fobjc-arc %darwin_min_target_with_full_runtime_arc_support
+// RUN:     %run %t 6 2>&1 | FileCheck %s --check-prefix=SIX
+// RUN: not %run %t 7 2>&1 | FileCheck %s --check-prefix=SEVEN
+// XFAIL: *
+
+#import <Foundation/Foundation.h>
+
+static bool isTaggedPtr(id obj) {
+  uintptr_t ptr = (uintptr_t) obj;
+  return (ptr & 0x8000000000000001ull) != 0;
+}
+
+int main(int argc, char* argv[]) {
+  assert(argc == 2);
+  int arg = atoi(argv[0]);
+
+  @autoreleasepool {
+    NSObject* obj = [NSObject new];
+    NSObject* num1 = @7;
+    NSObject* num2 = [NSNumber numberWithInt:arg];
+
+    assert(!isTaggedPtr(obj));
+    assert(isTaggedPtr(num1) && isTaggedPtr(num2));
+
+    // obj -> num1 (includes num2)
+    @synchronized(obj) {
+      @synchronized(num1) {
+      }
+    }
+
+    // num2 -> obj1
+    @synchronized(num2) {
+      @synchronized(obj) {
+// SEVEN: ThreadSanitizer: lock-order-inversion (potential deadlock)
+      }
+    }
+  }
+
+  NSLog(@"PASS");
+// SIX-NOT: ThreadSanitizer
+// SIX: PASS
+  return 0;
+}

test/tsan/Darwin/objc-synchronize-cycle.mm

+// RUN: %clangxx_tsan %s -o %t -framework Foundation -fobjc-arc %darwin_min_target_with_full_runtime_arc_support
+// RUN:                                   not %run %t 2>&1 | FileCheck %s
+// RUN: %env_tsan_opts=detect_deadlocks=1 not %run %t 2>&1 | FileCheck %s
+// RUN: %env_tsan_opts=detect_deadlocks=0     %run %t 2>&1 | FileCheck %s --check-prefix=DISABLED
+
+#import <Foundation/Foundation.h>
+
+int main() {
+  @autoreleasepool {
+    NSObject* obj1 = [NSObject new];
+    NSObject* obj2 = [NSObject new];
+
+    // obj1 -> obj2
+    @synchronized(obj1) {
+      @synchronized(obj2) {
+      }
+    }
+
+    // obj1 -> obj1
+    @synchronized(obj2) {
+      @synchronized(obj1) {
+// CHECK: ThreadSanitizer: lock-order-inversion (potential deadlock)
+      }
+    }
+  }
+
+  NSLog(@"PASS");
+// DISABLED-NOT: ThreadSanitizer
+// DISABLED: PASS
+  return 0;
+}

test/tsan/Darwin/objc-synchronize-nested-recursive.mm

+// RUN: %clangxx_tsan %s -o %t -framework Foundation -fobjc-arc %darwin_min_target_with_full_runtime_arc_support
+// RUN: %run %t 2>&1 | FileCheck %s
+
+#import <Foundation/Foundation.h>
+
+int main() {
+  @autoreleasepool {
+    NSObject* obj1 = [NSObject new];
+    NSObject* obj2 = [NSObject new];
+
+    @synchronized(obj1) {
+      @synchronized(obj1) {
+        NSLog(@"nested 1-1");
+// CHECK: nested 1-1
+      }
+    }
+
+    @synchronized(obj1) {
+      @synchronized(obj2) {
+        @synchronized(obj1) {
+          @synchronized(obj2) {
+            NSLog(@"nested 1-2-1-2");
+// CHECK: nested 1-2-1-2
+          }
+        }
+      }
+    }
+
+  }
+
+  NSLog(@"PASS");
+// CHECK-NOT: ThreadSanitizer
+// CHECK: PASS
+  return 0;
+}