[tsan] Ignore memory accesses for libignored modules for "external" races

[kubamracek]

Summary

On Darwin, the setting ignore_noninstrumented_modules is used to suppress false positives in code that users don't have control of. The recently added "external" API (which can be used to detect races on objects provided by system libraries, but the race is actually user's fault) ignores this flag and it can report issues in non-instrumented modules. This patch fixes that.

There's more discussion about this at https://reviews.llvm.org/D28836.

[dvyukov]

Wait, the external API was meant specifically to be used from non-instrumented libraries. Quoting you in D28836:

It might not be feasible/easy to recompile the library with TSan instrumentation... The idea of this patch is to allow a non-instrumented library to call into TSan runtime.

With this change it all become pointless. We annotate non-instrumented libraries and then ignore that. What am I missing?

[kubamracek]

In D31553#716166, @dvyukov wrote:

Wait, the external API was meant specifically to be used from non-instrumented libraries. Quoting you in D28836:

It might not be feasible/easy to recompile the library with TSan instrumentation... The idea of this patch is to allow a non-instrumented library to call into TSan runtime.

With this change it all become pointless. We annotate non-instrumented libraries and then ignore that. What am I missing?

There's multiple scenarios, but a typical one can be: An API provider, which is shipped with the OS as a non-instrumented library, e.g. libcurl, adopts the TSan external API. Now users of libcurl don't need to recompile libcurl to detect when they're violating the libcurl's API threading rules. Even with the non-instrumented version of libcurl, TSan will report whenever you're using a CURL * object from multiple threads without synchronization. However, if you're using a 3rd party library (or a system library) that is *also non-instrumented* and uses libcurl, let's call it libftp, we would report bugs as well, even though they're not your bugs. And they could be real bugs (libftp has races) or false positives (libftp uses some atomic-based synchronization that is TSan-invisible), but they're still out of the user's control to fix. This patch silences reports that come from instrumented_module -> libftp -> libcurl, but still reports bugs from instrumented_module -> libcurl.

If the user actually wants to see races even in non-instrumented code (typically system libraries and 3rd party binary frameworks), they can always set the flag ignore_noninstrumented_modules back to 0.

[kubamracek]

ping

[dvyukov]

lib/tsan/rtl/tsan_external.cc

61 ↗

(On Diff #93720)

I think I understand why I was confused. I assumed that caller_pc is literally caller pc. And caller can well point into the same library, and then this all does not make sense. But you seem to assume that caller_pc points user code (can be caller pc or several frames above that).

We now have include/sanitizer/tsan_interface.h, so please add __tsan_external_* functions there with proper comments that document these assumptions.

64 ↗

(On Diff #93720)

Unrelated to this change, but why did we decide to use kSizeLog8? It would be more reasonable to use kSizeLog1. A library may need several different addresses to model several unrelated things and if the object is only 8 bytes, it won't be able to do so. Also if size of the object is only, say, 4 bytes, 8-byte access can lead to false positives.
Is there any reason to use 8? If not, please change it to 1.

[kubamracek]

lib/tsan/rtl/tsan_external.cc

61 ↗

(On Diff #93720)

https://reviews.llvm.org/D32358

[kubamracek]

lib/tsan/rtl/tsan_external.cc

64 ↗

(On Diff #93720)

https://reviews.llvm.org/D32359

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

 //===-- tsan_external.cc --------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of ThreadSanitizer (TSan), a race detector.
 //
 //===----------------------------------------------------------------------===//
 #include "tsan_rtl.h"
+#include "tsan_interceptors.h"
 
 namespace __tsan {
 
 #define CALLERPC ((uptr)__builtin_return_address(0))
 
 const uptr kMaxTag = 128;  // Limited to 65,536, since MBlock only stores tags
                            // as 16-bit values, see tsan_defs.h.
 
void __tsan_external_assign_tag(void *addr, void *tag) {
   }
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_external_read(void *addr, void *caller_pc, void *tag) {
   CHECK_LT(tag, atomic_load(&used_tags, memory_order_relaxed));
   ThreadState *thr = cur_thread();
   thr->external_tag = (uptr)tag;
-  FuncEntry(thr, (uptr)caller_pc);
+  if (caller_pc) FuncEntry(thr, (uptr)caller_pc);
+  bool in_ignored_lib;
+  if (!caller_pc || !libignore()->IsIgnored((uptr)caller_pc, &in_ignored_lib)) {
     MemoryRead(thr, CALLERPC, (uptr)addr, kSizeLog8);
-  FuncExit(thr);
+  }
+  if (caller_pc) FuncExit(thr);
   thr->external_tag = 0;
 }
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_external_write(void *addr, void *caller_pc, void *tag) {
   CHECK_LT(tag, atomic_load(&used_tags, memory_order_relaxed));
   ThreadState *thr = cur_thread();
   thr->external_tag = (uptr)tag;
-  FuncEntry(thr, (uptr)caller_pc);
+  if (caller_pc) FuncEntry(thr, (uptr)caller_pc);
+  bool in_ignored_lib;
+  if (!caller_pc || !libignore()->IsIgnored((uptr)caller_pc, &in_ignored_lib)) {
     MemoryWrite(thr, CALLERPC, (uptr)addr, kSizeLog8);
-  FuncExit(thr);
+  }
+  if (caller_pc) FuncExit(thr);
   thr->external_tag = 0;
 }
 }  // extern "C"
 
 }  // namespace __tsan

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

public:
   void EnableIgnores();
  private:
   ThreadState *const thr_;
   const uptr pc_;
   bool in_ignored_lib_;
   bool ignoring_;
 };
 
+LibIgnore *libignore();
+
 }  // 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; \

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

struct ThreadSignalContext {
   // emptyset and oldset are too big for stack.
   __sanitizer_sigset_t emptyset;
   __sanitizer_sigset_t oldset;
 };
 
 // The object is 64-byte aligned, because we want hot data to be located in
 // a single cache line if possible (it's accessed in every interceptor).
 static ALIGNED(64) char libignore_placeholder[sizeof(LibIgnore)];
-static LibIgnore *libignore() {
+LibIgnore *libignore() {
   return reinterpret_cast<LibIgnore*>(&libignore_placeholder[0]);
 }
 
 void InitializeLibIgnore() {
   const SuppressionContext &supp = *Suppressions();
   const uptr n = supp.SuppressionCount();
   for (uptr i = 0; i < n; i++) {
     const Suppression *s = supp.SuppressionAt(i);

compiler-rt/trunk/test/tsan/Darwin/external-ignore-noninstrumented.cc

+// RUN: %clangxx_tsan -shared %p/external-lib.cc -fno-sanitize=thread -DUSE_TSAN_CALLBACKS \
+// RUN:   -o %t-lib.dylib -install_name @rpath/`basename %t-lib.dylib`
+
+// RUN: %clangxx_tsan -shared %p/external-noninstrumented-module.cc %t-lib.dylib -fno-sanitize=thread \
+// RUN:   -o %t-module.dylib -install_name @rpath/`basename %t-module.dylib`
+
+// RUN: %clangxx_tsan %s %t-module.dylib -o %t
+// RUN: %run %t 2>&1 | FileCheck %s
+
+#include <stdio.h>
+
+extern "C" void NonInstrumentedModule();
+int main(int argc, char *argv[]) {
+  NonInstrumentedModule();
+  fprintf(stderr, "Done.\n");
+}
+
+// CHECK-NOT: WARNING: ThreadSanitizer
+// CHECK: Done.

compiler-rt/trunk/test/tsan/Darwin/external-lib.cc

+// This file is used from other tests.
+// RUN: true
+
+#include <dlfcn.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+struct MyObject;
+typedef MyObject *MyObjectRef;
+extern "C" {
+  void InitializeLibrary();
+  MyObject *ObjectCreate();
+  long ObjectRead(MyObject *);
+  void ObjectWrite(MyObject *, long);
+  void ObjectWriteAnother(MyObject *, long);
+}
+
+struct MyObject {
+  long _val;
+  long _another;
+};
+
+#if defined(USE_TSAN_CALLBACKS)
+static void *tag;
+void *(*callback_register_tag)(const char *object_type);
+void *(*callback_assign_tag)(void *addr, void *tag);
+void (*callback_read)(void *addr, void *caller_pc, void *tag);
+void (*callback_write)(void *addr, void *caller_pc, void *tag);
+#endif
+
+void InitializeLibrary() {
+#if defined(USE_TSAN_CALLBACKS)
+  callback_register_tag = (decltype(callback_register_tag))dlsym(RTLD_DEFAULT, "__tsan_external_register_tag");
+  callback_assign_tag = (decltype(callback_assign_tag))dlsym(RTLD_DEFAULT, "__tsan_external_assign_tag");
+  callback_read = (decltype(callback_read))dlsym(RTLD_DEFAULT, "__tsan_external_read");
+  callback_write = (decltype(callback_write))dlsym(RTLD_DEFAULT, "__tsan_external_write");
+  tag = callback_register_tag("MyLibrary::MyObject");
+#endif
+}
+
+MyObject *ObjectCreate() {
+  MyObject *ref = (MyObject *)malloc(sizeof(MyObject));
+#if defined(USE_TSAN_CALLBACKS)
+  callback_assign_tag(ref, tag);
+#endif
+  return ref;
+}
+
+long ObjectRead(MyObject *ref) {
+#if defined(USE_TSAN_CALLBACKS)
+  callback_read(ref, __builtin_return_address(0), tag);
+#endif
+  return ref->_val;
+}
+
+void ObjectWrite(MyObject *ref, long val) {
+#if defined(USE_TSAN_CALLBACKS)
+  callback_write(ref, __builtin_return_address(0), tag);
+#endif
+  ref->_val = val;
+}
+
+void ObjectWriteAnother(MyObject *ref, long val) {
+#if defined(USE_TSAN_CALLBACKS)
+  callback_write(ref, __builtin_return_address(0), tag);
+#endif
+  ref->_another = val;
+}

compiler-rt/trunk/test/tsan/Darwin/external-noninstrumented-module.cc

+// This file is used from other tests.
+// RUN: true
+
+#include <thread>
+
+#include <stdio.h>
+#include <stdlib.h>
+
+struct MyObject;
+typedef MyObject *MyObjectRef;
+extern "C" {
+  void InitializeLibrary();
+  MyObject *ObjectCreate();
+  long ObjectRead(MyObject *);
+  void ObjectWrite(MyObject *, long);
+  void ObjectWriteAnother(MyObject *, long);
+}
+
+extern "C" void NonInstrumentedModule() {
+  InitializeLibrary();
+  
+  MyObjectRef ref = ObjectCreate();
+  std::thread t1([ref]{ ObjectWrite(ref, 42); });
+  std::thread t2([ref]{ ObjectWrite(ref, 43); });
+  t1.join();
+  t2.join();
+}

compiler-rt/trunk/test/tsan/Darwin/external.cc

-// RUN: %clangxx_tsan %s -shared -DSHARED_LIB \
+// RUN: %clangxx_tsan %p/external-lib.cc -shared \
 // RUN:                               -o %t-lib-instrumented.dylib \
 // RUN:   -install_name @rpath/`basename %t-lib-instrumented.dylib`
 
-// RUN: %clangxx_tsan %s -shared -DSHARED_LIB -fno-sanitize=thread \
+// RUN: %clangxx_tsan %p/external-lib.cc -shared -fno-sanitize=thread \
 // RUN:                               -o %t-lib-noninstrumented.dylib \
 // RUN:   -install_name @rpath/`basename %t-lib-noninstrumented.dylib`
 
-// RUN: %clangxx_tsan %s -shared -DSHARED_LIB -fno-sanitize=thread -DUSE_TSAN_CALLBACKS \
+// RUN: %clangxx_tsan %p/external-lib.cc -shared -fno-sanitize=thread -DUSE_TSAN_CALLBACKS \
 // RUN:                               -o %t-lib-noninstrumented-callbacks.dylib \
 // RUN:   -install_name @rpath/`basename %t-lib-noninstrumented-callbacks.dylib`
 
 // RUN: %clangxx_tsan %s %t-lib-instrumented.dylib -o %t-lib-instrumented
 // RUN: %clangxx_tsan %s %t-lib-noninstrumented.dylib -o %t-lib-noninstrumented
 // RUN: %clangxx_tsan %s %t-lib-noninstrumented-callbacks.dylib -o %t-lib-noninstrumented-callbacks
 
 // RUN: %deflake %run %t-lib-instrumented              2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK --check-prefix=TEST1
 // RUN:          %run %t-lib-noninstrumented           2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK --check-prefix=TEST2
 // RUN: %deflake %run %t-lib-noninstrumented-callbacks 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK --check-prefix=TEST3
 
 #include <thread>
 
-#include <dlfcn.h>
-#include <pthread.h>
 #include <stdio.h>
 #include <stdlib.h>
 
 struct MyObject;
 typedef MyObject *MyObjectRef;
 extern "C" {
   void InitializeLibrary();
   MyObject *ObjectCreate();
   long ObjectRead(MyObject *);
   void ObjectWrite(MyObject *, long);
   void ObjectWriteAnother(MyObject *, long);
 }
 
-#if defined(SHARED_LIB)
-
-struct MyObject {
-  long _val;
-  long _another;
-};
-
-#if defined(USE_TSAN_CALLBACKS)
-static void *tag;
-void *(*callback_register_tag)(const char *object_type);
-void *(*callback_assign_tag)(void *addr, void *tag);
-void (*callback_read)(void *addr, void *caller_pc, void *tag);
-void (*callback_write)(void *addr, void *caller_pc, void *tag);
-#endif
-
-void InitializeLibrary() {
-#if defined(USE_TSAN_CALLBACKS)
-  callback_register_tag = (decltype(callback_register_tag))dlsym(RTLD_DEFAULT, "__tsan_external_register_tag");
-  callback_assign_tag = (decltype(callback_assign_tag))dlsym(RTLD_DEFAULT, "__tsan_external_assign_tag");
-  callback_read = (decltype(callback_read))dlsym(RTLD_DEFAULT, "__tsan_external_read");
-  callback_write = (decltype(callback_write))dlsym(RTLD_DEFAULT, "__tsan_external_write");
-  tag = callback_register_tag("MyLibrary::MyObject");
-#endif
-}
-
-MyObject *ObjectCreate() {
-  MyObject *ref = (MyObject *)malloc(sizeof(MyObject));
-#if defined(USE_TSAN_CALLBACKS)
-  callback_assign_tag(ref, tag);
-#endif
-  return ref;
-}
-
-long ObjectRead(MyObject *ref) {
-#if defined(USE_TSAN_CALLBACKS)
-  callback_read(ref, __builtin_return_address(0), tag);
-#endif
-  return ref->_val;
-}
-
-void ObjectWrite(MyObject *ref, long val) {
-#if defined(USE_TSAN_CALLBACKS)
-  callback_write(ref, __builtin_return_address(0), tag);
-#endif
-  ref->_val = val;
-}
-
-void ObjectWriteAnother(MyObject *ref, long val) {
-#if defined(USE_TSAN_CALLBACKS)
-  callback_write(ref, __builtin_return_address(0), tag);
-#endif
-  ref->_another = val;
-}
-
-#else  // defined(SHARED_LIB)
-
 int main(int argc, char *argv[]) {
   InitializeLibrary();
   
   {
     MyObjectRef ref = ObjectCreate();
     std::thread t1([ref]{ ObjectRead(ref); });
     std::thread t2([ref]{ ObjectRead(ref); });
     t1.join();
int main(int argc, char *argv[]) {
   // TEST3: Previous mutating access of object MyLibrary::MyObject at
   // TEST3: {{ObjectWrite|ObjectWriteAnother}}
   // TEST3: Location is MyLibrary::MyObject object of size 16 at
   // TEST3: {{ObjectCreate}}
 
   fprintf(stderr, "WW test done\n");
   // CHECK: WW test done
 }
-
-#endif  // defined(SHARED_LIB)