From 6eea08549a7bc0a2b92aad30f74b554f4e27faa8 Mon Sep 17 00:00:00 2001 From: Dan Blackwell Date: Fri, 18 Jul 2025 14:47:21 +0100 Subject: [PATCH] [TSan][compiler-rt] Move symbolization in ReportRace outside of locks on Apple platforms --- .../lib/tsan/rtl/tsan_interceptors_posix.cpp | 1 + .../lib/tsan/rtl/tsan_interface_ann.cpp | 1 + compiler-rt/lib/tsan/rtl/tsan_mman.cpp | 1 + compiler-rt/lib/tsan/rtl/tsan_report.h | 8 + compiler-rt/lib/tsan/rtl/tsan_rtl.h | 1 + compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp | 4 + compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp | 9 +- compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp | 185 ++++++++++++------ compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp | 1 + 9 files changed, 146 insertions(+), 65 deletions(-) diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp index 14b25a8995dab..274a38390bda8 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp @@ -2146,6 +2146,7 @@ static void ReportErrnoSpoiling(ThreadState *thr, uptr pc, int sig) { rep.SetSigNum(sig); if (!IsFiredSuppression(ctx, ReportTypeErrnoInSignal, stack)) { rep.AddStack(stack, true); + rep.SymbolizeStackElems(); OutputReport(thr, rep); } } diff --git a/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp b/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp index befd6a369026d..d16fd4cfb00d9 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp @@ -446,6 +446,7 @@ static void ReportMutexHeldWrongContext(ThreadState *thr, uptr pc) { VarSizeStackTrace trace; ObtainCurrentStack(thr, pc, &trace); rep.AddStack(trace, true); + rep.SymbolizeStackElems(); OutputReport(thr, rep); } diff --git a/compiler-rt/lib/tsan/rtl/tsan_mman.cpp b/compiler-rt/lib/tsan/rtl/tsan_mman.cpp index 0ea83fb3b5982..3a0a6a1a4564f 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_mman.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_mman.cpp @@ -185,6 +185,7 @@ static void SignalUnsafeCall(ThreadState *thr, uptr pc) { ThreadRegistryLock l(&ctx->thread_registry); ScopedReport rep(ReportTypeSignalUnsafe); rep.AddStack(stack, true); + rep.SymbolizeStackElems(); OutputReport(thr, rep); } diff --git a/compiler-rt/lib/tsan/rtl/tsan_report.h b/compiler-rt/lib/tsan/rtl/tsan_report.h index bfe470797f8f7..50501060840a4 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_report.h +++ b/compiler-rt/lib/tsan/rtl/tsan_report.h @@ -12,6 +12,8 @@ #ifndef TSAN_REPORT_H #define TSAN_REPORT_H +#include "sanitizer_common/sanitizer_internal_defs.h" +#include "sanitizer_common/sanitizer_stacktrace.h" #include "sanitizer_common/sanitizer_symbolizer.h" #include "sanitizer_common/sanitizer_thread_registry.h" #include "sanitizer_common/sanitizer_vector.h" @@ -56,6 +58,7 @@ struct ReportMop { bool atomic; uptr external_tag; Vector mset; + StackTrace stack_trace; ReportStack *stack; ReportMop(); @@ -79,6 +82,7 @@ struct ReportLocation { int fd = 0; bool fd_closed = false; bool suppressable = false; + StackID stack_id = 0; ReportStack *stack = nullptr; }; @@ -89,12 +93,15 @@ struct ReportThread { ThreadType thread_type; char *name; Tid parent_tid; + StackID stack_id; ReportStack *stack; + bool suppressable; }; struct ReportMutex { int id; uptr addr; + StackID stack_id; ReportStack *stack; }; @@ -105,6 +112,7 @@ class ReportDesc { Vector stacks; Vector mops; Vector locs; + Vector added_location_addrs; Vector mutexes; Vector threads; Vector unique_tids; diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl.h b/compiler-rt/lib/tsan/rtl/tsan_rtl.h index dc32980e905f2..7122c15bb2c13 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl.h +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl.h @@ -420,6 +420,7 @@ class ScopedReportBase { void AddSleep(StackID stack_id); void SetCount(int count); void SetSigNum(int sig); + void SymbolizeStackElems(void); const ReportDesc *GetReport() const; diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp index 487fa490636eb..5715115934013 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp @@ -419,6 +419,10 @@ NOINLINE void TraceRestartMemoryAccess(ThreadState* thr, uptr pc, uptr addr, ALWAYS_INLINE USED void MemoryAccess(ThreadState* thr, uptr pc, uptr addr, uptr size, AccessType typ) { +#if SANITIZER_APPLE && !SANITIZER_GO + if (thr->in_symbolizer) + return; +#endif RawShadow* shadow_mem = MemToShadow(addr); UNUSED char memBuf[4][64]; DPrintf2("#%d: Access: %d@%d %p/%zd typ=0x%x {%s, %s, %s, %s}\n", thr->tid, diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp index 2a8aa1915c9ae..f546931ff0e56 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp @@ -62,6 +62,7 @@ static void ReportMutexMisuse(ThreadState *thr, uptr pc, ReportType typ, ObtainCurrentStack(thr, pc, &trace); rep.AddStack(trace, true); rep.AddLocation(addr, 1); + rep.SymbolizeStackElems(); OutputReport(thr, rep); } @@ -539,13 +540,16 @@ void ReportDeadlock(ThreadState *thr, uptr pc, DDReport *r) { 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) { - rep.AddStack(StackDepotGet(stk), true); + 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. - rep.AddStack(StackTrace(&dummy_pc, 1), true); + stack = StackTrace(&dummy_pc, 1); } + rep.AddStack(stack, true); + rep.SymbolizeStackElems(); } } OutputReport(thr, rep); @@ -572,6 +576,7 @@ void ReportDestroyLocked(ThreadState *thr, uptr pc, uptr addr, return; rep.AddStack(trace, true); rep.AddLocation(addr, 1); + rep.SymbolizeStackElems(); 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 0820bf1adee43..991e5c60399f0 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp @@ -10,6 +10,8 @@ // //===----------------------------------------------------------------------===// +#include + #include "sanitizer_common/sanitizer_common.h" #include "sanitizer_common/sanitizer_libc.h" #include "sanitizer_common/sanitizer_placement_new.h" @@ -187,10 +189,8 @@ void ScopedReportBase::AddMemoryAccess(uptr addr, uptr external_tag, Shadow s, mop->size = size; mop->write = !(typ & kAccessRead); mop->atomic = typ & kAccessAtomic; - mop->stack = SymbolizeStack(stack); mop->external_tag = external_tag; - if (mop->stack) - mop->stack->suppressable = true; + mop->stack_trace = stack; for (uptr i = 0; i < mset->Size(); i++) { MutexSet::Desc d = mset->Get(i); int id = this->AddMutex(d.addr, d.stack_id); @@ -199,6 +199,45 @@ void ScopedReportBase::AddMemoryAccess(uptr addr, uptr external_tag, Shadow s, } } +void ScopedReportBase::SymbolizeStackElems() { + // symbolize memory ops + for (usize i = 0; i < rep_->mops.Size(); i++) { + ReportMop *mop = rep_->mops[i]; + mop->stack = SymbolizeStack(mop->stack_trace); + if (mop->stack) + mop->stack->suppressable = true; + } + + // symbolize locations + for (usize i = 0; i < rep_->locs.Size(); i++) { + ReportLocation *loc = rep_->locs[i]; + loc->stack = SymbolizeStackId(loc->stack_id); + } + + for (usize i = 0; i < rep_->added_location_addrs.Size(); i++) { + if (ReportLocation *loc = SymbolizeData(rep_->added_location_addrs[i])) { + loc->suppressable = true; + // TODO: These all end up at the back of locs - line them up with their + // corresponding location + rep_->locs.PushBack(loc); + } + } + + // symbolize threads + for (usize i = 0; i < rep_->threads.Size(); i++) { + ReportThread *rt = rep_->threads[i]; + rt->stack = SymbolizeStackId(rt->stack_id); + if (rt->stack) + rt->stack->suppressable = rt->suppressable; + } + + // symbolize mutexes + for (usize i = 0; i < rep_->mutexes.Size(); i++) { + ReportMutex *rm = rep_->mutexes[i]; + rm->stack = SymbolizeStackId(rm->stack_id); + } +} + void ScopedReportBase::AddUniqueTid(Tid unique_tid) { rep_->unique_tids.PushBack(unique_tid); } @@ -216,10 +255,8 @@ void ScopedReportBase::AddThread(const ThreadContext *tctx, bool suppressable) { rt->name = internal_strdup(tctx->name); rt->parent_tid = tctx->parent_tid; rt->thread_type = tctx->thread_type; - rt->stack = 0; - rt->stack = SymbolizeStackId(tctx->creation_stack_id); - if (rt->stack) - rt->stack->suppressable = suppressable; + rt->stack_id = tctx->creation_stack_id; + rt->suppressable = suppressable; } #if !SANITIZER_GO @@ -270,7 +307,7 @@ int ScopedReportBase::AddMutex(uptr addr, StackID creation_stack_id) { rep_->mutexes.PushBack(rm); rm->id = rep_->mutexes.Size() - 1; rm->addr = addr; - rm->stack = SymbolizeStackId(creation_stack_id); + rm->stack_id = creation_stack_id; return rm->id; } @@ -288,7 +325,7 @@ void ScopedReportBase::AddLocation(uptr addr, uptr size) { loc->fd_closed = closed; loc->fd = fd; loc->tid = creat_tid; - loc->stack = SymbolizeStackId(creat_stack); + loc->stack_id = creat_stack; rep_->locs.PushBack(loc); AddThread(creat_tid); return; @@ -310,7 +347,7 @@ void ScopedReportBase::AddLocation(uptr addr, uptr size) { loc->heap_chunk_size = b->siz; loc->external_tag = b->tag; loc->tid = b->tid; - loc->stack = SymbolizeStackId(b->stk); + loc->stack_id = b->stk; rep_->locs.PushBack(loc); AddThread(b->tid); return; @@ -324,11 +361,7 @@ void ScopedReportBase::AddLocation(uptr addr, uptr size) { AddThread(tctx); } #endif - if (ReportLocation *loc = SymbolizeData(addr)) { - loc->suppressable = true; - rep_->locs.PushBack(loc); - return; - } + rep_->added_location_addrs.PushBack(addr); } #if !SANITIZER_GO @@ -761,65 +794,91 @@ void ReportRace(ThreadState *thr, RawShadow *shadow_mem, Shadow cur, Shadow old, DynamicMutexSet mset1; MutexSet *mset[kMop] = {&thr->mset, mset1}; - // 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 (SpuriousRace(old)) - return; - if (!RestoreStack(EventType::kAccessExt, s[1].sid(), s[1].epoch(), addr1, - size1, typ1, &tids[1], &traces[1], mset[1], &tags[1])) { - StoreShadow(&ctx->last_spurious_race, old.raw()); - return; - } +#if SANITIZER_APPLE + static Mutex report_write_mutex; + Lock report_write_lock(&report_write_mutex); + ScopedReport *_rep = (ScopedReport *)alloca(sizeof(ScopedReport)); +#endif + // Take a new scope as Apple platforms need to release the below locks, + // before writing out the report, in order to avoid a deadlock + { + // 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 (SpuriousRace(old)) + return; + if (!RestoreStack(EventType::kAccessExt, s[1].sid(), s[1].epoch(), addr1, + size1, typ1, &tids[1], &traces[1], mset[1], &tags[1])) { + StoreShadow(&ctx->last_spurious_race, old.raw()); + return; + } - if (IsFiredSuppression(ctx, rep_typ, traces[1])) - return; + if (IsFiredSuppression(ctx, rep_typ, traces[1])) + return; - if (HandleRacyStacks(thr, traces)) - return; + if (HandleRacyStacks(thr, traces)) + return; - // If any of the accesses has a tag, treat this as an "external" race. - uptr tag = kExternalTagNone; - for (uptr i = 0; i < kMop; i++) { - if (tags[i] != kExternalTagNone) { - rep_typ = ReportTypeExternalRace; - tag = tags[i]; - break; + // If any of the accesses has a tag, treat this as an "external" race. + uptr tag = kExternalTagNone; + for (uptr i = 0; i < kMop; i++) { + if (tags[i] != kExternalTagNone) { + rep_typ = ReportTypeExternalRace; + tag = tags[i]; + break; + } } - } - ScopedReport rep(rep_typ, tag); - for (uptr i = 0; i < kMop; i++) - rep.AddMemoryAccess(addr, tags[i], s[i], tids[i], traces[i], mset[i]); +#if SANITIZER_APPLE + new (_rep) ScopedReport(rep_typ, tag); + ScopedReport &rep = *_rep; +#else + ScopedReport rep(rep_typ, tag); +#endif + 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++) { - ThreadContext *tctx = static_cast( - ctx->thread_registry.GetThreadLocked(tids[i])); - rep.AddThread(tctx); - } + for (uptr i = 0; i < kMop; i++) { + ThreadContext *tctx = static_cast( + ctx->thread_registry.GetThreadLocked(tids[i])); + rep.AddThread(tctx); + } - 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; - ThreadContext *parent_tctx = static_cast( - ctx->thread_registry.GetThreadLocked(parent_tid)); - rep.AddThread(parent_tctx); + 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; + ThreadContext *parent_tctx = static_cast( + ctx->thread_registry.GetThreadLocked(parent_tid)); + rep.AddThread(parent_tctx); + } } - } #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 + +#if SANITIZER_APPLE + } // Close this scope to release the locks + + _rep->SymbolizeStackElems(); + OutputReport(thr, *_rep); + + // Need to manually destroy this because we used placement new to allocate + _rep->~ScopedReport(); +#else + rep.SymbolizeStackElems(); + OutputReport(thr, rep); + } #endif - OutputReport(thr, rep); } void PrintCurrentStack(ThreadState *thr, uptr pc) { diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp index 8d29e25a6dd20..d7083e3dace7b 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp +++ b/compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp @@ -96,6 +96,7 @@ void ThreadFinalize(ThreadState *thr) { ScopedReport rep(ReportTypeThreadLeak); rep.AddThread(leaks[i].tctx, true); rep.SetCount(leaks[i].count); + rep.SymbolizeStackElems(); OutputReport(thr, rep); } #endif