[tsan] Don't report bugs from interceptors called from libignored modules

[kubamracek]

Summary

I'd like to expand the ignore_noninstrumented_modules=1 flag to cover all report types (it currently only suppresses data race detection). We're seeing some false positives from system libraries where they guard the mutex acquisition via atomics (so the deadlock cannot happen). The general approach that I'd like to achieve with ignore_noninstrumented_modules=1 is to only report bugs where we know they actually come from instrumented code, regardless of bug type (race, mutex problem, deadlock).

This patch make sure we don't report deadlocks and other bug types when we're inside an interceptor that was called from a noninstrumented module (when ignore_noninstrumented_modules=1 is set). Adding a testcase that shows that deadlock detection still works on Darwin (to make sure we're not silencing too many reports).

[kcc]

I don't like this change -- it is very un-intuitive that reads/writes affects deadlock checker.
Also remember that deadlock detector does not require instrumentation at all.
Can we solve it with a blacklist?

[kubamracek]

remember that deadlock detector does not require instrumentation at all.

Unfortunately, the fact that TSan is able to detect issues on non-instrumented code brings more harm than benefits, at least on macOS/iOS. Almost all reports that don't come from instrumented code are false positives. Our users are usually fine with recompiling their source code with instrumentation and they expect to only find issues with their code, not some 3rd party library. Same applies to mutexes and deadlocks. If a user wants to see reports from non-instrumented libraries, they can just turn the ignore_noninstrumented_modules flag to zero.

Can we solve it with a blacklist?

Maybe, but to use libignore (which I would need for this), the same patch would be needed. Libignore currently sets ignore_reads_and_writes, which doesn't prevent reporting deadlocks.

it is very un-intuitive that reads/writes affects deadlock checker.

That's why the patches renamed ignore_reads_and_writes to ignore_reads_writes_and_reports. Would it help if I added a separate ignore_reports (or even ignore_deadlocks) flag?

[dvyukov]

When an interceptor comes from a non-instrumented library, we set thr->in_ignored_lib=true. This causes SCOPED_TSAN_INTERCEPTOR to call the real function an skip all tsan processing. Consequently mutex lock/unlock operations in non-instrumented libraries are not handled by tsan already. So it's not clear to me how you get deadlock reports there. What am I missing?

One problem is if one mutex in deadlock cycles is taken from non-instrumented code, but the second mutex is taken from instrumented code, tsan won't report the deadlock on the mutex operations coming from non-instrumented code, but it will still report it on operations coming from instrumented code. So it seems to me that a better way to handle it is to hide all mutex operations in non-instrumented code from tsan. But it already seems to be happening, so I am confused.

[kubamracek]

In D31449#714004, @dvyukov wrote:

When an interceptor comes from a non-instrumented library, we set thr->in_ignored_lib=true. This causes SCOPED_TSAN_INTERCEPTOR to call the real function an skip all tsan processing. Consequently mutex lock/unlock operations in non-instrumented libraries are not handled by tsan already. So it's not clear to me how you get deadlock reports there. What am I missing?

There's two issues with the current state of things:

1. in_ignored_lib=true only affects second (nested) calls to other interceptor. So if a libignored library directly calls pthread_mutex_lock+pthread_mutex_unlock, those interceptors are still called and they detect deadlocks.
2. Even if libignore affects direct interceptor calls, we'd have even more issues, because the pthread_mutex_lock+pthread_mutex_unlock interceptors provide important synchronization and if we just ignore them, we'll get even more false positives.

What I'm suggesting is: Non-instrumented modules should still call interceptors like pthread_mutex_lock+pthread_mutex_unlock, but these should just not report any issues, including deadlocks. Do you disagree?

[dvyukov]

lib/tsan/dd/dd_rtl.cc

40 ↗

(On Diff #93331)

This is a Thread from dd_rtl.h, this is not ThreadState from tsan_rtl.h. It does not have that field.

[dvyukov]

1. s/ignore_reads_and_writes/ignore_reads_writes_and_reports/ does not look like a good idea to me, because these things are not necessary related. If we go this route we need separate flags.
2. From the description it seems that your intention is to ignore all reports, but you are ignoring only deadlocks. There are many more types of reports.

How about:

 bool OutputReport(ThreadState *thr, const ScopedReport &srep) {
-  if (!flags()->report_bugs)
+  if (!flags()->report_bugs || thr->suppress_reports)
     return false;
   atomic_store_relaxed(&ctx->last_symbolize_time_ns, NanoTime());
   const ReportDesc *rep = srep.GetReport();

This will be a minimalistic change and it will suppress all reports and won't affect anything other than non-instrumented libs.
But note that it will effectively nullify the external API thing as it will never report anything.

[kubamracek]

Addressing review comments, renaming.

From the description it seems that your intention is to ignore all reports, but you are ignoring only deadlocks. There are many more types of reports.

Right, this patch now suppresses all types.

note that it will effectively nullify the external API thing as it will never report anything.

I tested this and the external API still works fine. This is because this API isn't an interceptor and we're not using ScopedInterceptor in it.

[kubamracek]

ping

[kubamracek]

Is this okay to land?

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

if (!thr_->ignore_interceptors) {
     FuncExit(thr_);
     CheckNoLocks(thr_);
   }
 }
 
 void ScopedInterceptor::EnableIgnores() {
   if (ignoring_) {
     ThreadIgnoreBegin(thr_, pc_, false);
+    if (flags()->ignore_noninstrumented_modules) thr_->suppress_reports++;
     if (in_ignored_lib_) {
       DCHECK(!thr_->in_ignored_lib);
       thr_->in_ignored_lib = true;
     }
   }
 }
 
 void ScopedInterceptor::DisableIgnores() {
   if (ignoring_) {
     ThreadIgnoreEnd(thr_, pc_);
+    if (flags()->ignore_noninstrumented_modules) thr_->suppress_reports--;
     if (in_ignored_lib_) {
       DCHECK(thr_->in_ignored_lib);
       thr_->in_ignored_lib = false;
     }
   }
 }
 
 #define TSAN_INTERCEPT(func) INTERCEPT_FUNCTION(func)

compiler-rt/trunk/lib/tsan/rtl/tsan_rtl.h

struct ThreadState {
   // taken by epoch between synchs.
   // This way we can save one load from tls.
   u64 fast_synch_epoch;
   // This is a slow path flag. On fast path, fast_state.GetIgnoreBit() is read.
   // We do not distinguish beteween ignoring reads and writes
   // for better performance.
   int ignore_reads_and_writes;
   int ignore_sync;
+  int suppress_reports;
   // Go does not support ignores.
 #if !SANITIZER_GO
   IgnoreSet mop_ignore_set;
   IgnoreSet sync_ignore_set;
 #endif
   // C/C++ uses fixed size shadow stack embed into Trace.
   // Go uses malloc-allocated shadow stack with dynamic size.
   uptr *shadow_stack;

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

static void AddRacyStacks(ThreadState *thr, VarSizeStackTrace traces[2],
   }
   if (flags()->suppress_equal_addresses) {
     RacyAddress ra0 = {addr_min, addr_max};
     ctx->racy_addresses.PushBack(ra0);
   }
 }
 
 bool OutputReport(ThreadState *thr, const ScopedReport &srep) {
-  if (!flags()->report_bugs)
+  if (!flags()->report_bugs || thr->suppress_reports)
     return false;
   atomic_store_relaxed(&ctx->last_symbolize_time_ns, NanoTime());
   const ReportDesc *rep = srep.GetReport();
   CHECK_EQ(thr->current_report, nullptr);
   thr->current_report = rep;
   Suppression *supp = 0;
   uptr pc_or_addr = 0;
   for (uptr i = 0; pc_or_addr == 0 && i < rep->mops.Size(); i++)

compiler-rt/trunk/test/tsan/Darwin/deadlock.mm

+// RUN: %clang_tsan %s -o %t -framework Foundation
+// RUN: %deflake %run %t 2>&1 | FileCheck %s
+
+#import <Foundation/Foundation.h>
+
+#import "../test.h"
+
+pthread_mutex_t m1;
+pthread_mutex_t m2;
+
+int main(int argc, const char *argv[]) {
+  barrier_init(&barrier, 2);
+  fprintf(stderr, "Hello world.\n");
+
+  pthread_mutex_init(&m1, NULL);
+  pthread_mutex_init(&m2, NULL);
+  
+  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+    pthread_mutex_lock(&m1);
+    pthread_mutex_lock(&m2);
+    pthread_mutex_unlock(&m2);
+    pthread_mutex_unlock(&m1);
+
+    barrier_wait(&barrier);
+  });
+  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+    barrier_wait(&barrier);
+
+    pthread_mutex_lock(&m2);
+    pthread_mutex_lock(&m1);
+    pthread_mutex_unlock(&m1);
+    pthread_mutex_unlock(&m2);
+
+    dispatch_sync(dispatch_get_main_queue(), ^{
+      CFRunLoopStop(CFRunLoopGetCurrent());
+    });
+  });
+
+  CFRunLoopRun();
+  
+  fprintf(stderr, "Done.\n");
+  return 0;
+}
+
+// CHECK: Hello world.
+// CHECK: WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock)
+// CHECK: Done.