From 96313d2de45ace49d40606dda71f03396f13ddef Mon Sep 17 00:00:00 2001 From: Joachim Protze Date: Thu, 16 Jul 2020 16:15:21 +0200 Subject: [TSan] Optimize handling of racy address This patch splits the handling of racy address and racy stack into separate functions. If a race was already reported for the address, we can avoid the cost for collecting the involved stacks. This patch also removes the race condition in storing the racy address / racy stack. This race condition allowed all threads to report the race. This patch changes the transitive suppression of reports. Previously suppression could transitively chain memory location and racy stacks. Now racy memory and racy stack are separate suppressions. Commit again, now with fixed tests. Reviewed by: dvyukov Differential Revision: https://reviews.llvm.org/D83625 (cherry picked from commit 7358a1104a02d5f5e645ebff0530787453ae98da) --- compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp | 103 ++++++++++----------- .../lib/tsan/tests/rtl/tsan_test_util_posix.cpp | 51 +++++++--- 2 files changed, 87 insertions(+), 67 deletions(-) diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp index 949beac..3354546 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp @@ -439,65 +439,61 @@ void RestoreStack(int tid, const u64 epoch, VarSizeStackTrace *stk, ExtractTagFromStack(stk, tag); } -static bool HandleRacyStacks(ThreadState *thr, VarSizeStackTrace traces[2], - uptr addr_min, uptr addr_max) { - bool equal_stack = false; - RacyStacks hash; - bool equal_address = false; - RacyAddress ra0 = {addr_min, addr_max}; - { - ReadLock lock(&ctx->racy_mtx); - if (flags()->suppress_equal_stacks) { - hash.hash[0] = md5_hash(traces[0].trace, traces[0].size * sizeof(uptr)); - hash.hash[1] = md5_hash(traces[1].trace, traces[1].size * sizeof(uptr)); - for (uptr i = 0; i < ctx->racy_stacks.Size(); i++) { - if (hash == ctx->racy_stacks[i]) { - VPrintf(2, - "ThreadSanitizer: suppressing report as doubled (stack)\n"); - equal_stack = true; - break; - } - } - } - if (flags()->suppress_equal_addresses) { - for (uptr i = 0; i < ctx->racy_addresses.Size(); i++) { - RacyAddress ra2 = ctx->racy_addresses[i]; - uptr maxbeg = max(ra0.addr_min, ra2.addr_min); - uptr minend = min(ra0.addr_max, ra2.addr_max); - if (maxbeg < minend) { - VPrintf(2, "ThreadSanitizer: suppressing report as doubled (addr)\n"); - equal_address = true; - break; - } - } +static bool FindRacyStacks(const RacyStacks &hash) { + for (uptr i = 0; i < ctx->racy_stacks.Size(); i++) { + if (hash == ctx->racy_stacks[i]) { + VPrintf(2, "ThreadSanitizer: suppressing report as doubled (stack)\n"); + return true; } } - if (!equal_stack && !equal_address) + return false; +} + +static bool HandleRacyStacks(ThreadState *thr, VarSizeStackTrace traces[2]) { + if (!flags()->suppress_equal_stacks) return false; - if (!equal_stack) { - Lock lock(&ctx->racy_mtx); - ctx->racy_stacks.PushBack(hash); - } - if (!equal_address) { - Lock lock(&ctx->racy_mtx); - ctx->racy_addresses.PushBack(ra0); + RacyStacks hash; + hash.hash[0] = md5_hash(traces[0].trace, traces[0].size * sizeof(uptr)); + hash.hash[1] = md5_hash(traces[1].trace, traces[1].size * sizeof(uptr)); + { + ReadLock lock(&ctx->racy_mtx); + if (FindRacyStacks(hash)) + return true; } - return true; + Lock lock(&ctx->racy_mtx); + if (FindRacyStacks(hash)) + return true; + ctx->racy_stacks.PushBack(hash); + return false; } -static void AddRacyStacks(ThreadState *thr, VarSizeStackTrace traces[2], - uptr addr_min, uptr addr_max) { - Lock lock(&ctx->racy_mtx); - if (flags()->suppress_equal_stacks) { - RacyStacks hash; - hash.hash[0] = md5_hash(traces[0].trace, traces[0].size * sizeof(uptr)); - hash.hash[1] = md5_hash(traces[1].trace, traces[1].size * sizeof(uptr)); - ctx->racy_stacks.PushBack(hash); +static bool FindRacyAddress(const RacyAddress &ra0) { + for (uptr i = 0; i < ctx->racy_addresses.Size(); i++) { + RacyAddress ra2 = ctx->racy_addresses[i]; + uptr maxbeg = max(ra0.addr_min, ra2.addr_min); + uptr minend = min(ra0.addr_max, ra2.addr_max); + if (maxbeg < minend) { + VPrintf(2, "ThreadSanitizer: suppressing report as doubled (addr)\n"); + return true; + } } - if (flags()->suppress_equal_addresses) { - RacyAddress ra0 = {addr_min, addr_max}; - ctx->racy_addresses.PushBack(ra0); + return false; +} + +static bool HandleRacyAddress(ThreadState *thr, uptr addr_min, uptr addr_max) { + if (!flags()->suppress_equal_addresses) + return false; + RacyAddress ra0 = {addr_min, addr_max}; + { + ReadLock lock(&ctx->racy_mtx); + if (FindRacyAddress(ra0)) + return true; } + Lock lock(&ctx->racy_mtx); + if (FindRacyAddress(ra0)) + return true; + ctx->racy_addresses.PushBack(ra0); + return false; } bool OutputReport(ThreadState *thr, const ScopedReport &srep) { @@ -618,6 +614,8 @@ void ReportRace(ThreadState *thr) { if (IsExpectedReport(addr_min, addr_max - addr_min)) return; } + if (HandleRacyAddress(thr, addr_min, addr_max)) + return; ReportType typ = ReportTypeRace; if (thr->is_vptr_access && freed) @@ -668,7 +666,7 @@ void ReportRace(ThreadState *thr) { if (IsFiredSuppression(ctx, typ, traces[1])) return; - if (HandleRacyStacks(thr, traces, addr_min, addr_max)) + if (HandleRacyStacks(thr, traces)) return; // If any of the accesses has a tag, treat this as an "external" race. @@ -711,7 +709,6 @@ void ReportRace(ThreadState *thr) { if (!OutputReport(thr, rep)) return; - AddRacyStacks(thr, traces, addr_min, addr_max); } void PrintCurrentStack(ThreadState *thr, uptr pc) { diff --git a/compiler-rt/lib/tsan/tests/rtl/tsan_test_util_posix.cpp b/compiler-rt/lib/tsan/tests/rtl/tsan_test_util_posix.cpp index a24d04f..733e5d2 100644 --- a/compiler-rt/lib/tsan/tests/rtl/tsan_test_util_posix.cpp +++ b/compiler-rt/lib/tsan/tests/rtl/tsan_test_util_posix.cpp @@ -27,6 +27,8 @@ #include #include +#define CALLERPC (__builtin_return_address(0)) + using namespace __tsan; static __thread bool expect_report; @@ -249,22 +251,42 @@ void ScopedThread::Impl::HandleEvent(Event *ev) { switch (ev->type) { case Event::READ: case Event::WRITE: { - void (*tsan_mop)(void *addr) = 0; + void (*tsan_mop)(void *addr, void *pc) = 0; if (ev->type == Event::READ) { switch (ev->arg /*size*/) { - case 1: tsan_mop = __tsan_read1; break; - case 2: tsan_mop = __tsan_read2; break; - case 4: tsan_mop = __tsan_read4; break; - case 8: tsan_mop = __tsan_read8; break; - case 16: tsan_mop = __tsan_read16; break; + case 1: + tsan_mop = __tsan_read1_pc; + break; + case 2: + tsan_mop = __tsan_read2_pc; + break; + case 4: + tsan_mop = __tsan_read4_pc; + break; + case 8: + tsan_mop = __tsan_read8_pc; + break; + case 16: + tsan_mop = __tsan_read16_pc; + break; } } else { switch (ev->arg /*size*/) { - case 1: tsan_mop = __tsan_write1; break; - case 2: tsan_mop = __tsan_write2; break; - case 4: tsan_mop = __tsan_write4; break; - case 8: tsan_mop = __tsan_write8; break; - case 16: tsan_mop = __tsan_write16; break; + case 1: + tsan_mop = __tsan_write1_pc; + break; + case 2: + tsan_mop = __tsan_write2_pc; + break; + case 4: + tsan_mop = __tsan_write4_pc; + break; + case 8: + tsan_mop = __tsan_write8_pc; + break; + case 16: + tsan_mop = __tsan_write16_pc; + break; } } CHECK_NE(tsan_mop, 0); @@ -274,7 +296,7 @@ void ScopedThread::Impl::HandleEvent(Event *ev) { const int ErrCode = ECHRNG; #endif errno = ErrCode; - tsan_mop(ev->ptr); + tsan_mop(ev->ptr, (void *)ev->arg2); CHECK_EQ(ErrCode, errno); // In no case must errno be changed. break; } @@ -327,7 +349,7 @@ void ScopedThread::Impl::HandleEvent(Event *ev) { } void *ScopedThread::Impl::ScopedThreadCallback(void *arg) { - __tsan_func_entry(__builtin_return_address(0)); + __tsan_func_entry(CALLERPC); Impl *impl = (Impl*)arg; for (;;) { Event* ev = (Event*)atomic_load(&impl->event, memory_order_acquire); @@ -392,7 +414,8 @@ void ScopedThread::Detach() { void ScopedThread::Access(void *addr, bool is_write, int size, bool expect_race) { - Event event(is_write ? Event::WRITE : Event::READ, addr, size); + Event event(is_write ? Event::WRITE : Event::READ, addr, size, + (uptr)CALLERPC); if (expect_race) event.ExpectReport(ReportTypeRace); impl_->send(&event); -- cgit v1.1