Auto merge of rust-lang#2537 - saethlin:dont-back-up-too-far, r=RalfJung

Don't back up past the caller when looking for an FnEntry span

Fixes rust-lang/miri#2536

This adds a fix for the logic as well as a regression test. In the new test `tests/fail/stacked_borrows/fnentry_invalidation2.rs`, before this PR, we display this diagnostic:
```
help: <3278> was later invalidated at offsets [0x0..0xc] by a Unique FnEntry retag
  --> tests/fail/stacked_borrows/fnentry_invalidation2.rs:13:5
   |
13 |     inner(&mut t);
   |     ^^^^^^^^^^^^^
```
Which is very misleading. It is not this call itself, but what happens within the call that invalidates the tag we want. With this PR, we get:
```
help: <2798> was later invalidated at offsets [0x0..0xc] by a Unique FnEntry retag inside this call
  --> tests/fail/stacked_borrows/fnentry_invalidation2.rs:20:13
   |
20 |     let _ = t.sli.as_mut_ptr();
   |             ^^^^^^^^^^^^^^^^^^
```
Which is much better.

Author: bors

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 {

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())
     }
 }
 
@@ -275,7 +282,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))
             }

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);

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();

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();
+}

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
+