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

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 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).

Differential Revision: https://reviews.llvm.org/D28264



git-svn-id: https://llvm.org/svn/llvm-project/compiler-rt/trunk@291631 91177308-0d34-0410-b5e6-96231b3b80d8

6 files changed, 153 insertions, 45 deletions

diff --git a/lib/sanitizer_common/sanitizer_libignore.cc b/lib/sanitizer_common/sanitizer_libignore.cc
index 33a1763d2..aa4fa88ef 100644
--- a/lib/sanitizer_common/sanitizer_libignore.cc
+++ b/lib/sanitizer_common/sanitizer_libignore.cc
@@ -78,10 +78,12 @@ void LibIgnore::OnLibraryLoaded(const char *name) {
                 lib->templ, mod.full_name());
         lib->loaded = true;
         lib->name = internal_strdup(mod.full_name());
-        const uptr idx = atomic_load(&loaded_count_, memory_order_relaxed);
-        code_ranges_[idx].begin = range.beg;
-        code_ranges_[idx].end = range.end;
-        atomic_store(&loaded_count_, idx + 1, memory_order_release);
+        const uptr idx =
+            atomic_load(&ignored_ranges_count_, memory_order_relaxed);
+        CHECK_LT(idx, kMaxLibs);
+        ignored_code_ranges_[idx].begin = range.beg;
+        ignored_code_ranges_[idx].end = range.end;
+        atomic_store(&ignored_ranges_count_, idx + 1, memory_order_release);
         break;
       }
     }
@@ -92,6 +94,29 @@ void LibIgnore::OnLibraryLoaded(const char *name) {
       Die();
     }
   }
+
+  // Track instrumented ranges.
+  if (track_instrumented_libs_) {
+    for (const auto &mod : modules) {
+      if (!mod.instrumented())
+        continue;
+      for (const auto &range : mod.ranges()) {
+        if (!range.executable)
+          continue;
+        if (IsPcInstrumented(range.beg) && IsPcInstrumented(range.end - 1))
+          continue;
+        VReport(1, "Adding instrumented range %p-%p from library '%s'\n",
+                range.beg, range.end, mod.full_name());
+        const uptr idx =
+            atomic_load(&instrumented_ranges_count_, memory_order_relaxed);
+        CHECK_LT(idx, kMaxLibs);
+        instrumented_code_ranges_[idx].begin = range.beg;
+        instrumented_code_ranges_[idx].end = range.end;
+        atomic_store(&instrumented_ranges_count_, idx + 1,
+                     memory_order_release);
+      }
+    }
+  }
 }
 
 void LibIgnore::OnLibraryUnloaded() {
diff --git a/lib/sanitizer_common/sanitizer_libignore.h b/lib/sanitizer_common/sanitizer_libignore.h
index cd56c36c1..17b0f563d 100644
--- a/lib/sanitizer_common/sanitizer_libignore.h
+++ b/lib/sanitizer_common/sanitizer_libignore.h
@@ -30,6 +30,9 @@ class LibIgnore {
 
   // Must be called during initialization.
   void AddIgnoredLibrary(const char *name_templ);
+  void IgnoreNoninstrumentedModules(bool enable) {
+    track_instrumented_libs_ = enable;
+  }
 
   // Must be called after a new dynamic library is loaded.
   void OnLibraryLoaded(const char *name);
@@ -37,8 +40,14 @@ class LibIgnore {
   // Must be called after a dynamic library is unloaded.
   void OnLibraryUnloaded();
 
-  // Checks whether the provided PC belongs to one of the ignored libraries.
-  bool IsIgnored(uptr pc) const;
+  // Checks whether the provided PC belongs to one of the ignored libraries or
+  // the PC should be ignored because it belongs to an non-instrumented module
+  // (when ignore_noninstrumented_modules=1). Also returns true via
+  // "pc_in_ignored_lib" if the PC is in an ignored library, false otherwise.
+  bool IsIgnored(uptr pc, bool *pc_in_ignored_lib) const;
+
+  // Checks whether the provided PC belongs to an instrumented module.
+  bool IsPcInstrumented(uptr pc) const;
 
  private:
   struct Lib {
@@ -53,26 +62,48 @@ class LibIgnore {
     uptr end;
   };
 
+  inline bool IsInRange(uptr pc, const LibCodeRange &range) const {
+    return (pc >= range.begin && pc < range.end);
+  }
+
   static const uptr kMaxLibs = 128;
 
   // Hot part:
-  atomic_uintptr_t loaded_count_;
-  LibCodeRange code_ranges_[kMaxLibs];
+  atomic_uintptr_t ignored_ranges_count_;
+  LibCodeRange ignored_code_ranges_[kMaxLibs];
+
+  atomic_uintptr_t instrumented_ranges_count_;
+  LibCodeRange instrumented_code_ranges_[kMaxLibs];
 
   // Cold part:
   BlockingMutex mutex_;
   uptr count_;
   Lib libs_[kMaxLibs];
+  bool track_instrumented_libs_;
 
   // Disallow copying of LibIgnore objects.
   LibIgnore(const LibIgnore&);  // not implemented
   void operator = (const LibIgnore&);  // not implemented
 };
 
-inline bool LibIgnore::IsIgnored(uptr pc) const {
-  const uptr n = atomic_load(&loaded_count_, memory_order_acquire);
+inline bool LibIgnore::IsIgnored(uptr pc, bool *pc_in_ignored_lib) const {
+  const uptr n = atomic_load(&ignored_ranges_count_, memory_order_acquire);
+  for (uptr i = 0; i < n; i++) {
+    if (IsInRange(pc, ignored_code_ranges_[i])) {
+      *pc_in_ignored_lib = true;
+      return true;
+    }
+  }
+  *pc_in_ignored_lib = false;
+  if (track_instrumented_libs_ && !IsPcInstrumented(pc))
+    return true;
+  return false;
+}
+
+inline bool LibIgnore::IsPcInstrumented(uptr pc) const {
+  const uptr n = atomic_load(&instrumented_ranges_count_, memory_order_acquire);
   for (uptr i = 0; i < n; i++) {
-    if (pc >= code_ranges_[i].begin && pc < code_ranges_[i].end)
+    if (IsInRange(pc, instrumented_code_ranges_[i]))
       return true;
   }
   return false;
diff --git a/lib/tsan/rtl/tsan_flags.inc b/lib/tsan/rtl/tsan_flags.inc
index 071cf427d..a48545c43 100644
--- a/lib/tsan/rtl/tsan_flags.inc
+++ b/lib/tsan/rtl/tsan_flags.inc
@@ -79,5 +79,8 @@ TSAN_FLAG(bool, die_after_fork, true,
 TSAN_FLAG(const char *, suppressions, "", "Suppressions file name.")
 TSAN_FLAG(bool, ignore_interceptors_accesses, false,
           "Ignore reads and writes from all interceptors.")
+TSAN_FLAG(bool, ignore_noninstrumented_modules, false,
+          "Interceptors should only detect races when called from instrumented "
+          "modules.")
 TSAN_FLAG(bool, shared_ptr_interceptor, true,
           "Track atomic reference counting in libc++ shared_ptr and weak_ptr.")
diff --git a/lib/tsan/rtl/tsan_interceptors.cc b/lib/tsan/rtl/tsan_interceptors.cc
index a3a50e13f..898f32df1 100644
--- a/lib/tsan/rtl/tsan_interceptors.cc
+++ b/lib/tsan/rtl/tsan_interceptors.cc
@@ -231,6 +231,8 @@ void InitializeLibIgnore() {
     if (0 == internal_strcmp(s->type, kSuppressionLib))
       libignore()->AddIgnoredLibrary(s->templ);
   }
+  if (flags()->ignore_noninstrumented_modules)
+    libignore()->IgnoreNoninstrumentedModules(true);
   libignore()->OnLibraryLoaded(0);
 }
 
@@ -252,31 +254,20 @@ static unsigned g_thread_finalize_key;
 
 ScopedInterceptor::ScopedInterceptor(ThreadState *thr, const char *fname,
                                      uptr pc)
-    : thr_(thr)
-    , pc_(pc)
-    , in_ignored_lib_(false) {
+    : thr_(thr), pc_(pc), in_ignored_lib_(false), ignoring_(false) {
   Initialize(thr);
-  if (!thr_->is_inited)
-    return;
-  if (!thr_->ignore_interceptors)
-    FuncEntry(thr, pc);
+  if (!thr_->is_inited) return;
+  if (!thr_->ignore_interceptors) FuncEntry(thr, pc);
   DPrintf("#%d: intercept %s()\n", thr_->tid, fname);
-  if (!thr_->in_ignored_lib && libignore()->IsIgnored(pc)) {
-    in_ignored_lib_ = true;
-    thr_->in_ignored_lib = true;
-    ThreadIgnoreBegin(thr_, pc_);
-  }
-  if (flags()->ignore_interceptors_accesses) ThreadIgnoreBegin(thr_, pc_);
+  ignoring_ =
+      !thr_->in_ignored_lib && (flags()->ignore_interceptors_accesses ||
+                                libignore()->IsIgnored(pc, &in_ignored_lib_));
+  EnableIgnores();
 }
 
 ScopedInterceptor::~ScopedInterceptor() {
-  if (!thr_->is_inited)
-    return;
-  if (flags()->ignore_interceptors_accesses) ThreadIgnoreEnd(thr_, pc_);
-  if (in_ignored_lib_) {
-    thr_->in_ignored_lib = false;
-    ThreadIgnoreEnd(thr_, pc_);
-  }
+  if (!thr_->is_inited) return;
+  DisableIgnores();
   if (!thr_->ignore_interceptors) {
     ProcessPendingSignals(thr_);
     FuncExit(thr_);
@@ -284,20 +275,24 @@ ScopedInterceptor::~ScopedInterceptor() {
   }
 }
 
-void ScopedInterceptor::UserCallbackStart() {
-  if (flags()->ignore_interceptors_accesses) ThreadIgnoreEnd(thr_, pc_);
-  if (in_ignored_lib_) {
-    thr_->in_ignored_lib = false;
-    ThreadIgnoreEnd(thr_, pc_);
+void ScopedInterceptor::EnableIgnores() {
+  if (ignoring_) {
+    ThreadIgnoreBegin(thr_, pc_);
+    if (in_ignored_lib_) {
+      DCHECK(!thr_->in_ignored_lib);
+      thr_->in_ignored_lib = true;
+    }
   }
 }
 
-void ScopedInterceptor::UserCallbackEnd() {
-  if (in_ignored_lib_) {
-    thr_->in_ignored_lib = true;
-    ThreadIgnoreBegin(thr_, pc_);
+void ScopedInterceptor::DisableIgnores() {
+  if (ignoring_) {
+    ThreadIgnoreEnd(thr_, pc_);
+    if (in_ignored_lib_) {
+      DCHECK(thr_->in_ignored_lib);
+      thr_->in_ignored_lib = false;
+    }
   }
-  if (flags()->ignore_interceptors_accesses) ThreadIgnoreBegin(thr_, pc_);
 }
 
 #define TSAN_INTERCEPT(func) INTERCEPT_FUNCTION(func)
diff --git a/lib/tsan/rtl/tsan_interceptors.h b/lib/tsan/rtl/tsan_interceptors.h
index a0f9a0753..72534f4a2 100644
--- a/lib/tsan/rtl/tsan_interceptors.h
+++ b/lib/tsan/rtl/tsan_interceptors.h
@@ -10,12 +10,13 @@ class ScopedInterceptor {
  public:
   ScopedInterceptor(ThreadState *thr, const char *fname, uptr pc);
   ~ScopedInterceptor();
-  void UserCallbackStart();
-  void UserCallbackEnd();
+  void DisableIgnores();
+  void EnableIgnores();
  private:
   ThreadState *const thr_;
   const uptr pc_;
   bool in_ignored_lib_;
+  bool ignoring_;
 };
 
 }  // namespace __tsan
@@ -39,10 +40,10 @@ class ScopedInterceptor {
 /**/
 
 #define SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START() \
-    si.UserCallbackStart();
+    si.DisableIgnores();
 
 #define SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END() \
-    si.UserCallbackEnd();
+    si.EnableIgnores();
 
 #define TSAN_INTERCEPTOR(ret, func, ...) INTERCEPTOR(ret, func, __VA_ARGS__)
 
diff --git a/test/tsan/Darwin/ignore-noninstrumented.mm b/test/tsan/Darwin/ignore-noninstrumented.mm
new file mode 100644
index 000000000..5e4453102
--- /dev/null
+++ b/test/tsan/Darwin/ignore-noninstrumented.mm
@@ -0,0 +1,53 @@
+// Check that ignore_noninstrumented_modules=1 supresses races from system libraries on OS X.
+
+// RUN: %clang_tsan %s -o %t -framework Foundation
+
+// Check that without the flag, there are false positives.
+// RUN: %deflake %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-RACE
+
+// With ignore_noninstrumented_modules=1, no races are reported.
+// RUN: %env_tsan_opts=ignore_noninstrumented_modules=1 %run %t 2>&1 | FileCheck %s
+
+// With ignore_noninstrumented_modules=1, races in user's code are still reported.
+// RUN: %env_tsan_opts=ignore_noninstrumented_modules=1 %deflake %run %t race 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-RACE
+
+#import <Foundation/Foundation.h>
+
+#import "../test.h"
+
+char global_buf[64];
+
+void *Thread1(void *x) {
+  barrier_wait(&barrier);
+  strcpy(global_buf, "hello world");
+  return NULL;
+}
+
+void *Thread2(void *x) {
+  strcpy(global_buf, "world hello");
+  barrier_wait(&barrier);
+  return NULL;
+}
+
+int main(int argc, char *argv[]) {
+  fprintf(stderr, "Hello world.\n");
+  
+  // NSUserDefaults uses XPC which triggers the false positive.
+  NSDictionary *d = [[NSUserDefaults standardUserDefaults] dictionaryRepresentation];
+  fprintf(stderr, "d = %p\n", d);
+
+  if (argc > 1 && strcmp(argv[1], "race") == 0) {
+    barrier_init(&barrier, 2);
+    pthread_t t[2];
+    pthread_create(&t[0], NULL, Thread1, NULL);
+    pthread_create(&t[1], NULL, Thread2, NULL);
+    pthread_join(t[0], NULL);
+    pthread_join(t[1], NULL);
+  }
+
+  fprintf(stderr, "Done.\n");
+}
+
+// CHECK: Hello world.
+// CHECK-RACE: SUMMARY: ThreadSanitizer: data race
+// CHECK: Done.