Auto merge of #2694 - RalfJung:retag-deref-check, r=saethlin

fix handling of spurious accesses during retag

The `dereferenceable` attribute we emit for LLVM is checked during retag in Stacked Borrows.
However, we currently don't properly do that for retagging of `&mut !Unpin`, which this PR fixes.
Also this adjusts retagging to inform the data race model of the accesses as well.

Fixes #2648.
Also fixes #2693 since the same issue arose for retagging as well.

r? `@saethlin`

src/concurrency/data_race.rs

@@ -838,18 +838,18 @@ impl VClockAlloc {
         &self,
         alloc_id: AllocId,
         range: AllocRange,
-        global: &GlobalState,
-        thread_mgr: &ThreadManager<'_, '_>,
+        machine: &MiriMachine<'_, '_>,
     ) -> InterpResult<'tcx> {
+        let global = machine.data_race.as_ref().unwrap();
         if global.race_detecting() {
-            let (index, clocks) = global.current_thread_state(thread_mgr);
+            let (index, clocks) = global.current_thread_state(&machine.threads);
             let mut alloc_ranges = self.alloc_ranges.borrow_mut();
             for (offset, range) in alloc_ranges.iter_mut(range.start, range.size) {
                 if let Err(DataRace) = range.read_race_detect(&clocks, index) {
                     // Report data-race.
                     return Self::report_data_race(
                         global,
-                        thread_mgr,
+                        &machine.threads,
                         range,
                         "Read",
                         false,
@@ -869,17 +869,17 @@ impl VClockAlloc {
         alloc_id: AllocId,
         range: AllocRange,
         write_type: WriteType,
-        global: &mut GlobalState,
-        thread_mgr: &ThreadManager<'_, '_>,
+        machine: &mut MiriMachine<'_, '_>,
     ) -> InterpResult<'tcx> {
+        let global = machine.data_race.as_mut().unwrap();
         if global.race_detecting() {
-            let (index, clocks) = global.current_thread_state(thread_mgr);
+            let (index, clocks) = global.current_thread_state(&machine.threads);
             for (offset, range) in self.alloc_ranges.get_mut().iter_mut(range.start, range.size) {
                 if let Err(DataRace) = range.write_race_detect(&clocks, index, write_type) {
                     // Report data-race
                     return Self::report_data_race(
                         global,
-                        thread_mgr,
+                        &machine.threads,
                         range,
                         write_type.get_descriptor(),
                         false,
@@ -901,10 +901,9 @@ impl VClockAlloc {
         &mut self,
         alloc_id: AllocId,
         range: AllocRange,
-        global: &mut GlobalState,
-        thread_mgr: &ThreadManager<'_, '_>,
+        machine: &mut MiriMachine<'_, '_>,
     ) -> InterpResult<'tcx> {
-        self.unique_access(alloc_id, range, WriteType::Write, global, thread_mgr)
+        self.unique_access(alloc_id, range, WriteType::Write, machine)
     }
 
     /// Detect data-races for an unsynchronized deallocate operation, will not perform
@@ -915,10 +914,9 @@ impl VClockAlloc {
         &mut self,
         alloc_id: AllocId,
         range: AllocRange,
-        global: &mut GlobalState,
-        thread_mgr: &ThreadManager<'_, '_>,
+        machine: &mut MiriMachine<'_, '_>,
     ) -> InterpResult<'tcx> {
-        self.unique_access(alloc_id, range, WriteType::Deallocate, global, thread_mgr)
+        self.unique_access(alloc_id, range, WriteType::Deallocate, machine)
     }
 }
 

src/machine.rs

@@ -1003,21 +1003,12 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
         range: AllocRange,
     ) -> InterpResult<'tcx> {
         if let Some(data_race) = &alloc_extra.data_race {
-            data_race.read(
-                alloc_id,
-                range,
-                machine.data_race.as_ref().unwrap(),
-                &machine.threads,
-            )?;
+            data_race.read(alloc_id, range, machine)?;
         }
         if let Some(stacked_borrows) = &alloc_extra.stacked_borrows {
-            stacked_borrows.borrow_mut().before_memory_read(
-                alloc_id,
-                prov_extra,
-                range,
-                machine.stacked_borrows.as_ref().unwrap(),
-                machine,
-            )?;
+            stacked_borrows
+                .borrow_mut()
+                .before_memory_read(alloc_id, prov_extra, range, machine)?;
         }
         if let Some(weak_memory) = &alloc_extra.weak_memory {
             weak_memory.memory_accessed(range, machine.data_race.as_ref().unwrap());
@@ -1034,21 +1025,10 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
         range: AllocRange,
     ) -> InterpResult<'tcx> {
         if let Some(data_race) = &mut alloc_extra.data_race {
-            data_race.write(
-                alloc_id,
-                range,
-                machine.data_race.as_mut().unwrap(),
-                &machine.threads,
-            )?;
+            data_race.write(alloc_id, range, machine)?;
         }
         if let Some(stacked_borrows) = &mut alloc_extra.stacked_borrows {
-            stacked_borrows.get_mut().before_memory_write(
-                alloc_id,
-                prov_extra,
-                range,
-                machine.stacked_borrows.as_ref().unwrap(),
-                machine,
-            )?;
+            stacked_borrows.get_mut().before_memory_write(alloc_id, prov_extra, range, machine)?;
         }
         if let Some(weak_memory) = &alloc_extra.weak_memory {
             weak_memory.memory_accessed(range, machine.data_race.as_ref().unwrap());
@@ -1068,19 +1048,13 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
             machine.emit_diagnostic(NonHaltingDiagnostic::FreedAlloc(alloc_id));
         }
         if let Some(data_race) = &mut alloc_extra.data_race {
-            data_race.deallocate(
-                alloc_id,
-                range,
-                machine.data_race.as_mut().unwrap(),
-                &machine.threads,
-            )?;
+            data_race.deallocate(alloc_id, range, machine)?;
         }
         if let Some(stacked_borrows) = &mut alloc_extra.stacked_borrows {
             stacked_borrows.get_mut().before_memory_deallocation(
                 alloc_id,
                 prove_extra,
                 range,
-                machine.stacked_borrows.as_ref().unwrap(),
                 machine,
             )
         } else {

src/stacked_borrows/diagnostics.rs

@@ -353,10 +353,12 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
 
     /// Report a descriptive error when `new` could not be granted from `derived_from`.
     #[inline(never)] // This is only called on fatal code paths
-    pub(super) fn grant_error(&self, perm: Permission, stack: &Stack) -> InterpError<'tcx> {
+    pub(super) fn grant_error(&self, stack: &Stack) -> InterpError<'tcx> {
         let Operation::Retag(op) = &self.operation else {
             unreachable!("grant_error should only be called during a retag")
         };
+        let perm =
+            op.permission.expect("`start_grant` must be called before calling `grant_error`");
         let action = format!(
             "trying to retag from {:?} for {:?} permission at {:?}[{:#x}]",
             op.orig_tag,
@@ -374,8 +376,11 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
     /// Report a descriptive error when `access` is not permitted based on `tag`.
     #[inline(never)] // This is only called on fatal code paths
     pub(super) fn access_error(&self, stack: &Stack) -> InterpError<'tcx> {
-        let Operation::Access(op) = &self.operation  else {
-            unreachable!("access_error should only be called during an access")
+        // Deallocation and retagging also do an access as part of their thing, so handle that here, too.
+        let op = match &self.operation {
+            Operation::Access(op) => op,
+            Operation::Retag(_) => return self.grant_error(stack),
+            Operation::Dealloc(_) => return self.dealloc_error(stack),
         };
         let action = format!(
             "attempting a {access} using {tag:?} at {alloc_id:?}[{offset:#x}]",
@@ -428,14 +433,16 @@ impl<'history, 'ecx, 'mir, 'tcx> DiagnosticCx<'history, 'ecx, 'mir, 'tcx> {
     }
 
     #[inline(never)] // This is only called on fatal code paths
-    pub fn dealloc_error(&self) -> InterpError<'tcx> {
+    pub fn dealloc_error(&self, stack: &Stack) -> InterpError<'tcx> {
         let Operation::Dealloc(op) = &self.operation else {
             unreachable!("dealloc_error should only be called during a deallocation")
         };
         err_sb_ub(
             format!(
-                "no item granting write access for deallocation to tag {:?} at {:?} found in borrow stack",
-                op.tag, self.history.id,
+                "attempting deallocation using {tag:?} at {alloc_id:?}{cause}",
+                tag = op.tag,
+                alloc_id = self.history.id,
+                cause = error_cause(stack, op.tag),
             ),
             None,
             op.tag.and_then(|tag| self.get_logs_relevant_to(tag, None)),