[PATCH] D21889: [tsan] Add support for GCD IO channels on Darwin

[Kuba Brecka via llvm-commits]

kubabrecka added inline comments.

================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:469
@@ +468,3 @@
+    }
+    DISPATCH_CALLBACK_WITH_BLOCK(q, h(data, error));
+  };
----------------
dvyukov wrote:
> dispatch_callback_wrap will Acquire on context address, which is at best wasteful.
> What do we want to synchronize with what? Why do we acquire/release on the queue address? Do we want to synchronize all reads and writes? If we want to synchronize submission with callback execution, then existing dispatch_callback_wrap machinery will do what we need in more precise way (using context address).
Yes, I want to synchronize submission with callback execution, plus the usual “within-one-queue” rules.  I wanted to avoid malloc’ing the context, but I guess it’s okay here.

================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:528
@@ +527,3 @@
+  };
+  Release(thr, pc, (uptr)channel);
+  REAL(dispatch_io_barrier)(channel, new_block);
----------------
dvyukov wrote:
> Documentation says:
> 
> The barrier operation applies to the channel’s file descriptor and not to a specific channel. In other words, if multiple channels are associated with the same file descriptor, a barrier operation scheduled on any of the channels acts as a barrier across all of the channels. All previously scheduled operations on any of those channels must complete before the barrier block is executed.
> 
> But here you synchronize on channel address. So I guess we will have some false positives.
> 
> Do you release on channel in read/write to synchronize with barriers? If so, it seems to be exactly the kind of reader/writer synchronization we added for barriers recently. Can we reuse that machinery? It captures precisely who needs to synchronize with whom.
The io_barrier is a bit more tricky, since it’s not submitted to the same queue as the read&write callbacks, so user’s shouldn’t be using this to synchronize with their code.  So I basically only care about synchronizing submission and execution.

================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:543
@@ +542,3 @@
+  };
+  Release(thr, pc, (uptr)q);
+  new_channel = REAL(dispatch_io_create)(type, fd, q, new_h);
----------------
dvyukov wrote:
> When is the cleanup handler executed? The documentation is quite vague on this. Is it executed when the channel is closed? If so, should we use a more specific address to not interfere with all other operations (e.g. chan+8)?
The cleanup handler is *submitted* (executed asynchronously) when the channel is closed explicitly (dispatch_io_close) or implicitly by releasing all references to the channel (dispatch_release or removing the reference in ARC).  The second case isn’t handled by this patch, because I didn’t want to get into the complexity of intercepting dispatch_release.

================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:587
@@ +586,3 @@
+  SCOPED_TSAN_INTERCEPTOR(dispatch_io_close, channel, flags);
+  Release(thr, pc, (uptr)channel);
+  return REAL(dispatch_io_close)(channel, flags);
----------------
dvyukov wrote:
> Where is the pairing acquire?
The Acquire is in the cleanup handler in dispatch_io_create_*.


http://reviews.llvm.org/D21889