Skip to content

Commit 7e7fb8d

Browse files
RalfJungRalfJung
committed
user-relevant span: if no frame is in a local crate, use topmost non-track_caller frame
1 parent 22dd266 commit 7e7fb8d

File tree

9 files changed

+185
-92
lines changed

9 files changed

+185
-92
lines changed

‎src/concurrency/thread.rs‎

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,6 @@ pub struct Thread<'tcx> {
181181

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

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

273+
pub fn current_user_relevance(&self) -> u8 {
274+
self.top_user_relevant_frame()
275+
.map(|frame_idx| self.stack[frame_idx].extra.user_relevance)
276+
.unwrap_or(0)
277+
}
278+
266279
pub fn current_user_relevant_span(&self) -> Span {
280+
debug_assert_eq!(self.top_user_relevant_frame, self.compute_top_user_relevant_frame(0));
267281
self.top_user_relevant_frame()
268282
.map(|frame_idx| self.stack[frame_idx].current_span())
269283
.unwrap_or(rustc_span::DUMMY_SP)

‎src/diagnostics.rs‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -187,16 +187,14 @@ pub fn prune_stacktrace<'tcx>(
187187
}
188188
BacktraceStyle::Short => {
189189
let original_len = stacktrace.len();
190-
// Only prune frames if there is at least one local frame. This check ensures that if
191-
// we get a backtrace that never makes it to the user code because it has detected a
192-
// bug in the Rust runtime, we don't prune away every frame.
193-
let has_local_frame = stacktrace.iter().any(|frame| machine.is_local(frame));
190+
// Remove all frames marked with `caller_location` -- that attribute indicates we
191+
// usually want to point at the caller, not them.
192+
stacktrace.retain(|frame| !frame.instance.def.requires_caller_location(machine.tcx));
193+
// Only prune further frames if there is at least one local frame. This check ensures
194+
// that if we get a backtrace that never makes it to the user code because it has
195+
// detected a bug in the Rust runtime, we don't prune away every frame.
196+
let has_local_frame = stacktrace.iter().any(|frame| machine.is_local(frame.instance));
194197
if has_local_frame {
195-
// Remove all frames marked with `caller_location` -- that attribute indicates we
196-
// usually want to point at the caller, not them.
197-
stacktrace
198-
.retain(|frame| !frame.instance.def.requires_caller_location(machine.tcx));
199-
200198
// This is part of the logic that `std` uses to select the relevant part of a
201199
// backtrace. But here, we only look for __rust_begin_short_backtrace, not
202200
// __rust_end_short_backtrace because the end symbol comes from a call to the default
@@ -216,7 +214,7 @@ pub fn prune_stacktrace<'tcx>(
216214
// This len check ensures that we don't somehow remove every frame, as doing so breaks
217215
// the primary error message.
218216
while stacktrace.len() > 1
219-
&& stacktrace.last().is_some_and(|frame| !machine.is_local(frame))
217+
&& stacktrace.last().is_some_and(|frame| !machine.is_local(frame.instance))
220218
{
221219
stacktrace.pop();
222220
}
@@ -618,7 +616,7 @@ pub fn report_msg<'tcx>(
618616
write!(backtrace_title, ":").unwrap();
619617
err.note(backtrace_title);
620618
for (idx, frame_info) in stacktrace.iter().enumerate() {
621-
let is_local = machine.is_local(frame_info);
619+
let is_local = machine.is_local(frame_info.instance);
622620
// No span for non-local frames and the first frame (which is the error site).
623621
if is_local && idx > 0 {
624622
err.subdiagnostic(frame_info.as_note(machine.tcx));

‎src/helpers.rs‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1088,11 +1088,18 @@ impl<'tcx> MiriMachine<'tcx> {
10881088
self.threads.active_thread_ref().top_user_relevant_frame()
10891089
}
10901090

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

‎src/machine.rs‎

Lines changed: 22 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -139,11 +139,10 @@ pub struct FrameExtra<'tcx> {
139139
/// we use this to register a completed event with `measureme`.
140140
pub timing: Option<measureme::DetachedTiming>,
141141

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

148147
/// Data race detector per-frame data.
149148
pub data_race: Option<data_race::FrameState>,
@@ -152,26 +151,21 @@ pub struct FrameExtra<'tcx> {
152151
impl<'tcx> std::fmt::Debug for FrameExtra<'tcx> {
153152
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
154153
// Omitting `timing`, it does not support `Debug`.
155-
let FrameExtra { borrow_tracker, catch_unwind, timing: _, is_user_relevant, data_race } =
154+
let FrameExtra { borrow_tracker, catch_unwind, timing: _, user_relevance, data_race } =
156155
self;
157156
f.debug_struct("FrameData")
158157
.field("borrow_tracker", borrow_tracker)
159158
.field("catch_unwind", catch_unwind)
160-
.field("is_user_relevant", is_user_relevant)
159+
.field("user_relevance", user_relevance)
161160
.field("data_race", data_race)
162161
.finish()
163162
}
164163
}
165164

166165
impl VisitProvenance for FrameExtra<'_> {
167166
fn visit_provenance(&self, visit: &mut VisitWith<'_>) {
168-
let FrameExtra {
169-
catch_unwind,
170-
borrow_tracker,
171-
timing: _,
172-
is_user_relevant: _,
173-
data_race: _,
174-
} = self;
167+
let FrameExtra { catch_unwind, borrow_tracker, timing: _, user_relevance: _, data_race: _ } =
168+
self;
175169

176170
catch_unwind.visit_provenance(visit);
177171
borrow_tracker.visit_provenance(visit);
@@ -910,8 +904,8 @@ impl<'tcx> MiriMachine<'tcx> {
910904
}
911905

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

@@ -1698,7 +1692,7 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
16981692
borrow_tracker: borrow_tracker.map(|bt| bt.borrow_mut().new_frame()),
16991693
catch_unwind: None,
17001694
timing,
1701-
is_user_relevant: ecx.machine.is_user_relevant(&frame),
1695+
user_relevance: ecx.machine.user_relevance(&frame),
17021696
data_race: ecx
17031697
.machine
17041698
.data_race
@@ -1754,9 +1748,9 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
17541748

17551749
#[inline(always)]
17561750
fn after_stack_push(ecx: &mut InterpCx<'tcx, Self>) -> InterpResult<'tcx> {
1757-
if ecx.frame().extra.is_user_relevant {
1758-
// We just pushed a local frame, so we know that the topmost local frame is the topmost
1759-
// frame. If we push a non-local frame, there's no need to do anything.
1751+
if ecx.frame().extra.user_relevance >= ecx.active_thread_ref().current_user_relevance() {
1752+
// We just pushed a frame that's at least as relevant as the so-far most relevant frame.
1753+
// That means we are now the most relevant frame.
17601754
let stack_len = ecx.active_thread_stack().len();
17611755
ecx.active_thread_mut().set_top_user_relevant_frame(stack_len - 1);
17621756
}
@@ -1770,9 +1764,14 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
17701764
if ecx.machine.borrow_tracker.is_some() {
17711765
ecx.on_stack_pop(frame)?;
17721766
}
1773-
if frame.extra.is_user_relevant {
1774-
// All that we store is whether or not the frame we just removed is local, so now we
1775-
// have no idea where the next topmost local frame is. So we recompute it.
1767+
if ecx
1768+
.active_thread_ref()
1769+
.top_user_relevant_frame()
1770+
.expect("there should always be a most relevant frame for a non-empty stack")
1771+
== ecx.frame_idx()
1772+
{
1773+
// We are popping the most relevant frame. We have no clue what the next relevant frame
1774+
// below that is, so we recompute that.
17761775
// (If this ever becomes a bottleneck, we could have `push` store the previous
17771776
// user-relevant frame and restore that here.)
17781777
// We have to skip the frame that is just being popped.

‎tests/genmc/pass/std/arc.check_count.stderr‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
Running GenMC Verification...
22
warning: GenMC currently does not model spurious failures of `compare_exchange_weak`. Miri with GenMC might miss bugs related to spurious failures.
3-
--> RUSTLIB/core/src/sync/atomic.rs:LL:CC
3+
--> RUSTLIB/std/src/thread/mod.rs:LL:CC
44
|
5-
LL | intrinsics::atomic_cxchgweak::<T, { AO::Relaxed }, { AO::Relaxed }>(dst, old, new)
6-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
5+
LL | match COUNTER.compare_exchange_weak(last, id, Ordering::Relaxed, Ordering::Relaxed) {
6+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
77
|
88
= note: BACKTRACE:
99
= note: inside closure at RUSTLIB/std/src/thread/current.rs:LL:CC
@@ -91,19 +91,34 @@ LL | handle.join().unwrap();
9191
| ^^^^^^^^^^^^^
9292

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

106+
warning: GenMC currently does not model spurious failures of `compare_exchange_weak`. Miri with GenMC might miss bugs related to spurious failures.
107+
--> RUSTLIB/std/src/sync/once.rs:LL:CC
108+
|
109+
LL | self.inner.call(true, &mut |p| f.take().unwrap()(p));
110+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
111+
|
112+
= note: BACKTRACE:
113+
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC
114+
= note: inside closure at RUSTLIB/std/src/sync/once.rs:LL:CC
115+
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC
116+
102117
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.
103-
--> RUSTLIB/core/src/sync/atomic.rs:LL:CC
118+
--> RUSTLIB/std/src/sys/exit_guard.rs:LL:CC
104119
|
105-
LL | intrinsics::atomic_cxchg::<T, { AO::Acquire }, { AO::Relaxed }>(dst, old, new)
106-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
120+
LL | match EXITING_THREAD_ID.compare_exchange(ptr::null_mut(), this_thread_id, Acquire, Relaxed) {
121+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
107122
|
108123
= note: BACKTRACE:
109124
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC

‎tests/genmc/pass/std/arc.try_upgrade.stderr‎

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
Running GenMC Verification...
22
warning: GenMC currently does not model spurious failures of `compare_exchange_weak`. Miri with GenMC might miss bugs related to spurious failures.
3-
--> RUSTLIB/core/src/sync/atomic.rs:LL:CC
3+
--> RUSTLIB/std/src/thread/mod.rs:LL:CC
44
|
5-
LL | intrinsics::atomic_cxchgweak::<T, { AO::Relaxed }, { AO::Relaxed }>(dst, old, new)
6-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
5+
LL | match COUNTER.compare_exchange_weak(last, id, Ordering::Relaxed, Ordering::Relaxed) {
6+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
77
|
88
= note: BACKTRACE:
99
= note: inside closure at RUSTLIB/std/src/thread/current.rs:LL:CC
@@ -91,19 +91,34 @@ LL | handle.join().unwrap();
9191
| ^^^^^^^^^^^^^
9292

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

106+
warning: GenMC currently does not model spurious failures of `compare_exchange_weak`. Miri with GenMC might miss bugs related to spurious failures.
107+
--> RUSTLIB/std/src/sync/once.rs:LL:CC
108+
|
109+
LL | self.inner.call(true, &mut |p| f.take().unwrap()(p));
110+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
111+
|
112+
= note: BACKTRACE:
113+
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC
114+
= note: inside closure at RUSTLIB/std/src/sync/once.rs:LL:CC
115+
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC
116+
102117
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.
103-
--> RUSTLIB/core/src/sync/atomic.rs:LL:CC
118+
--> RUSTLIB/std/src/sys/exit_guard.rs:LL:CC
104119
|
105-
LL | intrinsics::atomic_cxchg::<T, { AO::Acquire }, { AO::Relaxed }>(dst, old, new)
106-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
120+
LL | match EXITING_THREAD_ID.compare_exchange(ptr::null_mut(), this_thread_id, Acquire, Relaxed) {
121+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ GenMC might miss possible behaviors of this code
107122
|
108123
= note: BACKTRACE:
109124
= note: inside closure at RUSTLIB/std/src/rt.rs:LL:CC

0 commit comments

Comments
 (0)