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

[kubamracek]

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

[dvyukov]

lib/tsan/rtl/tsan_interceptors.cc

263

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]

lib/tsan/rtl/tsan_interceptors.cc

263

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]

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]

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

82

underscore at the name end:

track_instrumented_libs_

[kubamracek]

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

[dvyukov]

Thanks!

lib/sanitizer_common/sanitizer_libignore.h

 namespace __sanitizer {
 
 class LibIgnore {
  public:
   explicit LibIgnore(LinkerInitialized);
 
   // 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);
 
   // 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 {
     char *templ;
     char *name;
     char *real_name;  // target of symlink
     bool loaded;
   };
 
   struct LibCodeRange {
     uptr begin;
     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;
 }
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_LIBIGNORE_H

lib/sanitizer_common/sanitizer_libignore.cc

for (const auto &mod : modules) {
         if (lib->loaded)
           continue;
         VReport(1,
                 "Matched called_from_lib suppression '%s' against library"
                 " '%s'\n",
                 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;
       }
     }
     if (lib->loaded && !loaded) {
       Report("%s: library '%s' that was matched against called_from_lib"
              " suppression '%s' is unloaded\n",
              SanitizerToolName, lib->name, lib->templ);
       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() {
   OnLibraryLoaded(nullptr);
 }
 
 } // namespace __sanitizer
 
 #endif // #if SANITIZER_FREEBSD || SANITIZER_LINUX

lib/tsan/rtl/tsan_flags.inc

TSAN_FLAG(int, io_sync, 1,
           "0 - no synchronization "
           "1 - reasonable level of synchronization (write->read)"
           "2 - global synchronization of all IO operations.")
 TSAN_FLAG(bool, die_after_fork, true,
           "Die after multi-threaded fork if the child creates new threads.")
 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.")

lib/tsan/rtl/tsan_interceptors.h

 #ifndef TSAN_INTERCEPTORS_H
 #define TSAN_INTERCEPTORS_H
 
 #include "sanitizer_common/sanitizer_stacktrace.h"
 #include "tsan_rtl.h"
 
 namespace __tsan {
 
 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
 
 #define SCOPED_INTERCEPTOR_RAW(func, ...) \
     ThreadState *thr = cur_thread(); \
     const uptr caller_pc = GET_CALLER_PC(); \
     ScopedInterceptor si(thr, #func, caller_pc); \
     const uptr pc = StackTrace::GetCurrentPc(); \
     (void)pc; \
 /**/
 
 #define SCOPED_TSAN_INTERCEPTOR(func, ...) \
     SCOPED_INTERCEPTOR_RAW(func, __VA_ARGS__); \
     if (REAL(func) == 0) { \
       Report("FATAL: ThreadSanitizer: failed to intercept %s\n", #func); \
       Die(); \
     }                                                    \
     if (!thr->is_inited || thr->ignore_interceptors || thr->in_ignored_lib) \
       return REAL(func)(__VA_ARGS__); \
 /**/
 
 #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__)
 
 #endif  // TSAN_INTERCEPTORS_H

lib/tsan/rtl/tsan_interceptors.cc

 void InitializeLibIgnore() {
   const SuppressionContext &supp = *Suppressions();
   const uptr n = supp.SuppressionCount();
   for (uptr i = 0; i < n; i++) {
     const Suppression *s = supp.SuppressionAt(i);
     if (0 == internal_strcmp(s->type, kSuppressionLib))
       libignore()->AddIgnoredLibrary(s->templ);
   }
+  if (flags()->ignore_noninstrumented_modules)
+    libignore()->IgnoreNoninstrumentedModules(true);
   libignore()->OnLibraryLoaded(0);
 }
 
 }  // namespace __tsan
 
 static ThreadSignalContext *SigCtx(ThreadState *thr) {
   ThreadSignalContext *ctx = (ThreadSignalContext*)thr->signal_ctx;
   if (ctx == 0 && !thr->is_dead) {
     ctx = (ThreadSignalContext*)MmapOrDie(sizeof(*ctx), "ThreadSignalContext");
     MemoryResetRange(thr, (uptr)&SigCtx, (uptr)ctx, sizeof(*ctx));
     thr->signal_ctx = ctx;
   }
   return ctx;
 }
 
 #if !SANITIZER_MAC
 static unsigned g_thread_finalize_key;
 #endif
 
 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_);
     CheckNoLocks(thr_);
   }
 }
 
-void ScopedInterceptor::UserCallbackStart() {
-  if (flags()->ignore_interceptors_accesses) ThreadIgnoreEnd(thr_, pc_);
+void ScopedInterceptor::EnableIgnores() {
+  if (ignoring_) {
+    ThreadIgnoreBegin(thr_, pc_);
     if (in_ignored_lib_) {
-    thr_->in_ignored_lib = false;
-    ThreadIgnoreEnd(thr_, pc_);
+      DCHECK(!thr_->in_ignored_lib);
+      thr_->in_ignored_lib = true;
+    }
   }
 }
 
-void ScopedInterceptor::UserCallbackEnd() {
+void ScopedInterceptor::DisableIgnores() {
+  if (ignoring_) {
+    ThreadIgnoreEnd(thr_, pc_);
     if (in_ignored_lib_) {
-    thr_->in_ignored_lib = true;
-    ThreadIgnoreBegin(thr_, pc_);
+      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)
 #if SANITIZER_FREEBSD
 # define TSAN_INTERCEPT_VER(func, ver) INTERCEPT_FUNCTION(func)
 #else
 # define TSAN_INTERCEPT_VER(func, ver) INTERCEPT_FUNCTION_VER(func, ver)
 #endif

test/tsan/Darwin/ignore-noninstrumented.mm

+// 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.