tsan: fix pthread_detach with called_from_lib suppressions

[dvyukov]

Summary

Generally we ignore interceptors coming from called_from_lib-suppressed libraries.
However, we must not ignore critical interceptors like e.g. pthread_create,
otherwise runtime will lost track of threads.
pthread_detach is one of these interceptors we should not ignore as it affects
thread states and behavior of pthread_join which we don't ignore as well.
Currently we can produce very obscure false positives. For more context see:
https://groups.google.com/forum/#!topic/thread-sanitizer/ecH2P0QUqPs
The added test captures this pattern.

While we are here rename ThreadTid to ThreadConsumeTid to make it clear that
it's not just a "getter", it resets user_id to 0. This lead to confusion recently.

[dvyukov]

clang-format

[dvyukov]

Vitaly, ping.

[yln]

Good changes, thanks! LGTM with a few small suggestions from my side. Last word of course by @vitalybuka.

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

778

Thanks for this as well! :)

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

313

This preserves the original semantics. Should we consider asserting that we found a thread (with an unique uid)? None of the callers seem to deal with the error case (when we return kUnknownTid here).

compiler-rt/test/tsan/ignore_lib6.cpp

56

Maybe just check absence of any report? CHECK-NOT: WARNING: ThreadSanitizer:. Also: lately, I have been using --implicit-check-not='ThreadSanitizer' at the FileCheck invocation. Do you guys think that is a good pattern? It results in stricter tests, e.g., it would catch a false report after DONE here.

62

Would it be possible (without heroic effort) to express this test without sleeping?

[dvyukov]

changed warning checking in test
rebased

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

313

There is checking later.
If pthread_t is bogus then pthread_join should fail as well, in such case pthread_join interceptor will not use kUnknownTid.
Otherwise it will be passed to ThreadJoin, which has checks for tid value in the beginning.
Crashing/producing warning on invalid argument will change behavior of programs, without tsan pthread_joing should just return an error. Arguably programs should not do it, but deploying it now may be hard. I don't want to touch this just as a side effect of completely unrelated bug fix.

compiler-rt/test/tsan/ignore_lib6.cpp

56

Changed this to "CHECK-NOT: WARNING: ThreadSanitizer:".
I agree that lots of our checks don't capture everything we want to check. E.g. as you noted race after DONE.
I vaguely remember that in some cases tsan may print some messages that include "ThreadSanitizer", e.g. when we switch stack from unlimited to limited and re-exec (?). So --implicit-check-not='ThreadSanitizer' may break some bots. Need to be careful. I would prefer to not bundle this here.

62

We need a detached thread to actually terminate.
One way would be to poll procfs. Another would be to actually ensure that we are getting reused pthread_t's. But I would qualify this as half-heroic :)
procfs is linux-specific and also depends on particular distro. Reuse of pthread_t is not portable either, we may loop forever without getting any reuse.
sleep is just 1 line. Good portability. And I checked that it allows to catch the bug with high probability.

[dvyukov]

Merged as https://github.com/llvm/llvm-project/commit/2dcbdba85406e498b885570c05e9573ddc3e4a81

[yln]

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

313

Got it, thanks for explaining.

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

TSAN_INTERCEPTOR(int, pthread_create,
   }
   if (attr == &myattr)
     pthread_attr_destroy(&myattr);
   return res;
 }
 
 TSAN_INTERCEPTOR(int, pthread_join, void *th, void **ret) {
   SCOPED_INTERCEPTOR_RAW(pthread_join, th, ret);
-  int tid = ThreadTid(thr, pc, (uptr)th);
+  int tid = ThreadConsumeTid(thr, pc, (uptr)th);
   ThreadIgnoreBegin(thr, pc);
   int res = BLOCK_REAL(pthread_join)(th, ret);
   ThreadIgnoreEnd(thr, pc);
   if (res == 0) {
     ThreadJoin(thr, pc, tid);
   }
   return res;
 }
 
 DEFINE_REAL_PTHREAD_FUNCTIONS
 
 TSAN_INTERCEPTOR(int, pthread_detach, void *th) {
-  SCOPED_TSAN_INTERCEPTOR(pthread_detach, th);
-  int tid = ThreadTid(thr, pc, (uptr)th);
+  SCOPED_INTERCEPTOR_RAW(pthread_detach, th);
+  int tid = ThreadConsumeTid(thr, pc, (uptr)th);
   int res = REAL(pthread_detach)(th);
   if (res == 0) {
     ThreadDetach(thr, pc, tid);
   }
   return res;
 }
 
 TSAN_INTERCEPTOR(void, pthread_exit, void *retval) {
   {
     SCOPED_INTERCEPTOR_RAW(pthread_exit, retval);
 #if !SANITIZER_MAC && !SANITIZER_ANDROID
     CHECK_EQ(thr, &cur_thread_placeholder);
 #endif
   }
   REAL(pthread_exit)(retval);
 }
 
 #if SANITIZER_LINUX
 TSAN_INTERCEPTOR(int, pthread_tryjoin_np, void *th, void **ret) {
-  SCOPED_TSAN_INTERCEPTOR(pthread_tryjoin_np, th, ret);
-  int tid = ThreadTid(thr, pc, (uptr)th);
+  SCOPED_INTERCEPTOR_RAW(pthread_tryjoin_np, th, ret);
+  int tid = ThreadConsumeTid(thr, pc, (uptr)th);
   ThreadIgnoreBegin(thr, pc);
   int res = REAL(pthread_tryjoin_np)(th, ret);
   ThreadIgnoreEnd(thr, pc);
   if (res == 0)
     ThreadJoin(thr, pc, tid);
   else
     ThreadNotJoined(thr, pc, tid, (uptr)th);
   return res;
 }
 
 TSAN_INTERCEPTOR(int, pthread_timedjoin_np, void *th, void **ret,
                  const struct timespec *abstime) {
-  SCOPED_TSAN_INTERCEPTOR(pthread_timedjoin_np, th, ret, abstime);
-  int tid = ThreadTid(thr, pc, (uptr)th);
+  SCOPED_INTERCEPTOR_RAW(pthread_timedjoin_np, th, ret, abstime);
+  int tid = ThreadConsumeTid(thr, pc, (uptr)th);
   ThreadIgnoreBegin(thr, pc);
   int res = BLOCK_REAL(pthread_timedjoin_np)(th, ret, abstime);
   ThreadIgnoreEnd(thr, pc);
   if (res == 0)
     ThreadJoin(thr, pc, tid);
   else
     ThreadNotJoined(thr, pc, tid, (uptr)th);
   return res;

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

 
 void FuncEntry(ThreadState *thr, uptr pc);
 void FuncExit(ThreadState *thr);
 
 int ThreadCreate(ThreadState *thr, uptr pc, uptr uid, bool detached);
 void ThreadStart(ThreadState *thr, int tid, tid_t os_id,
                  ThreadType thread_type);
 void ThreadFinish(ThreadState *thr);
-int ThreadTid(ThreadState *thr, uptr pc, uptr uid);
+int ThreadConsumeTid(ThreadState *thr, uptr pc, uptr uid);
 void ThreadJoin(ThreadState *thr, uptr pc, int tid);
 void ThreadDetach(ThreadState *thr, uptr pc, int tid);
 void ThreadFinalize(ThreadState *thr);
 void ThreadSetName(ThreadState *thr, const char *name);
 int ThreadCount(ThreadState *thr);
 void ProcessPendingSignals(ThreadState *thr);
 void ThreadNotJoined(ThreadState *thr, uptr pc, int tid, uptr uid);
 

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

void ThreadFinish(ThreadState *thr) {
   if (thr->stk_addr && thr->stk_size)
     DontNeedShadowFor(thr->stk_addr, thr->stk_size);
   if (thr->tls_addr && thr->tls_size)
     DontNeedShadowFor(thr->tls_addr, thr->tls_size);
   thr->is_dead = true;
   ctx->thread_registry->FinishThread(thr->tid);
 }
 
-static bool FindThreadByUid(ThreadContextBase *tctx, void *arg) {
-  uptr uid = (uptr)arg;
-  if (tctx->user_id == uid && tctx->status != ThreadStatusInvalid) {
+struct ConsumeThreadContext {
+  uptr uid;
+  ThreadContextBase *tctx;
+};
+
+static bool ConsumeThreadByUid(ThreadContextBase *tctx, void *arg) {
+  ConsumeThreadContext *findCtx = (ConsumeThreadContext *)arg;
+  if (tctx->user_id == findCtx->uid && tctx->status != ThreadStatusInvalid) {
+    if (findCtx->tctx) {
+      // Ensure that user_id is unique. If it's not the case we are screwed.
+      // Something went wrong before, but now there is no way to recover.
+      // Returning a wrong thread is not an option, it may lead to very hard
+      // to debug false positives (e.g. if we join a wrong thread).
+      Report("ThreadSanitizer: dup thread with used id 0x%zx\n", findCtx->uid);
+      Die();
+    }
+    findCtx->tctx = tctx;
     tctx->user_id = 0;
-    return true;
   }
   return false;
 }
 
-int ThreadTid(ThreadState *thr, uptr pc, uptr uid) {
-  int res = ctx->thread_registry->FindThread(FindThreadByUid, (void*)uid);
-  DPrintf("#%d: ThreadTid uid=%zu tid=%d\n", thr->tid, uid, res);
-  return res;
+int ThreadConsumeTid(ThreadState *thr, uptr pc, uptr uid) {
+  ConsumeThreadContext findCtx = {uid, nullptr};
+  ctx->thread_registry->FindThread(ConsumeThreadByUid, &findCtx);
+  int tid = findCtx.tctx ? findCtx.tctx->tid : ThreadRegistry::kUnknownTid;
+  DPrintf("#%d: ThreadTid uid=%zu tid=%d\n", thr->tid, uid, tid);
+  return tid;
 }
 
 void ThreadJoin(ThreadState *thr, uptr pc, int tid) {
   CHECK_GT(tid, 0);
   CHECK_LT(tid, kMaxTid);
   DPrintf("#%d: ThreadJoin tid=%d\n", thr->tid, tid);
   ctx->thread_registry->JoinThread(tid, thr);
 }

compiler-rt/test/tsan/ignore_lib6.cpp

+// RUN: rm -rf %t-dir
+// RUN: mkdir %t-dir
+// RUN: %clangxx_tsan -O1 %s -DLIB -fPIC -fno-sanitize=thread -shared -o %t-dir/libignore_lib.so
+// RUN: %clangxx_tsan -O1 %s %link_libcxx_tsan -o %t-dir/executable
+// RUN: %env_tsan_opts=suppressions='%s.supp' %run %t-dir/executable 2>&1 | FileCheck %s
+
+// Copied from ignore_lib5.cpp:
+// REQUIRES: stable-runtime
+// UNSUPPORTED: powerpc64le
+// UNSUPPORTED: netbsd
+
+// Test that pthread_detach works in libraries ignored by called_from_lib.
+// For more context see:
+// https://groups.google.com/forum/#!topic/thread-sanitizer/ecH2P0QUqPs
+
+#include "test.h"
+#include <dlfcn.h>
+#include <errno.h>
+#include <libgen.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string>
+#include <sys/mman.h>
+
+#ifndef LIB
+
+void *thr(void *arg) {
+  *(volatile long long *)arg = 1;
+  return 0;
+}
+
+int main(int argc, char **argv) {
+  std::string lib = std::string(dirname(argv[0])) + "/libignore_lib.so";
+  void *h = dlopen(lib.c_str(), RTLD_GLOBAL | RTLD_NOW);
+  if (h == 0)
+    exit(printf("failed to load the library (%d)\n", errno));
+  void (*libfunc)() = (void (*)())dlsym(h, "libfunc");
+  if (libfunc == 0)
+    exit(printf("failed to find the func (%d)\n", errno));
+  libfunc();
+
+  const int kThreads = 10;
+  pthread_t t[kThreads];
+  volatile long long data[kThreads];
+  for (int i = 0; i < kThreads; i++)
+    pthread_create(&t[i], 0, thr, (void *)&data[i]);
+  for (int i = 0; i < kThreads; i++) {
+    pthread_join(t[i], 0);
+    data[i] = 2;
+  }
+  fprintf(stderr, "DONE\n");
+}
+
+// CHECK-NOT: WARNING: ThreadSanitizer:
+// CHECK: DONE
+// CHECK-NOT: WARNING: ThreadSanitizer:
+
+#else // #ifdef LIB
+
+void *thr(void *p) {
+  sleep(1);
+  pthread_detach(pthread_self());
+  return 0;
+}
+
+extern "C" void libfunc() {
+  const int kThreads = 10;
+  pthread_t t[kThreads];
+  for (int i = 0; i < kThreads; i++)
+    pthread_create(&t[i], 0, thr, 0);
+  sleep(2);
+}
+
+#endif // #ifdef LIB

compiler-rt/test/tsan/ignore_lib6.cpp.supp

+called_from_lib:/libignore_lib.so$