Rollup merge of #71005 - jonas-schievink:no-place-like-return, r=oli-obk

Reading from the return place is fine

Const eval thinks that reading from local `_0` is UB, but it isn't. `_0` is just a normal local like any other, and codegen handles it that way too. The only special thing is that the `Return` terminator will read from it.

I've hit these errors while working on an NRVO pass that can merge other locals with `_0` in #71003.

r? @oli-obk

Author: Dylan-DPC

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"),
         }
     }
 }

src/librustc_middle/mir/interpret/value.rs

@@ -188,11 +188,6 @@ impl<'tcx, Tag> Scalar<Tag> {
         }
     }
 
-    #[inline]
-    pub fn null_ptr(cx: &impl HasDataLayout) -> Self {
-        Scalar::Raw { data: 0, size: cx.data_layout().pointer_size.bytes() as u8 }
-    }
-
     #[inline]
     pub fn zst() -> Self {
         Scalar::Raw { data: 0, size: 0 }

src/librustc_mir/interpret/eval_context.rs

@@ -628,35 +628,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);
@@ -734,6 +729,17 @@ 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 !unwinding {
+            // Copy the return value to the caller's stack frame.
+            if let Some(return_place) = frame.return_place {
+                let op = self.access_local(&frame, mir::RETURN_PLACE, None)?;
+                self.copy_op_transmute(op, return_place)?;
+                self.dump_place(*return_place);
+            } else {
+                throw_ub!(Unreachable);
+            }
+        }
+
         // Now where do we jump next?
 
         // Usually we want to clean up (deallocate locals), but in a few rare cases we don't.
@@ -759,7 +765,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             self.deallocate_local(local.value)?;
         }
 
-        let return_place = frame.return_place;
         if M::after_stack_pop(self, frame, unwinding)? == StackPopJump::NoJump {
             // The hook already did everything.
             // We want to skip the `info!` below, hence early return.
@@ -772,25 +777,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             self.unwind_to_block(unwind);
         } else {
             // Follow the normal return edge.
-            // Validate the return value. Do this after deallocating so that we catch dangling
-            // references.
-            if let Some(return_place) = return_place {
-                if M::enforce_validity(self) {
-                    // Data got changed, better make sure it matches the type!
-                    // It is still possible that the return place held invalid data while
-                    // the function is running, but that's okay because nobody could have
-                    // accessed that same data from the "outside" to observe any broken
-                    // invariant -- that is, unless a function somehow has a ptr to
-                    // its return place... but the way MIR is currently generated, the
-                    // return place is always a local and then this cannot happen.
-                    self.validate_operand(self.place_to_op(return_place)?)?;
-                }
-            } else {
-                // Uh, that shouldn't happen... the function did not intend to return
-                throw_ub!(Unreachable);
-            }
-
-            // Jump to new block -- *after* validation so that the spans make more sense.
             if let Some(ret) = next_block {
                 self.return_to_block(ret)?;
             }

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,16 +453,11 @@ 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)?
-            }
-        };
+        // 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 };
+
+        let base_op = self.access_local(self.frame(), place.local, layout)?;
 
         let op = place
             .projection

src/librustc_mir/interpret/place.rs

@@ -135,12 +135,6 @@ impl<Tag> MemPlace<Tag> {
         MemPlace { ptr, align, meta: MemPlaceMeta::None }
     }
 
-    /// Produces a Place that will error if attempted to be read from or written to
-    #[inline(always)]
-    fn null(cx: &impl HasDataLayout) -> Self {
-        Self::from_scalar_ptr(Scalar::null_ptr(cx), Align::from_bytes(1).unwrap())
-    }
-
     #[inline(always)]
     pub fn from_ptr(ptr: Pointer<Tag>, align: Align) -> Self {
         Self::from_scalar_ptr(ptr.into(), align)
@@ -260,12 +254,6 @@ impl<'tcx, Tag: ::std::fmt::Debug + Copy> OpTy<'tcx, Tag> {
 }
 
 impl<Tag: ::std::fmt::Debug> Place<Tag> {
-    /// Produces a Place that will error if attempted to be read from or written to
-    #[inline(always)]
-    fn null(cx: &impl HasDataLayout) -> Self {
-        Place::Ptr(MemPlace::null(cx))
-    }
-
     #[inline]
     pub fn assert_mem_place(self) -> MemPlace<Tag> {
         match self {
@@ -641,35 +629,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() {

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)?
             }
 

src/librustc_mir/transform/const_prop.rs

@@ -330,7 +330,6 @@ struct ConstPropagator<'mir, 'tcx> {
     // by accessing them through `ecx` instead.
     source_scopes: IndexVec<SourceScope, SourceScopeData>,
     local_decls: IndexVec<Local, LocalDecl<'tcx>>,
-    ret: Option<OpTy<'tcx, ()>>,
     // Because we have `MutVisitor` we can't obtain the `SourceInfo` from a `Location`. So we store
     // the last known `SourceInfo` here and just keep revisiting it.
     source_info: Option<SourceInfo>,
@@ -402,22 +401,19 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
             source_scopes: body.source_scopes.clone(),
             //FIXME(wesleywiser) we can't steal this because `Visitor::super_visit_body()` needs it
             local_decls: body.local_decls.clone(),
-            ret: ret.map(Into::into),
             source_info: None,
         }
     }
 
     fn get_const(&self, local: Local) -> Option<OpTy<'tcx>> {
-        if local == RETURN_PLACE {
-            // Try to read the return place as an immediate so that if it is representable as a
-            // scalar, we can handle it as such, but otherwise, just return the value as is.
-            return match self.ret.map(|ret| self.ecx.try_read_immediate(ret)) {
-                Some(Ok(Ok(imm))) => Some(imm.into()),
-                _ => self.ret,
-            };
-        }
+        let op = self.ecx.access_local(self.ecx.frame(), local, None).ok();
 
-        self.ecx.access_local(self.ecx.frame(), local, None).ok()
+        // Try to read the local as an immediate so that if it is representable as a scalar, we can
+        // handle it as such, but otherwise, just return the value as is.
+        match op.map(|ret| self.ecx.try_read_immediate(ret)) {
+            Some(Ok(Ok(imm))) => Some(imm.into()),
+            _ => op,
+        }
     }
 
     fn remove_const(&mut self, local: Local) {

src/test/codegen/consts.rs

@@ -10,11 +10,11 @@
 // CHECK: @STATIC = {{.*}}, align 4
 
 // This checks the constants from inline_enum_const
-// CHECK: @alloc5 = {{.*}}, align 2
+// CHECK: @alloc7 = {{.*}}, align 2
 
 // This checks the constants from {low,high}_align_const, they share the same
 // constant, but the alignment differs, so the higher one should be used
-// CHECK: [[LOW_HIGH:@[0-9]+]] = {{.*}} getelementptr inbounds (<{ [8 x i8] }>, <{ [8 x i8] }>* @alloc15, i32 0, i32 0, i32 0), {{.*}}
+// CHECK: [[LOW_HIGH:@[0-9]+]] = {{.*}} getelementptr inbounds (<{ [8 x i8] }>, <{ [8 x i8] }>* @alloc19, i32 0, i32 0, i32 0), {{.*}}
 
 #[derive(Copy, Clone)]
 // repr(i16) is required for the {low,high}_align_const test

src/test/incremental/hashes/enum_constructors.rs

@@ -274,29 +274,29 @@ pub enum Clike2 {
 // Change constructor path (C-like) --------------------------------------
 #[cfg(cfail1)]
 pub fn change_constructor_path_c_like() {
-    let _ = Clike::B;
+    let _x = Clike::B;
 }
 
 #[cfg(not(cfail1))]
 #[rustc_clean(cfg="cfail2", except="hir_owner_nodes,optimized_mir,mir_built,typeck_tables_of")]
 #[rustc_clean(cfg="cfail3")]
 pub fn change_constructor_path_c_like() {
-    let _ = Clike2::B;
+    let _x = Clike2::B;
 }
 
 
 
 // Change constructor variant (C-like) --------------------------------------
 #[cfg(cfail1)]
 pub fn change_constructor_variant_c_like() {
-    let _ = Clike::A;
+    let _x = Clike::A;
 }
 
 #[cfg(not(cfail1))]
 #[rustc_clean(cfg="cfail2", except="hir_owner_nodes,optimized_mir,mir_built")]
 #[rustc_clean(cfg="cfail3")]
 pub fn change_constructor_variant_c_like() {
-    let _ = Clike::C;
+    let _x = Clike::C;
 }
 
 

src/test/mir-opt/const_allocation2/32bit/rustc.main.ConstProp.after.mir

@@ -30,41 +30,41 @@ fn main() -> () {
 }
 
 alloc0 (static: FOO, size: 8, align: 4) {
-    ╾alloc24+0╼ 03 00 00 00                         │ ╾──╼....
+    ╾alloc25+0╼ 03 00 00 00                         │ ╾──╼....
 }
 
-alloc24 (size: 48, align: 4) {
-    0x00 │ 00 00 00 00 __ __ __ __ ╾alloc9+0─╼ 00 00 00 00 │ ....░░░░╾──╼....
-    0x10 │ 00 00 00 00 __ __ __ __ ╾alloc14+0╼ 02 00 00 00 │ ....░░░░╾──╼....
-    0x20 │ 01 00 00 00 2a 00 00 00 ╾alloc22+0╼ 03 00 00 00 │ ....*...╾──╼....
+alloc25 (size: 48, align: 4) {
+    0x00 │ 00 00 00 00 __ __ __ __ ╾alloc10+0╼ 00 00 00 00 │ ....░░░░╾──╼....
+    0x10 │ 00 00 00 00 __ __ __ __ ╾alloc15+0╼ 02 00 00 00 │ ....░░░░╾──╼....
+    0x20 │ 01 00 00 00 2a 00 00 00 ╾alloc23+0╼ 03 00 00 00 │ ....*...╾──╼....
 }
 
-alloc9 (size: 0, align: 4) {}
+alloc10 (size: 0, align: 4) {}
 
-alloc14 (size: 8, align: 4) {
-    ╾alloc12+0╼ ╾alloc13+0╼                         │ ╾──╼╾──╼
+alloc15 (size: 8, align: 4) {
+    ╾alloc13+0╼ ╾alloc14+0╼                         │ ╾──╼╾──╼
 }
 
-alloc12 (size: 1, align: 1) {
+alloc13 (size: 1, align: 1) {
     05                                              │ .
 }
 
-alloc13 (size: 1, align: 1) {
+alloc14 (size: 1, align: 1) {
     06                                              │ .
 }
 
-alloc22 (size: 12, align: 4) {
-    ╾alloc18+3╼ ╾alloc19+0╼ ╾alloc21+2╼             │ ╾──╼╾──╼╾──╼
+alloc23 (size: 12, align: 4) {
+    ╾alloc19+3╼ ╾alloc20+0╼ ╾alloc22+2╼             │ ╾──╼╾──╼╾──╼
 }
 
-alloc18 (size: 4, align: 1) {
+alloc19 (size: 4, align: 1) {
     2a 45 15 6f                                     │ *E.o
 }
 
-alloc19 (size: 1, align: 1) {
+alloc20 (size: 1, align: 1) {
     2a                                              │ *
 }
 
-alloc21 (size: 4, align: 1) {
+alloc22 (size: 4, align: 1) {
     2a 45 15 6f                                     │ *E.o
 }