Auto merge of #118408 - RalfJung:aggregate-assign-uninit, r=saethlin

miri: add test checking that aggregate assignments reset memory to uninit

Also, `write_aggregate` is really just a helper for evaluating `Aggregate` rvalues, so it should be in `step.rs`, not `place.rs`. Also factor out `Repeat` rvalues into their own function while we are at it.

r? `@saethlin`
Fixes https://github.com/rust-lang/miri/issues/3195

compiler/rustc_const_eval/src/interpret/place.rs

@@ -7,12 +7,11 @@ use std::assert_matches::assert_matches;
 use either::{Either, Left, Right};
 
 use rustc_ast::Mutability;
-use rustc_index::IndexSlice;
 use rustc_middle::mir;
 use rustc_middle::ty;
 use rustc_middle::ty::layout::{LayoutOf, TyAndLayout};
 use rustc_middle::ty::Ty;
-use rustc_target::abi::{Abi, Align, FieldIdx, HasDataLayout, Size, FIRST_VARIANT};
+use rustc_target::abi::{Abi, Align, HasDataLayout, Size};
 
 use super::{
     alloc_range, mir_assign_valid_types, AllocId, AllocRef, AllocRefMut, CheckAlignMsg, ImmTy,
@@ -977,34 +976,6 @@ where
         Ok(self.ptr_with_meta_to_mplace(ptr.into(), MemPlaceMeta::Meta(meta), layout))
     }
 
-    /// Writes the aggregate to the destination.
-    #[instrument(skip(self), level = "trace")]
-    pub fn write_aggregate(
-        &mut self,
-        kind: &mir::AggregateKind<'tcx>,
-        operands: &IndexSlice<FieldIdx, mir::Operand<'tcx>>,
-        dest: &PlaceTy<'tcx, M::Provenance>,
-    ) -> InterpResult<'tcx> {
-        self.write_uninit(dest)?;
-        let (variant_index, variant_dest, active_field_index) = match *kind {
-            mir::AggregateKind::Adt(_, variant_index, _, _, active_field_index) => {
-                let variant_dest = self.project_downcast(dest, variant_index)?;
-                (variant_index, variant_dest, active_field_index)
-            }
-            _ => (FIRST_VARIANT, dest.clone(), None),
-        };
-        if active_field_index.is_some() {
-            assert_eq!(operands.len(), 1);
-        }
-        for (field_index, operand) in operands.iter_enumerated() {
-            let field_index = active_field_index.unwrap_or(field_index);
-            let field_dest = self.project_field(&variant_dest, field_index.as_usize())?;
-            let op = self.eval_operand(operand, Some(field_dest.layout))?;
-            self.copy_op(&op, &field_dest, /*allow_transmute*/ false)?;
-        }
-        self.write_discriminant(variant_index, dest)
-    }
-
     pub fn raw_const_to_mplace(
         &self,
         raw: mir::ConstAlloc<'tcx>,

compiler/rustc_const_eval/src/interpret/step.rs

@@ -4,11 +4,12 @@
 
 use either::Either;
 
+use rustc_index::IndexSlice;
 use rustc_middle::mir;
-use rustc_middle::mir::interpret::{InterpResult, Scalar};
 use rustc_middle::ty::layout::LayoutOf;
+use rustc_target::abi::{FieldIdx, FIRST_VARIANT};
 
-use super::{ImmTy, InterpCx, Machine, Projectable};
+use super::{ImmTy, InterpCx, InterpResult, Machine, PlaceTy, Projectable, Scalar};
 use crate::util;
 
 impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
@@ -187,34 +188,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             }
 
             Repeat(ref operand, _) => {
-                let src = self.eval_operand(operand, None)?;
+                self.write_repeat(operand, &dest)?;
-                assert!(src.layout.is_sized());
-                let dest = self.force_allocation(&dest)?;
-                let length = dest.len(self)?;
-
-                if length == 0 {
-                    // Nothing to copy... but let's still make sure that `dest` as a place is valid.
-                    self.get_place_alloc_mut(&dest)?;
-                } else {
-                    // Write the src to the first element.
-                    let first = self.project_index(&dest, 0)?;
-                    self.copy_op(&src, &first, /*allow_transmute*/ false)?;
-
-                    // This is performance-sensitive code for big static/const arrays! So we
-                    // avoid writing each operand individually and instead just make many copies
-                    // of the first element.
-                    let elem_size = first.layout.size;
-                    let first_ptr = first.ptr();
-                    let rest_ptr = first_ptr.offset(elem_size, self)?;
-                    // No alignment requirement since `copy_op` above already checked it.
-                    self.mem_copy_repeatedly(
-                        first_ptr,
-                        rest_ptr,
-                        elem_size,
-                        length - 1,
-                        /*nonoverlapping:*/ true,
-                    )?;
-                }
             }
 
             Len(place) => {
@@ -307,6 +281,73 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         Ok(())
     }
 
+    /// Writes the aggregate to the destination.
+    #[instrument(skip(self), level = "trace")]
+    fn write_aggregate(
+        &mut self,
+        kind: &mir::AggregateKind<'tcx>,
+        operands: &IndexSlice<FieldIdx, mir::Operand<'tcx>>,
+        dest: &PlaceTy<'tcx, M::Provenance>,
+    ) -> InterpResult<'tcx> {
+        self.write_uninit(dest)?; // make sure all the padding ends up as uninit
+        let (variant_index, variant_dest, active_field_index) = match *kind {
+            mir::AggregateKind::Adt(_, variant_index, _, _, active_field_index) => {
+                let variant_dest = self.project_downcast(dest, variant_index)?;
+                (variant_index, variant_dest, active_field_index)
+            }
+            _ => (FIRST_VARIANT, dest.clone(), None),
+        };
+        if active_field_index.is_some() {
+            assert_eq!(operands.len(), 1);
+        }
+        for (field_index, operand) in operands.iter_enumerated() {
+            let field_index = active_field_index.unwrap_or(field_index);
+            let field_dest = self.project_field(&variant_dest, field_index.as_usize())?;
+            let op = self.eval_operand(operand, Some(field_dest.layout))?;
+            self.copy_op(&op, &field_dest, /*allow_transmute*/ false)?;
+        }
+        self.write_discriminant(variant_index, dest)
+    }
+
+    /// Repeats `operand` into the destination. `dest` must have array type, and that type
+    /// determines how often `operand` is repeated.
+    fn write_repeat(
+        &mut self,
+        operand: &mir::Operand<'tcx>,
+        dest: &PlaceTy<'tcx, M::Provenance>,
+    ) -> InterpResult<'tcx> {
+        let src = self.eval_operand(operand, None)?;
+        assert!(src.layout.is_sized());
+        let dest = self.force_allocation(&dest)?;
+        let length = dest.len(self)?;
+
+        if length == 0 {
+            // Nothing to copy... but let's still make sure that `dest` as a place is valid.
+            self.get_place_alloc_mut(&dest)?;
+        } else {
+            // Write the src to the first element.
+            let first = self.project_index(&dest, 0)?;
+            self.copy_op(&src, &first, /*allow_transmute*/ false)?;
+
+            // This is performance-sensitive code for big static/const arrays! So we
+            // avoid writing each operand individually and instead just make many copies
+            // of the first element.
+            let elem_size = first.layout.size;
+            let first_ptr = first.ptr();
+            let rest_ptr = first_ptr.offset(elem_size, self)?;
+            // No alignment requirement since `copy_op` above already checked it.
+            self.mem_copy_repeatedly(
+                first_ptr,
+                rest_ptr,
+                elem_size,
+                length - 1,
+                /*nonoverlapping:*/ true,
+            )?;
+        }
+
+        Ok(())
+    }
+
     /// Evaluate the given terminator. Will also adjust the stack frame and statement position accordingly.
     fn terminator(&mut self, terminator: &mir::Terminator<'tcx>) -> InterpResult<'tcx> {
         info!("{:?}", terminator.kind);

src/tools/miri/tests/fail/uninit-after-aggregate-assign.rs

@@ -0,0 +1,27 @@
+#![feature(core_intrinsics)]
+#![feature(custom_mir)]
+
+use std::intrinsics::mir::*;
+use std::ptr;
+
+#[repr(C)]
+struct S(u8, u16);
+
+#[custom_mir(dialect = "runtime", phase = "optimized")]
+fn main() {
+    mir! {
+        let s: S;
+        let sptr;
+        let sptr2;
+        let _val;
+        {
+            sptr = ptr::addr_of_mut!(s);
+            sptr2 = sptr as *mut [u8; 4];
+            *sptr2 = [0; 4];
+            *sptr = S(0, 0); // should reset the padding
+            _val = *sptr2; // should hence be UB
+            //~^ERROR: encountered uninitialized memory
+            Return()
+        }
+    }
+}

src/tools/miri/tests/fail/uninit-after-aggregate-assign.stderr

@@ -0,0 +1,15 @@
+error: Undefined Behavior: constructing invalid value at [1]: encountered uninitialized memory, but expected an integer
+  --> $DIR/uninit-after-aggregate-assign.rs:LL:CC
+   |
+LL |             _val = *sptr2; // should hence be UB
+   |             ^^^^^^^^^^^^^ constructing invalid value at [1]: encountered uninitialized memory, but expected an integer
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `main` at $DIR/uninit-after-aggregate-assign.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+