Skip to content

[TSan][compiler-rt] Defer symbolization of Reports to as late as possible #151120

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jul 30, 2025
+80 −19
Merged

[TSan][compiler-rt] Defer symbolization of Reports to as late as possible #151120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4a6ba6a
[TSan][compiler-rt] Defer symbolization of Reports to as late as poss…
DanBlackwell Jul 18, 2025
d7cb86d
Fixup - simplify the location symbolizing logic
DanBlackwell Jul 29, 2025
4b76f89
Do in-place filtering of added locs, and cache Vector::Size() when lo…
DanBlackwell Jul 30, 2025
0c3539e
Move SymbolizeStackElems into OutputReport
DanBlackwell Jul 30, 2025
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
13 changes: 13 additions & 0 deletions compiler-rt/lib/tsan/rtl/tsan_report.h
View file
Open in desktop
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,22 +93,31 @@ 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;
};

struct AddedLocationAddr {
uptr addr;
usize locs_idx;
};

class ReportDesc {
public:
ReportType typ;
uptr tag;
Vector<ReportStack*> stacks;
Vector<ReportMop*> mops;
Vector<ReportLocation*> locs;
Vector<AddedLocationAddr> added_location_addrs;
Vector<ReportMutex*> mutexes;
Vector<ReportThread*> threads;
Vector<Tid> unique_tids;
Expand Down
3 changes: 2 additions & 1 deletion compiler-rt/lib/tsan/rtl/tsan_rtl.h
View file
Open in desktop
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 Expand Up @@ -498,7 +499,7 @@ void ForkChildAfter(ThreadState *thr, uptr pc, bool start_thread);

void ReportRace(ThreadState *thr, RawShadow *shadow_mem, Shadow cur, Shadow old,
AccessType typ);
bool OutputReport(ThreadState *thr, const ScopedReport &srep);
bool OutputReport(ThreadState *thr, ScopedReport &srep);
bool IsFiredSuppression(Context *ctx, ReportType type, StackTrace trace);
bool IsExpectedReport(uptr addr, uptr size);

Expand Down
6 changes: 4 additions & 2 deletions compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,13 +539,15 @@ 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);
}
}
OutputReport(thr, rep);
Expand Down
77 changes: 61 additions & 16 deletions compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
//===----------------------------------------------------------------------===//

#include "sanitizer_common/sanitizer_common.h"
#include "sanitizer_common/sanitizer_internal_defs.h"
#include "sanitizer_common/sanitizer_libc.h"
#include "sanitizer_common/sanitizer_placement_new.h"
#include "sanitizer_common/sanitizer_stackdepot.h"
Expand Down Expand Up @@ -187,10 +188,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 +198,56 @@ void ScopedReportBase::AddMemoryAccess(uptr addr, uptr external_tag, Shadow s,
}
}

void ScopedReportBase::SymbolizeStackElems() {
// symbolize memory ops
for (usize i = 0, size = rep_->mops.Size(); i < 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, size = rep_->locs.Size(); i < size; i++) {
// added locations have a NULL placeholder - don't dereference them
if (ReportLocation *loc = rep_->locs[i])
loc->stack = SymbolizeStackId(loc->stack_id);
}

// symbolize any added locations
for (usize i = 0, size = rep_->added_location_addrs.Size(); i < size; i++) {
AddedLocationAddr *added_loc = &rep_->added_location_addrs[i];
if (ReportLocation *loc = SymbolizeData(added_loc->addr)) {
loc->suppressable = true;
rep_->locs[added_loc->locs_idx] = loc;
}
}

// Filter out any added location placeholders that could not be symbolized
usize j = 0;
for (usize i = 0, size = rep_->locs.Size(); i < size; i++) {
if (rep_->locs[i] != nullptr) {
rep_->locs[j] = rep_->locs[i];
j++;
}
}
rep_->locs.Resize(j);

// symbolize threads
for (usize i = 0, size = rep_->threads.Size(); i < 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, size = rep_->mutexes.Size(); i < 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 +265,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 +317,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 +335,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 +357,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 +371,8 @@ 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, rep_->locs.Size()});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if we also added a placeholder value here:

rep_->locs.PushBack(nullptr);

then ScopedReportBase::SymbolizeStackElems() could be greatly simplified: loop through added_location_addrs and overwrite the placeholder values at rep_->locs[added_loc_addr->locs_idx].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try doing that originally, IIRC it ended up with me having to add null checks in other places (that felt kind of 'magic') - I'll redo the change and push it up as an additional commit and we can discuss which is the better approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I remember the tricky bit - we need to delete the placeholder if if (ReportLocation *loc = SymbolizeData(added_loc->addr)) evaluates to false (as we wouldn't PushBack in that case previously).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, that's a tricky bit I hadn't considered.

I suspect adding the placeholders here and then deleting them in SymbolizeStackElems() would still be simpler than the current approach. Conditional deletion of placeholders in rep_->locs (effectively, compaction) could be done in-place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup; kept it simple with the null filtering as the internal Vector type has limited functionality. imo this looks way neater than that mess I had there before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to say - just pushed up a commit with the change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it looks pretty good! Left a few comments.

rep_->locs.PushBack(nullptr);
}

#if !SANITIZER_GO
Expand Down Expand Up @@ -628,11 +672,12 @@ static bool HandleRacyStacks(ThreadState *thr, VarSizeStackTrace traces[2]) {
return false;
}

bool OutputReport(ThreadState *thr, const ScopedReport &srep) {
bool OutputReport(ThreadState *thr, ScopedReport &srep) {
// These should have been checked in ShouldReport.
// It's too late to check them here, we have already taken locks.
CHECK(flags()->report_bugs);
CHECK(!thr->suppress_reports);
srep.SymbolizeStackElems();
atomic_store_relaxed(&ctx->last_symbolize_time_ns, NanoTime());
const ReportDesc *rep = srep.GetReport();
CHECK_EQ(thr->current_report, nullptr);
Expand Down
Loading