From 45d7121e9edb15873dce8868fe09c666aa451ecc Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Thu, 8 Sep 2022 19:16:50 -0400 Subject: [PATCH 1/2] Don't move too far down the call stack when reporting FnEntry diagnostics --- src/tools/miri/src/helpers.rs | 26 +++++++++-------- .../miri/src/stacked_borrows/diagnostics.rs | 4 +-- .../fail/stacked_borrows/aliasing_mut3.stderr | 2 +- .../fnentry_invalidation.stderr | 2 +- .../stacked_borrows/fnentry_invalidation2.rs | 21 ++++++++++++++ .../fnentry_invalidation2.stderr | 28 +++++++++++++++++++ 6 files changed, 67 insertions(+), 16 deletions(-) create mode 100644 src/tools/miri/tests/fail/stacked_borrows/fnentry_invalidation2.rs create mode 100644 src/tools/miri/tests/fail/stacked_borrows/fnentry_invalidation2.stderr diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs index 0f0bfa355bdc..15833fe42adc 100644 --- a/src/tools/miri/src/helpers.rs +++ b/src/tools/miri/src/helpers.rs @@ -1,5 +1,6 @@ pub mod convert; +use std::cmp; use std::mem; use std::num::NonZeroUsize; use std::time::Duration; @@ -908,24 +909,25 @@ impl<'a, 'mir: 'a, 'tcx: 'a + 'mir> CurrentSpan<'a, 'mir, 'tcx> { /// This function is backed by a cache, and can be assumed to be very fast. pub fn get(&mut self) -> Span { let idx = self.current_frame_idx(); - Self::frame_span(self.machine, idx) + self.stack().get(idx).map(Frame::current_span).unwrap_or(rustc_span::DUMMY_SP) } - /// Similar to `CurrentSpan::get`, but retrieves the parent frame of the first non-local frame. + /// Returns the span of the *caller* of the current operation, again + /// walking down the stack to find the closest frame in a local crate, if the caller of the + /// current operation is not in a local crate. /// This is useful when we are processing something which occurs on function-entry and we want /// to point at the call to the function, not the function definition generally. - pub fn get_parent(&mut self) -> Span { - let idx = self.current_frame_idx(); - Self::frame_span(self.machine, idx.wrapping_sub(1)) + pub fn get_caller(&mut self) -> Span { + // We need to go down at least to the caller (len - 2), or however + // far we have to go to find a frame in a local crate. + let local_frame_idx = self.current_frame_idx(); + let stack = self.stack(); + let idx = cmp::min(local_frame_idx, stack.len().saturating_sub(2)); + stack.get(idx).map(Frame::current_span).unwrap_or(rustc_span::DUMMY_SP) } - fn frame_span(machine: &MiriMachine<'_, '_>, idx: usize) -> Span { - machine - .threads - .active_thread_stack() - .get(idx) - .map(Frame::current_span) - .unwrap_or(rustc_span::DUMMY_SP) + fn stack(&self) -> &[Frame<'mir, 'tcx, Provenance, machine::FrameData<'tcx>>] { + self.machine.threads.active_thread_stack() } fn current_frame_idx(&mut self) -> usize { diff --git a/src/tools/miri/src/stacked_borrows/diagnostics.rs b/src/tools/miri/src/stacked_borrows/diagnostics.rs index 0d76ed4e3087..8deb8cda2e17 100644 --- a/src/tools/miri/src/stacked_borrows/diagnostics.rs +++ b/src/tools/miri/src/stacked_borrows/diagnostics.rs @@ -82,7 +82,7 @@ impl fmt::Display for InvalidationCause { InvalidationCause::Access(kind) => write!(f, "{}", kind), InvalidationCause::Retag(perm, kind) => if *kind == RetagCause::FnEntry { - write!(f, "{:?} FnEntry retag", perm) + write!(f, "{:?} FnEntry retag inside this call", perm) } else { write!(f, "{:?} retag", perm) }, @@ -275,7 +275,7 @@ impl<'span, 'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'span, 'history, 'ecx, 'mir let (range, cause) = match &self.operation { Operation::Retag(RetagOp { cause, range, permission, .. }) => { if *cause == RetagCause::FnEntry { - span = self.current_span.get_parent(); + span = self.current_span.get_caller(); } (*range, InvalidationCause::Retag(permission.unwrap(), *cause)) } diff --git a/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut3.stderr b/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut3.stderr index c2ea90f242a2..eb6b01fc6b12 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut3.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/aliasing_mut3.stderr @@ -14,7 +14,7 @@ help: was created by a SharedReadOnly retag at offsets [0x0..0x4] | LL | safe_raw(xraw, xshr); | ^^^^ -help: was later invalidated at offsets [0x0..0x4] by a Unique FnEntry retag +help: was later invalidated at offsets [0x0..0x4] by a Unique FnEntry retag inside this call --> $DIR/aliasing_mut3.rs:LL:CC | LL | safe_raw(xraw, xshr); diff --git a/src/tools/miri/tests/fail/stacked_borrows/fnentry_invalidation.stderr b/src/tools/miri/tests/fail/stacked_borrows/fnentry_invalidation.stderr index 653ceca85885..e81411bbdd86 100644 --- a/src/tools/miri/tests/fail/stacked_borrows/fnentry_invalidation.stderr +++ b/src/tools/miri/tests/fail/stacked_borrows/fnentry_invalidation.stderr @@ -14,7 +14,7 @@ help: was created by a SharedReadWrite retag at offsets [0x0..0x4] | LL | let z = &mut x as *mut i32; | ^^^^^^ -help: was later invalidated at offsets [0x0..0x4] by a Unique FnEntry retag +help: was later invalidated at offsets [0x0..0x4] by a Unique FnEntry retag inside this call --> $DIR/fnentry_invalidation.rs:LL:CC | LL | x.do_bad(); diff --git a/src/tools/miri/tests/fail/stacked_borrows/fnentry_invalidation2.rs b/src/tools/miri/tests/fail/stacked_borrows/fnentry_invalidation2.rs new file mode 100644 index 000000000000..dc51a8a8ac6c --- /dev/null +++ b/src/tools/miri/tests/fail/stacked_borrows/fnentry_invalidation2.rs @@ -0,0 +1,21 @@ +// Regression test for https://github.com/rust-lang/miri/issues/2536 +// This tests that we don't try to back too far up the stack when selecting a span to report. +// We should display the as_mut_ptr() call as the location of the invalidation, not the call to +// inner + +struct Thing<'a> { + sli: &'a mut [i32], +} + +fn main() { + let mut t = Thing { sli: &mut [0, 1, 2] }; + let ptr = t.sli.as_ptr(); + inner(&mut t); + unsafe { + let _oof = *ptr; //~ ERROR: /read access .* tag does not exist in the borrow stack/ + } +} + +fn inner(t: &mut Thing) { + let _ = t.sli.as_mut_ptr(); +} diff --git a/src/tools/miri/tests/fail/stacked_borrows/fnentry_invalidation2.stderr b/src/tools/miri/tests/fail/stacked_borrows/fnentry_invalidation2.stderr new file mode 100644 index 000000000000..d6d0084fa2a7 --- /dev/null +++ b/src/tools/miri/tests/fail/stacked_borrows/fnentry_invalidation2.stderr @@ -0,0 +1,28 @@ +error: Undefined Behavior: attempting a read access using at ALLOC[0x0], but that tag does not exist in the borrow stack for this location + --> $DIR/fnentry_invalidation2.rs:LL:CC + | +LL | let _oof = *ptr; + | ^^^^ + | | + | attempting a read access using at ALLOC[0x0], but that tag does not exist in the borrow stack for this location + | this error occurs as part of an access at ALLOC[0x0..0x4] + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental + = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information +help: was created by a SharedReadOnly retag at offsets [0x0..0xc] + --> $DIR/fnentry_invalidation2.rs:LL:CC + | +LL | let ptr = t.sli.as_ptr(); + | ^^^^^^^^^^^^^^ +help: was later invalidated at offsets [0x0..0xc] by a Unique FnEntry retag inside this call + --> $DIR/fnentry_invalidation2.rs:LL:CC + | +LL | let _ = t.sli.as_mut_ptr(); + | ^^^^^^^^^^^^^^^^^^ + = note: BACKTRACE: + = note: inside `main` at $DIR/fnentry_invalidation2.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to previous error + From 5f498cab13800da77be7f6450a8d5264d31b0d1f Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Thu, 22 Sep 2022 10:44:03 -0400 Subject: [PATCH 2/2] Only add 'inside this call' for Invalidation diagnostics --- .../miri/src/stacked_borrows/diagnostics.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/tools/miri/src/stacked_borrows/diagnostics.rs b/src/tools/miri/src/stacked_borrows/diagnostics.rs index 8deb8cda2e17..2cc7a88704ea 100644 --- a/src/tools/miri/src/stacked_borrows/diagnostics.rs +++ b/src/tools/miri/src/stacked_borrows/diagnostics.rs @@ -66,13 +66,20 @@ enum InvalidationCause { impl Invalidation { fn generate_diagnostic(&self) -> (String, SpanData) { - ( + let message = if let InvalidationCause::Retag(_, RetagCause::FnEntry) = self.cause { + // For a FnEntry retag, our Span points at the caller. + // See `DiagnosticCx::log_invalidation`. + format!( + "{:?} was later invalidated at offsets {:?} by a {} inside this call", + self.tag, self.range, self.cause + ) + } else { format!( "{:?} was later invalidated at offsets {:?} by a {}", self.tag, self.range, self.cause - ), - self.span.data(), - ) + ) + }; + (message, self.span.data()) } } @@ -82,7 +89,7 @@ impl fmt::Display for InvalidationCause { InvalidationCause::Access(kind) => write!(f, "{}", kind), InvalidationCause::Retag(perm, kind) => if *kind == RetagCause::FnEntry { - write!(f, "{:?} FnEntry retag inside this call", perm) + write!(f, "{:?} FnEntry retag", perm) } else { write!(f, "{:?} retag", perm) },