[PATCH] tsan: ignore interceptors coming from specified libraries

[Dmitry Vyukov]

  PTAL


================
Comment at: sanitizer_common/sanitizer_libignore.h:27
@@ +26,3 @@
+
+class ALIGNED(64) LibIgnore {
+ public:
----------------
Kostya Serebryany wrote:
> Will this work on Windows? (Check with Timur)
It's not used on windows.

================
Comment at: sanitizer_common/sanitizer_libignore.h:29
@@ +28,3 @@
+ public:
+  LibIgnore(LinkerInitialized);
+
----------------
Kostya Serebryany wrote:
> If this is linker-initialized, the empty body should be here. No?
It's in cc file.

================
Comment at: sanitizer_common/sanitizer_libignore.h:41
@@ +40,3 @@
+ private:
+  struct Lib {
+    char *name;
----------------
Alexey Samsonov wrote:
> Consider reusing LoadedModule from sanitizer_common.h
LoadedModule is too general and slow for this use case.

================
Comment at: sanitizer_common/sanitizer_libignore.cc:21
@@ +20,3 @@
+  CHECK_EQ(count_, 0);
+  const InternalMmapVector<Suppression> *list = supp.GetSuppressions();
+  for (uptr i = 0; i < list->size(); i++) {
----------------
Kostya Serebryany wrote:
> I'd prefer to have size() and get(i) methods in supp instead of giving away the guts.
Done

================
Comment at: sanitizer_common/sanitizer_suppressions.h:46
@@ -44,2 +45,3 @@
   void GetMatched(InternalMmapVector<Suppression *> *matched);
+  const InternalMmapVector<Suppression> *GetSuppressions() const;
 
----------------
Alexey Samsonov wrote:
> Can we do better than return the class internals to the caller?
Done

================
Comment at: tsan/lit_tests/ignore_lib0.cc:1
@@ +1,2 @@
+// RUN: %clangxx_tsan -O1 %s -DLIB -fPIC -fno-sanitize=thread -shared -o ./Output/libignore_lib0.so
+// RUN: %clangxx_tsan -O1 %s -L./Output/ -lignore_lib0 -o %t
----------------
Alexey Samsonov wrote:
> Please use %p and %t wildcards (see asan/lit_tests/TestCases/dlclose-test.cc as an example)
I need to know the exact library name to put into suppressions file, I do not see how to handle this.

================
Comment at: tsan/lit_tests/ignore_lib0.cc:4
@@ +3,3 @@
+// RUN: LD_LIBRARY_PATH=./Output/ not %t 2>&1 | FileCheck %s --check-prefix=CHECK-NOSUPP
+// RUN: LD_LIBRARY_PATH=./Output/ TSAN_OPTIONS=suppressions=%s.supp not %t 2>&1 | FileCheck %s --check-prefix=CHECK-WITHSUPP
+
----------------
Alexey Samsonov wrote:
> TSAN_OPTIONS=$TSAN_OPTIONS:....
Done

================
Comment at: tsan/rtl/tsan_interceptors.cc:130
@@ -128,13 +129,3 @@
 
-// Used to ignore interceptors coming directly from libjvm.so.
-atomic_uintptr_t libjvm_begin;
-atomic_uintptr_t libjvm_end;
-
-static bool libjvm_check(uptr pc) {
-  uptr begin = atomic_load(&libjvm_begin, memory_order_relaxed);
-  if (begin != 0 && pc >= begin) {
-    uptr end = atomic_load(&libjvm_end, memory_order_relaxed);
-    if (end != 0 && pc < end)
-      return true;
-  }
-  return false;
+static uptr libignore_placeholder[sizeof(LibIgnore) / sizeof(uptr) + 1];
+static LibIgnore *libignore() {
----------------
Kostya Serebryany wrote:
> Alexey Samsonov wrote:
> > * do you need to use ALIGNED(64) here?
> > * why not static char [sizeof(LibIgnore)]?
> shouldn't this be aligned(64)? 
> do you need to use ALIGNED(64) here?

The class itself is aligned.

================
Comment at: tsan/rtl/tsan_interceptors.cc:130
@@ -128,13 +129,3 @@
 
-// Used to ignore interceptors coming directly from libjvm.so.
-atomic_uintptr_t libjvm_begin;
-atomic_uintptr_t libjvm_end;
-
-static bool libjvm_check(uptr pc) {
-  uptr begin = atomic_load(&libjvm_begin, memory_order_relaxed);
-  if (begin != 0 && pc >= begin) {
-    uptr end = atomic_load(&libjvm_end, memory_order_relaxed);
-    if (end != 0 && pc < end)
-      return true;
-  }
-  return false;
+static uptr libignore_placeholder[sizeof(LibIgnore) / sizeof(uptr) + 1];
+static LibIgnore *libignore() {
----------------
Dmitry Vyukov wrote:
> Kostya Serebryany wrote:
> > Alexey Samsonov wrote:
> > > * do you need to use ALIGNED(64) here?
> > > * why not static char [sizeof(LibIgnore)]?
> > shouldn't this be aligned(64)? 
> > do you need to use ALIGNED(64) here?
> 
> The class itself is aligned.
> why not static char [sizeof(LibIgnore)]?

That does not provide necessary storage alignment -> undefined behavior.

================
Comment at: tsan/rtl/tsan_interceptors.cc:130
@@ -128,13 +129,3 @@
 
-// Used to ignore interceptors coming directly from libjvm.so.
-atomic_uintptr_t libjvm_begin;
-atomic_uintptr_t libjvm_end;
-
-static bool libjvm_check(uptr pc) {
-  uptr begin = atomic_load(&libjvm_begin, memory_order_relaxed);
-  if (begin != 0 && pc >= begin) {
-    uptr end = atomic_load(&libjvm_end, memory_order_relaxed);
-    if (end != 0 && pc < end)
-      return true;
-  }
-  return false;
+static uptr libignore_placeholder[sizeof(LibIgnore) / sizeof(uptr) + 1];
+static LibIgnore *libignore() {
----------------
Dmitry Vyukov wrote:
> Dmitry Vyukov wrote:
> > Kostya Serebryany wrote:
> > > Alexey Samsonov wrote:
> > > > * do you need to use ALIGNED(64) here?
> > > > * why not static char [sizeof(LibIgnore)]?
> > > shouldn't this be aligned(64)? 
> > > do you need to use ALIGNED(64) here?
> > 
> > The class itself is aligned.
> > why not static char [sizeof(LibIgnore)]?
> 
> That does not provide necessary storage alignment -> undefined behavior.
> shouldn't this be aligned(64)?

The class itself is aligned.

================
Comment at: tsan/rtl/tsan_interceptors.cc:169
@@ -171,1 +168,3 @@
+    , in_rtl_(thr->in_rtl)
+    , in_ignored_lib_() {
   if (thr_->in_rtl == 0) {
----------------
Alexey Samsonov wrote:
> in_ingored_lib_(false)
why?

================
Comment at: tsan/rtl/tsan_rtl.cc:216
@@ -215,2 +215,3 @@
   InitializeSuppressions();
 #ifndef TSAN_GO
+  InitializeLibIgnore();
----------------
Kostya Serebryany wrote:
> Two #ifndef TSAN_GO in a row? 
> 
> Also, can we start using   if (!TSAN_GO) instead ?
> Two #ifndef TSAN_GO in a row?

Done

================
Comment at: tsan/rtl/tsan_rtl.cc:216
@@ -215,2 +215,3 @@
   InitializeSuppressions();
 #ifndef TSAN_GO
+  InitializeLibIgnore();
----------------
Dmitry Vyukov wrote:
> Kostya Serebryany wrote:
> > Two #ifndef TSAN_GO in a row? 
> > 
> > Also, can we start using   if (!TSAN_GO) instead ?
> > Two #ifndef TSAN_GO in a row?
> 
> Done
> Also, can we start using if (!TSAN_GO) instead ?

There is no such function when TSAN_GO, so it may not link.

================
Comment at: tsan/rtl/tsan_rtl.cc:217
@@ -216,1 +216,3 @@
 #ifndef TSAN_GO
+  InitializeLibIgnore();
+#endif
----------------
Alexey Samsonov wrote:
> Why not put this under a single ifndef?
Done


http://llvm-reviews.chandlerc.com/D1808