Treat RETURN_PLACE as a normal Local

jonas-schievink · jonas-schievink · commit 4eaf535d2b4f · 2020-04-20T21:18:18.000+02:00
Copy its value to the `return_place` upon leaving a call frame
diff --git a/src/librustc_middle/mir/interpret/error.rs b/src/librustc_middle/mir/interpret/error.rs
@@ -361,8 +361,6 @@ pub enum UndefinedBehaviorInfo {
     InvalidUndefBytes(Option<Pointer>),
     /// Working with a local that is not currently live.
     DeadLocal,
-    /// Trying to read from the return place of a function.
-    ReadFromReturnPlace,
 }
 
 impl fmt::Debug for UndefinedBehaviorInfo {
@@ -424,7 +422,6 @@ impl fmt::Debug for UndefinedBehaviorInfo {
                 "using uninitialized data, but this operation requires initialized memory"
             ),
             DeadLocal => write!(f, "accessing a dead local variable"),
-            ReadFromReturnPlace => write!(f, "reading from return place"),
         }
     }
 }
diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs
@@ -623,35 +623,30 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         let frame = M::init_frame_extra(self, pre_frame)?;
         self.stack_mut().push(frame);
 
-        // don't allocate at all for trivial constants
-        if body.local_decls.len() > 1 {
-            // Locals are initially uninitialized.
-            let dummy = LocalState { value: LocalValue::Uninitialized, layout: Cell::new(None) };
-            let mut locals = IndexVec::from_elem(dummy, &body.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].value = LocalValue::Dead;
-            // Now mark those locals as dead that we do not want to initialize
-            match self.tcx.def_kind(instance.def_id()) {
-                // statics and constants don't have `Storage*` statements, no need to look for them
-                //
-                // FIXME: The above is likely untrue. See
-                // <https://github.com/rust-lang/rust/pull/70004#issuecomment-602022110>. Is it
-                // okay to ignore `StorageDead`/`StorageLive` annotations during CTFE?
-                Some(DefKind::Static | DefKind::Const | DefKind::AssocConst) => {}
-                _ => {
-                    // Mark locals that use `Storage*` annotations as dead on function entry.
-                    let always_live = AlwaysLiveLocals::new(self.body());
-                    for local in locals.indices() {
-                        if !always_live.contains(local) {
-                            locals[local].value = LocalValue::Dead;
-                        }
+        // Locals are initially uninitialized.
+        let dummy = LocalState { value: LocalValue::Uninitialized, layout: Cell::new(None) };
+        let mut locals = IndexVec::from_elem(dummy, &body.local_decls);
+
+        // Now mark those locals as dead that we do not want to initialize
+        match self.tcx.def_kind(instance.def_id()) {
+            // statics and constants don't have `Storage*` statements, no need to look for them
+            //
+            // FIXME: The above is likely untrue. See
+            // <https://github.com/rust-lang/rust/pull/70004#issuecomment-602022110>. Is it
+            // okay to ignore `StorageDead`/`StorageLive` annotations during CTFE?
+            Some(DefKind::Static | DefKind::Const | DefKind::AssocConst) => {}
+            _ => {
+                // Mark locals that use `Storage*` annotations as dead on function entry.
+                let always_live = AlwaysLiveLocals::new(self.body());
+                for local in locals.indices() {
+                    if !always_live.contains(local) {
+                        locals[local].value = LocalValue::Dead;
                     }
                 }
             }
-            // done
-            self.frame_mut().locals = locals;
         }
+        // done
+        self.frame_mut().locals = locals;
 
         M::after_stack_push(self)?;
         info!("ENTERING({}) {}", self.frame_idx(), self.frame().instance);
@@ -729,6 +724,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         let frame =
             self.stack_mut().pop().expect("tried to pop a stack frame, but there were none");
 
+        if let Some(return_place) = frame.return_place {
+            // Copy the return value to the caller's stack frame.
+            let op = self.access_local(&frame, mir::RETURN_PLACE, None)?;
+            self.copy_op(op, return_place)?;
+        }
+
         // Now where do we jump next?
 
         // Usually we want to clean up (deallocate locals), but in a few rare cases we don't.
diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs
@@ -419,7 +419,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         local: mir::Local,
         layout: Option<TyAndLayout<'tcx>>,
     ) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
-        assert_ne!(local, mir::RETURN_PLACE);
         let layout = self.layout_of_local(frame, local, layout)?;
         let op = if layout.is_zst() {
             // Do not read from ZST, they might not be initialized
@@ -454,15 +453,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         place: mir::Place<'tcx>,
         layout: Option<TyAndLayout<'tcx>>,
     ) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
-        let base_op = match place.local {
-            mir::RETURN_PLACE => throw_ub!(ReadFromReturnPlace),
-            local => {
-                // Do not use the layout passed in as argument if the base we are looking at
-                // here is not the entire place.
-                let layout = if place.projection.is_empty() { layout } else { None };
-
-                self.access_local(self.frame(), local, layout)?
-            }
+        let base_op = {
+            // Do not use the layout passed in as argument if the base we are looking at
+            // here is not the entire place.
+            let layout = if place.projection.is_empty() { layout } else { None };
+
+            self.access_local(self.frame(), place.local, layout)?
         };
 
         let op = place
diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs
@@ -276,6 +276,10 @@ impl<Tag: ::std::fmt::Debug> Place<Tag> {
 }
 
 impl<'tcx, Tag: ::std::fmt::Debug> PlaceTy<'tcx, Tag> {
+    pub fn null(cx: &impl HasDataLayout, layout: TyAndLayout<'tcx>) -> Self {
+        Self { place: Place::null(cx), layout }
+    }
+
     #[inline]
     pub fn assert_mem_place(self) -> MPlaceTy<'tcx, Tag> {
         MPlaceTy { mplace: self.place.assert_mem_place(), layout: self.layout }
@@ -636,35 +640,10 @@ where
         &mut self,
         place: mir::Place<'tcx>,
     ) -> InterpResult<'tcx, PlaceTy<'tcx, M::PointerTag>> {
-        let mut place_ty = match place.local {
-            mir::RETURN_PLACE => {
-                // `return_place` has the *caller* layout, but we want to use our
-                // `layout to verify our assumption. The caller will validate
-                // their layout on return.
-                PlaceTy {
-                    place: match self.frame().return_place {
-                        Some(p) => *p,
-                        // Even if we don't have a return place, we sometimes need to
-                        // create this place, but any attempt to read from / write to it
-                        // (even a ZST read/write) needs to error, so let us make this
-                        // a NULL place.
-                        //
-                        // FIXME: Ideally we'd make sure that the place projections also
-                        // bail out.
-                        None => Place::null(&*self),
-                    },
-                    layout: self.layout_of(
-                        self.subst_from_current_frame_and_normalize_erasing_regions(
-                            self.frame().body.return_ty(),
-                        ),
-                    )?,
-                }
-            }
-            local => PlaceTy {
-                // This works even for dead/uninitialized locals; we check further when writing
-                place: Place::Local { frame: self.frame_idx(), local },
-                layout: self.layout_of_local(self.frame(), local, None)?,
-            },
+        let mut place_ty = PlaceTy {
+            // This works even for dead/uninitialized locals; we check further when writing
+            place: Place::Local { frame: self.frame_idx(), local: place.local },
+            layout: self.layout_of_local(self.frame(), place.local, None)?,
         };
 
         for elem in place.projection.iter() {
diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs
@@ -19,7 +19,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         use rustc_middle::mir::TerminatorKind::*;
         match terminator.kind {
             Return => {
-                self.frame().return_place.map(|r| self.dump_place(*r));
                 self.pop_stack_frame(/* unwinding */ false)?
             }
 
diff --git a/src/test/ui/consts/const-eval/ub-nonnull.stderr b/src/test/ui/consts/const-eval/ub-nonnull.stderr
@@ -13,7 +13,7 @@ LL | / const OUT_OF_BOUNDS_PTR: NonNull<u8> = { unsafe {
 LL | |     let ptr: &[u8; 256] = mem::transmute(&0u8); // &0 gets promoted so it does not dangle
 LL | |     // Use address-of-element for pointer arithmetic. This could wrap around to NULL!
 LL | |     let out_of_bounds_ptr = &ptr[255];
-   | |                             ^^^^^^^^^ Memory access failed: pointer must be in-bounds at offset 256, but is outside bounds of alloc8 which has size 1
+   | |                             ^^^^^^^^^ Memory access failed: pointer must be in-bounds at offset 256, but is outside bounds of alloc11 which has size 1
 LL | |     mem::transmute(out_of_bounds_ptr)
 LL | | } };
    | |____-
diff --git a/src/test/ui/consts/miri_unleashed/mutable_const.stderr b/src/test/ui/consts/miri_unleashed/mutable_const.stderr
@@ -11,7 +11,7 @@ LL | / const MUTATING_BEHIND_RAW: () = {
 LL | |     // Test that `MUTABLE_BEHIND_RAW` is actually immutable, by doing this at const time.
 LL | |     unsafe {
 LL | |         *MUTABLE_BEHIND_RAW = 99
-   | |         ^^^^^^^^^^^^^^^^^^^^^^^^ writing to alloc1 which is read-only
+   | |         ^^^^^^^^^^^^^^^^^^^^^^^^ writing to alloc2 which is read-only
 LL | |     }
 LL | | };
    | |__-

Original file line number	Diff line number	Diff line change
`@@ -361,8 +361,6 @@ pub enum UndefinedBehaviorInfo {`
`361`	`361`	`InvalidUndefBytes(Option<Pointer>),`
`362`	`362`	`/// Working with a local that is not currently live.`
`363`	`363`	`DeadLocal,`
`364`		`- /// Trying to read from the return place of a function.`
`365`		`- ReadFromReturnPlace,`
`366`	`364`	`}`
`367`	`365`
`368`	`366`	`impl fmt::Debug for UndefinedBehaviorInfo {`
`@@ -424,7 +422,6 @@ impl fmt::Debug for UndefinedBehaviorInfo {`
`424`	`422`	`"using uninitialized data, but this operation requires initialized memory"`
`425`	`423`	`),`
`426`	`424`	`DeadLocal => write!(f, "accessing a dead local variable"),`
`427`		`- ReadFromReturnPlace => write!(f, "reading from return place"),`
`428`	`425`	`}`
`429`	`426`	`}`
`430`	`427`	`}`
Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {`
`19`	`19`	`use rustc_middle::mir::TerminatorKind::*;`
`20`	`20`	`match terminator.kind {`
`21`	`21`	`Return => {`
`22`		`- self.frame().return_place.map(\|r\| self.dump_place(*r));`
`23`	`22`	`self.pop_stack_frame(/* unwinding */ false)?`
`24`	`23`	`}`
`25`	`24`