Don't move too far down the call stack when reporting FnEntry diagnostics

6 files changed, 67 insertions, 16 deletions

diff --git a/src/tools/miri/src/helpers.rs b/src/tools/miri/src/helpers.rs
index 0f0bfa355bd..15833fe42ad 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 0d76ed4e308..8deb8cda2e1 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 c2ea90f242a..eb6b01fc6b1 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: <TAG> was created by a SharedReadOnly retag at offsets [0x0..0x4]
    |
 LL |     safe_raw(xraw, xshr);
    |                    ^^^^
-help: <TAG> was later invalidated at offsets [0x0..0x4] by a Unique FnEntry retag
+help: <TAG> 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 653ceca8588..e81411bbdd8 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: <TAG> was created by a SharedReadWrite retag at offsets [0x0..0x4]
    |
 LL |     let z = &mut x as *mut i32;
    |             ^^^^^^
-help: <TAG> was later invalidated at offsets [0x0..0x4] by a Unique FnEntry retag
+help: <TAG> 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 00000000000..dc51a8a8ac6
--- /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 00000000000..d6d0084fa2a
--- /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 <TAG> 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 <TAG> 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: <TAG> was created by a SharedReadOnly retag at offsets [0x0..0xc]
+  --> $DIR/fnentry_invalidation2.rs:LL:CC
+   |
+LL |     let ptr = t.sli.as_ptr();
+   |               ^^^^^^^^^^^^^^
+help: <TAG> 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
+