Add a flag to force tsan's background thread

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

690

Can we avoid the flag?

Is pthread already initialized here? Maybe safer later, with global constructor.

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

690

Can we avoid the flag?

I see another thread, @dvyukov asked for the flag.

Is pthread already initialized here? Maybe safer later, with global constructor.

for context: D55887 fixed Android and NetBSD issue in Asan

[fowles]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

690

As mentioned, the flag was explicitly requested elsewhere. I am open to moving the call to another location if you can suggest something better. I will observe that the test does pass...

[dvyukov]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

690

Yes, I don't know, if enable it by default it will probably break something somewhere out there. I am not a fan of flags myself, but I think it will be safer with a flag.

Vitaly, do you have any other comments? Or should I land this?

[fowles]

I don't have commit privs, so please land it when you both agree.

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

690

No other comments. If D55887 is not concern, then LGTM

[dvyukov]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

690

I guess D55887 is a concern if somebody uses this on NetBSD... and, well, tests will do it, so I guess they will fail.
I think we need to move creation of the thread to a global ctor as D55887 does.

[fowles]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

690

When I make the suggested change to use a global I get a crash

/usr/local/google/home/kfm/dev/llvm/compiler-rt/lib/tsan/rtl/../go/buildgo.sh: line 213: 1320312 Segmentation fault (core dumped) $DIR/test 2> /dev/null
make[3]: * [projects/compiler-rt/lib/tsan/rtl/CMakeFiles/GotsanRuntimeCheck.dir/build.make:72: projects/compiler-rt/lib/tsan/rtl/CMakeFiles/GotsanRuntimeCheck] Error 139
make[2]: * [CMakeFiles/Makefile2:39112: projects/compiler-rt/lib/tsan/rtl/CMakeFiles/GotsanRuntimeCheck.dir/all] Error 2

[dvyukov]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

690

Out it under #if !SANITIZER_GO. Go does not use the background thread.

[fowles]

Updating D115759: Summary: Add a flag to force tsan's background thread

[fowles]

Code updated, PTAL

[vitalybuka]

compiler-rt/test/tsan/force_background_thread.cpp

3

maybe we can add VPrintf into static void *BackgroundThread(void *arg) {
so we can observer it from the test with verbosity=1, and for future debugging.

[dvyukov]

compiler-rt/test/tsan/force_background_thread.cpp

3

It has several VReport(1). One can be triggered if flush_memory_ms is set as well.

[vitalybuka]

compiler-rt/test/tsan/force_background_thread.cpp

3

if it's easy to trigger it would be nice to be used in this test to make FileCheck see a difference force_background_thread=0 vs force_background_thread=1

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_flags.inc

49

[vitalybuka]

compiler-rt/test/tsan/force_background_thread.cpp

3

@dvyukov if it's OK to you I can update and land the patch?

// RUN: %clangxx_tsan -O1 %s -o %t
// RUN: %deflake %env_tsan_opts=force_background_thread=0:verbosity=1:memory_limit_mb=1000 %run %t 2>&1 | FileCheck %s --implicit-check-not "memory flush check"
// RUN: %deflake %env_tsan_opts=force_background_thread=1:verbosity=1:memory_limit_mb=1000 %run %t 2>&1 | FileCheck %s --check-prefixes=CHECK,THREAD
// RUN: %deflake %env_tsan_opts=force_background_thread=0:verbosity=1:memory_limit_mb=1000 %run %t 1 2>&1 | FileCheck %s --check-prefixes=CHECK,THREAD
#include "test.h"

void *Thread(void *a) {
  return nullptr;
}

int main(int argc, char* argv[]) {
  if (argc > 1) {
   pthread_t t;
   pthread_create(&t, nullptr, Thread, nullptr);
   void* p;
   pthread_join(t, &p);
  }
  sleep(3);
  return 1;
}

// CHECK: Running under ThreadSanitizer
// THREAD: ThreadSanitizer: memory flush check

[dvyukov]

compiler-rt/test/tsan/force_background_thread.cpp

3

if it's OK to you I can update and land the patch?

Please do.

[vitalybuka]

Suppress -Wglobal-constructors
add VPrint checks in test

[arphaman]

@vitalybuka This change caused a regression on Darwin CI for tsan -,
This is the CI job: https://green.lab.llvm.org/green/job/clang-stage1-RA, and the build: https://green.lab.llvm.org/green/job/clang-stage1-RA/26354/
ThreadSanitizer-x86_64.Darwin.dlopen.cpp is now failing because the initializer that you added runs before flags is initialized when it's being dlopened . Could you take a look at this issue?

[yln]

In D115759#3203532, @arphaman wrote:

@vitalybuka This change caused a regression on Darwin CI for tsan -,
This is the CI job: https://green.lab.llvm.org/green/job/clang-stage1-RA, and the build: https://green.lab.llvm.org/green/job/clang-stage1-RA/26354/
ThreadSanitizer-x86_64.Darwin.dlopen.cpp is now failing because the initializer that you added runs before flags is initialized when it's being dlopened . Could you take a look at this issue?

More context: it happens when we dlopen() uninstrumented code from instrumented code. In that case we always exit, but we used to print a useful error message. Now we just crash because ctx is still nullptr, when we ask flags()->force_background_thread from the global initializer.
Failing test: compiler-rt/test/tsan/Darwin/dlopen.cpp

I am not sure if this is a Darwin-specific issue or if it's just that we don't have an analogous test for Linux. @vitalybuka, can you check?

In any case, I am fine with just #ifdefing out the global initializer (i.e., not supporting the flag on Darwin) and going with whatever the default is. I think we are happy with both the current or "eager background thread" creation. It's just that accessing the flag value from the global initializer doesn't work in all cases.

[vitalybuka]

In D115759#3203584, @yln wrote:

In D115759#3203532, @arphaman wrote:

@vitalybuka This change caused a regression on Darwin CI for tsan -,
This is the CI job: https://green.lab.llvm.org/green/job/clang-stage1-RA, and the build: https://green.lab.llvm.org/green/job/clang-stage1-RA/26354/
ThreadSanitizer-x86_64.Darwin.dlopen.cpp is now failing because the initializer that you added runs before flags is initialized when it's being dlopened . Could you take a look at this issue?

More context: it happens when we dlopen() uninstrumented code from instrumented code. In that case we always exit, but we used to print a useful error message. Now we just crash because ctx is still nullptr, when we ask flags()->force_background_thread from the global initializer.
Failing test: compiler-rt/test/tsan/Darwin/dlopen.cpp

I am not sure if this is a Darwin-specific issue or if it's just that we don't have an analogous test for Linux. @vitalybuka, can you check?

In any case, I am fine with just #ifdefing out the global initializer (i.e., not supporting the flag on Darwin) and going with whatever the default is. I think we are happy with both the current or "eager background thread" creation. It's just that accessing the flag value from the global initializer doesn't work in all cases.

looking

[vitalybuka]

In D115759#3203584, @yln wrote:

In D115759#3203532, @arphaman wrote:

@vitalybuka This change caused a regression on Darwin CI for tsan -,

Now it fails in the force_background_thread.cpp itself, which I disabled on Darwin. Not sure what is the meaning of this, I have no access to OSX at the moment to debug.
I suspect pthread is not available yet.
https://green.lab.llvm.org/green/job/clang-stage1-RA/26463/consoleFull#946751328254eaf0-7326-4999-85b0-388101f2d404

compiler-rt/lib/tsan/rtl/tsan_flags.inc

TSAN_FLAG(bool, report_signal_unsafe, true,
           "Report violations of async signal-safety "
           "(e.g. malloc() call from a signal handler).")
 TSAN_FLAG(bool, report_atomic_races, true,
           "Report races between atomic and plain memory accesses.")
 TSAN_FLAG(
     bool, force_seq_cst_atomics, false,
     "If set, all atomics are effectively sequentially consistent (seq_cst), "
     "regardless of what user actually specified.")
+TSAN_FLAG(bool, force_background_thread, false,
+          "If set, eagerly launch a background thread for memory reclamation "
+          "instead of waiting for a user call to pthread_create.")
 TSAN_FLAG(bool, halt_on_error, false, "Exit after first reported error.")
 TSAN_FLAG(int, atexit_sleep_ms, 1000,
           "Sleep in main thread before exiting for that many ms "
           "(useful to catch \"at exit\" races).")
 TSAN_FLAG(const char *, profile_memory, "",
           "If set, periodically write memory profile to that file.")
 TSAN_FLAG(int, flush_memory_ms, 0, "Flush shadow memory every X ms.")
 TSAN_FLAG(int, flush_symbolizer_ms, 5000, "Flush symbolizer caches every X ms.")
 TSAN_FLAG(

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

#if TSAN_CONTAINS_UBSAN
   __ubsan::InitAsPlugin();
 #endif
 
 #if !SANITIZER_GO
   Symbolizer::LateInitialize();
   InitializeMemoryProfiler();
 #endif
   ctx->initialized = true;
 
   if (flags()->stop_on_start) {
     Printf("ThreadSanitizer is suspended at startup (pid %d)."
            " Call __tsan_resume().\n",
            (int)internal_getpid());
     while (__tsan_resumed == 0) {}
   }
 
   OnInitialize();
 }
 
+#if !SANITIZER_GO
+#  pragma clang diagnostic push
+// We intentionally use a global constructor to delay the pthread call.
+#  pragma clang diagnostic ignored "-Wglobal-constructors"
+static bool UNUSED __local_tsan_dyninit = [] {
+  if (flags()->force_background_thread)
+    MaybeSpawnBackgroundThread();
+  return false;
+}();
+#  pragma clang diagnostic pop
+#endif
+
 void MaybeSpawnBackgroundThread() {
   // On MIPS, TSan initialization is run before
   // __pthread_initialize_minimal_internal() is finished, so we can not spawn
   // new threads.
 #if !SANITIZER_GO && !defined(__mips__)
   static atomic_uint32_t bg_thread = {};
   if (atomic_load(&bg_thread, memory_order_relaxed) == 0 &&
       atomic_exchange(&bg_thread, 1, memory_order_relaxed) == 0) {

compiler-rt/test/tsan/force_background_thread.cpp

+// RUN: %clangxx_tsan -O1 %s -o %t
+// RUN: %deflake %env_tsan_opts=force_background_thread=0:verbosity=1:memory_limit_mb=1000 %run %t 2>&1 | FileCheck %s --implicit-check-not "memory flush check"
+// RUN: %deflake %env_tsan_opts=force_background_thread=1:verbosity=1:memory_limit_mb=1000 %run %t 2>&1 | FileCheck %s --check-prefixes=CHECK,THREAD
+// RUN: %deflake %env_tsan_opts=force_background_thread=0:verbosity=1:memory_limit_mb=1000 %run %t 1 2>&1 | FileCheck %s --check-prefixes=CHECK,THREAD
+#include "test.h"
+
+void *Thread(void *a) { return nullptr; }
+
+int main(int argc, char *argv[]) {
+  if (argc > 1) {
+    pthread_t t;
+    pthread_create(&t, nullptr, Thread, nullptr);
+    void *p;
+    pthread_join(t, &p);
+  }
+  sleep(3);
+  return 1;
+}
+
+// CHECK: Running under ThreadSanitizer
+// THREAD: ThreadSanitizer: memory flush check