[PATCH] D28264: [tsan] Implement a 'ignore_noninstrumented_modules' flag to better suppress false positive races

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added a comment.

> 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;
      }
    }
  }



================
Comment at: lib/sanitizer_common/sanitizer_libignore.h:79
   Lib libs_[kMaxLibs];
+  bool track_instrumented_libs;
 
----------------
underscore at the name end:
  track_instrumented_libs_


https://reviews.llvm.org/D28264