Auto merge of #146402 - RalfJung:aggregate-init, r=saethlin

interpret: fix overlapping aggregate initialization

This fixes the problem pointed out by `@saethlin` in #146383 (comment).

Also clarify when exactly current de-facto MIR semantics allow overlap of the LHS and RHS in an assignment.

Author: bors

compiler/rustc_codegen_ssa/src/mir/rvalue.rs

@@ -1,5 +1,5 @@
 use itertools::Itertools as _;
-use rustc_abi::{self as abi, FIRST_VARIANT};
+use rustc_abi::{self as abi, BackendRepr, FIRST_VARIANT};
 use rustc_middle::ty::adjustment::PointerCoercion;
 use rustc_middle::ty::layout::{HasTyCtxt, HasTypingEnv, LayoutOf, TyAndLayout};
 use rustc_middle::ty::{self, Instance, Ty, TyCtxt};
@@ -25,6 +25,15 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
         match *rvalue {
             mir::Rvalue::Use(ref operand) => {
                 let cg_operand = self.codegen_operand(bx, operand);
+                // Crucially, we do *not* use `OperandValue::Ref` for types with
+                // `BackendRepr::Scalar | BackendRepr::ScalarPair`. This ensures we match the MIR
+                // semantics regarding when assignment operators allow overlap of LHS and RHS.
+                if matches!(
+                    cg_operand.layout.backend_repr,
+                    BackendRepr::Scalar(..) | BackendRepr::ScalarPair(..),
+                ) {
+                    debug_assert!(!matches!(cg_operand.val, OperandValue::Ref(..)));
+                }
                 // FIXME: consider not copying constants through stack. (Fixable by codegen'ing
                 // constants into `OperandValue::Ref`; why don’t we do that yet if we don’t?)
                 cg_operand.val.store(bx, dest);

compiler/rustc_const_eval/src/interpret/place.rs

@@ -858,7 +858,7 @@ where
     /// Also, if you use this you are responsible for validating that things get copied at the
     /// right type.
     #[instrument(skip(self), level = "trace")]
-    fn copy_op_no_validate(
+    pub(super) fn copy_op_no_validate(
         &mut self,
         src: &impl Projectable<'tcx, M::Provenance>,
         dest: &impl Writeable<'tcx, M::Provenance>,

compiler/rustc_const_eval/src/interpret/step.rs

@@ -310,7 +310,6 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
         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)?;
@@ -346,9 +345,20 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
             let field_index = active_field_index.unwrap_or(field_index);
             let field_dest = self.project_field(&variant_dest, field_index)?;
             let op = self.eval_operand(operand, Some(field_dest.layout))?;
-            self.copy_op(&op, &field_dest)?;
+            // We validate manually below so we don't have to do it here.
+            self.copy_op_no_validate(&op, &field_dest, /*allow_transmute*/ false)?;
         }
-        self.write_discriminant(variant_index, dest)
+        self.write_discriminant(variant_index, dest)?;
+        // Validate that the entire thing is valid, and reset padding that might be in between the
+        // fields.
+        if M::enforce_validity(self, dest.layout()) {
+            self.validate_operand(
+                dest,
+                M::enforce_validity_recursively(self, dest.layout()),
+                /*reset_provenance_and_padding*/ true,
+            )?;
+        }
+        interp_ok(())
     }
 
     /// Repeats `operand` into the destination. `dest` must have array type, and that type

compiler/rustc_middle/src/mir/syntax.rs

@@ -327,9 +327,11 @@ pub enum StatementKind<'tcx> {
     /// interesting for optimizations? Do we want to allow such optimizations?
     ///
     /// **Needs clarification**: We currently require that the LHS place not overlap with any place
-    /// read as part of computation of the RHS for some rvalues (generally those not producing
-    /// primitives). This requirement is under discussion in [#68364]. As a part of this discussion,
-    /// it is also unclear in what order the components are evaluated.
+    /// read as part of computation of the RHS for some rvalues. This requirement is under
+    /// discussion in [#68364]. Specifically, overlap is permitted only for assignments of a type
+    /// with `BackendRepr::Scalar | BackendRepr::ScalarPair` where all the scalar fields are
+    /// [`Scalar::Initialized`][rustc_abi::Scalar::Initialized]. As a part of this discussion, it is
+    /// also unclear in what order the components are evaluated.
     ///
     /// [#68364]: https://github.com/rust-lang/rust/issues/68364
     ///

src/tools/miri/tests/fail/both_borrows/issue-miri-1050-1.rs

@@ -1,4 +1,6 @@
 //@revisions: stack tree
+// Ensure this even hits the aliasing model
+//@compile-flags: -Zmiri-disable-validation
 //@[tree]compile-flags: -Zmiri-tree-borrows
 //@error-in-other-file: pointer not dereferenceable
 

src/tools/miri/tests/fail/both_borrows/issue-miri-1050-2.rs

@@ -1,4 +1,6 @@
 //@revisions: stack tree
+// Ensure this even hits the aliasing model
+//@compile-flags: -Zmiri-disable-validation
 //@[tree]compile-flags: -Zmiri-tree-borrows
 //@error-in-other-file: is a dangling pointer
 use std::ptr::NonNull;

src/tools/miri/tests/fail/both_borrows/return_invalid_shr.stack.stderr

@@ -11,7 +11,7 @@ help: <TAG> was created by a SharedReadOnly retag at offsets [0x4..0x8]
    |
 LL |     let ret = unsafe { &(*xraw).1 };
    |                        ^^^^^^^^^^
-help: <TAG> was later invalidated at offsets [0x0..0x8] by a write access
+help: <TAG> was later invalidated at offsets [0x4..0x8] by a write access
   --> tests/fail/both_borrows/return_invalid_shr.rs:LL:CC
    |
 LL |     unsafe { *xraw = (42, 23) }; // unfreeze

src/tools/miri/tests/fail/both_borrows/return_invalid_shr.tree.stderr

@@ -12,7 +12,7 @@ help: the accessed tag <TAG> was created here, in the initial state Frozen
    |
 LL |     let ret = unsafe { &(*xraw).1 };
    |                        ^^^^^^^^^^
-help: the accessed tag <TAG> later transitioned to Disabled due to a foreign write access at offsets [0x0..0x8]
+help: the accessed tag <TAG> later transitioned to Disabled due to a foreign write access at offsets [0x4..0x8]
   --> tests/fail/both_borrows/return_invalid_shr.rs:LL:CC
    |
 LL |     unsafe { *xraw = (42, 23) }; // unfreeze

src/tools/miri/tests/fail/both_borrows/return_invalid_shr_option.stack.stderr

@@ -14,7 +14,7 @@ help: <TAG> was created by a SharedReadOnly retag at offsets [0x4..0x8]
    |
 LL |     let ret = Some(unsafe { &(*xraw).1 });
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
-help: <TAG> was later invalidated at offsets [0x0..0x8] by a write access
+help: <TAG> was later invalidated at offsets [0x4..0x8] by a write access
   --> tests/fail/both_borrows/return_invalid_shr_option.rs:LL:CC
    |
 LL |     unsafe { *xraw = (42, 23) }; // unfreeze

src/tools/miri/tests/fail/both_borrows/return_invalid_shr_option.tree.stderr

@@ -12,7 +12,7 @@ help: the accessed tag <TAG> was created here, in the initial state Frozen
    |
 LL |     let ret = Some(unsafe { &(*xraw).1 });
    |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
-help: the accessed tag <TAG> later transitioned to Disabled due to a foreign write access at offsets [0x0..0x8]
+help: the accessed tag <TAG> later transitioned to Disabled due to a foreign write access at offsets [0x4..0x8]
   --> tests/fail/both_borrows/return_invalid_shr_option.rs:LL:CC
    |
 LL |     unsafe { *xraw = (42, 23) }; // unfreeze