Rollup merge of #58000 - oli-obk:fixes_and_cleanups, r=RalfJung

Fixes and cleanups

Address the points raised in https://github.com/rust-lang/rust/pull/57677/files by @eddyb and @RalfJung

Author: Centril

src/librustc/mir/interpret/value.rs

@@ -14,7 +14,7 @@ pub struct RawConst<'tcx> {
 }
 
 /// Represents a constant value in Rust. Scalar and ScalarPair are optimizations which
-/// matches the LocalValue optimizations for easy conversions between Value and ConstValue.
+/// matches the LocalState optimizations for easy conversions between Value and ConstValue.
 #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, RustcEncodable, RustcDecodable, Hash)]
 pub enum ConstValue<'tcx> {
     /// Used only for types with layout::abi::Scalar ABI and ZSTs

src/librustc_mir/interpret/eval_context.rs

@@ -76,8 +76,7 @@ pub struct Frame<'mir, 'tcx: 'mir, Tag=(), Extra=()> {
     /// The locals are stored as `Option<Value>`s.
     /// `None` represents a local that is currently dead, while a live local
     /// can either directly contain `Scalar` or refer to some part of an `Allocation`.
-    pub locals: IndexVec<mir::Local, LocalValue<Tag>>,
-    pub local_layouts: IndexVec<mir::Local, Cell<Option<TyLayout<'tcx>>>>,
+    pub locals: IndexVec<mir::Local, LocalState<'tcx, Tag>>,
 
     ////////////////////////////////////////////////////////////////////////////////
     // Current position within the function
@@ -106,7 +105,15 @@ pub enum StackPopCleanup {
     None { cleanup: bool },
 }
 
-// State of a local variable
+/// State of a local variable including a memoized layout
+#[derive(Clone, PartialEq, Eq)]
+pub struct LocalState<'tcx, Tag=(), Id=AllocId> {
+    pub state: LocalValue<Tag, Id>,
+    /// Don't modify if `Some`, this is only used to prevent computing the layout twice
+    pub layout: Cell<Option<TyLayout<'tcx>>>,
+}
+
+/// State of a local variable
 #[derive(Copy, Clone, PartialEq, Eq, Hash)]
 pub enum LocalValue<Tag=(), Id=AllocId> {
     Dead,
@@ -117,16 +124,16 @@ pub enum LocalValue<Tag=(), Id=AllocId> {
     Live(Operand<Tag, Id>),
 }
 
-impl<'tcx, Tag> LocalValue<Tag> {
+impl<'tcx, Tag> LocalState<'tcx, Tag> {
     pub fn access(&self) -> EvalResult<'tcx, &Operand<Tag>> {
-        match self {
+        match self.state {
             LocalValue::Dead => err!(DeadLocal),
             LocalValue::Live(ref val) => Ok(val),
         }
     }
 
     pub fn access_mut(&mut self) -> EvalResult<'tcx, &mut Operand<Tag>> {
-        match self {
+        match self.state {
             LocalValue::Dead => err!(DeadLocal),
             LocalValue::Live(ref mut val) => Ok(val),
         }
@@ -310,17 +317,21 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
     pub fn layout_of_local(
         &self,
         frame: &Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra>,
-        local: mir::Local
+        local: mir::Local,
+        layout: Option<TyLayout<'tcx>>,
     ) -> EvalResult<'tcx, TyLayout<'tcx>> {
-        let cell = &frame.local_layouts[local];
-        if cell.get().is_none() {
-            let local_ty = frame.mir.local_decls[local].ty;
-            let local_ty = self.monomorphize_with_substs(local_ty, frame.instance.substs);
-            let layout = self.layout_of(local_ty)?;
-            cell.set(Some(layout));
+        match frame.locals[local].layout.get() {
+            None => {
+                let layout = ::interpret::operand::from_known_layout(layout, || {
+                    let local_ty = frame.mir.local_decls[local].ty;
+                    let local_ty = self.monomorphize_with_substs(local_ty, frame.instance.substs);
+                    self.layout_of(local_ty)
+                })?;
+                frame.locals[local].layout.set(Some(layout));
+                Ok(layout)
+            }
+            Some(layout) => Ok(layout),
         }
-
-        Ok(cell.get().unwrap())
     }
 
     pub fn str_to_immediate(&mut self, s: &str) -> EvalResult<'tcx, Immediate<M::PointerTag>> {
@@ -454,7 +465,6 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
             // empty local array, we fill it in below, after we are inside the stack frame and
             // all methods actually know about the frame
             locals: IndexVec::new(),
-            local_layouts: IndexVec::from_elem_n(Default::default(), mir.local_decls.len()),
             span,
             instance,
             stmt: 0,
@@ -466,12 +476,16 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
             // We put some marker immediate into the locals that we later want to initialize.
             // This can be anything except for LocalValue::Dead -- because *that* is the
             // value we use for things that we know are initially dead.
-            let dummy =
-                LocalValue::Live(Operand::Immediate(Immediate::Scalar(ScalarMaybeUndef::Undef)));
+            let dummy = LocalState {
+                state: LocalValue::Live(Operand::Immediate(Immediate::Scalar(
+                    ScalarMaybeUndef::Undef,
+                ))),
+                layout: Cell::new(None),
+            };
             let mut locals = IndexVec::from_elem(dummy, &mir.local_decls);
             // Return place is handled specially by the `eval_place` functions, and the
             // entry in `locals` should never be used. Make it dead, to be sure.
-            locals[mir::RETURN_PLACE] = LocalValue::Dead;
+            locals[mir::RETURN_PLACE].state = LocalValue::Dead;
             // Now mark those locals as dead that we do not want to initialize
             match self.tcx.describe_def(instance.def_id()) {
                 // statics and constants don't have `Storage*` statements, no need to look for them
@@ -484,7 +498,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
                             match stmt.kind {
                                 StorageLive(local) |
                                 StorageDead(local) => {
-                                    locals[local] = LocalValue::Dead;
+                                    locals[local].state = LocalValue::Dead;
                                 }
                                 _ => {}
                             }
@@ -494,11 +508,13 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
             }
             // Finally, properly initialize all those that still have the dummy value
             for (idx, local) in locals.iter_enumerated_mut() {
-                match *local {
+                match local.state {
                     LocalValue::Live(_) => {
-                        // This needs to be peoperly initialized.
-                        let layout = self.layout_of_local(self.frame(), idx)?;
-                        *local = LocalValue::Live(self.uninit_operand(layout)?);
+                        // This needs to be properly initialized.
+                        let ty = self.monomorphize(mir.local_decls[idx].ty)?;
+                        let layout = self.layout_of(ty)?;
+                        local.state = LocalValue::Live(self.uninit_operand(layout)?);
+                        local.layout = Cell::new(Some(layout));
                     }
                     LocalValue::Dead => {
                         // Nothing to do
@@ -543,7 +559,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
         }
         // Deallocate all locals that are backed by an allocation.
         for local in frame.locals {
-            self.deallocate_local(local)?;
+            self.deallocate_local(local.state)?;
         }
         // Validate the return value. Do this after deallocating so that we catch dangling
         // references.
@@ -591,10 +607,10 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
         assert!(local != mir::RETURN_PLACE, "Cannot make return place live");
         trace!("{:?} is now live", local);
 
-        let layout = self.layout_of_local(self.frame(), local)?;
+        let layout = self.layout_of_local(self.frame(), local, None)?;
         let init = LocalValue::Live(self.uninit_operand(layout)?);
         // StorageLive *always* kills the value that's currently stored
-        Ok(mem::replace(&mut self.frame_mut().locals[local], init))
+        Ok(mem::replace(&mut self.frame_mut().locals[local].state, init))
     }
 
     /// Returns the old value of the local.
@@ -603,7 +619,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc
         assert!(local != mir::RETURN_PLACE, "Cannot make return place dead");
         trace!("{:?} is now dead", local);
 
-        mem::replace(&mut self.frame_mut().locals[local], LocalValue::Dead)
+        mem::replace(&mut self.frame_mut().locals[local].state, LocalValue::Dead)
     }
 
     pub(super) fn deallocate_local(

src/librustc_mir/interpret/mod.rs

@@ -18,7 +18,7 @@ mod visitor;
 pub use rustc::mir::interpret::*; // have all the `interpret` symbols in one place: here
 
 pub use self::eval_context::{
-    EvalContext, Frame, StackPopCleanup, LocalValue,
+    EvalContext, Frame, StackPopCleanup, LocalState, LocalValue,
 };
 
 pub use self::place::{Place, PlaceTy, MemPlace, MPlaceTy};

src/librustc_mir/interpret/operand.rs

@@ -227,7 +227,7 @@ impl<'tcx, Tag> OpTy<'tcx, Tag>
 // Use the existing layout if given (but sanity check in debug mode),
 // or compute the layout.
 #[inline(always)]
-fn from_known_layout<'tcx>(
+pub(super) fn from_known_layout<'tcx>(
     layout: Option<TyLayout<'tcx>>,
     compute: impl FnOnce() -> EvalResult<'tcx, TyLayout<'tcx>>
 ) -> EvalResult<'tcx, TyLayout<'tcx>> {
@@ -457,14 +457,15 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
     }
 
     /// This is used by [priroda](https://github.com/oli-obk/priroda) to get an OpTy from a local
-    fn access_local(
+    pub fn access_local(
         &self,
         frame: &super::Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra>,
         local: mir::Local,
+        layout: Option<TyLayout<'tcx>>,
     ) -> EvalResult<'tcx, OpTy<'tcx, M::PointerTag>> {
         assert_ne!(local, mir::RETURN_PLACE);
         let op = *frame.locals[local].access()?;
-        let layout = self.layout_of_local(frame, local)?;
+        let layout = self.layout_of_local(frame, local, layout)?;
         Ok(OpTy { op, layout })
     }
 
@@ -473,14 +474,15 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
     fn eval_place_to_op(
         &self,
         mir_place: &mir::Place<'tcx>,
+        layout: Option<TyLayout<'tcx>>,
     ) -> EvalResult<'tcx, OpTy<'tcx, M::PointerTag>> {
         use rustc::mir::Place::*;
         let op = match *mir_place {
             Local(mir::RETURN_PLACE) => return err!(ReadFromReturnPointer),
-            Local(local) => self.access_local(self.frame(), local)?,
+            Local(local) => self.access_local(self.frame(), local, layout)?,
 
             Projection(ref proj) => {
-                let op = self.eval_place_to_op(&proj.base)?;
+                let op = self.eval_place_to_op(&proj.base, None)?;
                 self.operand_projection(op, &proj.elem)?
             }
 
@@ -504,7 +506,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
             // FIXME: do some more logic on `move` to invalidate the old location
             Copy(ref place) |
             Move(ref place) =>
-                self.eval_place_to_op(place)?,
+                self.eval_place_to_op(place, layout)?,
 
             Constant(ref constant) => {
                 let layout = from_known_layout(layout, || {

src/librustc_mir/interpret/place.rs

@@ -624,7 +624,7 @@ where
                     // their layout on return.
                     PlaceTy {
                         place: *return_place,
-                        layout: self.layout_of_local(self.frame(), mir::RETURN_PLACE)?,
+                        layout: self.layout_of(self.monomorphize(self.frame().mir.return_ty())?)?,
                     },
                 None => return err!(InvalidNullPointerUsage),
             },
@@ -633,7 +633,7 @@ where
                     frame: self.cur_frame(),
                     local,
                 },
-                layout: self.layout_of_local(self.frame(), local)?,
+                layout: self.layout_of_local(self.frame(), local, None)?,
             },
 
             Projection(ref proj) => {
@@ -901,7 +901,7 @@ where
                         // We need the layout of the local.  We can NOT use the layout we got,
                         // that might e.g., be an inner field of a struct with `Scalar` layout,
                         // that has different alignment than the outer field.
-                        let local_layout = self.layout_of_local(&self.stack[frame], local)?;
+                        let local_layout = self.layout_of_local(&self.stack[frame], local, None)?;
                         let ptr = self.allocate(local_layout, MemoryKind::Stack);
                         // We don't have to validate as we can assume the local
                         // was already valid for its type.

src/librustc_mir/interpret/snapshot.rs

@@ -23,8 +23,8 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use syntax::ast::Mutability;
 use syntax::source_map::Span;
 
-use super::eval_context::{LocalValue, StackPopCleanup};
-use super::{Frame, Memory, Operand, MemPlace, Place, Immediate, ScalarMaybeUndef};
+use super::eval_context::{LocalState, StackPopCleanup};
+use super::{Frame, Memory, Operand, MemPlace, Place, Immediate, ScalarMaybeUndef, LocalValue};
 use const_eval::CompileTimeInterpreter;
 
 #[derive(Default)]
@@ -321,7 +321,6 @@ impl_stable_hash_for!(impl<'mir, 'tcx: 'mir> for struct Frame<'mir, 'tcx> {
     return_to_block,
     return_place -> (return_place.as_ref().map(|r| &**r)),
     locals,
-    local_layouts -> _,
     block,
     stmt,
     extra,
@@ -340,7 +339,6 @@ impl<'a, 'mir, 'tcx, Ctx> Snapshot<'a, Ctx> for &'a Frame<'mir, 'tcx>
             return_to_block,
             return_place,
             locals,
-            local_layouts: _,
             block,
             stmt,
             extra: _,
@@ -358,6 +356,22 @@ impl<'a, 'mir, 'tcx, Ctx> Snapshot<'a, Ctx> for &'a Frame<'mir, 'tcx>
     }
 }
 
+impl<'a, 'tcx, Ctx> Snapshot<'a, Ctx> for &'a LocalState<'tcx>
+    where Ctx: SnapshotContext<'a>,
+{
+    type Item = LocalValue<(), AllocIdSnapshot<'a>>;
+
+    fn snapshot(&self, ctx: &'a Ctx) -> Self::Item {
+        let LocalState { state, layout: _ } = self;
+        state.snapshot(ctx)
+    }
+}
+
+impl_stable_hash_for!(struct LocalState<'tcx> {
+    state,
+    layout -> _,
+});
+
 impl<'a, 'b, 'mir, 'tcx: 'a+'mir> SnapshotContext<'b>
     for Memory<'a, 'mir, 'tcx, CompileTimeInterpreter<'a, 'mir, 'tcx>>
 {

src/librustc_mir/interpret/terminator.rs

@@ -309,7 +309,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
                         mir.spread_arg,
                         mir.args_iter()
                             .map(|local|
-                                (local, self.layout_of_local(self.frame(), local).unwrap().ty)
+                                (local, self.layout_of_local(self.frame(), local, None).unwrap().ty)
                             )
                             .collect::<Vec<_>>()
                     );
@@ -383,7 +383,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
                         }
                     } else {
                         let callee_layout =
-                            self.layout_of_local(self.frame(), mir::RETURN_PLACE)?;
+                            self.layout_of_local(self.frame(), mir::RETURN_PLACE, None)?;
                         if !callee_layout.abi.is_uninhabited() {
                             return err!(FunctionRetMismatch(
                                 self.tcx.types.never, callee_layout.ty