diff --git a/compiler-rt/lib/tsan/rtl/tsan_defs.h b/compiler-rt/lib/tsan/rtl/tsan_defs.h --- a/compiler-rt/lib/tsan/rtl/tsan_defs.h +++ b/compiler-rt/lib/tsan/rtl/tsan_defs.h @@ -243,7 +243,6 @@ MutexTypeTrace, MutexTypeSlot, MutexTypeSlots, - MutexTypeMultiSlot, }; } // namespace __tsan diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.h b/compiler-rt/lib/tsan/rtl/tsan_rtl.h --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.h +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.h @@ -333,8 +333,6 @@ // Protects global_epoch, slot_queue, trace_part_recycle. Mutex slot_mtx; - // Prevents lock order inversions when we lock more than 1 slot. - Mutex multi_slot_mtx; uptr global_epoch; // guarded by slot_mtx and by all slot mutexes bool resetting; // global reset is in progress IList slot_queue GUARDED_BY(slot_mtx); @@ -609,16 +607,6 @@ FiberSwitchFlagNoSync = 1 << 0, // __tsan_switch_to_fiber_no_sync }; -class SlotPairLocker { - public: - SlotPairLocker(ThreadState *thr, Sid sid); - ~SlotPairLocker(); - - private: - ThreadState *thr_; - TidSlot *slot_; -}; - class SlotLocker { public: ALWAYS_INLINE diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.cpp @@ -210,7 +210,6 @@ // error: expecting mutex 'slot.mtx' to be held at start of each loop void DoReset(ThreadState* thr, uptr epoch) NO_THREAD_SAFETY_ANALYSIS { { - Lock l(&ctx->multi_slot_mtx); for (auto& slot : ctx->slots) { slot.mtx.Lock(); if (UNLIKELY(epoch == 0)) @@ -337,6 +336,13 @@ void SlotLock(ThreadState* thr) NO_THREAD_SAFETY_ANALYSIS { DCHECK(!thr->slot_locked); +#if SANITIZER_DEBUG + // Check these mutexes are not locked. + // We can call DoReset from SlotAttachAndLock, which will lock + // these mutexes, but it happens only every once in a while. + { ThreadRegistryLock lock(&ctx->thread_registry); } + { Lock lock(&ctx->slot_mtx); } +#endif TidSlot* slot = thr->slot; slot->mtx.Lock(); thr->slot_locked = true; @@ -367,7 +373,6 @@ fired_suppressions_mtx(MutexTypeFired), clock_alloc(LINKER_INITIALIZED, "clock allocator"), slot_mtx(MutexTypeSlots), - multi_slot_mtx(MutexTypeMultiSlot), resetting() { fired_suppressions.reserve(8); for (uptr i = 0; i < ARRAY_SIZE(slots); i++) { @@ -767,7 +772,6 @@ // Detaching from the slot makes OnUserFree skip writing to the shadow. // The slot will be locked so any attempts to use it will deadlock anyway. SlotDetach(thr); - ctx->multi_slot_mtx.Lock(); for (auto& slot : ctx->slots) slot.mtx.Lock(); ctx->thread_registry.Lock(); ctx->slot_mtx.Lock(); @@ -799,7 +803,6 @@ ctx->slot_mtx.Unlock(); ctx->thread_registry.Unlock(); for (auto& slot : ctx->slots) slot.mtx.Unlock(); - ctx->multi_slot_mtx.Unlock(); SlotAttachAndLock(thr); SlotUnlock(thr); GlobalProcessorUnlock(); @@ -1053,9 +1056,7 @@ {MutexTypeAtExit, "AtExit", {}}, {MutexTypeFired, "Fired", {MutexLeaf}}, {MutexTypeRacy, "Racy", {MutexLeaf}}, - {MutexTypeGlobalProc, - "GlobalProc", - {MutexTypeSlot, MutexTypeSlots, MutexTypeMultiSlot}}, + {MutexTypeGlobalProc, "GlobalProc", {MutexTypeSlot, MutexTypeSlots}}, {MutexTypeInternalAlloc, "InternalAlloc", {MutexLeaf}}, {MutexTypeTrace, "Trace", {}}, {MutexTypeSlot, @@ -1063,7 +1064,6 @@ {MutexMulti, MutexTypeTrace, MutexTypeSyncVar, MutexThreadRegistry, MutexTypeSlots}}, {MutexTypeSlots, "Slots", {MutexTypeTrace, MutexTypeReport}}, - {MutexTypeMultiSlot, "MultiSlot", {MutexTypeSlot, MutexTypeSlots}}, {}, }; diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp @@ -552,7 +552,9 @@ void ReportDestroyLocked(ThreadState *thr, uptr pc, uptr addr, FastState last_lock, StackID creation_stack_id) { - SlotPairLocker locker(thr, last_lock.sid()); + // We need to lock the slot during RestoreStack because it protects + // the slot journal. + Lock slot_lock(&ctx->slots[static_cast(last_lock.sid())].mtx); ThreadRegistryLock l0(&ctx->thread_registry); Lock slots_lock(&ctx->slot_mtx); ScopedReport rep(ReportTypeMutexDestroyLocked); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp @@ -729,27 +729,6 @@ return false; } -// We need to lock the target slot during RestoreStack because it protects -// the slot journal. However, the target slot can be the slot of the current -// thread or a different slot. -SlotPairLocker::SlotPairLocker(ThreadState *thr, - Sid sid) NO_THREAD_SAFETY_ANALYSIS : thr_(thr), - slot_() { - CHECK_NE(sid, kFreeSid); - Lock l(&ctx->multi_slot_mtx); - SlotLock(thr); - if (sid == thr->slot->sid) - return; - slot_ = &ctx->slots[static_cast(sid)]; - slot_->mtx.Lock(); -} - -SlotPairLocker::~SlotPairLocker() NO_THREAD_SAFETY_ANALYSIS { - SlotUnlock(thr_); - if (slot_) - slot_->mtx.Unlock(); -} - void ReportRace(ThreadState *thr, RawShadow *shadow_mem, Shadow cur, Shadow old, AccessType typ0) { CheckedMutex::CheckNoLocks(); @@ -806,7 +785,9 @@ DynamicMutexSet mset1; MutexSet *mset[kMop] = {&thr->mset, mset1}; - SlotPairLocker locker(thr, s[1].sid()); + // We need to lock the slot during RestoreStack because it protects + // the slot journal. + Lock slot_lock(&ctx->slots[static_cast(s[1].sid())].mtx); ThreadRegistryLock l0(&ctx->thread_registry); Lock slots_lock(&ctx->slot_mtx); if (!RestoreStack(EventType::kAccessExt, s[1].sid(), s[1].epoch(), addr1, diff --git a/compiler-rt/lib/tsan/tests/unit/tsan_trace_test.cpp b/compiler-rt/lib/tsan/tests/unit/tsan_trace_test.cpp --- a/compiler-rt/lib/tsan/tests/unit/tsan_trace_test.cpp +++ b/compiler-rt/lib/tsan/tests/unit/tsan_trace_test.cpp @@ -88,7 +88,7 @@ // The previous one is equivalent, but RestoreStack must prefer // the last of the matchig accesses. CHECK(TryTraceMemoryAccess(thr, 0x2002, 0x3000, 8, kAccessRead)); - SlotPairLocker locker(thr, thr->fast_state.sid()); + Lock slot_lock(&ctx->slots[static_cast(thr->fast_state.sid())].mtx); ThreadRegistryLock lock1(&ctx->thread_registry); Lock lock2(&ctx->slot_mtx); Tid tid = kInvalidTid; @@ -148,7 +148,7 @@ kAccessRead); break; } - SlotPairLocker locker(thr, thr->fast_state.sid()); + Lock slot_lock(&ctx->slots[static_cast(thr->fast_state.sid())].mtx); ThreadRegistryLock lock1(&ctx->thread_registry); Lock lock2(&ctx->slot_mtx); Tid tid = kInvalidTid; @@ -176,7 +176,7 @@ TraceMutexLock(thr, EventType::kLock, 0x4000, 0x5000, 0x6000); TraceMutexLock(thr, EventType::kRLock, 0x4001, 0x5001, 0x6001); TraceMutexLock(thr, EventType::kRLock, 0x4002, 0x5001, 0x6002); - SlotPairLocker locker(thr, thr->fast_state.sid()); + Lock slot_lock(&ctx->slots[static_cast(thr->fast_state.sid())].mtx); ThreadRegistryLock lock1(&ctx->thread_registry); Lock lock2(&ctx->slot_mtx); Tid tid = kInvalidTid; @@ -219,7 +219,7 @@ FuncEntry(thr, 0x4000); TraceMutexLock(thr, EventType::kRLock, 0x4001, 0x5001, 0x6001); CHECK(TryTraceMemoryAccess(thr, 0x2002, 0x3000, 8, kAccessRead)); - SlotPairLocker locker(thr, thr->fast_state.sid()); + Lock slot_lock(&ctx->slots[static_cast(thr->fast_state.sid())].mtx); ThreadRegistryLock lock1(&ctx->thread_registry); Lock lock2(&ctx->slot_mtx); Tid tid = kInvalidTid; diff --git a/compiler-rt/test/tsan/stress.cpp b/compiler-rt/test/tsan/stress.cpp --- a/compiler-rt/test/tsan/stress.cpp +++ b/compiler-rt/test/tsan/stress.cpp @@ -1,10 +1,13 @@ -// RUN: %clangxx_tsan -O1 %s -o %t && %env_tsan_opts=flush_memory_ms=1:flush_symbolizer_ms=1:memory_limit_mb=1 %run %t 2>&1 | FileCheck %s +// This run stresses global reset happenning concurrently with everything else. +// RUN: %clangxx_tsan -O1 %s -o %t && %env_tsan_opts=flush_memory_ms=1:flush_symbolizer_ms=1:memory_limit_mb=1 %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-NORACE +// This run stresses race reporting happenning concurrently with everything else. +// RUN: %clangxx_tsan -O1 %s -DRACE=1 -o %t && %env_tsan_opts=suppress_equal_stacks=0:suppress_equal_addresses=0 %deflake %run %t | FileCheck %s --check-prefix=CHECK-RACE #include "test.h" #include #include volatile long stop; -long atomic, read_only; +long atomic, read_only, racy; int fds[2]; __attribute__((noinline)) void *SecondaryThread(void *x) { @@ -22,7 +25,7 @@ // just read the stop variable } if (me == 0 || me == 2) { - __atomic_store_n(&atomic, 1, __ATOMIC_RELEASE); + __atomic_store_n(&atomic, sink, __ATOMIC_RELEASE); } if (me == 0 || me == 3) { sink += __atomic_fetch_add(&atomic, 1, __ATOMIC_ACQ_REL); @@ -47,6 +50,13 @@ memcpy(&buf, &read_only, sizeof(buf)); sink += buf; } + if (me == 0 || me == 9) { +#if RACE + sink += racy++; +#else + sink += racy; +#endif + } // If you add more actions, update kActions in main. } return NULL; @@ -60,8 +70,12 @@ exit((perror("fcntl"), 1)); if (fcntl(fds[1], F_SETFL, O_NONBLOCK)) exit((perror("fcntl"), 1)); - const int kActions = 9; + const int kActions = 10; +#if RACE + const int kMultiplier = 1; +#else const int kMultiplier = 4; +#endif pthread_t t[kActions * kMultiplier]; for (int i = 0; i < kActions * kMultiplier; i++) pthread_create(&t[i], NULL, Thread, (void *)(long)(i % kActions)); @@ -73,6 +87,8 @@ return 0; } -// CHECK-NOT: ThreadSanitizer: -// CHECK: DONE -// CHECK-NOT: ThreadSanitizer: +// CHECK-NORACE-NOT: ThreadSanitizer: +// CHECK-NORACE: DONE +// CHECK-NORACE-NOT: ThreadSanitizer: +// CHECK-RACE: ThreadSanitizer: data race +// CHECK-RACE: DONE