[tsan] Avoid calling Block_copy in the "sync" GCD interceptors

[kubamracek]

Summary

The synchronous dispatch functions in GCD (dispatch_sync, dispatch_barrier_sync), don't make a copy of the passed block. To maintain binary compatibility, we should avoid doing that as well in TSan, as there's no reason to do that. The synchronous dispatch functions will not return before the block is actually executed.

rdar://problem/42242579

[delcypher]

Seems fine but with some minor nits.

test/tsan/Darwin/gcd-sync-block-copy.mm

6 ↗

(On Diff #161315)

For the purposes of examining test artifacts after tests have run, it's probably better to give your executables different names (e.g. %t_no_tsan and %t_with_tsan).

16 ↗

(On Diff #161315)

I presume you're relying on the copy of the block to increment the reference count to your object? Perhaps add a comment to this effect?

I presume it's guaranteed that the Objective-C runtime won't try to do any tagged pointer optimizations here which would break the assumption that a copy increases the reference count?

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

TSAN_INTERCEPTOR(void, name, dispatch_queue_t q, dispatch_block_t block) { \
     REAL(name##_f)(q, new_context, dispatch_callback_wrap);                  \
     SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();                             \
   }
 
 #define DISPATCH_INTERCEPT_SYNC_B(name, barrier)                             \
   TSAN_INTERCEPTOR(void, name, dispatch_queue_t q,                           \
                    DISPATCH_NOESCAPE dispatch_block_t block) {               \
     SCOPED_TSAN_INTERCEPTOR(name, q, block);                                 \
-    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();                           \
-    dispatch_block_t heap_block = Block_copy(block);                         \
-    SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();                             \
     tsan_block_context_t new_context = {                                     \
-        q, heap_block, &invoke_and_release_block, false, true, barrier, 0};  \
+        q, block, &invoke_and_release_block, false, true, barrier, 0};       \
     Release(thr, pc, (uptr)&new_context);                                    \
     SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();                           \
     REAL(name##_f)(q, &new_context, dispatch_callback_wrap);                 \
     SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();                             \
     Acquire(thr, pc, (uptr)&new_context);                                    \
   }
 
 #define DISPATCH_INTERCEPT_F(name, barrier)                       \

compiler-rt/trunk/test/tsan/Darwin/gcd-sync-block-copy.mm

+// This test verifies that dispatch_sync() doesn't actually copy the block under TSan (without TSan, it doesn't).
+
+// RUN: %clang_tsan -fno-sanitize=thread %s -o %t_no_tsan -framework Foundation
+// RUN: %run %t_no_tsan 2>&1 | FileCheck %s
+
+// RUN: %clang_tsan %s -o %t_with_tsan -framework Foundation
+// RUN: %run %t_with_tsan 2>&1 | FileCheck %s
+
+#import <Foundation/Foundation.h>
+
+@interface MyClass : NSObject
+@end
+
+@implementation MyClass
+- (instancetype)retain {
+  // Copying the dispatch_sync'd block below will increment the retain count of
+  // this object. Abort if that happens.
+  abort();
+}
+@end
+
+int main(int argc, const char* argv[]) {
+  dispatch_queue_t q = dispatch_queue_create("my.queue", NULL);
+  id object = [[MyClass alloc] init];
+  dispatch_sync(q, ^{
+    NSLog(@"%@", object);
+  });
+  [object release];
+  NSLog(@"Done.");
+  return 0;
+}
+
+// CHECK: Done.
+// CHECK-NOT: WARNING: ThreadSanitizer