Page MenuHomePhabricator

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

Authored by yln on Aug 12 2020, 1:46 PM.

Details

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

Diff Detail

Event Timeline

yln created this revision.Aug 12 2020, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2020, 1:46 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
yln requested review of this revision.Aug 12 2020, 1:46 PM

Does the testcase actually fail without the code change?

delcypher added inline comments.Aug 13 2020, 12:20 PM
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 added a comment.Aug 13 2020, 12:29 PM

Does the testcase actually fail without the code change?

Yes!

yln marked an inline comment as done.Aug 13 2020, 12:41 PM
yln added inline comments.
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 accepted this revision.Aug 13 2020, 7:57 PM

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

This revision is now accepted and ready to land.Aug 13 2020, 7:57 PM
yln marked an inline comment as done.Aug 14 2020, 9:01 AM

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.

This revision was landed with ongoing or failed builds.Aug 14 2020, 9:40 AM
This revision was automatically updated to reflect the committed changes.
dmajor added a subscriber: dmajor.Aug 17 2020, 8:23 AM

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 added a comment.EditedAug 17 2020, 9:48 AM

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).

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