[tsan] Introduce a "ignore_interceptors_accesses" option

[kubamracek]

Summary

On OS X, TSan already passes all unit and lit tests, but for real-world applications (even very simple ones), we currently produce a lot of false positive reports about data races. This makes TSan useless at this point, because the noise dominates real bugs. I'd like to introduce a runtime flag, "ignore_interceptors_accesses", off by default, which will make TSan ignore all memory accesses that happen from interceptors. This will significantly lower the coverage and miss a lot of bugs, but it eliminates most of the current false positives on OS X.

The false positives are mostly coming from system libraries that use custom synchronization (atomics) and/or synchronize via APIs that we haven't written interceptors for yet. This flag, "ignore_interceptors_accesses", is intended as a temporary bring-up tool, so that TSan can actually be used on a GUI app.

[dvyukov]

This flag, "ignore_interceptors_accesses", is intended as a temporary bring-up tool, so that TSan can actually be used on a GUI app.

Do we need to submit it then? How long will it live? What is supposed to be a permanent solution?

Can we have a test for these false positives? I am not sure whether it is possible to run GUI tests within lit tests. is it possible to use these libraries without GUI? It would be useful to have a test that just invokes some fat functions from system libraries and test that we don't report any races.

I think we want to use something along the lines of called_from_lib suppressions. Or we want to ignore mops from all libraries? I guess we want to handle memory access coming at least from the main binary, right?

Also I would prefer to pass the flag under the covers. This change is too pervasive, while all we need is:

ScopedInterceptor::ScopedInterceptor(...) {
  if (...) {
    ThreadIgnoreBegin(thr_, pc_);
    unignore_ = true;
  }

ScopedInterceptor::~ScopedInterceptor() {
  if (unignore_)
      ThreadIgnoreEnd(thr_, pc_);

[kubamracek]

Updating patch to only enable/disable tracking of memory accesses in ScopedInterceptor. Adding a test case.

[dvyukov]

lib/sanitizer_common/sanitizer_common_interceptors.inc

3311 ↗

(On Diff #44009)

I think you need to put it before REAL(_exit), because it is no-return. Though, it is probably don't matter much.

lib/tsan/rtl/tsan_interceptors.cc

2155 ↗

(On Diff #44009)

Why is this a user callback?

[kubamracek]

lib/tsan/rtl/tsan_interceptors.cc

2155 ↗

(On Diff #44009)

It ends up calling ThreadFinalize, which expects ignores to be disabled (check that ignore_reads_and_writes==0). Suggestions to handle this better?

[dvyukov]

lib/tsan/rtl/tsan_interceptors.cc

2155 ↗

(On Diff #44009)

How does libc function call ThreadFinalize (stack trace)? Does it happen for pthread_kill of pthread_self?

[kubamracek]

I was mistaken about pthread_kill, it doesn't need to be wrapped with USER_CALLBACK_START/END.

[dvyukov]

LGTM if you move REAL(_exit). Code after _exit just looks strange.

compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc

 #ifndef COMMON_INTERCEPTOR_ACQUIRE
 #define COMMON_INTERCEPTOR_ACQUIRE(ctx, u) {}
 #endif
 
 #ifndef COMMON_INTERCEPTOR_RELEASE
 #define COMMON_INTERCEPTOR_RELEASE(ctx, u) {}
 #endif
 
+#ifndef COMMON_INTERCEPTOR_USER_CALLBACK_START
+#define COMMON_INTERCEPTOR_USER_CALLBACK_START() {}
+#endif
+
+#ifndef COMMON_INTERCEPTOR_USER_CALLBACK_END
+#define COMMON_INTERCEPTOR_USER_CALLBACK_END() {}
+#endif
+
 struct FileMetadata {
   // For open_memstream().
   char **addr;
   SIZE_T *size;
 };
 
 struct CommonInterceptorMetadata {
   enum {

 #else
 #define INIT_BACKTRACE
 #endif
 
 #if SANITIZER_INTERCEPT__EXIT
 INTERCEPTOR(void, _exit, int status) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, _exit, status);
+  COMMON_INTERCEPTOR_USER_CALLBACK_START();
   int status1 = COMMON_INTERCEPTOR_ON_EXIT(ctx);
+  COMMON_INTERCEPTOR_USER_CALLBACK_END();
   if (status == 0) status = status1;
   REAL(_exit)(status);
 }
 #define INIT__EXIT COMMON_INTERCEPT_FUNCTION(_exit);
 #else
 #define INIT__EXIT
 #endif
 

compiler-rt/trunk/lib/tsan/rtl/tsan_flags.inc

 TSAN_FLAG(int, io_sync, 1,
           "Controls level of synchronization implied by IO operations. "
           "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.")

compiler-rt/trunk/lib/tsan/rtl/tsan_interceptors.cc

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_);
 }
 
 ScopedInterceptor::~ScopedInterceptor() {
+  if (flags()->ignore_interceptors_accesses) ThreadIgnoreEnd(thr_, pc_);
   if (in_ignored_lib_) {
     thr_->in_ignored_lib = false;
     ThreadIgnoreEnd(thr_, pc_);
   }
   if (!thr_->ignore_interceptors) {
     ProcessPendingSignals(thr_);
     FuncExit(thr_);
     CheckNoLocks(thr_);
   }
 }
 
 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::UserCallbackEnd() {
   if (in_ignored_lib_) {
     thr_->in_ignored_lib = true;
     ThreadIgnoreBegin(thr_, pc_);
   }
+  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

 #define COMMON_INTERCEPTOR_GET_TLS_RANGE(begin, end)                           \
   if (TsanThread *t = GetCurrentThread()) {                                    \
     *begin = t->tls_begin();                                                   \
     *end = t->tls_end();                                                       \
   } else {                                                                     \
     *begin = *end = 0;                                                         \
   }
 
+#define COMMON_INTERCEPTOR_USER_CALLBACK_START() \
+  SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START()
+
+#define COMMON_INTERCEPTOR_USER_CALLBACK_END() \
+  SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_END()
+
 #include "sanitizer_common/sanitizer_common_interceptors.inc"
 
 #define TSAN_SYSCALL() \
   ThreadState *thr = cur_thread(); \
   if (thr->ignore_interceptors) \
     return; \
   ScopedSyscall scoped_syscall(thr) \
 /**/

compiler-rt/trunk/test/tsan/Darwin/ignored-interceptors.mm

+// Check that ignore_interceptors_accesses=1 supresses reporting races from
+// system libraries on OS X. There are currently false positives coming from
+// libxpc, libdispatch, CoreFoundation and others, because these libraries use
+// TSan-invisible atomics as synchronization.
+
+// 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 --check-prefix=CHECK-RACE
+
+// With ignore_interceptors_accesses=1, no races are reported.
+// RUN: %env_tsan_opts=ignore_interceptors_accesses=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK
+
+// With ignore_interceptors_accesses=1, races in user's code are still reported.
+// RUN: %env_tsan_opts=ignore_interceptors_accesses=1 %deflake %run %t race 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-RACE
+
+#import <Foundation/Foundation.h>
+
+#import "../test.h"
+
+long global;
+
+void *Thread1(void *x) {
+  barrier_wait(&barrier);
+  global = 42;
+  return NULL;
+}
+
+void *Thread2(void *x) {
+  global = 43;
+  barrier_wait(&barrier);
+  return NULL;
+}
+
+int main(int argc, char *argv[]) {
+  NSLog(@"Hello world.");
+  
+  // NSUserDefaults uses XPC which triggers the false positive.
+  NSDictionary *d = [[NSUserDefaults standardUserDefaults] dictionaryRepresentation];
+  NSLog(@"d = %@", 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);
+  }
+
+  NSLog(@"Done.");
+}
+
+// CHECK: Hello world.
+// CHECK-RACE: SUMMARY: ThreadSanitizer: data race
+// CHECK: Done.