[PATCH] D74828: tsan: fix pthread_detach with called_from_lib suppressions

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added inline comments.


================
Comment at: compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp:313
+  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);
----------------
yln wrote:
> 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).
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.


================
Comment at: compiler-rt/test/tsan/ignore_lib6.cpp:55
+// CHECK-NOT: WARNING: ThreadSanitizer: data race
+// CHECK-NOT: WARNING: ThreadSanitizer: thread leak
+// CHECK: DONE
----------------
yln wrote:
> 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.
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.


================
Comment at: compiler-rt/test/tsan/ignore_lib6.cpp:61
+void *thr(void *p) {
+  sleep(1);
+  pthread_detach(pthread_self());
----------------
yln wrote:
> Would it be possible (without heroic effort) to express this test without `sleep`ing?
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.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74828/new/

https://reviews.llvm.org/D74828