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
Merged

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

Show file tree
Hide file tree
Changes from 2 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
1 change: 1 addition & 0 deletions compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
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
1 change: 1 addition & 0 deletions 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
9 changes: 7 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 @@ -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,15 +540,18 @@ 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
74 changes: 59 additions & 15 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,55 @@ 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++) {
Copy link
Contributor

@thurstond thurstond Jul 29, 2025

Choose a reason for hiding this comment

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

Nits throughout this function: https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop

Copy link
Contributor Author

@DanBlackwell DanBlackwell Jul 30, 2025

Choose a reason for hiding this comment

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

Have addressed this now.

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++) {
// added locations have a NULL placeholder - don't dereference them
if (ReportLocation *loc = rep_->locs[i])
loc->stack = SymbolizeStackId(loc->stack_id);
}
Copy link
Contributor

@thurstond thurstond Jul 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This can be simplified by moving this code block after the filtering step (or combining them)

Copy link
Contributor Author

@DanBlackwell DanBlackwell Jul 30, 2025

Choose a reason for hiding this comment

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

We run SymboliseData on any added locations, but SymbolizeStackId on the other locs; so I don't think we could move this after and drop the if-statement unfortunately :/.

I could interleave it with the // symbolize any added locations block, but given that this is (hopefully) a cold path my preference would be for legibility over optimisation.

Copy link
Contributor

@thurstond thurstond Jul 30, 2025

Choose a reason for hiding this comment

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

SGTM!


// symbolize any added locations
for (usize i = 0; i < rep_->added_location_addrs.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
Vector<ReportLocation *> filtered_locs;
for (usize i = 0; i < rep_->locs.Size(); i++)
if (rep_->locs[i] != nullptr)
filtered_locs.PushBack(rep_->locs[i]);
rep_->locs.Resize(filtered_locs.Size());
for (usize i = 0; i < filtered_locs.Size(); i++)
rep_->locs[i] = (filtered_locs[i]);
Copy link
Contributor

@thurstond thurstond Jul 29, 2025

Choose a reason for hiding this comment

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

FWIW I think the in-place compaction could be done with (untested):

  usize j = 0; // Index past the last compacted element
  usize size = rep_->locs.Size();
  for (usize i = 0; i < size; i++) {
    if (rep_->locs[i] != nullptr) {
      rep_->locs[j] = rep_->locs[i];
      j++;
    }
  }
  rep_->locs.Resize(j);

but I'm ok with the current code.

Copy link
Contributor Author

@DanBlackwell DanBlackwell Jul 30, 2025

Choose a reason for hiding this comment

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

I have swapped this in-place in now


// 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 +264,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 +316,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 +334,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 +356,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 +370,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

@thurstond thurstond Jul 29, 2025

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

@DanBlackwell DanBlackwell Jul 29, 2025

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

@DanBlackwell DanBlackwell Jul 29, 2025

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

@thurstond thurstond Jul 29, 2025

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

@DanBlackwell DanBlackwell Jul 29, 2025

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

@DanBlackwell DanBlackwell Jul 29, 2025

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

@thurstond thurstond Jul 29, 2025

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 @@ -819,6 +862,7 @@ void ReportRace(ThreadState *thr, RawShadow *shadow_mem, Shadow cur, Shadow old,
s[1].epoch() <= thr->last_sleep_clock.Get(s[1].sid()))
rep.AddSleep(thr->last_sleep_stack_id);
#endif
rep.SymbolizeStackElems();
OutputReport(thr, rep);
}

Expand Down
1 change: 1 addition & 0 deletions compiler-rt/lib/tsan/rtl/tsan_rtl_thread.cpp
View file
Open in desktop
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