Auto merge of rust-lang#112307 - lcnr:operand-ref, r=compiler-errors

bors · bors · commit bb95b7dcd6a2 · 2023-06-28T00:41:37.000Z
mir opt + codegen: handle subtyping fixes rust-lang#107205 the same issue was caused in multiple places: - mir opts: both copy and destination propagation - codegen: assigning operands to locals (which also propagates values) I changed codegen to always update the type in the operands used for locals which should guard against any new occurrences of this bug going forward. I don't know how to make mir optimizations more resilient here. Hopefully the added tests will be enough to detect any trivially wrong optimizations going forward.
diff --git a/compiler/rustc_codegen_ssa/src/lib.rs b/compiler/rustc_codegen_ssa/src/lib.rs
@@ -4,6 +4,7 @@
 #![feature(if_let_guard)]
 #![feature(int_roundings)]
 #![feature(let_chains)]
+#![feature(negative_impls)]
 #![feature(never_type)]
 #![feature(strict_provenance)]
 #![feature(try_blocks)]
diff --git a/compiler/rustc_codegen_ssa/src/mir/block.rs b/compiler/rustc_codegen_ssa/src/mir/block.rs
@@ -1729,7 +1729,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             IndirectOperand(tmp, index) => {
                 let op = bx.load_operand(tmp);
                 tmp.storage_dead(bx);
-                self.locals[index] = LocalRef::Operand(op);
+                self.overwrite_local(index, LocalRef::Operand(op));
                 self.debug_introduce_local(bx, index);
             }
             DirectOperand(index) => {
@@ -1744,7 +1744,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                 } else {
                     OperandRef::from_immediate_or_packed_pair(bx, llval, ret_abi.layout)
                 };
-                self.locals[index] = LocalRef::Operand(op);
+                self.overwrite_local(index, LocalRef::Operand(op));
                 self.debug_introduce_local(bx, index);
             }
         }
diff --git a/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs b/compiler/rustc_codegen_ssa/src/mir/debuginfo.rs
@@ -248,7 +248,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
     }
 
     fn spill_operand_to_stack(
-        operand: &OperandRef<'tcx, Bx::Value>,
+        operand: OperandRef<'tcx, Bx::Value>,
         name: Option<String>,
         bx: &mut Bx,
     ) -> PlaceRef<'tcx, Bx::Value> {
@@ -375,7 +375,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                     return;
                 }
 
-                Self::spill_operand_to_stack(operand, name, bx)
+                Self::spill_operand_to_stack(*operand, name, bx)
             }
 
             LocalRef::Place(place) => *place,
@@ -550,7 +550,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                         if let Ok(operand) = self.eval_mir_constant_to_operand(bx, &c) {
                             self.set_debug_loc(bx, var.source_info);
                             let base = Self::spill_operand_to_stack(
-                                &operand,
+                                operand,
                                 Some(var.name.to_string()),
                                 bx,
                             );
diff --git a/compiler/rustc_codegen_ssa/src/mir/locals.rs b/compiler/rustc_codegen_ssa/src/mir/locals.rs
@@ -0,0 +1,75 @@
+//! Locals are in a private module as updating `LocalRef::Operand` has to
+//! be careful wrt to subtyping. To deal with this we only allow updates by using
+//! `FunctionCx::overwrite_local` which handles it automatically.
+use crate::mir::{FunctionCx, LocalRef};
+use crate::traits::BuilderMethods;
+use rustc_index::IndexVec;
+use rustc_middle::mir;
+use rustc_middle::ty::print::with_no_trimmed_paths;
+use std::ops::{Index, IndexMut};
+
+pub(super) struct Locals<'tcx, V> {
+    values: IndexVec<mir::Local, LocalRef<'tcx, V>>,
+}
+
+impl<'tcx, V> Index<mir::Local> for Locals<'tcx, V> {
+    type Output = LocalRef<'tcx, V>;
+    #[inline]
+    fn index(&self, index: mir::Local) -> &LocalRef<'tcx, V> {
+        &self.values[index]
+    }
+}
+
+/// To mutate locals, use `FunctionCx::overwrite_local` instead.
+impl<'tcx, V, Idx: ?Sized> !IndexMut<Idx> for Locals<'tcx, V> {}
+
+impl<'tcx, V> Locals<'tcx, V> {
+    pub(super) fn empty() -> Locals<'tcx, V> {
+        Locals { values: IndexVec::default() }
+    }
+
+    pub(super) fn indices(&self) -> impl DoubleEndedIterator<Item = mir::Local> + Clone + 'tcx {
+        self.values.indices()
+    }
+}
+
+impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
+    pub(super) fn initialize_locals(&mut self, values: Vec<LocalRef<'tcx, Bx::Value>>) {
+        assert!(self.locals.values.is_empty());
+
+        for (local, value) in values.into_iter().enumerate() {
+            match value {
+                LocalRef::Place(_) | LocalRef::UnsizedPlace(_) | LocalRef::PendingOperand => (),
+                LocalRef::Operand(op) => {
+                    let local = mir::Local::from_usize(local);
+                    let expected_ty = self.monomorphize(self.mir.local_decls[local].ty);
+                    assert_eq!(expected_ty, op.layout.ty, "unexpected initial operand type");
+                }
+            }
+
+            self.locals.values.push(value);
+        }
+    }
+
+    pub(super) fn overwrite_local(
+        &mut self,
+        local: mir::Local,
+        mut value: LocalRef<'tcx, Bx::Value>,
+    ) {
+        match value {
+            LocalRef::Place(_) | LocalRef::UnsizedPlace(_) | LocalRef::PendingOperand => (),
+            LocalRef::Operand(ref mut op) => {
+                let local_ty = self.monomorphize(self.mir.local_decls[local].ty);
+                if local_ty != op.layout.ty {
+                    // FIXME(#112651): This can be changed to an ICE afterwards.
+                    debug!("updating type of operand due to subtyping");
+                    with_no_trimmed_paths!(debug!(?op.layout.ty));
+                    with_no_trimmed_paths!(debug!(?local_ty));
+                    op.layout.ty = local_ty;
+                }
+            }
+        };
+
+        self.locals.values[local] = value;
+    }
+}
diff --git a/compiler/rustc_codegen_ssa/src/mir/mod.rs b/compiler/rustc_codegen_ssa/src/mir/mod.rs
@@ -1,21 +1,31 @@
 use crate::base;
 use crate::traits::*;
+use rustc_index::bit_set::BitSet;
+use rustc_index::IndexVec;
 use rustc_middle::mir;
 use rustc_middle::mir::interpret::ErrorHandled;
+use rustc_middle::mir::traversal;
 use rustc_middle::ty::layout::{FnAbiOf, HasTyCtxt, TyAndLayout};
 use rustc_middle::ty::{self, Instance, Ty, TyCtxt, TypeFoldable, TypeVisitableExt};
 use rustc_target::abi::call::{FnAbi, PassMode};
 
 use std::iter;
 
-use rustc_index::bit_set::BitSet;
-use rustc_index::IndexVec;
+mod analyze;
+mod block;
+pub mod constant;
+pub mod coverageinfo;
+pub mod debuginfo;
+mod intrinsic;
+mod locals;
+pub mod operand;
+pub mod place;
+mod rvalue;
+mod statement;
 
 use self::debuginfo::{FunctionDebugContext, PerLocalVarDebugInfo};
-use self::place::PlaceRef;
-use rustc_middle::mir::traversal;
-
 use self::operand::{OperandRef, OperandValue};
+use self::place::PlaceRef;
 
 // Used for tracking the state of generated basic blocks.
 enum CachedLlbb<T> {
@@ -91,7 +101,7 @@ pub struct FunctionCx<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> {
     ///
     /// Avoiding allocs can also be important for certain intrinsics,
     /// notably `expect`.
-    locals: IndexVec<mir::Local, LocalRef<'tcx, Bx::Value>>,
+    locals: locals::Locals<'tcx, Bx::Value>,
 
     /// All `VarDebugInfo` from the MIR body, partitioned by `Local`.
     /// This is `None` if no var`#[non_exhaustive]`iable debuginfo/names are needed.
@@ -192,7 +202,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
         cleanup_kinds,
         landing_pads: IndexVec::from_elem(None, &mir.basic_blocks),
         funclets: IndexVec::from_fn_n(|_| None, mir.basic_blocks.len()),
-        locals: IndexVec::new(),
+        locals: locals::Locals::empty(),
         debug_context,
         per_local_var_debug_info: None,
         caller_location: None,
@@ -223,7 +233,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
     let memory_locals = analyze::non_ssa_locals(&fx);
 
     // Allocate variable and temp allocas
-    fx.locals = {
+    let local_values = {
         let args = arg_local_refs(&mut start_bx, &mut fx, &memory_locals);
 
         let mut allocate_local = |local| {
@@ -256,6 +266,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
             .chain(mir.vars_and_temps_iter().map(allocate_local))
             .collect()
     };
+    fx.initialize_locals(local_values);
 
     // Apply debuginfo to the newly allocated locals.
     fx.debug_introduce_locals(&mut start_bx);
@@ -289,14 +300,13 @@ fn arg_local_refs<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
         .enumerate()
         .map(|(arg_index, local)| {
             let arg_decl = &mir.local_decls[local];
+            let arg_ty = fx.monomorphize(arg_decl.ty);
 
             if Some(local) == mir.spread_arg {
                 // This argument (e.g., the last argument in the "rust-call" ABI)
                 // is a tuple that was spread at the ABI level and now we have
                 // to reconstruct it into a tuple local variable, from multiple
                 // individual LLVM function arguments.
-
-                let arg_ty = fx.monomorphize(arg_decl.ty);
                 let ty::Tuple(tupled_arg_tys) = arg_ty.kind() else {
                     bug!("spread argument isn't a tuple?!");
                 };
@@ -331,8 +341,6 @@ fn arg_local_refs<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
             }
 
             if fx.fn_abi.c_variadic && arg_index == fx.fn_abi.args.len() {
-                let arg_ty = fx.monomorphize(arg_decl.ty);
-
                 let va_list = PlaceRef::alloca(bx, bx.layout_of(arg_ty));
                 bx.va_start(va_list.llval);
 
@@ -429,14 +437,3 @@ fn arg_local_refs<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
 
     args
 }
-
-mod analyze;
-mod block;
-pub mod constant;
-pub mod coverageinfo;
-pub mod debuginfo;
-mod intrinsic;
-pub mod operand;
-pub mod place;
-mod rvalue;
-mod statement;
diff --git a/compiler/rustc_codegen_ssa/src/mir/statement.rs b/compiler/rustc_codegen_ssa/src/mir/statement.rs
@@ -20,7 +20,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                         }
                         LocalRef::PendingOperand => {
                             let operand = self.codegen_rvalue_operand(bx, rvalue);
-                            self.locals[index] = LocalRef::Operand(operand);
+                            self.overwrite_local(index, LocalRef::Operand(operand));
                             self.debug_introduce_local(bx, index);
                         }
                         LocalRef::Operand(op) => {
diff --git a/compiler/rustc_mir_transform/src/dest_prop.rs b/compiler/rustc_mir_transform/src/dest_prop.rs
@@ -37,6 +37,10 @@
 //!   if they do not consistently refer to the same place in memory. This is satisfied if they do
 //!   not contain any indirection through a pointer or any indexing projections.
 //!
+//! * `p` and `q` must have the **same type**. If we replace a local with a subtype or supertype,
+//!   we may end up with a differnet vtable for that local. See the `subtyping-impacts-selection`
+//!   tests for an example where that causes issues.
+//!
 //! * We need to make sure that the goal of "merging the memory" is actually structurally possible
 //!   in MIR. For example, even if all the other conditions are satisfied, there is no way to
 //!   "merge" `_5.foo` and `_6.bar`. For now, we ensure this by requiring that both `p` and `q` are
@@ -134,6 +138,7 @@ use crate::MirPass;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_index::bit_set::BitSet;
 use rustc_middle::mir::visit::{MutVisitor, PlaceContext, Visitor};
+use rustc_middle::mir::HasLocalDecls;
 use rustc_middle::mir::{dump_mir, PassWhere};
 use rustc_middle::mir::{
     traversal, Body, InlineAsmOperand, Local, LocalKind, Location, Operand, Place, Rvalue,
@@ -763,12 +768,22 @@ impl<'tcx> Visitor<'tcx> for FindAssignments<'_, '_, 'tcx> {
                 return;
             };
 
-            // As described at the top of the file, we do not go near things that have their address
-            // taken.
+            // As described at the top of the file, we do not go near things that have
+            // their address taken.
             if self.borrowed.contains(src) || self.borrowed.contains(dest) {
                 return;
             }
 
+            // As described at the top of this file, we do not touch locals which have
+            // different types.
+            let src_ty = self.body.local_decls()[src].ty;
+            let dest_ty = self.body.local_decls()[dest].ty;
+            if src_ty != dest_ty {
+                // FIXME(#112651): This can be removed afterwards. Also update the module description.
+                trace!("skipped `{src:?} = {dest:?}` due to subtyping: {src_ty} != {dest_ty}");
+                return;
+            }
+
             // Also, we need to make sure that MIR actually allows the `src` to be removed
             if is_local_required(src, self.body) {
                 return;
diff --git a/compiler/rustc_mir_transform/src/shim.rs b/compiler/rustc_mir_transform/src/shim.rs
@@ -32,13 +32,15 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<'
     let mut result = match instance {
         ty::InstanceDef::Item(..) => bug!("item {:?} passed to make_shim", instance),
         ty::InstanceDef::VTableShim(def_id) => {
-            build_call_shim(tcx, instance, Some(Adjustment::Deref), CallKind::Direct(def_id))
+            let adjustment = Adjustment::Deref { source: DerefSource::MutPtr };
+            build_call_shim(tcx, instance, Some(adjustment), CallKind::Direct(def_id))
         }
         ty::InstanceDef::FnPtrShim(def_id, ty) => {
             let trait_ = tcx.trait_of_item(def_id).unwrap();
             let adjustment = match tcx.fn_trait_kind_from_def_id(trait_) {
                 Some(ty::ClosureKind::FnOnce) => Adjustment::Identity,
-                Some(ty::ClosureKind::FnMut | ty::ClosureKind::Fn) => Adjustment::Deref,
+                Some(ty::ClosureKind::Fn) => Adjustment::Deref { source: DerefSource::ImmRef },
+                Some(ty::ClosureKind::FnMut) => Adjustment::Deref { source: DerefSource::MutRef },
                 None => bug!("fn pointer {:?} is not an fn", ty),
             };
 
@@ -107,16 +109,26 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<'
     result
 }
 
+#[derive(Copy, Clone, Debug, PartialEq)]
+enum DerefSource {
+    /// `fn shim(&self) { inner(*self )}`.
+    ImmRef,
+    /// `fn shim(&mut self) { inner(*self )}`.
+    MutRef,
+    /// `fn shim(*mut self) { inner(*self )}`.
+    MutPtr,
+}
+
 #[derive(Copy, Clone, Debug, PartialEq)]
 enum Adjustment {
     /// Pass the receiver as-is.
     Identity,
 
-    /// We get passed `&[mut] self` and call the target with `*self`.
+    /// We get passed a reference or a raw pointer to `self` and call the target with `*self`.
     ///
     /// This either copies `self` (if `Self: Copy`, eg. for function items), or moves out of it
     /// (for `VTableShim`, which effectively is passed `&own Self`).
-    Deref,
+    Deref { source: DerefSource },
 
     /// We get passed `self: Self` and call the target with `&mut self`.
     ///
@@ -667,8 +679,12 @@ fn build_call_shim<'tcx>(
         let self_arg = &mut inputs_and_output[0];
         *self_arg = match rcvr_adjustment.unwrap() {
             Adjustment::Identity => fnty,
-            Adjustment::Deref => tcx.mk_imm_ptr(fnty),
-            Adjustment::RefMut => tcx.mk_mut_ptr(fnty),
+            Adjustment::Deref { source } => match source {
+                DerefSource::ImmRef => tcx.mk_imm_ref(tcx.lifetimes.re_erased, fnty),
+                DerefSource::MutRef => tcx.mk_mut_ref(tcx.lifetimes.re_erased, fnty),
+                DerefSource::MutPtr => tcx.mk_mut_ptr(fnty),
+            },
+            Adjustment::RefMut => bug!("`RefMut` is never used with indirect calls: {instance:?}"),
         };
         sig.inputs_and_output = tcx.mk_type_list(&inputs_and_output);
     }
@@ -699,7 +715,7 @@ fn build_call_shim<'tcx>(
 
     let rcvr = rcvr_adjustment.map(|rcvr_adjustment| match rcvr_adjustment {
         Adjustment::Identity => Operand::Move(rcvr_place()),
-        Adjustment::Deref => Operand::Move(tcx.mk_place_deref(rcvr_place())),
+        Adjustment::Deref { source: _ } => Operand::Move(tcx.mk_place_deref(rcvr_place())),
         Adjustment::RefMut => {
             // let rcvr = &mut rcvr;
             let ref_rcvr = local_decls.push(
diff --git a/compiler/rustc_mir_transform/src/ssa.rs b/compiler/rustc_mir_transform/src/ssa.rs
@@ -271,6 +271,14 @@ fn compute_copy_classes(ssa: &mut SsaLocals, body: &Body<'_>) {
         else { continue };
 
         let Some(rhs) = place.as_local() else { continue };
+        let local_ty = body.local_decls()[local].ty;
+        let rhs_ty = body.local_decls()[rhs].ty;
+        if local_ty != rhs_ty {
+            // FIXME(#112651): This can be removed afterwards.
+            trace!("skipped `{local:?} = {rhs:?}` due to subtyping: {local_ty} != {rhs_ty}");
+            continue;
+        }
+
         if !ssa.is_ssa(rhs) {
             continue;
         }
diff --git a/tests/mir-opt/fn_ptr_shim.core.ops-function-Fn-call.AddMovesForPackedDrops.before.mir b/tests/mir-opt/fn_ptr_shim.core.ops-function-Fn-call.AddMovesForPackedDrops.before.mir
@@ -1,6 +1,6 @@
 // MIR for `std::ops::Fn::call` before AddMovesForPackedDrops
 
-fn std::ops::Fn::call(_1: *const fn(), _2: ()) -> <fn() as FnOnce<()>>::Output {
+fn std::ops::Fn::call(_1: &fn(), _2: ()) -> <fn() as FnOnce<()>>::Output {
     let mut _0: <fn() as std::ops::FnOnce<()>>::Output;
 
     bb0: {
diff --git a/tests/ui/codegen/subtyping-impacts-selection-1.rs b/tests/ui/codegen/subtyping-impacts-selection-1.rs
diff --git a/tests/ui/codegen/subtyping-impacts-selection-2.rs b/tests/ui/codegen/subtyping-impacts-selection-2.rs

Original file line number	Diff line number	Diff line change
`@@ -1729,7 +1729,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {`
`1729`	`1729`	`IndirectOperand(tmp, index) => {`
`1730`	`1730`	`let op = bx.load_operand(tmp);`
`1731`	`1731`	`tmp.storage_dead(bx);`
`1732`		`- self.locals[index] = LocalRef::Operand(op);`
	`1732`	`+ self.overwrite_local(index, LocalRef::Operand(op));`
`1733`	`1733`	`self.debug_introduce_local(bx, index);`
`1734`	`1734`	`}`
`1735`	`1735`	`DirectOperand(index) => {`
`@@ -1744,7 +1744,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {`
`1744`	`1744`	`} else {`
`1745`	`1745`	`OperandRef::from_immediate_or_packed_pair(bx, llval, ret_abi.layout)`
`1746`	`1746`	`};`
`1747`		`- self.locals[index] = LocalRef::Operand(op);`
	`1747`	`+ self.overwrite_local(index, LocalRef::Operand(op));`
`1748`	`1748`	`self.debug_introduce_local(bx, index);`
`1749`	`1749`	`}`
`1750`	`1750`	`}`
Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {`
`20`	`20`	`}`
`21`	`21`	`LocalRef::PendingOperand => {`
`22`	`22`	`let operand = self.codegen_rvalue_operand(bx, rvalue);`
`23`		`- self.locals[index] = LocalRef::Operand(operand);`
	`23`	`+ self.overwrite_local(index, LocalRef::Operand(operand));`
`24`	`24`	`self.debug_introduce_local(bx, index);`
`25`	`25`	`}`
`26`	`26`	`LocalRef::Operand(op) => {`