Skip to content

[TSan][compiler-rt] Move symbolization in ReportRace outside of locks on Apple platforms #149516

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
1 change: 1 addition & 0 deletions compiler-rt/lib/tsan/rtl/tsan_interface_ann.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
1 change: 1 addition & 0 deletions compiler-rt/lib/tsan/rtl/tsan_mman.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
8 changes: 8 additions & 0 deletions compiler-rt/lib/tsan/rtl/tsan_report.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -56,6 +58,7 @@ struct ReportMop {
bool atomic;
uptr external_tag;
Vector<ReportMopMutex> mset;
StackTrace stack_trace;
ReportStack *stack;

ReportMop();
Expand All @@ -79,6 +82,7 @@ struct ReportLocation {
int fd = 0;
bool fd_closed = false;
bool suppressable = false;
StackID stack_id = 0;
ReportStack *stack = nullptr;
};

Expand All @@ -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;
};

Expand All @@ -105,6 +112,7 @@ class ReportDesc {
Vector<ReportStack*> stacks;
Vector<ReportMop*> mops;
Vector<ReportLocation*> locs;
Vector<uptr> added_location_addrs;
Vector<ReportMutex*> mutexes;
Vector<ReportThread*> threads;
Vector<Tid> unique_tids;
Expand Down
1 change: 1 addition & 0 deletions compiler-rt/lib/tsan/rtl/tsan_rtl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
4 changes: 4 additions & 0 deletions compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
9 changes: 7 additions & 2 deletions compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
Expand All @@ -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);
}

Expand Down
185 changes: 122 additions & 63 deletions compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
//
//===----------------------------------------------------------------------===//

#include <alloca.h>

#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_libc.h"
#include "sanitizer_common/sanitizer_placement_new.h"
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
Expand All @@ -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
Expand Down Expand Up @@ -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;
}

Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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<uptr>(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<uptr>(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<ThreadContext *>(
ctx->thread_registry.GetThreadLocked(tids[i]));
rep.AddThread(tctx);
}
for (uptr i = 0; i < kMop; i++) {
ThreadContext *tctx = static_cast<ThreadContext *>(
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<ThreadContext *>(
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<ThreadContext *>(
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) {
Expand Down
1 change: 1 addition & 0 deletions compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading