[TSan][libdispatch] Add interceptors for dispatch_async_and_wait()

[yln]

Summary

Add interceptors for dispatch_async_and_wait[_f]() which was added in
macOS 10.14. This pair of functions is similar to dispatch_sync(),
but does not force a context switch of the queue onto the caller thread
when the queue is active (and hence is more efficient). For TSan, we
can apply the same semantics as for dispatch_sync().

From the header docs:

Differences with dispatch_sync()

When the runtime has brought up a thread to invoke the asynchronous
workitems already submitted to the specified queue, that servicing
thread will also be used to execute synchronous work submitted to the
queue with dispatch_async_and_wait().

However, if the runtime has not brought up a thread to service the
specified queue (because it has no workitems enqueued, or only
synchronous workitems), then dispatch_async_and_wait() will invoke the
workitem on the calling thread, similar to the behaviour of functions
in the dispatch_sync family.

Additional context:

The guidance is to use dispatch_async_and_wait() instead of
dispatch_sync() when it is necessary to mix async and sync calls on
the same queue. dispatch_async_and_wait() does not guarantee
execution on the caller thread which allows to reduce context switches
when the target queue is active.
https://gist.github.com/tclementdev/6af616354912b0347cdf6db159c37057

rdar://35757961

[kubamracek]

Does the testcase actually fail without the code change?

[delcypher]

compiler-rt/test/tsan/libdispatch/async_and_wait.c

2

Don't we need something on this test to ensure it only runs on 10.14 and newer?

I.e. using something like --> https://reviews.llvm.org/D70152

[yln]

In D85854#2216455, @kubamracek wrote:

Does the testcase actually fail without the code change?

Yes!

[yln]

compiler-rt/test/tsan/libdispatch/async_and_wait.c

2

That is correct! I would like to deal with this in a follow-up.

[kubamracek]

LGTM. Maybe add an if to check if the 10.14-only API is available and just do nothing in the test otherwise?

[yln]

In D85854#2217386, @kubamracek wrote:

LGTM. Maybe add an if to check if the 10.14-only API is available and just do nothing in the test otherwise?

If we decide that this is worth doing, then I think we should go with what Dan suggested: have a feature for this in the lit test infrastructure.

[dmajor]

Our automation builds are broken after this change, with undefined symbols for _dispatch_async_and_wait{,_f}. We build using the 10.12 SDK with a deployment target of 10.11. Should this code have a version check?

[yln]

In D85854#2221529, @dmajor wrote:

Our automation builds are broken after this change, with undefined symbols for _dispatch_async_and_wait{,_f}. We build using the 10.12 SDK with a deployment target of 10.11. Should this code have a version check?

I have identified the issue (the reference to _dispatch_async_and_wait_f is not a weak import when building with SDKs <= 10.14) and working on a fix (need to forward declare the API with appropriate availability).

[aeubanks]

This has been broken for over a day and the fix is taking a while, reverting.

compiler-rt/lib/tsan/rtl/tsan_interceptors_libdispatch.cpp

 
 // We wrap dispatch_async, dispatch_sync and friends where we allocate a new
 // context, which is used to synchronize (we release the context before
 // submitting, and the callback acquires it before executing the original
 // callback).
 DISPATCH_INTERCEPT(dispatch, false)
 DISPATCH_INTERCEPT(dispatch_barrier, true)
 
+DISPATCH_INTERCEPT_SYNC_F(dispatch_async_and_wait_f, false)
+DISPATCH_INTERCEPT_SYNC_B(dispatch_async_and_wait, false)
+
 DECLARE_REAL(void, dispatch_after_f, dispatch_time_t when,
              dispatch_queue_t queue, void *context, dispatch_function_t work)
 
 TSAN_INTERCEPTOR(void, dispatch_after, dispatch_time_t when,
                  dispatch_queue_t queue, dispatch_block_t block) {
   SCOPED_TSAN_INTERCEPTOR(dispatch_after, when, queue, block);
   SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();
   dispatch_block_t heap_block = Block_copy(block);
void InitializeLibdispatchInterceptors() {
   INTERCEPT_FUNCTION(dispatch_async);
   INTERCEPT_FUNCTION(dispatch_async_f);
   INTERCEPT_FUNCTION(dispatch_sync);
   INTERCEPT_FUNCTION(dispatch_sync_f);
   INTERCEPT_FUNCTION(dispatch_barrier_async);
   INTERCEPT_FUNCTION(dispatch_barrier_async_f);
   INTERCEPT_FUNCTION(dispatch_barrier_sync);
   INTERCEPT_FUNCTION(dispatch_barrier_sync_f);
+  INTERCEPT_FUNCTION(dispatch_async_and_wait);
+  INTERCEPT_FUNCTION(dispatch_async_and_wait_f);
   INTERCEPT_FUNCTION(dispatch_after);
   INTERCEPT_FUNCTION(dispatch_after_f);
   INTERCEPT_FUNCTION(dispatch_once);
   INTERCEPT_FUNCTION(dispatch_once_f);
   INTERCEPT_FUNCTION(dispatch_semaphore_signal);
   INTERCEPT_FUNCTION(dispatch_semaphore_wait);
   INTERCEPT_FUNCTION(dispatch_group_wait);
   INTERCEPT_FUNCTION(dispatch_group_leave);

compiler-rt/test/tsan/libdispatch/async_and_wait.c

+// RUN: %clang_tsan %s -o %t
+// RUN: %run %t 2>&1 | FileCheck %s --implicit-check-not='ThreadSanitizer'
+
+#include "dispatch/dispatch.h"
+
+#include <stdio.h>
+
+long global;
+
+int main() {
+  dispatch_queue_t q = dispatch_queue_create("my.queue", DISPATCH_QUEUE_SERIAL);
+  dispatch_semaphore_t s = dispatch_semaphore_create(0);
+
+  // Force queue to context switch onto separate thread.
+  dispatch_async(q, ^{
+    dispatch_semaphore_wait(s, DISPATCH_TIME_FOREVER);
+  });
+  dispatch_semaphore_signal(s);
+
+  global++;
+  dispatch_async_and_wait(q, ^{
+    // The queue continues to execute on separate thread.  This would cause a
+    // race if we had used `dispatch_async()` without the `_and_wait` part.
+    global++;
+  });
+  global++;
+
+  fprintf(stderr, "Done.\n");
+}
+
+// CHECK: Done.