Rollup merge of rust-lang#100239 - RalfJung:const-prop-uninit, r=oli-obk

remove an ineffective check in const_prop

Based on rust-lang#100043, only the last two commits are new.

ConstProp has a special check when reading from a local that prevents reading uninit locals. However, if that local flows into `force_allocation`, then no check fires and evaluation proceeds. So this check is not really effective at preventing accesses to uninit locals.

With rust-lang#100043, `read_immediate` and friends always fail when reading uninit locals, so I don't see why ConstProp would need a separate check. Thus I propose we remove it. This is needed to be able to do rust-lang#100085.

Author: Dylan-DPC

compiler/rustc_const_eval/src/interpret/eval_context.rs

@@ -187,9 +187,6 @@ pub enum LocalValue<Prov: Provenance = AllocId> {
 
 impl<'tcx, Prov: Provenance + 'static> LocalState<'tcx, Prov> {
     /// Read the local's value or error if the local is not yet live or not live anymore.
-    ///
-    /// Note: This may only be invoked from the `Machine::access_local` hook and not from
-    /// anywhere else. You may be invalidating machine invariants if you do!
     #[inline]
     pub fn access(&self) -> InterpResult<'tcx, &Operand<Prov>> {
         match &self.value {

compiler/rustc_const_eval/src/interpret/machine.rs

@@ -215,23 +215,12 @@ pub trait Machine<'mir, 'tcx>: Sized {
         right: &ImmTy<'tcx, Self::Provenance>,
     ) -> InterpResult<'tcx, (Scalar<Self::Provenance>, bool, Ty<'tcx>)>;
 
-    /// Called to read the specified `local` from the `frame`.
-    /// Since reading a ZST is not actually accessing memory or locals, this is never invoked
-    /// for ZST reads.
-    #[inline]
-    fn access_local<'a>(
-        frame: &'a Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>,
-        local: mir::Local,
-    ) -> InterpResult<'tcx, &'a Operand<Self::Provenance>>
-    where
-        'tcx: 'mir,
-    {
-        frame.locals[local].access()
-    }
-
     /// Called to write the specified `local` from the `frame`.
     /// Since writing a ZST is not actually accessing memory or locals, this is never invoked
     /// for ZST reads.
+    ///
+    /// Due to borrow checker trouble, we indicate the `frame` as an index rather than an `&mut
+    /// Frame`.
     #[inline]
     fn access_local_mut<'a>(
         ecx: &'a mut InterpCx<'mir, 'tcx, Self>,

compiler/rustc_const_eval/src/interpret/operand.rs

@@ -444,7 +444,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         }
     }
 
-    /// Read from a local. Will not actually access the local if reading from a ZST.
+    /// Read from a local.
     /// Will not access memory, instead an indirect `Operand` is returned.
     ///
     /// This is public because it is used by [priroda](https://github.com/oli-obk/priroda) to get an
@@ -456,12 +456,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         layout: Option<TyAndLayout<'tcx>>,
     ) -> InterpResult<'tcx, OpTy<'tcx, M::Provenance>> {
         let layout = self.layout_of_local(frame, local, layout)?;
-        let op = if layout.is_zst() {
-            // Bypass `access_local` (helps in ConstProp)
-            Operand::Immediate(Immediate::Uninit)
-        } else {
-            *M::access_local(frame, local)?
-        };
+        let op = *frame.locals[local].access()?;
         Ok(OpTy { op, layout, align: Some(layout.align.abi) })
     }
 

compiler/rustc_const_eval/src/interpret/place.rs

@@ -642,7 +642,7 @@ where
         // avoid force_allocation.
         let src = match self.read_immediate_raw(src)? {
             Ok(src_val) => {
-                assert!(!src.layout.is_unsized(), "cannot have unsized immediates");
+                assert!(!src.layout.is_unsized(), "cannot copy unsized immediates");
                 assert!(
                     !dest.layout.is_unsized(),
                     "the src is sized, so the dest must also be sized"

compiler/rustc_const_eval/src/interpret/projection.rs

@@ -100,6 +100,8 @@ where
         // This makes several assumptions about what layouts we will encounter; we match what
         // codegen does as good as we can (see `extract_field` in `rustc_codegen_ssa/src/mir/operand.rs`).
         let field_val: Immediate<_> = match (*base, base.layout.abi) {
+            // if the entire value is uninit, then so is the field (can happen in ConstProp)
+            (Immediate::Uninit, _) => Immediate::Uninit,
             // the field contains no information, can be left uninit
             _ if field_layout.is_zst() => Immediate::Uninit,
             // the field covers the entire type
@@ -124,6 +126,7 @@ where
                     b_val
                 })
             }
+            // everything else is a bug
             _ => span_bug!(
                 self.cur_span(),
                 "invalid field access on immediate {}, layout {:#?}",

compiler/rustc_mir_transform/src/const_prop.rs

@@ -243,24 +243,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>
         throw_machine_stop_str!("pointer arithmetic or comparisons aren't supported in ConstProp")
     }
 
-    fn access_local<'a>(
-        frame: &'a Frame<'mir, 'tcx, Self::Provenance, Self::FrameExtra>,
-        local: Local,
-    ) -> InterpResult<'tcx, &'a interpret::Operand<Self::Provenance>> {
-        let l = &frame.locals[local];
-
-        if matches!(
-            l.value,
-            LocalValue::Live(interpret::Operand::Immediate(interpret::Immediate::Uninit))
-        ) {
-            // For us "uninit" means "we don't know its value, might be initiailized or not".
-            // So stop here.
-            throw_machine_stop_str!("tried to access alocal with unknown value ")
-        }
-
-        l.access()
-    }
-
     fn access_local_mut<'a>(
         ecx: &'a mut InterpCx<'mir, 'tcx, Self>,
         frame: usize,
@@ -431,7 +413,13 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
 
     fn get_const(&self, place: Place<'tcx>) -> Option<OpTy<'tcx>> {
         let op = match self.ecx.eval_place_to_op(place, None) {
-            Ok(op) => op,
+            Ok(op) => {
+                if matches!(*op, interpret::Operand::Immediate(Immediate::Uninit)) {
+                    // Make sure nobody accidentally uses this value.
+                    return None;
+                }
+                op
+            }
             Err(e) => {
                 trace!("get_const failed: {}", e);
                 return None;
@@ -643,6 +631,14 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
         if rvalue.needs_subst() {
             return None;
         }
+        if !rvalue
+            .ty(&self.ecx.frame().body.local_decls, *self.ecx.tcx)
+            .is_sized(self.ecx.tcx, self.param_env)
+        {
+            // the interpreter doesn't support unsized locals (only unsized arguments),
+            // but rustc does (in a kinda broken way), so we have to skip them here
+            return None;
+        }
 
         if self.tcx.sess.mir_opt_level() >= 4 {
             self.eval_rvalue_with_identities(rvalue, place)
@@ -660,18 +656,20 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
         self.use_ecx(|this| match rvalue {
             Rvalue::BinaryOp(op, box (left, right))
             | Rvalue::CheckedBinaryOp(op, box (left, right)) => {
-                let l = this.ecx.eval_operand(left, None);
-                let r = this.ecx.eval_operand(right, None);
+                let l = this.ecx.eval_operand(left, None).and_then(|x| this.ecx.read_immediate(&x));
+                let r =
+                    this.ecx.eval_operand(right, None).and_then(|x| this.ecx.read_immediate(&x));
 
                 let const_arg = match (l, r) {
-                    (Ok(ref x), Err(_)) | (Err(_), Ok(ref x)) => this.ecx.read_immediate(x)?,
-                    (Err(e), Err(_)) => return Err(e),
-                    (Ok(_), Ok(_)) => return this.ecx.eval_rvalue_into_place(rvalue, place),
+                    (Ok(x), Err(_)) | (Err(_), Ok(x)) => x, // exactly one side is known
+                    (Err(e), Err(_)) => return Err(e),      // neither side is known
+                    (Ok(_), Ok(_)) => return this.ecx.eval_rvalue_into_place(rvalue, place), // both sides are known
                 };
 
                 if !matches!(const_arg.layout.abi, abi::Abi::Scalar(..)) {
                     // We cannot handle Scalar Pair stuff.
-                    return this.ecx.eval_rvalue_into_place(rvalue, place);
+                    // No point in calling `eval_rvalue_into_place`, since only one side is known
+                    throw_machine_stop_str!("cannot optimize this")
                 }
 
                 let arg_value = const_arg.to_scalar().to_bits(const_arg.layout.size)?;
@@ -696,7 +694,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
                             this.ecx.write_immediate(*const_arg, &dest)
                         }
                     }
-                    _ => this.ecx.eval_rvalue_into_place(rvalue, place),
+                    _ => throw_machine_stop_str!("cannot optimize this"),
                 }
             }
             _ => this.ecx.eval_rvalue_into_place(rvalue, place),
@@ -1073,7 +1071,11 @@ impl<'tcx> MutVisitor<'tcx> for ConstPropagator<'_, 'tcx> {
                 if let Some(ref value) = self.eval_operand(&cond) {
                     trace!("assertion on {:?} should be {:?}", value, expected);
                     let expected = Scalar::from_bool(*expected);
-                    let value_const = self.ecx.read_scalar(&value).unwrap();
+                    let Ok(value_const) = self.ecx.read_scalar(&value) else {
+                        // FIXME should be used use_ecx rather than a local match... but we have
+                        // quite a few of these read_scalar/read_immediate that need fixing.
+                        return
+                    };
                     if expected != value_const {
                         // Poison all places this operand references so that further code
                         // doesn't use the invalid value

compiler/rustc_mir_transform/src/const_prop_lint.rs

@@ -6,6 +6,7 @@ use crate::const_prop::ConstPropMachine;
 use crate::const_prop::ConstPropMode;
 use crate::MirLint;
 use rustc_const_eval::const_eval::ConstEvalErr;
+use rustc_const_eval::interpret::Immediate;
 use rustc_const_eval::interpret::{
     self, InterpCx, InterpResult, LocalState, LocalValue, MemoryKind, OpTy, Scalar, StackPopCleanup,
 };
@@ -229,7 +230,13 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
 
     fn get_const(&self, place: Place<'tcx>) -> Option<OpTy<'tcx>> {
         let op = match self.ecx.eval_place_to_op(place, None) {
-            Ok(op) => op,
+            Ok(op) => {
+                if matches!(*op, interpret::Operand::Immediate(Immediate::Uninit)) {
+                    // Make sure nobody accidentally uses this value.
+                    return None;
+                }
+                op
+            }
             Err(e) => {
                 trace!("get_const failed: {}", e);
                 return None;
@@ -515,6 +522,14 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
         if rvalue.needs_subst() {
             return None;
         }
+        if !rvalue
+            .ty(&self.ecx.frame().body.local_decls, *self.ecx.tcx)
+            .is_sized(self.ecx.tcx, self.param_env)
+        {
+            // the interpreter doesn't support unsized locals (only unsized arguments),
+            // but rustc does (in a kinda broken way), so we have to skip them here
+            return None;
+        }
 
         self.use_ecx(source_info, |this| this.ecx.eval_rvalue_into_place(rvalue, place))
     }
@@ -624,7 +639,11 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> {
                 if let Some(ref value) = self.eval_operand(&cond, source_info) {
                     trace!("assertion on {:?} should be {:?}", value, expected);
                     let expected = Scalar::from_bool(*expected);
-                    let value_const = self.ecx.read_scalar(&value).unwrap();
+                    let Ok(value_const) = self.ecx.read_scalar(&value) else {
+                        // FIXME should be used use_ecx rather than a local match... but we have
+                        // quite a few of these read_scalar/read_immediate that need fixing.
+                        return
+                    };
                     if expected != value_const {
                         enum DbgVal<T> {
                             Val(T),
@@ -641,9 +660,9 @@ impl<'tcx> Visitor<'tcx> for ConstPropagator<'_, 'tcx> {
                         let mut eval_to_int = |op| {
                             // This can be `None` if the lhs wasn't const propagated and we just
                             // triggered the assert on the value of the rhs.
-                            self.eval_operand(op, source_info).map_or(DbgVal::Underscore, |op| {
-                                DbgVal::Val(self.ecx.read_immediate(&op).unwrap().to_const_int())
-                            })
+                            self.eval_operand(op, source_info)
+                                .and_then(|op| self.ecx.read_immediate(&op).ok())
+                                .map_or(DbgVal::Underscore, |op| DbgVal::Val(op.to_const_int()))
                         };
                         let msg = match msg {
                             AssertKind::DivisionByZero(op) => {

src/test/mir-opt/const_prop/mutable_variable_unprop_assign.main.ConstProp.diff

@@ -41,7 +41,8 @@
           StorageLive(_4);                 // scope 2 at $DIR/mutable_variable_unprop_assign.rs:+4:9: +4:10
           _4 = (_2.1: i32);                // scope 2 at $DIR/mutable_variable_unprop_assign.rs:+4:13: +4:16
           StorageLive(_5);                 // scope 3 at $DIR/mutable_variable_unprop_assign.rs:+5:9: +5:10
-          _5 = (_2.0: i32);                // scope 3 at $DIR/mutable_variable_unprop_assign.rs:+5:13: +5:16
+-         _5 = (_2.0: i32);                // scope 3 at $DIR/mutable_variable_unprop_assign.rs:+5:13: +5:16
++         _5 = const 1_i32;                // scope 3 at $DIR/mutable_variable_unprop_assign.rs:+5:13: +5:16
           nop;                             // scope 0 at $DIR/mutable_variable_unprop_assign.rs:+0:11: +6:2
           StorageDead(_5);                 // scope 3 at $DIR/mutable_variable_unprop_assign.rs:+6:1: +6:2
           StorageDead(_4);                 // scope 2 at $DIR/mutable_variable_unprop_assign.rs:+6:1: +6:2