sanitizer_common: split LibIgnore into fast/slow paths

[dvyukov]

Summary

LibIgnore is checked in every interceptor.
Currently it has all logic in the single function
in the header, which makes it uninlinable.
Split it into fast path (no libraries ignored)
and slow path (have ignored libraries).
It makes the fast path inlinable (single load).

[protze.joachim]

This patch seems to break TSAN_OPTION=ignore_noninstrumented_modules=1 behavior. Before this patch, make check-libarcher succeeds, after this patch, the tests fail. As I understand the description, on semantic change is intended? Any idea, what happened here?

make check-libarcher runt the tests for libarcher providing the TsanAnnotations for OpenMP. Since some intercepted pthread calls in libomp are identified as data race, while they are synchronized, we set this environmental variable to suppress analysis of the non-instrumented libomp.

[dvyukov]

I've tried:

$ ninja check-libarcher
ninja: error: unknown target 'check-libarcher'

Please extract what libarcher is doing and add as a tsan unit test. It will help to fix this and prevent future breakages of libarcher.
You can see existing libignore tests here:
https://github.com/llvm/llvm-project/blob/main/compiler-rt/test/tsan/ignore_lib0.cpp

[protze.joachim]

This might have one of two reasons: libarcher is part of the openmp project, so you need to add archer to the enabled projects. Furthermore, libarcher is only supported for linux/macos and I think x86_64.

I'll try to come back with a standalone reproducer.

[dvyukov]

I was actually thinking about reverting this change, because e.g. Java, Mac (and now OpenMP) always require some ignores enabled, so the "fast path" includes checking ranges in these contexts. But this change is definitely positive for synthetic microbenchmarks :)

I have not decided re revert as there were no pressing need to decide quickly. And a test would be good anyway.

[protze.joachim]

https://bugs.llvm.org/show_bug.cgi?id=51205 reports a small reproducer of the issue:

int main()
{
  #pragma omp parallel
  {  }
  #pragma omp parallel
  {  }
}

compiled with

clang -fopenmp -fsanitize=thread main.c

execute with

env OMP_NUM_THREADS=4 ./a.out

The application code does not have any data race. The application does not even access any data. Nevertheless threadsanitizer reports data race for pthread functions intercepted from libomp:

WARNING: ThreadSanitizer: data race (pid=8344)
  Atomic read of size 1 at 0x7b6800002f40 by main thread:
    #0 pthread_mutex_lock sanitizer_common_interceptors.inc:4249:215 (a.out+0x464758)
    #1 void __kmp_resume_64<false, true>(int, kmp_flag_64<false, true>*) <null> (libomp.so+0x8ef94)

  Previous write of size 1 at 0x7b6800002f40 by thread T4:
    #0 pthread_mutex_init tsan_interceptors_posix.cpp:1269:215 (a.out+0x447bdf)
    #1 __kmp_suspend_initialize_thread <null> (libomp.so+0x8e19e)

TSAN_OPTIONS='ignore_noninstrumented_modules=1' previously helped to suppress such reports, so that we actually suggest to set this variable and print a warning, if the variable is not set at runtime (Warning: please export TSAN_OPTIONS='ignore_noninstrumented_modules=1' to avoid false positive reports from the OpenMP runtime!)

From my perspective, it would make sense to name-shift the intercepted functions during TSan compiler instrumentation. A runtime flag could then easily limit the handling of intercepted functions to name-shifted function.

[protze.joachim]

Here is my reproducer:

lib.cpp (Tsan detects a race between malloc and pthread_mutex_lock):

#include <thread>
#include <mutex>
#include <atomic>

struct globals{
  std::atomic<int> g_inited{0};
  std::mutex* g_mutex;
} g;

void worker(){
  while(g.g_inited<1);
  std::lock_guard<std::mutex> lock(*g.g_mutex);
}

extern "C"
int init(){
  auto t1 = std::thread(worker);
  g.g_mutex = new std::mutex();
  g.g_inited=1;
  t1.join();
  return 0;
}

main.c:

int init();

int main(){
  return init();
}

build (the rpath part might be linux-specific laziness to allow finding the shared library in the current directory):

clang++ -g -fPIC -shared -pthread lib.cpp -o libtest.so
clang -g -fsanitize=thread -L. -Wl,--rpath,. -ltest main.c

exec:

TSAN_OPTIONS='ignore_noninstrumented_modules=1' ./a.out

[protze.joachim]

After revisiting the existing test cases, I guess it would be sufficient to add another runline:

// RUN: echo running with generic suppression of noninstrumented code:
// RUN: env  %env_tsan_opts=ignore_noninstrumented_modules=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-WITHSUPP

[dvyukov]

In D105719#2902718, @protze.joachim wrote:

After revisiting the existing test cases, I guess it would be sufficient to add another runline:

// RUN: echo running with generic suppression of noninstrumented code:
// RUN: env  %env_tsan_opts=ignore_noninstrumented_modules=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-WITHSUPP

Somehow we did not have any portable tests with ignore_noninstrumented_modules=1 (only Darwin ones).
I've sent https://reviews.llvm.org/D106855 with a revert. Thanks for tracking down the reproducer.

[protze.joachim]

The reason for using ignore_noninstrumented_modules=1 is the general interception of pthread / libc functions.
If the instrumentation pass would name shift the functions during compilation, turning off the analysis for non-name shifted functions, i.e., function calls from non-instrumented code, could be done at runtime without looking at the caller.

compiler-rt/lib/sanitizer_common/sanitizer_libignore.h

public:
   void OnLibraryUnloaded();
 
   // Checks whether the provided PC belongs to one of the ignored libraries or
   // the PC should be ignored because it belongs to an non-instrumented module
   // (when ignore_noninstrumented_modules=1). Also returns true via
   // "pc_in_ignored_lib" if the PC is in an ignored library, false otherwise.
   bool IsIgnored(uptr pc, bool *pc_in_ignored_lib) const;
 
-  // Checks whether the provided PC belongs to an instrumented module.
-  bool IsPcInstrumented(uptr pc) const;
-
  private:
   struct Lib {
     char *templ;
     char *name;
     char *real_name;  // target of symlink
     bool loaded;
   };
 
   struct LibCodeRange {
     uptr begin;
     uptr end;
   };
 
+  // Checks whether the provided PC belongs to an instrumented module.
+  bool IsPcInstrumented(uptr pc) const;
+  bool IsIgnoredSlow(uptr pc, bool *pc_in_ignored_lib) const;
+
   inline bool IsInRange(uptr pc, const LibCodeRange &range) const {
     return (pc >= range.begin && pc < range.end);
   }
 
   static const uptr kMaxIgnoredRanges = 128;
   static const uptr kMaxInstrumentedRanges = 1024;
   static const uptr kMaxLibs = 1024;
 
   // Hot part:
+  atomic_uintptr_t enabled_;
+
   atomic_uintptr_t ignored_ranges_count_;
   LibCodeRange ignored_code_ranges_[kMaxIgnoredRanges];
 
   atomic_uintptr_t instrumented_ranges_count_;
   LibCodeRange instrumented_code_ranges_[kMaxInstrumentedRanges];
 
   // Cold part:
   BlockingMutex mutex_;
   uptr count_;
   Lib libs_[kMaxLibs];
   bool track_instrumented_libs_;
 
   // Disallow copying of LibIgnore objects.
   LibIgnore(const LibIgnore&);  // not implemented
   void operator = (const LibIgnore&);  // not implemented
 };
 
-inline bool LibIgnore::IsIgnored(uptr pc, bool *pc_in_ignored_lib) const {
-  const uptr n = atomic_load(&ignored_ranges_count_, memory_order_acquire);
-  for (uptr i = 0; i < n; i++) {
-    if (IsInRange(pc, ignored_code_ranges_[i])) {
-      *pc_in_ignored_lib = true;
-      return true;
-    }
-  }
-  *pc_in_ignored_lib = false;
-  if (track_instrumented_libs_ && !IsPcInstrumented(pc))
-    return true;
-  return false;
-}
-
-inline bool LibIgnore::IsPcInstrumented(uptr pc) const {
-  const uptr n = atomic_load(&instrumented_ranges_count_, memory_order_acquire);
-  for (uptr i = 0; i < n; i++) {
-    if (IsInRange(pc, instrumented_code_ranges_[i]))
-      return true;
-  }
+ALWAYS_INLINE
+bool LibIgnore::IsIgnored(uptr pc, bool *pc_in_ignored_lib) const {
+  if (LIKELY(atomic_load(&enabled_, memory_order_acquire) == 0))
     return false;
+  return IsIgnoredSlow(pc, pc_in_ignored_lib);
 }
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_LIBIGNORE_H

compiler-rt/lib/sanitizer_common/sanitizer_libignore.cpp

for (const auto &mod : modules) {
         lib->loaded = true;
         lib->name = internal_strdup(mod.full_name());
         const uptr idx =
             atomic_load(&ignored_ranges_count_, memory_order_relaxed);
         CHECK_LT(idx, ARRAY_SIZE(ignored_code_ranges_));
         ignored_code_ranges_[idx].begin = range.beg;
         ignored_code_ranges_[idx].end = range.end;
         atomic_store(&ignored_ranges_count_, idx + 1, memory_order_release);
+        atomic_store(&enabled_, 1, memory_order_release);
         break;
       }
     }
     if (lib->loaded && !loaded) {
       Report("%s: library '%s' that was matched against called_from_lib"
              " suppression '%s' is unloaded\n",
              SanitizerToolName, lib->name, lib->templ);
       Die();
for (const auto &mod : modules) {
                 range.beg, range.end, mod.full_name());
         const uptr idx =
             atomic_load(&instrumented_ranges_count_, memory_order_relaxed);
         CHECK_LT(idx, ARRAY_SIZE(instrumented_code_ranges_));
         instrumented_code_ranges_[idx].begin = range.beg;
         instrumented_code_ranges_[idx].end = range.end;
         atomic_store(&instrumented_ranges_count_, idx + 1,
                      memory_order_release);
+        atomic_store(&enabled_, 1, memory_order_release);
       }
     }
   }
 }
 
 void LibIgnore::OnLibraryUnloaded() {
   OnLibraryLoaded(nullptr);
 }
 
+bool LibIgnore::IsIgnoredSlow(uptr pc, bool *pc_in_ignored_lib) const {
+  const uptr n = atomic_load(&ignored_ranges_count_, memory_order_acquire);
+  for (uptr i = 0; i < n; i++) {
+    if (IsInRange(pc, ignored_code_ranges_[i])) {
+      *pc_in_ignored_lib = true;
+      return true;
+    }
+  }
+  *pc_in_ignored_lib = false;
+  if (track_instrumented_libs_ && !IsPcInstrumented(pc))
+    return true;
+  return false;
+}
+
+bool LibIgnore::IsPcInstrumented(uptr pc) const {
+  const uptr n = atomic_load(&instrumented_ranges_count_, memory_order_acquire);
+  for (uptr i = 0; i < n; i++) {
+    if (IsInRange(pc, instrumented_code_ranges_[i]))
+      return true;
+  }
+  return false;
+}
+
 } // namespace __sanitizer
 
 #endif  // SANITIZER_FREEBSD || SANITIZER_LINUX || SANITIZER_MAC ||
         // SANITIZER_NETBSD