This is an archive of the discontinued LLVM Phabricator instance.

[tsan] Add HB edges for GCD barrier blocks
ClosedPublic

Authored by kubamracek on Jun 22 2016, 3:28 AM.

Details

Summary

We currently have false positives when using GCD barrier blocks with concurrent queues. The semantics of a submitted barrier block are:

  1. All previously submitted blocks are completely finished before the barrier block starts executing.
  2. The barrier block executed exclusively in the queue (as if the queue was serial for the duration of the block).
  3. All subsequently submitted blocks are only executed after the barrier block has completely finished.

TSan needs two more HB edges to cover these semantics: One edge from previous (regular) blocks to the barrier block and one edge from the barrier block to all subsequent (regular) blocks. Only applies to concurrent queues.

Diff Detail

Repository
rL LLVM

Event Timeline

kubamracek updated this revision to Diff 61526.Jun 22 2016, 3:28 AM
kubamracek retitled this revision from to [tsan] Add HB edges for GCD barrier blocks.
kubamracek updated this object.
kubamracek added reviewers: dvyukov, kcc, glider.
kubamracek added a project: Restricted Project.
dvyukov added inline comments.Jun 22 2016, 5:36 AM
lib/tsan/rtl/tsan_libdispatch_mac.cc
127 ↗(On Diff #61526)

Shouldn't it be if (!queue_serial && !context->is_barrier_block). Because serial blocks and barriers blocks are already synchronized by other means.

It is very difficult to understand serial/concurrent/barrier/non-barrier synchronization now.
First observation is that serial queues are equivalent to concurrent queue with all blocks being barriers.
Second observation is that barrier/non-barrier blocks are equivalent to reader-writer mutexes with barriers being writers and non-barriers being readers.
Reader-writer synchronization is handled with 2 sync objects as:

uptr serial_sync = ((uptr)context->queue);
uptr concurrent_sync = ((uptr)context->queue) + sizeof(uptr);
bool serial_task = IsQueueSerial(context->queue) || context->is_barrier_block;

Acquire(thr, pc, serial_sync);
if (serial_task)
  Acquire(thr, pc, concurrent_sync);

SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START();
context->orig_work(context->orig_context);
SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END();

Release(thr, pc, serial_task ? serial_sync : concurrent_sync);

This looks simpler than your new code. Please use it.

kubamracek updated this revision to Diff 61908.Jun 26 2016, 5:40 AM

Updating patch using Dmitry’s suggestion for simplifying serial and concurrent sync objects. I’m also simplifying some other stuff: Removing sync_object and object_to_release, because they were only used for dispatch groups, but they can now use is_barrier_block instead. Renaming release_sync_object_in_callback to submitted_synchronously.

kubamracek updated this revision to Diff 61909.Jun 26 2016, 5:47 AM

One more typo fix in the dispatch groups interceptors.

dvyukov accepted this revision.Jun 27 2016, 4:19 AM
dvyukov edited edge metadata.
This revision is now accepted and ready to land.Jun 27 2016, 4:19 AM

I've submitted:
http://llvm.org/viewvc/llvm-project?view=revision&revision=273862
It should make these annotations more efficient for some cases (serial queues and concurrent queues with only concurrent tasks).

This revision was automatically updated to reflect the committed changes.