[tsan] Add dispatch_group API interceptors and synchronization

[kubamracek]

Summary

This patch adds release and acquire semantics for dispatch groups, plus a test case.

[dvyukov]

lib/tsan/rtl/tsan_libdispatch_mac.cc

72 ↗

(On Diff #41315)

Is it compiled without warnings?
You use new_context within its own initialization statement. It is not yet initialized when you read it.

test/tsan/Darwin/gcd-groups-norace.mm

21 ↗

(On Diff #41315)

assign to global after this statement

[kubamracek]

Updating patch.

[kubamracek]

ping

[dvyukov]

LGTM

It seems there is some potential for group API misuse. For example, it is possible to "enter" inside of the async piece of work, instead of before spawning that async piece of work (quite common bug in Go WaitGroup's). Also, since there is ref-counting involved, it can be subject to race use-after-frees (e.g. dispatch_group_notify instead of dispatch_group_async).
Don't know how common are such bugs in real life. But on the other hand we won't know that until we implement detection of these bugs. You can look at https://github.com/golang/go/blob/master/src/sync/waitgroup.go for some ideas (search for "race").

lib/tsan/rtl/tsan_libdispatch_mac.cc

193 ↗

(On Diff #41403)

Why don't we need Block_copy here, but need it below? Is the block automatically referenced from captured by another block?

[kubamracek]

lib/tsan/rtl/tsan_libdispatch_mac.cc

193 ↗

(On Diff #41403)

Technically, we don't need Block_copy for correctness, because the function that will store a reference to a block for later (dispatch_async in this case) will do it anyway. The reason for a Block_copy here is to get rid of a very common false positive, because Block_copy will call malloc and the malloc'd region is then accessed from another thread. Since this happens *inside* dispatch_async, it's already after we've Release()'d. So I'm just forcing the allocation to happen early (before Release()).

I'm not calling Block_copy here, because WRAP(dispatch_async) will call it anyway. Below, we're calling REAL(...), so we need to Block_copy.

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

 typedef long long_t;  // NOLINT
 
 namespace __tsan {
 
 typedef struct {
   dispatch_queue_t queue;
   void *orig_context;
   dispatch_function_t orig_work;
+  uptr object_to_acquire;
 } tsan_block_context_t;
 
 // The offsets of different fields of the dispatch_queue_t structure, exported
 // by libdispatch.dylib.
 extern "C" struct dispatch_queue_offsets_s {
   const uint16_t dqo_version;
   const uint16_t dqo_label;
   const uint16_t dqo_label_size;
static tsan_block_context_t *AllocContext(ThreadState *thr, uptr pc,
                                           dispatch_queue_t queue,
                                           void *orig_context,
                                           dispatch_function_t orig_work) {
   tsan_block_context_t *new_context =
       (tsan_block_context_t *)user_alloc(thr, pc, sizeof(tsan_block_context_t));
   new_context->queue = queue;
   new_context->orig_context = orig_context;
   new_context->orig_work = orig_work;
+  new_context->object_to_acquire = (uptr)new_context;
   return new_context;
 }
 
 static void dispatch_callback_wrap_acquire(void *param) {
   SCOPED_INTERCEPTOR_RAW(dispatch_async_f_callback_wrap);
   tsan_block_context_t *context = (tsan_block_context_t *)param;
-  Acquire(thr, pc, (uptr)context);
+  Acquire(thr, pc, context->object_to_acquire);
   // In serial queues, work items can be executed on different threads, we need
   // to explicitly synchronize on the queue itself.
   if (IsQueueSerial(context->queue)) Acquire(thr, pc, (uptr)context->queue);
   context->orig_work(context->orig_context);
   if (IsQueueSerial(context->queue)) Release(thr, pc, (uptr)context->queue);
   user_free(thr, pc, context);
 }
 

 TSAN_INTERCEPTOR(long_t, dispatch_semaphore_wait, dispatch_semaphore_t dsema,
                  dispatch_time_t timeout) {
   SCOPED_TSAN_INTERCEPTOR(dispatch_semaphore_wait, dsema, timeout);
   long_t result = REAL(dispatch_semaphore_wait)(dsema, timeout);
   if (result == 0) Acquire(thr, pc, (uptr)dsema);
   return result;
 }
 
+TSAN_INTERCEPTOR(long_t, dispatch_group_wait, dispatch_group_t group,
+                 dispatch_time_t timeout) {
+  SCOPED_TSAN_INTERCEPTOR(dispatch_group_wait, group, timeout);
+  long_t result = REAL(dispatch_group_wait)(group, timeout);
+  if (result == 0) Acquire(thr, pc, (uptr)group);
+  return result;
+}
+
+TSAN_INTERCEPTOR(void, dispatch_group_leave, dispatch_group_t group) {
+  SCOPED_TSAN_INTERCEPTOR(dispatch_group_leave, group);
+  Release(thr, pc, (uptr)group);
+  REAL(dispatch_group_leave)(group);
+}
+
+TSAN_INTERCEPTOR(void, dispatch_group_async, dispatch_group_t group,
+                 dispatch_queue_t queue, dispatch_block_t block) {
+  SCOPED_TSAN_INTERCEPTOR(dispatch_group_async, group, queue, block);
+  dispatch_retain(group);
+  dispatch_group_enter(group);
+  WRAP(dispatch_async)(queue, ^(void) {
+    block();
+    WRAP(dispatch_group_leave)(group);
+    dispatch_release(group);
+  });
+}
+
+TSAN_INTERCEPTOR(void, dispatch_group_async_f, dispatch_group_t group,
+                 dispatch_queue_t queue, void *context,
+                 dispatch_function_t work) {
+  SCOPED_TSAN_INTERCEPTOR(dispatch_group_async_f, group, queue, context, work);
+  dispatch_retain(group);
+  dispatch_group_enter(group);
+  WRAP(dispatch_async)(queue, ^(void) {
+    work(context);
+    WRAP(dispatch_group_leave)(group);
+    dispatch_release(group);
+  });
+}
+
+TSAN_INTERCEPTOR(void, dispatch_group_notify, dispatch_group_t group,
+                 dispatch_queue_t q, dispatch_block_t block) {
+  SCOPED_TSAN_INTERCEPTOR(dispatch_group_notify, group, q, block);
+  dispatch_block_t heap_block = Block_copy(block);
+  tsan_block_context_t *new_context =
+      AllocContext(thr, pc, q, heap_block, &invoke_and_release_block);
+  new_context->object_to_acquire = (uptr)group;
+  Release(thr, pc, (uptr)group);
+  REAL(dispatch_group_notify_f)(group, q, new_context,
+                                dispatch_callback_wrap_acquire);
+}
+
+TSAN_INTERCEPTOR(void, dispatch_group_notify_f, dispatch_group_t group,
+                 dispatch_queue_t q, void *context, dispatch_function_t work) {
+  SCOPED_TSAN_INTERCEPTOR(dispatch_group_notify_f, group, q, context, work);
+  tsan_block_context_t *new_context = AllocContext(thr, pc, q, context, work);
+  new_context->object_to_acquire = (uptr)group;
+  Release(thr, pc, (uptr)group);
+  REAL(dispatch_group_notify_f)(group, q, new_context,
+                                dispatch_callback_wrap_acquire);
+}
+
 }  // namespace __tsan
 
 #endif  // SANITIZER_MAC

compiler-rt/trunk/test/tsan/Darwin/gcd-groups-norace.mm

+// RUN: %clang_tsan %s -o %t -framework Foundation
+// RUN: %run %t 2>&1
+
+#import <Foundation/Foundation.h>
+
+#import "../test.h"
+
+long global;
+
+int main() {
+  NSLog(@"Hello world.");
+  NSLog(@"addr=%p\n", &global);
+
+  dispatch_queue_t q = dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0);
+  global = 42;
+
+  dispatch_group_t g = dispatch_group_create();
+  dispatch_group_async(g, q, ^{
+    global = 43;
+  });
+  dispatch_group_wait(g, DISPATCH_TIME_FOREVER);
+
+  global = 44;
+
+  dispatch_group_enter(g);
+  dispatch_async(q, ^{
+    global = 45;
+    dispatch_group_leave(g);
+  });
+  dispatch_group_wait(g, DISPATCH_TIME_FOREVER);
+
+  global = 46;
+
+  dispatch_group_enter(g);
+  dispatch_async(q, ^{
+    global = 47;
+    dispatch_group_leave(g);
+  });
+  dispatch_group_notify(g, q, ^{
+    global = 48;
+
+    dispatch_sync(dispatch_get_main_queue(), ^{
+      CFRunLoopStop(CFRunLoopGetCurrent());
+    });
+  });
+
+  CFRunLoopRun();
+  NSLog(@"Done.");
+}
+
+// CHECK: Hello world.
+// CHECK: Done.
+// CHECK-NOT: WARNING: ThreadSanitizer