This is an archive of the discontinued LLVM Phabricator instance.

[tsan] Implement a 'ignore_noninstrumented_modules' flag to better suppress false positive races
ClosedPublic

Authored by kubamracek on Jan 3 2017, 4:34 PM.

Details

Summary

On Darwin, we currently use 'ignore_interceptors_accesses', which is a heavy-weight solution that simply turns of race detection in all interceptors. This was done to suppress false positives coming from system libraries (non-instrumented code), but it also silences a lot of real races. This patch implements an alternative approach that should allow us to enable interceptors and report races coming from them, but only if they are called directly from instrumented code.

The patch requires https://reviews.llvm.org/D28263 and tracks instrumented code ranges, then simply matches the caller PC in each interceptors. For non-instrumented code, we call ThreadIgnoreBegin.

The assumption here is that the number of instrumented modules is low. Most likely there's only one (the instrumented main executable) and all the other modules are system libraries (non-instrumented).

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 82973.Jan 3 2017, 4:34 PM
kubamracek retitled this revision from to [tsan] Implement a 'ignore_noninstrumented_modules' flag to better suppress false positive races.
kubamracek updated this object.
kubamracek added reviewers: dvyukov, zaks.anna, kcc.
kubamracek set the repository for this revision to rL LLVM.
kubamracek added a project: Restricted Project.
kubamracek added a subscriber: llvm-commits.
dvyukov added inline comments.Jan 5 2017, 2:01 AM
lib/tsan/rtl/tsan_interceptors.cc
272

These changes to ScopedInterceptor does not feel right to me.
We already have checks of libignore()->IsIgnored() and undo of ignoring in all the same places you added new code. So it is better to piggy back on the existing logic to avoid duplication. We can either (1) pass the new flag to IsIgnored:

if (!thr_->in_ignored_lib && libignore()->IsIgnored(pc, flags()->ignore_noninstrumented_modules))

then it will also check the pc against instrumented modules; or (2) collect instrumented modules in LibIgnore only if the flag is set, then IsIgnored will check:

if pc is in ignored libraries { return true }
if instrumented libraries are not empty and pc is not in instrumented libraries { return true }
return false

Or (3) if the flag is set add non-instrumented libraries to ignored libraries.
As far as I understand you don't want to do (3) because there can be lots of non-instrumented libraries.

kubamracek added inline comments.Jan 6 2017, 5:58 PM
lib/tsan/rtl/tsan_interceptors.cc
272

I'd go with (2), but the current behavior of thr->in_ignored_lib being set is that *all* interceptors called when we're within an ignored module are ignored:

#define SCOPED_TSAN_INTERCEPTOR(func, ...) \
... \
if (!thr->is_inited || thr->ignore_interceptors || thr->in_ignored_lib) \
  return REAL(func)(__VA_ARGS__); \

This also means we're missing a lot of synchronization (mutexes, semaphores, etc.). Is that behavior of thr->in_ignored_lib intentional?

kubamracek updated this revision to Diff 83479.Jan 6 2017, 6:00 PM
kubamracek removed rL LLVM as the repository for this revision.

Updating the patch to use solution (2), but note that I had to remove || thr->in_ignored_lib from SCOPED_TSAN_INTERCEPTOR, which is probably wrong...

dvyukov edited edge metadata.Jan 7 2017, 2:26 AM

I'd go with (2), but the current behavior of thr->in_ignored_lib being set is that *all* interceptors called when we're within an ignored module are ignored:

Oh, I missed that difference between current ignores and ignores that you need.

This also means we're missing a lot of synchronization (mutexes, semaphores, etc.). Is that behavior of thr->in_ignored_lib intentional?

Yes, this is intentional. We use it to ignore Java runtime modules, they have internal synchronization that is not user-visible.

Updating the patch to use solution (2), but note that I had to remove || thr->in_ignored_lib from SCOPED_TSAN_INTERCEPTOR, which is probably wrong...

Yes, this is wrong.

There is too much duplication between ctor/dtor/UserCallbackStart/UserCallbackEnd. What about something along the following lines? It is semantically equivalent to your old code and does not change semantics of current ignores (hopefully). It requires LibIgnore::IsIgnored to also return a flag if PC is ignores or not-instrumented.

ScopedInterceptor::ScopedInterceptor(...) {
  ...
  ignoring_ = !thr_->in_ignored_lib && (flags()->ignore_interceptors_accesses || libignore()->IsIgnored(pc, &in_ignored_lib_));
  UserCallbackEnd();
}

ScopedInterceptor::~ScopedInterceptor() {
  ...
  UserCallbackStart();
  ...
}

void ScopedInterceptor::UserCallbackStart() {
  if (ignoring_)
    ThreadIgnoreEnd(thr_, pc_);
  if (in_ignored_lib_)
    thr_->in_ignored_lib = false;
}

void ScopedInterceptor::UserCallbackEnd() {
  if (in_ignored_lib_)
    thr_->in_ignored_lib = true;
  if (ignoring_)
    ThreadIgnoreBegin(thr_, pc_);
}

Or, for better performance/checking:

void ScopedInterceptor::UserCallbackStart() {
  if (ignoring_) {
    ThreadIgnoreEnd(thr_, pc_);
    if (in_ignored_lib_) {
      DCHECK(thr_->in_ignored_lib);
      thr_->in_ignored_lib = false;
    }
  }
}

void ScopedInterceptor::UserCallbackEnd() {
  if (ignoring_) {
    ThreadIgnoreBegin(thr_, pc_);
    if (in_ignored_lib_) {
      DCHECK(!thr_->in_ignored_lib);
      thr_->in_ignored_lib = true;
    }
  }
}
lib/sanitizer_common/sanitizer_libignore.h
79

underscore at the name end:

track_instrumented_libs_
kubamracek updated this revision to Diff 83573.Jan 8 2017, 2:27 PM
kubamracek edited edge metadata.

Updating patch as suggested. Also renamig UserCallbackStart()/UserCallbackEnd() to EnableIgnores()/DisableIgnores(), which better describes the purpose of the functions (calling UserCallbackStart from ScopedInterceptor's constructor sounds weird).

kubamracek updated this revision to Diff 83575.Jan 8 2017, 2:30 PM
dvyukov accepted this revision.Jan 10 2017, 6:25 AM
dvyukov edited edge metadata.

Thanks!

This revision is now accepted and ready to land.Jan 10 2017, 6:25 AM
This revision was automatically updated to reflect the committed changes.