[tsan] Add interceptors for objc_sync_enter and objc_sync_exit

[kubamracek]

Summary

Objective-C's @synchronize synchronization primitive uses calls to objc_sync_enter and objc_sync_exit runtime functions. In most cases, they end up just calling pthread_mutex_lock/pthread_mutex_unlock, but there are some cases where the synchronization from pthread_mutex_lock/pthread_mutex_unlock interceptors isn't enough. Let's add explicit interceptors for objc_sync_enter and objc_sync_exit to handle all cases.

[kubamracek]

Adding test.

[george.karpenkov]

Would it be hard to add another test where those interceptors do produce a warning?

[delcypher]

lib/tsan/CMakeLists.txt

121 ↗

(On Diff #141862)

Does this mean that clang_rt.tsan will now always link against libobjc? Is that really necessary. Won't that mean it gets linked in for sanitized C programs that have absolutely no business doing anything with the ObjectiveC runtime?

[kubamracek]

lib/tsan/CMakeLists.txt

121 ↗

(On Diff #141862)

You're right, this always links libobjc, even into program that are not using Objective-C. I don't know how to avoid this dependency, because the dyld interposition requires us to link against the symbols that we intercept.

On the bright side, libobjc gets loaded into every process on macOS anyway. So I believe this is actually safe and shouldn't break anything.

[kubamracek]

In D45487#1063319, @george.karpenkov wrote:

Would it be hard to add another test where those interceptors do produce a warning?

Whaat kind of test are you looking for? As this adds more synchronization edges, this strictly removes reports (false positives in this case). I can add a test that incorrectly uses @synchronized that still triggers a TSan report, if you want.

[george.karpenkov]

this strictly removes reports

Ah sorry, didn't realize that.
I thought it would remove some reports and add some as well (deadlocks? but now I see those are not fully supported)
LGTM in that case.

[delcypher]

lib/tsan/CMakeLists.txt

121 ↗

(On Diff #141862)

Okay. Presumably, TSan will still initialise properly (i.e. the fact that TSan links libobjc won't break initialisation order)?

[kubamracek]

lib/tsan/CMakeLists.txt

121 ↗

(On Diff #141862)

Although libobjc isn't part of libSystem, it's actually loaded by libSystem unconditionally to all processes, and libobjc initializer (_objc_init) is called early in process startup. So explicitly linking against libobjc shouldn't really change any initialization order.

compiler-rt/trunk/lib/tsan/CMakeLists.txt

add_compiler_rt_runtime(clang_rt.tsan
     SOURCES ${TSAN_SOURCES} ${TSAN_CXX_SOURCES} ${TSAN_ASM_SOURCES}
     OBJECT_LIBS RTInterception
                 RTSanitizerCommon
                 RTSanitizerCommonLibc
                 RTSanitizerCommonCoverage
                 RTUbsan
     CFLAGS ${TSAN_RTL_CFLAGS}
     LINK_FLAGS ${SANITIZER_COMMON_LINK_FLAGS} ${WEAK_SYMBOL_LINK_FLAGS}
-    LINK_LIBS ${TSAN_LINK_LIBS}
+    LINK_LIBS ${TSAN_LINK_LIBS} objc
     PARENT_TARGET tsan)
   add_compiler_rt_object_libraries(RTTsan_dynamic
     OS ${TSAN_SUPPORTED_OS}
     ARCHS ${TSAN_SUPPORTED_ARCH}
     SOURCES ${TSAN_SOURCES} ${TSAN_CXX_SOURCES} ${TSAN_ASM_SOURCES}
     CFLAGS ${TSAN_RTL_CFLAGS})
 
   # Build and check Go runtime.

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

 TSAN_INTERCEPTOR(void, xpc_connection_cancel, xpc_connection_t connection) {
   SCOPED_TSAN_INTERCEPTOR(xpc_connection_cancel, connection);
   Release(thr, pc, (uptr)connection);
   REAL(xpc_connection_cancel)(connection);
 }
 
 #endif  // #if defined(__has_include) && __has_include(<xpc/xpc.h>)
 
+TSAN_INTERCEPTOR(int, objc_sync_enter, void *obj) {
+  SCOPED_TSAN_INTERCEPTOR(objc_sync_enter, obj);
+  int result = REAL(objc_sync_enter)(obj);
+  if (obj) Acquire(thr, pc, (uptr)obj);
+  return result;
+}
+
+TSAN_INTERCEPTOR(int, objc_sync_exit, void *obj) {
+  SCOPED_TSAN_INTERCEPTOR(objc_sync_enter, obj);
+  if (obj) Release(thr, pc, (uptr)obj);
+  return REAL(objc_sync_exit)(obj);
+}
+
 // On macOS, libc++ is always linked dynamically, so intercepting works the
 // usual way.
 #define STDCXX_INTERCEPTOR TSAN_INTERCEPTOR
 
 namespace {
 struct fake_shared_weak_count {
   volatile a64 shared_owners;
   volatile a64 shared_weak_owners;

compiler-rt/trunk/test/tsan/Darwin/objc-synchronize.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>
+
+@interface MyClass : NSObject {
+  long field;
+}
+@property (nonatomic, readonly) long value;
+@end
+
+dispatch_group_t group;
+
+@implementation MyClass
+
+- (void) start {
+  dispatch_queue_t q = dispatch_queue_create(NULL, NULL);
+  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+    for (int i = 0; i < 1000; i++) {
+      dispatch_async(q, ^{
+        @synchronized(self) {
+          self->field = i;
+        }
+      });
+    }
+  });
+}
+
+- (long) value {
+  @synchronized(self) {
+    return self->field;
+  }
+}
+
+- (void)dealloc {
+  dispatch_group_leave(group);
+}
+
+@end
+
+int main() {
+  group = dispatch_group_create();
+  @autoreleasepool {
+    for (int j = 0; j < 100; ++j) {
+      dispatch_group_enter(group);
+      MyClass *obj = [[MyClass alloc] init];
+      [obj start];
+      long x = obj.value;
+      (void)x;
+    }
+  }
+  dispatch_group_wait(group, DISPATCH_TIME_FOREVER);
+  NSLog(@"Hello world");
+}
+
+// CHECK: Hello world
+// CHECK-NOT: WARNING: ThreadSanitizer