llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-compiler-rt-sanitizer Author: Vitaly Buka (vitalybuka) <details> <summary>Changes</summary> Acquire ThreadRegistryLock inside ScopedReport::AddThread and ScopedReport::AddLocation instead of requiring callers to lock ctx->thread_registry around ScopedReport population. Assisted-by: Gemini --- Full diff: https://github.com/llvm/llvm-project/pull/228814.diff 4 Files Affected: - (modified) compiler-rt/lib/tsan/rtl/tsan_rtl.h (+2) - (modified) compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp (+23-38) - (modified) compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp (+32-28) - (modified) compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp (+2-7) ``````````diff diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.h b/compiler-rt/lib/tsan/rtl/tsan_rtl.h index a4bf790d5980b..5b4e5362779e7 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.h +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.h @@ -440,6 +440,8 @@ class ScopedReport { ScopedIgnoreInterceptors ignore_interceptors_; ScopedErrorReportLock lock_; + void AddThreadLocked(const ThreadContext* tctx, bool suppressable = false); + ScopedReport(const ScopedReport&) = delete; void operator=(const ScopedReport&) = delete; }; diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp index 8ae9c1f4b20e9..8d5bad370a5a0 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp @@ -58,14 +58,9 @@ static void ReportMutexMisuse(ThreadState *thr, uptr pc, ReportType typ, VarSizeStackTrace trace; ObtainCurrentStack(thr, pc, &trace); ScopedReport rep(typ); - // Release locks before symbolizing and outputting the report to avoid - // deadlocks. - { - ThreadRegistryLock l(&ctx->thread_registry); - rep.AddMutex(addr, creation_stack_id); - rep.AddStack(trace, true); - rep.AddLocation(addr, 1); - } + rep.AddMutex(addr, creation_stack_id); + rep.AddStack(trace, true); + rep.AddLocation(addr, 1); OutputReport(thr, rep); } @@ -534,28 +529,23 @@ void ReportDeadlock(ThreadState *thr, uptr pc, DDReport *r) { return; uptr dummy_pc = 0x42; ScopedReport rep(ReportTypeDeadlock); - // Release locks before symbolizing and outputting the report to avoid - // deadlocks. - { - ThreadRegistryLock l(&ctx->thread_registry); - for (int i = 0; i < r->n; i++) { - rep.AddMutex(r->loop[i].mtx_ctx0, r->loop[i].stk[0]); - rep.AddUniqueTid((int)r->loop[i].thr_ctx); - rep.AddThread((int)r->loop[i].thr_ctx); - } - for (int i = 0; i < r->n; i++) { - for (int j = 0; j < (flags()->second_deadlock_stack ? 2 : 1); j++) { - u32 stk = r->loop[i].stk[j]; - StackTrace stack; - if (stk && stk != kInvalidStackID) { - stack = StackDepotGet(stk); - } else { - // Sometimes we fail to extract the stack trace (FIXME: investigate), - // but we should still produce some stack trace in the report. - stack = StackTrace(&dummy_pc, 1); - } - rep.AddStack(stack, true); + for (int i = 0; i < r->n; i++) { + rep.AddMutex(r->loop[i].mtx_ctx0, r->loop[i].stk[0]); + rep.AddUniqueTid((int)r->loop[i].thr_ctx); + rep.AddThread((int)r->loop[i].thr_ctx); + } + for (int i = 0; i < r->n; i++) { + for (int j = 0; j < (flags()->second_deadlock_stack ? 2 : 1); j++) { + u32 stk = r->loop[i].stk[j]; + StackTrace stack; + if (stk && stk != kInvalidStackID) { + stack = StackDepotGet(stk); + } else { + // Sometimes we fail to extract the stack trace (FIXME: investigate), + // but we should still produce some stack trace in the report. + stack = StackTrace(&dummy_pc, 1); } + rep.AddStack(stack, true); } } OutputReport(thr, rep); @@ -583,15 +573,10 @@ void ReportDestroyLocked(ThreadState *thr, uptr pc, uptr addr, ObtainCurrentStack(thr, pc, &trace); ScopedReport rep(ReportTypeMutexDestroyLocked); - // Release locks before symbolizing and outputting the report to avoid - // deadlocks. - { - ThreadRegistryLock l0(&ctx->thread_registry); - rep.AddMutex(addr, creation_stack_id); - rep.AddStack(trace, true); - rep.AddStack(last_lock_stack, true); - rep.AddLocation(addr, 1); - } + rep.AddMutex(addr, creation_stack_id); + rep.AddStack(trace, true); + rep.AddStack(last_lock_stack, true); + rep.AddLocation(addr, 1); OutputReport(thr, rep); } diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp index 50448cfd1233b..1d76a886dc536 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp @@ -267,6 +267,12 @@ void ScopedReport::AddUniqueTid(Tid unique_tid) { } void ScopedReport::AddThread(const ThreadContext* tctx, bool suppressable) { + ThreadRegistryLock l(&ctx->thread_registry); + AddThreadLocked(tctx, suppressable); +} + +void ScopedReport::AddThreadLocked(const ThreadContext* tctx, + bool suppressable) { ctx->thread_registry.CheckLocked(); for (uptr i = 0; i < rep_->threads.Size(); i++) { if ((u32)rep_->threads[i]->id == tctx->tid) @@ -311,10 +317,10 @@ ThreadContext *IsThreadStackOrTls(uptr addr, bool *is_stack) { #endif void ScopedReport::AddThread(Tid tid, bool suppressable) { - ctx->thread_registry.CheckLocked(); + ThreadRegistryLock l(&ctx->thread_registry); if (const auto* tctx = static_cast<ThreadContext*>( ctx->thread_registry.GetThreadLocked(tid))) - AddThread(tctx, suppressable); + AddThreadLocked(tctx, suppressable); } int ScopedReport::AddMutex(uptr addr, StackID creation_stack_id) { @@ -372,12 +378,15 @@ void ScopedReport::AddLocation(uptr addr, uptr size) { return; } bool is_stack = false; - if (ThreadContext *tctx = IsThreadStackOrTls(addr, &is_stack)) { - auto *loc = New<ReportLocation>(); - loc->type = is_stack ? ReportLocationStack : ReportLocationTLS; - loc->tid = tctx->tid; - rep_->locs.PushBack(loc); - AddThread(tctx); + { + ThreadRegistryLock l(&ctx->thread_registry); + if (ThreadContext* tctx = IsThreadStackOrTls(addr, &is_stack)) { + auto* loc = New<ReportLocation>(); + loc->type = is_stack ? ReportLocationStack : ReportLocationTLS; + loc->tid = tctx->tid; + rep_->locs.PushBack(loc); + AddThreadLocked(tctx); + } } #endif rep_->added_location_addrs.PushBack({addr, rep_->locs.Size()}); @@ -843,33 +852,28 @@ void ReportRace(ThreadState *thr, RawShadow *shadow_mem, Shadow cur, Shadow old, } ScopedReport rep(rep_typ, tag); - // Release locks before symbolizing and outputting the report to avoid - // deadlocks. - { - ThreadRegistryLock l0(&ctx->thread_registry); - for (uptr i = 0; i < kMop; i++) - rep.AddMemoryAccess(addr, tags[i], s[i], tids[i], traces[i], mset[i]); + for (uptr i = 0; i < kMop; i++) + rep.AddMemoryAccess(addr, tags[i], s[i], tids[i], traces[i], mset[i]); - for (uptr i = 0; i < kMop; i++) rep.AddThread(tids[i]); + for (uptr i = 0; i < kMop; i++) rep.AddThread(tids[i]); - rep.AddLocation(addr_min, addr_max - addr_min); + rep.AddLocation(addr_min, addr_max - addr_min); - if (flags()->print_full_thread_history) { - const ReportDesc* rep_desc = rep.GetReport(); - for (uptr i = 0; i < rep_desc->threads.Size(); i++) { - Tid parent_tid = rep_desc->threads[i]->parent_tid; - if (parent_tid == kMainTid || parent_tid == kInvalidTid) - continue; - rep.AddThread(parent_tid); - } + if (flags()->print_full_thread_history) { + const ReportDesc* rep_desc = rep.GetReport(); + for (uptr i = 0; i < rep_desc->threads.Size(); i++) { + Tid parent_tid = rep_desc->threads[i]->parent_tid; + if (parent_tid == kMainTid || parent_tid == kInvalidTid) + continue; + rep.AddThread(parent_tid); } + } #if !SANITIZER_GO - if (!((typ0 | typ1) & kAccessFree) && - s[1].epoch() <= thr->last_sleep_clock.Get(s[1].sid())) - rep.AddSleep(thr->last_sleep_stack_id); + if (!((typ0 | typ1) & kAccessFree) && + s[1].epoch() <= thr->last_sleep_clock.Get(s[1].sid())) + rep.AddSleep(thr->last_sleep_stack_id); #endif - } OutputReport(thr, rep); } diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp index 5edf0c16fdd0d..da9e56a5b8f8d 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp @@ -97,13 +97,8 @@ void ThreadFinalize(ThreadState *thr) { for (uptr i = 0; i < leaks.Size(); i++) { ScopedReport rep(ReportTypeThreadLeak); - // Release locks before symbolizing and outputting the report to avoid - // deadlocks. - { - ThreadRegistryLock l(&ctx->thread_registry); - rep.AddThread(leaks[i].tctx, true); - rep.SetCount(leaks[i].count); - } + rep.AddThread(leaks[i].tctx, true); + rep.SetCount(leaks[i].count); OutputReport(thr, rep); } #endif `````````` </details> https://github.com/llvm/llvm-project/pull/228814 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
