This is an archive of the discontinued LLVM Phabricator instance.

[tsan] Support C++11 call_once in TSan on Darwin
ClosedPublic

Authored by kubamracek on Sep 2 2016, 6:22 AM.

Details

Summary

This patch adds a wrapper for call_once, which uses an already-compiled helper __call_once with an atomic release which is invisible to TSan. To avoid false positives, the interceptor performs an explicit atomic release in the callback wrapper.

The test passes only after a real race in libc++ is fixed, patch at https://reviews.llvm.org/D24028.

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 70151.Sep 2 2016, 6:22 AM
kubamracek retitled this revision from to [tsan] Support C++11 call_once in TSan on Darwin.
kubamracek updated this object.
kubamracek added a reviewer: dvyukov.
kubamracek added a project: Restricted Project.
kubamracek added a subscriber: zaks.anna.
dvyukov added inline comments.Sep 2 2016, 6:59 AM
lib/tsan/rtl/tsan_interceptors_mac.cc
355

Do we need an acquire here?
Consider that two threads fall onto this slow path. One calls the user function and does release. The other thread waits on some internal synchronization and returns. What's missing is release->acquire between the first thread and the second thread.

kubamracek added inline comments.Sep 2 2016, 7:08 AM
lib/tsan/rtl/tsan_interceptors_mac.cc
355

I don’t think it’s necessary, as __call_once uses a pthread condvar to suspend the other thread, which is TSan-visible and it already provides the release->acquire from thread A, after it has already run user code, to the waiting thread B.

dvyukov edited edge metadata.Sep 2 2016, 7:12 AM

asd

lib/tsan/rtl/tsan_interceptors_mac.cc
355

You mean pthread_mutex, right? Condition variables don't provide any synchronization.

kubamracek added inline comments.Sep 2 2016, 7:18 AM
lib/tsan/rtl/tsan_interceptors_mac.cc
355

I mean the thread waits by this:

pthread_mutex_lock(&mut);
while (flag == 1) pthread_condvar_wait(&cv, &mut);
pthread_mutex_unlock(&mut);

Doesn’t pthread_condvar_wait perform an “acquire” of the mutex before it returns?

dvyukov accepted this revision.Sep 2 2016, 8:22 AM
dvyukov edited edge metadata.
dvyukov added inline comments.
lib/tsan/rtl/tsan_interceptors_mac.cc
355

It locks the mutex (maybe), but it's the mutex that does acquire.
OK, nevermind.

This revision is now accepted and ready to land.Sep 2 2016, 8:22 AM
This revision was automatically updated to reflect the committed changes.
zaks.anna added inline comments.Oct 10 2016, 5:36 PM
compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors_mac.cc
352 ↗(On Diff #70672)

This does not have a call to SCOPED_TSAN_INTERCEPTOR. Does it need it?

dvyukov added inline comments.Oct 10 2016, 11:10 PM
compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors_mac.cc
352 ↗(On Diff #70672)

SCOPED_TSAN_INTERCEPTOR allows to skip/ignore an interceptor and process pending signals. It can also do logging in debug mode. All of that is not strictly necessary, but better to have here as well for consistency.

kubamracek added inline comments.Oct 11 2016, 6:37 AM
compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors_mac.cc
352 ↗(On Diff #70672)

Also, adding SCOPED_TSAN_INTERCEPTOR disables tail-call optimization and enabling ignores here could cause false positives. These were the reasons why I left it out.