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

[Dmitry Vyukov via llvm-commits]

dvyukov added inline comments.

================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:461
@@ +460,3 @@
+
+TSAN_INTERCEPTOR(void, dispatch_read, dispatch_fd_t fd, size_t length,
+                 dispatch_queue_t q, fd_handler_t h) {
----------------
Please outline in a comment here what operations we want to synchronize with what.
It's difficult to restore the intention from there acquires and released on queues and channels scattered across all these functions.


================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:469
@@ +468,3 @@
+    }
+    DISPATCH_CALLBACK_WITH_BLOCK(q, h(data, error));
+  };
----------------
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).

================
Comment at: lib/tsan/rtl/tsan_libdispatch_mac.cc:497
@@ +496,3 @@
+    }
+    DISPATCH_CALLBACK_WITH_BLOCK(q, h(done, data, error));
+  };
----------------
Same 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);
----------------
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.

================
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);
----------------
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)?

================
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);
----------------
Where is the pairing acquire?


http://reviews.llvm.org/D21889