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

[Julian Lettner via Phabricator via llvm-commits]

yln accepted this revision.
yln added a comment.
This revision is now accepted and ready to land.

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



================
Comment at: compiler-rt/lib/tsan/rtl/tsan_rtl.h:778
 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);
----------------
Thanks for this as well! :)


================
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);
----------------
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).


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


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


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

https://reviews.llvm.org/D74828