Skip to content

user-relevant span: if no frame is in a local crate, use topmost non-track_caller frame #4653

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+186 −92
Open

user-relevant span: if no frame is in a local crate, use topmost non-track_caller frame #4653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 22 additions & 8 deletions src/concurrency/thread.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ pub struct Thread<'tcx> {

/// The index of the topmost user-relevant frame in `stack`. This field must contain
/// the value produced by `get_top_user_relevant_frame`.
/// The `None` state here represents
/// This field is a cache to reduce how often we call that method. The cache is manually
/// maintained inside `MiriMachine::after_stack_push` and `MiriMachine::after_stack_pop`.
top_user_relevant_frame: Option<usize>,
Expand Down Expand Up @@ -232,12 +231,21 @@ impl<'tcx> Thread<'tcx> {
/// justifying the optimization that only pushes of user-relevant frames require updating the
/// `top_user_relevant_frame` field.
fn compute_top_user_relevant_frame(&self, skip: usize) -> Option<usize> {
self.stack
.iter()
.enumerate()
.rev()
.skip(skip)
.find_map(|(idx, frame)| if frame.extra.is_user_relevant { Some(idx) } else { None })
// We are search for the frame with maximum relevance.
let mut best = None;
for (idx, frame) in self.stack.iter().enumerate().rev().skip(skip) {
let relevance = frame.extra.user_relevance;
if relevance == u8::MAX {
// We can short-circuit this search.
return Some(idx);
}
if best.is_none_or(|(_best_idx, best_relevance)| best_relevance < relevance) {
// The previous best frame has strictly worse relevance, so despite us being lower
// in the stack, we win.
best = Some((idx, relevance));
}
}
best.map(|(idx, _relevance)| idx)
}

/// Re-compute the top user-relevant frame from scratch. `skip` indicates how many top frames
Expand All @@ -256,14 +264,20 @@ impl<'tcx> Thread<'tcx> {
/// Returns the topmost frame that is considered user-relevant, or the
/// top of the stack if there is no such frame, or `None` if the stack is empty.
pub fn top_user_relevant_frame(&self) -> Option<usize> {
debug_assert_eq!(self.top_user_relevant_frame, self.compute_top_user_relevant_frame(0));
// This can be called upon creation of an allocation. We create allocations while setting up
// parts of the Rust runtime when we do not have any stack frames yet, so we need to handle
// empty stacks.
self.top_user_relevant_frame.or_else(|| self.stack.len().checked_sub(1))
}

pub fn current_user_relevance(&self) -> u8 {
self.top_user_relevant_frame()
.map(|frame_idx| self.stack[frame_idx].extra.user_relevance)
.unwrap_or(0)
}

pub fn current_user_relevant_span(&self) -> Span {
debug_assert_eq!(self.top_user_relevant_frame, self.compute_top_user_relevant_frame(0));
self.top_user_relevant_frame()
.map(|frame_idx| self.stack[frame_idx].current_span())
.unwrap_or(rustc_span::DUMMY_SP)
Expand Down
20 changes: 9 additions & 11 deletions src/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,16 +187,14 @@ pub fn prune_stacktrace<'tcx>(
}
BacktraceStyle::Short => {
let original_len = stacktrace.len();
// Only prune frames if there is at least one local frame. This check ensures that if
// we get a backtrace that never makes it to the user code because it has detected a
// bug in the Rust runtime, we don't prune away every frame.
let has_local_frame = stacktrace.iter().any(|frame| machine.is_local(frame));
// Remove all frames marked with `caller_location` -- that attribute indicates we
// usually want to point at the caller, not them.
stacktrace.retain(|frame| !frame.instance.def.requires_caller_location(machine.tcx));
// Only prune further frames if there is at least one local frame. This check ensures
// that if we get a backtrace that never makes it to the user code because it has
// detected a bug in the Rust runtime, we don't prune away every frame.
let has_local_frame = stacktrace.iter().any(|frame| machine.is_local(frame.instance));
if has_local_frame {
// Remove all frames marked with `caller_location` -- that attribute indicates we
// usually want to point at the caller, not them.
stacktrace
.retain(|frame| !frame.instance.def.requires_caller_location(machine.tcx));

// This is part of the logic that `std` uses to select the relevant part of a
// backtrace. But here, we only look for __rust_begin_short_backtrace, not
// __rust_end_short_backtrace because the end symbol comes from a call to the default
Expand All @@ -216,7 +214,7 @@ pub fn prune_stacktrace<'tcx>(
// This len check ensures that we don't somehow remove every frame, as doing so breaks
// the primary error message.
while stacktrace.len() > 1
&& stacktrace.last().is_some_and(|frame| !machine.is_local(frame))
&& stacktrace.last().is_some_and(|frame| !machine.is_local(frame.instance))
{
stacktrace.pop();
}
Expand Down Expand Up @@ -618,7 +616,7 @@ pub fn report_msg<'tcx>(
write!(backtrace_title, ":").unwrap();
err.note(backtrace_title);
for (idx, frame_info) in stacktrace.iter().enumerate() {
let is_local = machine.is_local(frame_info);
let is_local = machine.is_local(frame_info.instance);
// No span for non-local frames and the first frame (which is the error site).
if is_local && idx > 0 {
err.subdiagnostic(frame_info.as_note(machine.tcx));
Expand Down
17 changes: 12 additions & 5 deletions src/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1088,11 +1088,18 @@ impl<'tcx> MiriMachine<'tcx> {
self.threads.active_thread_ref().top_user_relevant_frame()
}

/// This is the source of truth for the `is_user_relevant` flag in our `FrameExtra`.
pub fn is_user_relevant(&self, frame: &Frame<'tcx, Provenance>) -> bool {
let def_id = frame.instance().def_id();
(def_id.is_local() || self.user_relevant_crates.contains(&def_id.krate))
&& !frame.instance().def.requires_caller_location(self.tcx)
/// This is the source of truth for the `user_relevance` flag in our `FrameExtra`.
pub fn user_relevance(&self, frame: &Frame<'tcx, Provenance>) -> u8 {
if frame.instance().def.requires_caller_location(self.tcx) {
return 0;
}
if self.is_local(frame.instance()) {
u8::MAX
} else {
// A non-relevant frame, but at least it doesn't require a caller location, so
// better than nothing.
1
}
}
}

Expand Down
45 changes: 22 additions & 23 deletions src/machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,11 +139,10 @@ pub struct FrameExtra<'tcx> {
/// we use this to register a completed event with `measureme`.
pub timing: Option<measureme::DetachedTiming>,

/// Indicates whether a `Frame` is part of a workspace-local crate and is also not
/// `#[track_caller]`. We compute this once on creation and store the result, as an
/// optimization.
/// This is used by `MiriMachine::current_span` and `MiriMachine::caller_span`
pub is_user_relevant: bool,
/// Indicates how user-relevant this frame is. `#[track_caller]` frames are never relevant.
/// Frames from user-relevant crates are maximally relevant; frames from other crates are less
/// relevant.
pub user_relevance: u8,

/// Data race detector per-frame data.
pub data_race: Option<data_race::FrameState>,
Expand All @@ -152,26 +151,21 @@ pub struct FrameExtra<'tcx> {
impl<'tcx> std::fmt::Debug for FrameExtra<'tcx> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
// Omitting `timing`, it does not support `Debug`.
let FrameExtra { borrow_tracker, catch_unwind, timing: _, is_user_relevant, data_race } =
let FrameExtra { borrow_tracker, catch_unwind, timing: _, user_relevance, data_race } =
self;
f.debug_struct("FrameData")
.field("borrow_tracker", borrow_tracker)
.field("catch_unwind", catch_unwind)
.field("is_user_relevant", is_user_relevant)
.field("user_relevance", user_relevance)
.field("data_race", data_race)
.finish()
}
}

impl VisitProvenance for FrameExtra<'_> {
fn visit_provenance(&self, visit: &mut VisitWith<'_>) {
let FrameExtra {
catch_unwind,
borrow_tracker,
timing: _,
is_user_relevant: _,
data_race: _,
} = self;
let FrameExtra { catch_unwind, borrow_tracker, timing: _, user_relevance: _, data_race: _ } =
self;

catch_unwind.visit_provenance(visit);
borrow_tracker.visit_provenance(visit);
Expand Down Expand Up @@ -910,8 +904,8 @@ impl<'tcx> MiriMachine<'tcx> {
}

/// Check whether the stack frame that this `FrameInfo` refers to is part of a local crate.
pub(crate) fn is_local(&self, frame: &FrameInfo<'_>) -> bool {
let def_id = frame.instance.def_id();
pub(crate) fn is_local(&self, instance: ty::Instance<'tcx>) -> bool {
let def_id = instance.def_id();
def_id.is_local() || self.user_relevant_crates.contains(&def_id.krate)
}

Expand Down Expand Up @@ -1698,7 +1692,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
borrow_tracker: borrow_tracker.map(|bt| bt.borrow_mut().new_frame()),
catch_unwind: None,
timing,
is_user_relevant: ecx.machine.is_user_relevant(&frame),
user_relevance: ecx.machine.user_relevance(&frame),
data_race: ecx
.machine
.data_race
Expand Down Expand Up @@ -1754,9 +1748,9 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {

#[inline(always)]
fn after_stack_push(ecx: &mut InterpCx<'tcx, Self>) -> InterpResult<'tcx> {
if ecx.frame().extra.is_user_relevant {
// We just pushed a local frame, so we know that the topmost local frame is the topmost
// frame. If we push a non-local frame, there's no need to do anything.
if ecx.frame().extra.user_relevance >= ecx.active_thread_ref().current_user_relevance() {
// We just pushed a frame that's at least as relevant as the so-far most relevant frame.
// That means we are now the most relevant frame.
let stack_len = ecx.active_thread_stack().len();
ecx.active_thread_mut().set_top_user_relevant_frame(stack_len - 1);
}
Expand All @@ -1770,9 +1764,14 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
if ecx.machine.borrow_tracker.is_some() {
ecx.on_stack_pop(frame)?;
}
if frame.extra.is_user_relevant {
// All that we store is whether or not the frame we just removed is local, so now we
// have no idea where the next topmost local frame is. So we recompute it.
if ecx
.active_thread_ref()
.top_user_relevant_frame()
.expect("there should always be a most relevant frame for a non-empty stack")
== ecx.frame_idx()
{
// We are popping the most relevant frame. We have no clue what the next relevant frame
// below that is, so we recompute that.
// (If this ever becomes a bottleneck, we could have `push` store the previous
// user-relevant frame and restore that here.)
// We have to skip the frame that is just being popped.
Expand Down
1 change: 1 addition & 0 deletions tests/fail/panic/tls_macro_drop_panic.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ ow
fatal runtime error: thread local panicked on drop, aborting
error: abnormal termination: the program aborted execution


error: aborting due to 1 previous error

33 changes: 24 additions & 9 deletions tests/genmc/pass/std/arc.check_count.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
Running GenMC Verification...
warning: GenMC currently does not model spurious failures of `compare_exchange_weak`. Miri with GenMC might miss bugs related to spurious failures.
--> RUSTLIB/core/src/sync/atomic.rs:LL:CC
--> RUSTLIB/std/src/thread/mod.rs:LL:CC
|
LL | intrinsics::atomic_cxchgweak::<T, { AO::Relaxed }, { AO::Relaxed }>(dst, old, new)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
LL | match COUNTER.compare_exchange_weak(last, id, Ordering::Relaxed, Ordering::Relaxed) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
|
= note: BACKTRACE:
= note: inside closure at RUSTLIB/std/src/thread/current.rs:LL:CC
Expand Down Expand Up @@ -91,19 +91,34 @@ LL | handle.join().unwrap();
| ^^^^^^^^^^^^^

warning: GenMC currently does not model spurious failures of `compare_exchange_weak`. Miri with GenMC might miss bugs related to spurious failures.
--> RUSTLIB/core/src/sync/atomic.rs:LL:CC
--> RUSTLIB/std/src/rt.rs:LL:CC
|
LL | intrinsics::atomic_cxchgweak::<T, { AO::Acquire }, { AO::Acquire }>(dst, old, new)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
LL | / CLEANUP.call_once(|| unsafe {
LL | | // Flush stdout and disable buffering.
LL | | crate::io::cleanup();
... |
LL | | });
| |______^ GenMC might miss possible behaviors of this code
|
= note: BACKTRACE:
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC

warning: GenMC currently does not model spurious failures of `compare_exchange_weak`. Miri with GenMC might miss bugs related to spurious failures.
--> RUSTLIB/std/src/sync/once.rs:LL:CC
|
LL | self.inner.call(true, &mut |p| f.take().unwrap()(p));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
|
= note: BACKTRACE:
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC
= note: inside closure at RUSTLIB/std/src/sync/once.rs:LL:CC
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC

warning: GenMC currently does not model the failure ordering for `compare_exchange`. Due to success ordering 'Acquire', the failure ordering 'Relaxed' is treated like 'Acquire'. Miri with GenMC might miss bugs related to this memory access.
--> RUSTLIB/core/src/sync/atomic.rs:LL:CC
--> RUSTLIB/std/src/sys/exit_guard.rs:LL:CC
|
LL | intrinsics::atomic_cxchg::<T, { AO::Acquire }, { AO::Relaxed }>(dst, old, new)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
LL | match EXITING_THREAD_ID.compare_exchange(ptr::null_mut(), this_thread_id, Acquire, Relaxed) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
|
= note: BACKTRACE:
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC
Expand Down
33 changes: 24 additions & 9 deletions tests/genmc/pass/std/arc.try_upgrade.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
Running GenMC Verification...
warning: GenMC currently does not model spurious failures of `compare_exchange_weak`. Miri with GenMC might miss bugs related to spurious failures.
--> RUSTLIB/core/src/sync/atomic.rs:LL:CC
--> RUSTLIB/std/src/thread/mod.rs:LL:CC
|
LL | intrinsics::atomic_cxchgweak::<T, { AO::Relaxed }, { AO::Relaxed }>(dst, old, new)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
LL | match COUNTER.compare_exchange_weak(last, id, Ordering::Relaxed, Ordering::Relaxed) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
|
= note: BACKTRACE:
= note: inside closure at RUSTLIB/std/src/thread/current.rs:LL:CC
Expand Down Expand Up @@ -91,19 +91,34 @@ LL | handle.join().unwrap();
| ^^^^^^^^^^^^^

warning: GenMC currently does not model spurious failures of `compare_exchange_weak`. Miri with GenMC might miss bugs related to spurious failures.
--> RUSTLIB/core/src/sync/atomic.rs:LL:CC
--> RUSTLIB/std/src/rt.rs:LL:CC
|
LL | intrinsics::atomic_cxchgweak::<T, { AO::Acquire }, { AO::Acquire }>(dst, old, new)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
LL | / CLEANUP.call_once(|| unsafe {
LL | | // Flush stdout and disable buffering.
LL | | crate::io::cleanup();
... |
LL | | });
| |______^ GenMC might miss possible behaviors of this code
|
= note: BACKTRACE:
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC

warning: GenMC currently does not model spurious failures of `compare_exchange_weak`. Miri with GenMC might miss bugs related to spurious failures.
--> RUSTLIB/std/src/sync/once.rs:LL:CC
|
LL | self.inner.call(true, &mut |p| f.take().unwrap()(p));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
|
= note: BACKTRACE:
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC
= note: inside closure at RUSTLIB/std/src/sync/once.rs:LL:CC
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC

warning: GenMC currently does not model the failure ordering for `compare_exchange`. Due to success ordering 'Acquire', the failure ordering 'Relaxed' is treated like 'Acquire'. Miri with GenMC might miss bugs related to this memory access.
--> RUSTLIB/core/src/sync/atomic.rs:LL:CC
--> RUSTLIB/std/src/sys/exit_guard.rs:LL:CC
|
LL | intrinsics::atomic_cxchg::<T, { AO::Acquire }, { AO::Relaxed }>(dst, old, new)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
LL | match EXITING_THREAD_ID.compare_exchange(ptr::null_mut(), this_thread_id, Acquire, Relaxed) {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
|
= note: BACKTRACE:
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC
Expand Down
Loading