[PATCH] tsan: ignore interceptors coming from specified libraries

[Dmitry Vyukov]

================
Comment at: sanitizer_common/sanitizer_libignore.h:27
@@ +26,3 @@
+
+class ALIGNED(64) LibIgnore {
+ public:
----------------
Alexey Samsonov wrote:
> Dmitry Vyukov wrote:
> > Kostya Serebryany wrote:
> > > Will this work on Windows? (Check with Timur)
> > It's not used on windows.
> It is still compiled on Windows.
Done
Added #if SANITIZER_LINUX

================
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:
> Dmitry Vyukov wrote:
> > Alexey Samsonov wrote:
> > > in_ingored_lib_(false)
> > why?
> I like to actually see the default value (and notice that in_ignored_lib_ is a bool). Not sure if it's consistent with conventions you use in TSan code.
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() {
----------------
Alexey Samsonov wrote:
> Dmitry Vyukov wrote:
> > 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.
> This deserves a careful comment or maybe even compiler check.
Added a comment above the class definition.

================
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:
> Dmitry Vyukov wrote:
> > 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.
> You can use %T wildcard for a temporary directory.
Done

================
Comment at: tsan/lit_tests/ignore_lib1.cc:30
@@ +29,3 @@
+
+#include <pthread.h>
+#include <string.h>
----------------
Alexey Samsonov wrote:
> Wait, is this copied from ingore_lib0.cc? I think it's better to put this source under lit_tests/Helpers directory - see the way it's done in ASan.
Done

================
Comment at: msan/msan_interceptors.cc:1133
@@ -1132,2 +1132,3 @@
   ctx = (void *)&msan_ctx;                                    \
+  (void)ctx;                                                  \
   InterceptorScope interceptor_scope;                         \
----------------
Alexander Potapenko wrote:
> IIUC this file doesn't belong to this CL.
> If so, please commit it separately.
this file was removed from this CL

================
Comment at: sanitizer_common/sanitizer_common_interceptors.inc:1142
@@ -1141,3 +1141,3 @@
 
-INTERCEPTOR(struct __sanitizer_hostent *, gethostent) {
+INTERCEPTOR(struct __sanitizer_hostent *, gethostent, int fake) {
   void *ctx;
----------------
Alexander Potapenko wrote:
> Do these interceptors belong to this CL?
this file was removed from this CL

================
Comment at: sanitizer_common/sanitizer_libignore.h:35
@@ +34,3 @@
+  // Must be called after a new dynamic library is loaded.
+  void OnLibraryLoaded();
+
----------------
Alexander Potapenko wrote:
> Shouldn't we support dlclose()? Otherwise you may end up having overlapping and/or nesting ranges.
Done
See OnLibraryUnloaded

================
Comment at: sanitizer_common/sanitizer_libignore.h:10
@@ +9,3 @@
+//
+// LibIgnore allows to ignore all interceptors called from a particular set
+// of dynamic libraries. LibIgnore remembers all "called_from_lib" suppressions
----------------
Alexander Potapenko wrote:
> I suggest 'IgnoreLib' or anything else instead of 'LibIgnore', since 'libignore' associates with 'library named ignore'.
IgnoreLib is not much better, it's still Library if you read it this way

================
Comment at: sanitizer_common/sanitizer_libignore.cc:38
@@ +37,3 @@
+  MemoryMappingLayout proc_maps(/*cache_enabled*/false);
+  InternalMmapVector<char> fn(4096);
+  fn.push_back(0);
----------------
Alexey Samsonov wrote:
> Why not InternalScopedBuffer?
Done

================
Comment at: sanitizer_common/sanitizer_libignore.cc:44
@@ +43,3 @@
+    uptr b, e, off, prot;
+    while (proc_maps.Next(&b, &e, &off, &fn[0], fn.capacity(), &prot)) {
+      if ((prot & MemoryMappingLayout::kProtectionExecute) != 0
----------------
Alexey Samsonov wrote:
> I think that if you change the order of loops (scan ProcSelfMaps and match its contents against all libs), you can make the code more testable 
how can I make it more testable?

================
Comment at: sanitizer_common/sanitizer_libignore.cc:48
@@ +47,3 @@
+        if (loaded[i]) {
+          Printf("%s: called_from_lib suppression '%s' is matched against"
+              " 2 libraries: '%s' and '%s'\n",
----------------
Alexey Samsonov wrote:
> s/Printf/Report
Done

================
Comment at: sanitizer_common/sanitizer_libignore.cc:65
@@ +64,3 @@
+    if (lib->loaded && !loaded[i]) {
+      Printf("%s: library '%s' that was matched against called_from_lib"
+          " suppression '%s' is unloaded\n",
----------------
Alexey Samsonov wrote:
> Are you sure we want to report it?
another library will be ignored in this case, which is bad
I do not want to handle library unloading, so I just check that it does not happen


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