[PATCH] D14745: [tsan] Implement basic GCD interceptors for OS X

[Dmitry Vyukov via llvm-commits]

dvyukov added inline comments.

================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:65
@@ +64,3 @@
+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;
----------------
You've added libdispatch to suppressions, and caller_pc inside of SCOPED_INTERCEPTOR_RAW should point into libdispatch, and you call user functions in the scope of SCOPED_INTERCEPTOR_RAW. This means that ScopedInterceptor constructor will disable memory accesses inside of user callback as well and we don't get any race detection.
Does it work at all? Please add a test with a data race inside of the callback.

================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:75
@@ +74,3 @@
+  tsan_block_context_indexed_t *context = (tsan_block_context_indexed_t *)param;
+  Acquire(thr, pc, (uptr)context->queue);
+  context->orig_work(context->orig_context, index);
----------------
It would be good to synchronize on an address associated with the work item submitted, rather than on the queue address. Synchronization on queue address will introduce lots of parasitic synchronization. Think of a widely used concurrent queue.

I guess we can't use the context for synchronization, because internal allocator memory does not have meta shadow mapped. Don't know whether a code block address can be used for that purpose (and whether it can be casted to uptr at all or not). If block address can't be used, then it worth it to allocate a context with malloc and use it address for synchronization.

================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:81
@@ +80,3 @@
+  Release(thr, pc, (uptr)queue);                             \
+  REAL(func)(__VA_ARGS__, ^{                                 \
+    SCOPED_INTERCEPTOR_RAW(func);                            \
----------------
Are closures for these code blocks dynamically allocated? With malloc?

================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:86
@@ +85,3 @@
+  })
+#define RELEASE_ACQUIRE_QUEUE_BLOCK_INDEX(func, queue, block, ...) \
+  Release(thr, pc, (uptr)queue);                                   \
----------------
empty line between macros, please


================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:88
@@ +87,3 @@
+  Release(thr, pc, (uptr)queue);                                   \
+  REAL(func)(__VA_ARGS__, ^(size_t index) {                        \
+    SCOPED_INTERCEPTOR_RAW(func);                                  \
----------------
Why here you use a code block, but below you use an explicit context and function wrapper?

================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:153
@@ +152,3 @@
+  SCOPED_TSAN_INTERCEPTOR(dispatch_apply_f, iterations, queue, context, work);
+  RELEASE_ACQUIRE_QUEUE_FUNCTION_INDEX(dispatch_apply_f, queue, context, work,
+                                       iterations, queue);
----------------
Is RELEASE_ACQUIRE_QUEUE_FUNCTION_INDEX intended to be used more? Currently it is the only call site. Which makes introduction of the helper macro questionable.


http://reviews.llvm.org/D14745