This is an archive of the discontinued LLVM Phabricator instance.

Prevent Thread Exited/Joined events race
ClosedPublic

Authored by krytarowski on Nov 21 2017, 4:16 AM.

Details

Summary

Add atomic verification to ensure that Thread is Joined after marking it
Finished.

It is required for NetBSD in order to prevent Thread Exited/Joined race,
that may occur when native system libpthread(3) cannot be reliably traced
in a way to guarantee that the mentioned events happen one after another.

This change fixes at least TSan and LSan on NetBSD.

Sponsored by <The NetBSD Foundation>

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski created this revision.Nov 21 2017, 4:16 AM

This change proposal is in the same time request for improvement.

check-tsan can give nice looking results like:

Testing Time: 48.84s
********************
Failing Tests (13):
    ThreadSanitizer-x86_64 :: deadlock_detector_stress_test.cc
    ThreadSanitizer-x86_64 :: dtls.c
    ThreadSanitizer-x86_64 :: exceptions.cc
    ThreadSanitizer-x86_64 :: ignore_lib5.cc
    ThreadSanitizer-x86_64 :: ignored-interceptors-mmap.cc
    ThreadSanitizer-x86_64 :: longjmp3.cc
    ThreadSanitizer-x86_64 :: longjmp4.cc
    ThreadSanitizer-x86_64 :: mutex_lock_destroyed.cc
    ThreadSanitizer-x86_64 :: signal_errno.cc
    ThreadSanitizer-x86_64 :: signal_malloc.cc
    ThreadSanitizer-x86_64 :: signal_sync2.cc
    ThreadSanitizer-x86_64 :: signal_thread.cc
    ThreadSanitizer-x86_64 :: vfork.cc

  Expected Passes    : 245
  Expected Failures  : 1
  Unsupported Tests  : 83
  Unexpected Failures: 13

On the other hand there still can happen this race... so something is suboptimal. Improper usage of atomic functions?

Failing Tests (43):
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/Posix.ThreadLocalAccesses
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.BasicMutex
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.BasicSpinMutex
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.BenignRaceOnVptr
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.FuncCall
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.HarmfulRaceOnVptr
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.LockedWriteThenRead
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.Memcpy
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.MemcpyRace2
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.MemcpyRace3
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.MemcpyStack
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.MemsetRace1
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.Mutex
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.RaceWithOffset
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.RaceWithOffset2
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.ReadReadNoRace
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.ReadWriteRace
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.ReportDeadThread
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.ReportRace
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.RwMutex
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.SimpleWrite
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.SimpleWriteWrite
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.StaticMutex
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.ThreadDetach1
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.ThreadSync
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.WriteReadRace
    ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.WriteThenLockedRead
    ThreadSanitizer-Unit :: unit/./TsanUnitTest-x86_64-Test/Mutex.Write
    ThreadSanitizer-x86_64 :: deadlock_detector_stress_test.cc
    ThreadSanitizer-x86_64 :: dtls.c
    ThreadSanitizer-x86_64 :: exceptions.cc
    ThreadSanitizer-x86_64 :: fd_tid_recycled.cc
    ThreadSanitizer-x86_64 :: ignore_lib5.cc
    ThreadSanitizer-x86_64 :: ignored-interceptors-mmap.cc
    ThreadSanitizer-x86_64 :: longjmp3.cc
    ThreadSanitizer-x86_64 :: longjmp4.cc
    ThreadSanitizer-x86_64 :: mutex_lock_destroyed.cc
    ThreadSanitizer-x86_64 :: signal_errno.cc
    ThreadSanitizer-x86_64 :: signal_malloc.cc
    ThreadSanitizer-x86_64 :: signal_sync2.cc
    ThreadSanitizer-x86_64 :: signal_thread.cc
    ThreadSanitizer-x86_64 :: vfork.cc

  Expected Passes    : 252
  Expected Failures  : 1
  Unsupported Tests  : 83
  Unexpected Failures: 43
dvyukov added inline comments.Nov 21 2017, 6:54 AM
lib/sanitizer_common/sanitizer_thread_registry.cc
87

This also needs to reset thread_destroyed because contexts are reused.

97

Now looking at the code more closely, I think we can contain this all inside of ThreadRegistry without changing public API and exposing more guts.

We could set thread_destroyed in ThreadRegistry::FinishThread (right after tctx->SetFinished()), and wait for the flag in ThreadRegistry::JoinThread (need to release the mutex around yield).
This looks like a cleaner solution and will fix all sanitizers at once.
Will this work for netbsd?

Sorry for not noticing it earlier.

190

we now generally use nullptr for pointers

krytarowski added inline comments.Nov 21 2017, 7:12 AM
lib/sanitizer_common/sanitizer_thread_registry.cc
87

I see, this might be the reason why I observed the races!

97

I will give it a try!

190

OK!

krytarowski added inline comments.Nov 21 2017, 7:14 PM
lib/sanitizer_common/sanitizer_thread_registry.cc
87

I've confirmed that after resetting thread_destroyed in Reset() there are no longer races.

I'm going to revamp the patch now.

krytarowski edited the summary of this revision. (Show Details)
krytarowski marked 7 inline comments as done.

This approach appears to be stable for TSan/NetBSD, no single race or misbehavior has been observed so far.

dvyukov accepted this revision.Nov 26 2017, 10:36 AM
This revision is now accepted and ready to land.Nov 26 2017, 10:36 AM
krytarowski closed this revision.Nov 26 2017, 12:20 PM