Page MenuHomePhabricator

[tsan] Avoid false positives with GCD data callbacks
ClosedPublic

Authored by kubamracek on Jul 5 2016, 4:56 AM.

Details

Summary

This patch adds synchronization between the creation of the GCD data object and destructor’s execution. It’s far from perfect, because ideally we’d want to synchronize the destruction of the last reference (via dispatch_release) and the destructor’s execution, but intercepting objc_release is problematic.

Diff Detail

Repository
rL LLVM

Event Timeline

kubamracek updated this revision to Diff 62736.Jul 5 2016, 4:56 AM
kubamracek retitled this revision from to [tsan] Avoid false positives with GCD data callbacks.
kubamracek updated this object.
kubamracek added reviewers: dvyukov, kcc, glider.
kubamracek added a project: Restricted Project.
kubamracek added a subscriber: zaks.anna.
dvyukov edited edge metadata.Jul 5 2016, 6:19 AM

It’s far from perfect, because ideally we’d want to synchronize the destruction of the last reference (via dispatch_release) and the destructor’s execution, but intercepting objc_release is problematic.

Why don't we intercept dispatch_release?

dvyukov added inline comments.Jul 5 2016, 6:20 AM
lib/tsan/rtl/tsan_libdispatch_mac.cc
460 ↗(On Diff #62736)

Will it work with DISPATCH_DATA_DESTRUCTOR_FREE/DEFAULT?
DISPATCH_DATA_DESTRUCTOR_FREE seems to be initialized to some block, but at the same time runtime avoids calling it directly and instead calls free directly. Though, maybe it's just for performance reasons.

It’s far from perfect, because ideally we’d want to synchronize the destruction of the last reference (via dispatch_release) and the destructor’s execution, but intercepting objc_release is problematic.

Why don't we intercept dispatch_release?

  1. We’d need to intercept both dispatch_release and objc_release, in some cases they can be used interchangeably (toll-free bridging), which means we should add dispatch_retain and objc_retain as well. Theses functions are called *a lot* and I’m worried about performance, but more importantly about all the added synchronization on refcounts, which is going to cause false negatives.
  2. We cannot tell if the release is going to destroy the object or not. We can’t take a look at the refcount, because that’s an internal implementation detail and it actually changes.
  3. This will still not cover all the false positives with GCD objects, because there are shortcuts in GCD internals that release/destroy objects without going through dispatch_release.

Still, I’m probably going to try to add those interceptors at some point, but it sounds like quite a non-trivial task to do it right.

kubamracek updated this revision to Diff 62867.Jul 6 2016, 7:54 AM
kubamracek edited edge metadata.

Updating the patch to handle DISPATCH_DATA_DESTRUCTOR_FREE and other special values. The original patch actually crashed when these were used (the block that this magic value is pointing to isn’t supposed to be called at all). Thanks!

Also checking when queue is NULL, it seems there is some code that is using it (although the docs isn’t clear what the behavior is then).

dvyukov added inline comments.Jul 7 2016, 4:18 AM
lib/tsan/rtl/tsan_libdispatch_mac.cc
484 ↗(On Diff #62867)

Won't this cause false positives?
Tsan should intercept free in DISPATCH_DATA_DESTRUCTOR_FREE. If we don't synchronize dispatch_data_create with the free, free can race with allocation.

kubamracek added inline comments.Jul 7 2016, 4:23 AM
lib/tsan/rtl/tsan_libdispatch_mac.cc
484 ↗(On Diff #62867)

I guess you’re right, but we’re currently still using ignore_interceptors_accesses=1 on Darwin to avoid false positives from system libraries, which also suppresses races with free.

Anyway, what would be the solution here? Release() on buffer?

dvyukov added inline comments.Jul 7 2016, 4:27 AM
lib/tsan/rtl/tsan_libdispatch_mac.cc
484 ↗(On Diff #62867)

Anyway, what would be the solution here? Release() on buffer?

If destructor == DISPATCH_DATA_DESTRUCTOR_FREE, replace it with a block that calls free?

kubamracek updated this revision to Diff 63052.Jul 7 2016, 5:10 AM

Updating patch.

dvyukov accepted this revision.Jul 7 2016, 5:40 AM
dvyukov edited edge metadata.
This revision is now accepted and ready to land.Jul 7 2016, 5:40 AM
This revision was automatically updated to reflect the committed changes.