This is an archive of the discontinued LLVM Phabricator instance.

tsan: fix pthread_detach with called_from_lib suppressions
ClosedPublic

Authored by dvyukov on Feb 19 2020, 5:29 AM.

Details

Reviewers
vitalybuka
yln
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.

Diff Detail

Event Timeline

dvyukov created this revision.Feb 19 2020, 5:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2020, 5:29 AM
dvyukov updated this revision to Diff 245412.Feb 19 2020, 8:15 AM

clang-format

dmajor added a subscriber: dmajor.Feb 19 2020, 1:17 PM

Vitaly, ping.

yln accepted this revision.Feb 24 2020, 10:44 AM

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?

This revision is now accepted and ready to land.Feb 24 2020, 10:44 AM
vitalybuka accepted this revision.Feb 24 2020, 11:02 AM
dvyukov updated this revision to Diff 246664.Feb 26 2020, 3:58 AM
dvyukov marked 3 inline comments as done.

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.

yln added inline comments.Feb 26 2020, 10:19 AM
compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp
313

Got it, thanks for explaining.