Auto merge of #2454 - saethlin:diagnostics-cleanup, r=RalfJung

Improve information sharing across SB diagnostics

Previous Stacked Borrows diagnostics were missing a lot of information about the state of the interpreter, and it was difficult to add additional state because it was threaded through all the intervening function signatures.

This change factors a lot of the arguments which used to be passed individually to many stacked borrows functions into a single `DiagnosticCx`, which is built in `Stacks::for_each`, and since it wraps a handle to `AllocHistory`, we can now handle more nuanced things like heterogeneous borrow of `!Freeze` types.

Author: bors

src/diagnostics.rs

@@ -178,20 +178,15 @@ pub fn report_error<'tcx, 'mir>(
                         (None, format!("this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental")),
                         (None, format!("see {url} for further information")),
                     ];
-                    match history {
-                        Some(TagHistory::Tagged {tag, created: (created_range, created_span), invalidated, protected }) => {
-                            let msg = format!("{tag:?} was created by a retag at offsets {created_range:?}");
-                            helps.push((Some(*created_span), msg));
-                            if let Some((invalidated_range, invalidated_span)) = invalidated {
-                                let msg = format!("{tag:?} was later invalidated at offsets {invalidated_range:?}");
-                                helps.push((Some(*invalidated_span), msg));
-                            }
-                            if let Some((protecting_tag, protecting_tag_span, protection_span)) = protected {
-                                helps.push((Some(*protecting_tag_span), format!("{tag:?} was protected due to {protecting_tag:?} which was created here")));
-                                helps.push((Some(*protection_span), format!("this protector is live for this call")));
-                            }
+                    if let Some(TagHistory {created, invalidated, protected}) = history.clone() {
+                        helps.push((Some(created.1), created.0));
+                        if let Some((msg, span)) = invalidated {
+                            helps.push((Some(span), msg));
+                        }
+                        if let Some([(protector_msg, protector_span), (protection_msg, protection_span)]) = protected {
+                            helps.push((Some(protector_span), protector_msg));
+                            helps.push((Some(protection_span), protection_msg));
                         }
-                        None => {}
                     }
                     helps
                 }

src/helpers.rs

@@ -876,8 +876,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 }
 
 impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
-    pub fn current_span(&self) -> CurrentSpan<'_, 'mir, 'tcx> {
-        CurrentSpan { span: None, machine: self }
+    pub fn current_span(&self, tcx: TyCtxt<'tcx>) -> CurrentSpan<'_, 'mir, 'tcx> {
+        CurrentSpan { current_frame_idx: None, machine: self, tcx }
     }
 }
 
@@ -887,28 +887,63 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
 /// The result of that search is cached so that later calls are approximately free.
 #[derive(Clone)]
 pub struct CurrentSpan<'a, 'mir, 'tcx> {
-    span: Option<Span>,
+    current_frame_idx: Option<usize>,
+    tcx: TyCtxt<'tcx>,
     machine: &'a Evaluator<'mir, 'tcx>,
 }
 
-impl<'a, 'mir, 'tcx> CurrentSpan<'a, 'mir, 'tcx> {
+impl<'a, 'mir: 'a, 'tcx: 'a + 'mir> CurrentSpan<'a, 'mir, 'tcx> {
+    /// Get the current span, skipping non-local frames.
+    /// This function is backed by a cache, and can be assumed to be very fast.
     pub fn get(&mut self) -> Span {
-        *self.span.get_or_insert_with(|| Self::current_span(self.machine))
+        let idx = self.current_frame_idx();
+        Self::frame_span(self.machine, idx)
     }
 
+    /// Similar to `CurrentSpan::get`, but retrieves the parent frame of the first non-local frame.
+    /// 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))
+    }
+
+    fn frame_span(machine: &Evaluator<'_, '_>, idx: usize) -> Span {
+        machine
+            .threads
+            .active_thread_stack()
+            .get(idx)
+            .map(Frame::current_span)
+            .unwrap_or(rustc_span::DUMMY_SP)
+    }
+
+    fn current_frame_idx(&mut self) -> usize {
+        *self
+            .current_frame_idx
+            .get_or_insert_with(|| Self::compute_current_frame_index(self.tcx, self.machine))
+    }
+
+    // Find the position of the inner-most frame which is part of the crate being
+    // compiled/executed, part of the Cargo workspace, and is also not #[track_caller].
     #[inline(never)]
-    fn current_span(machine: &Evaluator<'_, '_>) -> Span {
+    fn compute_current_frame_index(tcx: TyCtxt<'_>, machine: &Evaluator<'_, '_>) -> usize {
         machine
             .threads
             .active_thread_stack()
             .iter()
+            .enumerate()
             .rev()
-            .find(|frame| {
+            .find_map(|(idx, frame)| {
                 let def_id = frame.instance.def_id();
-                def_id.is_local() || machine.local_crates.contains(&def_id.krate)
+                if (def_id.is_local() || machine.local_crates.contains(&def_id.krate))
+                    && !frame.instance.def.requires_caller_location(tcx)
+                {
+                    Some(idx)
+                } else {
+                    None
+                }
             })
-            .map(|frame| frame.current_span())
-            .unwrap_or(rustc_span::DUMMY_SP)
+            .unwrap_or(0)
     }
 }
 

src/machine.rs

@@ -137,7 +137,7 @@ pub enum Provenance {
 }
 
 /// The "extra" information a pointer has over a regular AllocId.
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, PartialEq)]
 pub enum ProvenanceExtra {
     Concrete(SbTag),
     Wildcard,
@@ -706,15 +706,10 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
         }
 
         let alloc = alloc.into_owned();
-        let stacks = ecx.machine.stacked_borrows.as_ref().map(|stacked_borrows| {
-            Stacks::new_allocation(
-                id,
-                alloc.size(),
-                stacked_borrows,
-                kind,
-                ecx.machine.current_span(),
-            )
-        });
+        let stacks =
+            ecx.machine.stacked_borrows.as_ref().map(|stacked_borrows| {
+                Stacks::new_allocation(id, alloc.size(), stacked_borrows, kind)
+            });
         let race_alloc = ecx.machine.data_race.as_ref().map(|data_race| {
             data_race::AllocExtra::new_allocation(
                 data_race,
@@ -808,7 +803,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
 
     #[inline(always)]
     fn before_memory_read(
-        _tcx: TyCtxt<'tcx>,
+        tcx: TyCtxt<'tcx>,
         machine: &Self,
         alloc_extra: &AllocExtra,
         (alloc_id, prov_extra): (AllocId, Self::ProvenanceExtra),
@@ -828,7 +823,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
                 prov_extra,
                 range,
                 machine.stacked_borrows.as_ref().unwrap(),
-                machine.current_span(),
+                machine.current_span(tcx),
                 &machine.threads,
             )?;
         }
@@ -840,7 +835,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
 
     #[inline(always)]
     fn before_memory_write(
-        _tcx: TyCtxt<'tcx>,
+        tcx: TyCtxt<'tcx>,
         machine: &mut Self,
         alloc_extra: &mut AllocExtra,
         (alloc_id, prov_extra): (AllocId, Self::ProvenanceExtra),
@@ -860,7 +855,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
                 prov_extra,
                 range,
                 machine.stacked_borrows.as_ref().unwrap(),
-                machine.current_span(),
+                machine.current_span(tcx),
                 &machine.threads,
             )?;
         }
@@ -872,7 +867,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
 
     #[inline(always)]
     fn before_memory_deallocation(
-        _tcx: TyCtxt<'tcx>,
+        tcx: TyCtxt<'tcx>,
         machine: &mut Self,
         alloc_extra: &mut AllocExtra,
         (alloc_id, prove_extra): (AllocId, Self::ProvenanceExtra),
@@ -895,6 +890,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
                 prove_extra,
                 range,
                 machine.stacked_borrows.as_ref().unwrap(),
+                machine.current_span(tcx),
                 &machine.threads,
             )
         } else {