Rollup merge of #129828 - RalfJung:miri-data-race, r=saethlin

miri: treat non-memory local variables properly for data race detection

Fixes rust-lang/miri#3242

Miri has an optimization where some local variables are not represented in memory until something forces them to be stored in memory (most notably, creating a pointer/reference to the local will do that). However, for a subsystem triggering on memory accesses -- such as the data race detector -- this means that the memory access seems to happen only when the local is moved to memory, instead of at the time that it actually happens. This can lead to UB reports in programs that do not actually have UB.

This PR fixes that by adding machine hooks for reads and writes to such efficiently represented local variables. The data race system tracks those very similar to how it would track reads and writes to addressable memory, and when a local is moved to memory, the clocks get overwritten with the information stored for the local.

Author: matthiaskrgr

Diff for: compiler/rustc_const_eval/src/interpret/machine.rs

@@ -540,10 +540,29 @@ pub trait Machine<'tcx>: Sized {
         Ok(ReturnAction::Normal)
     }
 
+    /// Called immediately after an "immediate" local variable is read
+    /// (i.e., this is called for reads that do not end up accessing addressable memory).
+    #[inline(always)]
+    fn after_local_read(_ecx: &InterpCx<'tcx, Self>, _local: mir::Local) -> InterpResult<'tcx> {
+        Ok(())
+    }
+
+    /// Called immediately after an "immediate" local variable is assigned a new value
+    /// (i.e., this is called for writes that do not end up in memory).
+    /// `storage_live` indicates whether this is the initial write upon `StorageLive`.
+    #[inline(always)]
+    fn after_local_write(
+        _ecx: &mut InterpCx<'tcx, Self>,
+        _local: mir::Local,
+        _storage_live: bool,
+    ) -> InterpResult<'tcx> {
+        Ok(())
+    }
+
     /// Called immediately after actual memory was allocated for a local
     /// but before the local's stack frame is updated to point to that memory.
     #[inline(always)]
-    fn after_local_allocated(
+    fn after_local_moved_to_memory(
         _ecx: &mut InterpCx<'tcx, Self>,
         _local: mir::Local,
         _mplace: &MPlaceTy<'tcx, Self::Provenance>,

Diff for: compiler/rustc_const_eval/src/interpret/memory.rs

@@ -1046,6 +1046,10 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         );
         res
     }
+
+    pub(super) fn validation_in_progress(&self) -> bool {
+        self.memory.validation_in_progress
+    }
 }
 
 #[doc(hidden)]

Diff for: compiler/rustc_const_eval/src/interpret/operand.rs

@@ -697,6 +697,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         if matches!(op, Operand::Immediate(_)) {
             assert!(!layout.is_unsized());
         }
+        M::after_local_read(self, local)?;
         Ok(OpTy { op, layout })
     }
 

Diff for: compiler/rustc_const_eval/src/interpret/place.rs

@@ -500,15 +500,13 @@ where
         &self,
         local: mir::Local,
     ) -> InterpResult<'tcx, PlaceTy<'tcx, M::Provenance>> {
-        // Other parts of the system rely on `Place::Local` never being unsized.
-        // So we eagerly check here if this local has an MPlace, and if yes we use it.
         let frame = self.frame();
         let layout = self.layout_of_local(frame, local, None)?;
         let place = if layout.is_sized() {
             // We can just always use the `Local` for sized values.
             Place::Local { local, offset: None, locals_addr: frame.locals_addr() }
         } else {
-            // Unsized `Local` isn't okay (we cannot store the metadata).
+            // Other parts of the system rely on `Place::Local` never being unsized.
             match frame.locals[local].access()? {
                 Operand::Immediate(_) => bug!(),
                 Operand::Indirect(mplace) => Place::Ptr(*mplace),
@@ -562,7 +560,10 @@ where
         place: &PlaceTy<'tcx, M::Provenance>,
     ) -> InterpResult<
         'tcx,
-        Either<MPlaceTy<'tcx, M::Provenance>, (&mut Immediate<M::Provenance>, TyAndLayout<'tcx>)>,
+        Either<
+            MPlaceTy<'tcx, M::Provenance>,
+            (&mut Immediate<M::Provenance>, TyAndLayout<'tcx>, mir::Local),
+        >,
     > {
         Ok(match place.to_place().as_mplace_or_local() {
             Left(mplace) => Left(mplace),
@@ -581,7 +582,7 @@ where
                         }
                         Operand::Immediate(local_val) => {
                             // The local still has the optimized representation.
-                            Right((local_val, layout))
+                            Right((local_val, layout, local))
                         }
                     }
                 }
@@ -643,9 +644,13 @@ where
         assert!(dest.layout().is_sized(), "Cannot write unsized immediate data");
 
         match self.as_mplace_or_mutable_local(&dest.to_place())? {
-            Right((local_val, local_layout)) => {
+            Right((local_val, local_layout, local)) => {
                 // Local can be updated in-place.
                 *local_val = src;
+                // Call the machine hook (the data race detector needs to know about this write).
+                if !self.validation_in_progress() {
+                    M::after_local_write(self, local, /*storage_live*/ false)?;
+                }
                 // Double-check that the value we are storing and the local fit to each other.
                 if cfg!(debug_assertions) {
                     src.assert_matches_abi(local_layout.abi, self);
@@ -714,8 +719,12 @@ where
         dest: &impl Writeable<'tcx, M::Provenance>,
     ) -> InterpResult<'tcx> {
         match self.as_mplace_or_mutable_local(&dest.to_place())? {
-            Right((local_val, _local_layout)) => {
+            Right((local_val, _local_layout, local)) => {
                 *local_val = Immediate::Uninit;
+                // Call the machine hook (the data race detector needs to know about this write).
+                if !self.validation_in_progress() {
+                    M::after_local_write(self, local, /*storage_live*/ false)?;
+                }
             }
             Left(mplace) => {
                 let Some(mut alloc) = self.get_place_alloc_mut(&mplace)? else {
@@ -734,8 +743,12 @@ where
         dest: &impl Writeable<'tcx, M::Provenance>,
     ) -> InterpResult<'tcx> {
         match self.as_mplace_or_mutable_local(&dest.to_place())? {
-            Right((local_val, _local_layout)) => {
+            Right((local_val, _local_layout, local)) => {
                 local_val.clear_provenance()?;
+                // Call the machine hook (the data race detector needs to know about this write).
+                if !self.validation_in_progress() {
+                    M::after_local_write(self, local, /*storage_live*/ false)?;
+                }
             }
             Left(mplace) => {
                 let Some(mut alloc) = self.get_place_alloc_mut(&mplace)? else {
@@ -941,7 +954,7 @@ where
                                 mplace.mplace,
                             )?;
                         }
-                        M::after_local_allocated(self, local, &mplace)?;
+                        M::after_local_moved_to_memory(self, local, &mplace)?;
                         // Now we can call `access_mut` again, asserting it goes well, and actually
                         // overwrite things. This points to the entire allocation, not just the part
                         // the place refers to, i.e. we do this before we apply `offset`.

Diff for: compiler/rustc_const_eval/src/interpret/stack.rs

@@ -534,8 +534,11 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             let dest_place = self.allocate_dyn(layout, MemoryKind::Stack, meta)?;
             Operand::Indirect(*dest_place.mplace())
         } else {
-            assert!(!meta.has_meta()); // we're dropping the metadata
             // Just make this an efficient immediate.
+            assert!(!meta.has_meta()); // we're dropping the metadata
+            // Make sure the machine knows this "write" is happening. (This is important so that
+            // races involving local variable allocation can be detected by Miri.)
+            M::after_local_write(self, local, /*storage_live*/ true)?;
             // Note that not calling `layout_of` here does have one real consequence:
             // if the type is too big, we'll only notice this when the local is actually initialized,
             // which is a bit too late -- we should ideally notice this already here, when the memory