move UnsafeCell-in-const check from interning to validation

RalfJung · RalfJung · commit 849c92952dc9 · 2020-10-24T20:49:17.000+02:00
diff --git a/compiler/rustc_mir/src/const_eval/eval_queries.rs b/compiler/rustc_mir/src/const_eval/eval_queries.rs
@@ -1,8 +1,8 @@
 use super::{CompileTimeEvalContext, CompileTimeInterpreter, ConstEvalErr, MemoryExtra};
 use crate::interpret::eval_nullary_intrinsic;
 use crate::interpret::{
-    intern_const_alloc_recursive, Allocation, ConstAlloc, ConstValue, GlobalId, Immediate,
-    InternKind, InterpCx, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking, Scalar,
+    intern_const_alloc_recursive, Allocation, ConstAlloc, ConstValue, CtfeValidationMode, GlobalId,
+    Immediate, InternKind, InterpCx, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking, Scalar,
     ScalarMaybeUninit, StackPopCleanup,
 };
 
@@ -376,13 +376,14 @@ pub fn eval_to_allocation_raw_provider<'tcx>(
                 // https://github.com/rust-lang/rust/issues/67465 is made
                 if cid.promoted.is_none() {
                     let mut ref_tracking = RefTracking::new(mplace);
+                    let mut inner = false;
                     while let Some((mplace, path)) = ref_tracking.todo.pop() {
-                        ecx.const_validate_operand(
-                            mplace.into(),
-                            path,
-                            &mut ref_tracking,
-                            /*may_ref_to_static*/ ecx.memory.extra.can_access_statics,
-                        )?;
+                        let mode = match tcx.static_mutability(cid.instance.def_id()) {
+                            Some(_) => CtfeValidationMode::Regular, // a `static`
+                            None => CtfeValidationMode::Const { inner },
+                        };
+                        ecx.const_validate_operand(mplace.into(), path, &mut ref_tracking, mode)?;
+                        inner = true;
                     }
                 }
             };
diff --git a/compiler/rustc_mir/src/interpret/intern.rs b/compiler/rustc_mir/src/interpret/intern.rs
@@ -129,9 +129,7 @@ fn intern_shallow<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>>(
         // See const_eval::machine::MemoryExtra::can_access_statics for why
         // immutability is so important.
 
-        // There are no sensible checks we can do here; grep for `mutable_memory_in_const` to
-        // find the checks we are doing elsewhere to avoid even getting here for memory
-        // that "wants" to be mutable.
+        // Validation will ensure that there is no `UnsafeCell` on an immutable allocation.
         alloc.mutability = Mutability::Not;
     };
     // link the alloc id to the actual allocation
@@ -176,7 +174,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir
                     // they caused.  It also helps us to find cases where const-checking
                     // failed to prevent an `UnsafeCell` (but as `ignore_interior_mut_in_const`
                     // shows that part is not airtight).
-                    mutable_memory_in_const(self.ecx.tcx, "`UnsafeCell`");
+                    //mutable_memory_in_const(self.ecx.tcx, "`UnsafeCell`");
                 }
                 // We are crossing over an `UnsafeCell`, we can mutate again. This means that
                 // References we encounter inside here are interned as pointing to mutable
diff --git a/compiler/rustc_mir/src/interpret/mod.rs b/compiler/rustc_mir/src/interpret/mod.rs
@@ -24,7 +24,7 @@ pub use self::machine::{compile_time_machine, AllocMap, Machine, MayLeak, StackP
 pub use self::memory::{AllocCheck, FnVal, Memory, MemoryKind};
 pub use self::operand::{ImmTy, Immediate, OpTy, Operand};
 pub use self::place::{MPlaceTy, MemPlace, MemPlaceMeta, Place, PlaceTy};
-pub use self::validity::RefTracking;
+pub use self::validity::{RefTracking, CtfeValidationMode};
 pub use self::visitor::{MutValueVisitor, ValueVisitor};
 
 crate use self::intrinsics::eval_nullary_intrinsic;
diff --git a/compiler/rustc_mir/src/interpret/validity.rs b/compiler/rustc_mir/src/interpret/validity.rs
@@ -113,6 +113,15 @@ pub enum PathElem {
     DynDowncast,
 }
 
+/// Extra things to check for during validation of CTFE results.
+pub enum CtfeValidationMode {
+    /// Regular validation, nothing special happening.
+    Regular,
+    /// Validation of a `const`. `inner` says if this is an inner, indirect allocation (as opposed
+    /// to the top-level const allocation).
+    Const { inner: bool },
+}
+
 /// State for tracking recursive validation of references
 pub struct RefTracking<T, PATH = ()> {
     pub seen: FxHashSet<T>,
@@ -202,9 +211,9 @@ struct ValidityVisitor<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> {
     /// starts must not be changed!  `visit_fields` and `visit_array` rely on
     /// this stack discipline.
     path: Vec<PathElem>,
-    ref_tracking_for_consts:
-        Option<&'rt mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>>,
-    may_ref_to_static: bool,
+    ref_tracking: Option<&'rt mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>>,
+    /// `None` indicates this is not validating for CTFE (but for runtime).
+    ctfe_mode: Option<CtfeValidationMode>,
     ecx: &'rt InterpCx<'mir, 'tcx, M>,
 }
 
@@ -418,27 +427,15 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                 { "a dangling {} (use-after-free)", kind },
         );
         // Recursive checking
-        if let Some(ref mut ref_tracking) = self.ref_tracking_for_consts {
+        if let Some(ref mut ref_tracking) = self.ref_tracking {
             if let Some(ptr) = ptr {
                 // not a ZST
                 // Skip validation entirely for some external statics
                 let alloc_kind = self.ecx.tcx.get_global_alloc(ptr.alloc_id);
                 if let Some(GlobalAlloc::Static(did)) = alloc_kind {
                     assert!(!self.ecx.tcx.is_thread_local_static(did));
                     assert!(self.ecx.tcx.is_static(did));
-                    if self.may_ref_to_static {
-                        // We skip checking other statics. These statics must be sound by
-                        // themselves, and the only way to get broken statics here is by using
-                        // unsafe code.
-                        // The reasons we don't check other statics is twofold. For one, in all
-                        // sound cases, the static was already validated on its own, and second, we
-                        // trigger cycle errors if we try to compute the value of the other static
-                        // and that static refers back to us.
-                        // We might miss const-invalid data,
-                        // but things are still sound otherwise (in particular re: consts
-                        // referring to statics).
-                        return Ok(());
-                    } else {
+                    if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { .. })) {
                         // See const_eval::machine::MemoryExtra::can_access_statics for why
                         // this check is so important.
                         // This check is reachable when the const just referenced the static,
@@ -447,6 +444,17 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                             { "a {} pointing to a static variable", kind }
                         );
                     }
+                    // We skip checking other statics. These statics must be sound by
+                    // themselves, and the only way to get broken statics here is by using
+                    // unsafe code.
+                    // The reasons we don't check other statics is twofold. For one, in all
+                    // sound cases, the static was already validated on its own, and second, we
+                    // trigger cycle errors if we try to compute the value of the other static
+                    // and that static refers back to us.
+                    // We might miss const-invalid data,
+                    // but things are still sound otherwise (in particular re: consts
+                    // referring to statics).
+                    return Ok(());
                 }
             }
             // Proceed recursively even for ZST, no reason to skip them!
@@ -504,7 +512,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                 let value = self.ecx.read_scalar(value)?;
                 // NOTE: Keep this in sync with the array optimization for int/float
                 // types below!
-                if self.ref_tracking_for_consts.is_some() {
+                if self.ctfe_mode.is_some() {
                     // Integers/floats in CTFE: Must be scalar bits, pointers are dangerous
                     let is_bits = value.check_init().map_or(false, |v| v.is_bits());
                     if !is_bits {
@@ -723,6 +731,15 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
         // Sanity check: `builtin_deref` does not know any pointers that are not primitive.
         assert!(op.layout.ty.builtin_deref(true).is_none());
 
+        // Special check preventing `UnsafeCell` in constants
+        if let Some(def) = op.layout.ty.ty_adt_def() {
+            if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { inner: true }))
+                && Some(def.did) == self.ecx.tcx.lang_items().unsafe_cell_type()
+            {
+                throw_validation_failure!(self.path, { "`UnsafeCell` in a `const`" });
+            }
+        }
+
         // Recursively walk the value at its type.
         self.walk_value(op)?;
 
@@ -814,7 +831,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
                     self.ecx,
                     ptr,
                     size,
-                    /*allow_uninit_and_ptr*/ self.ref_tracking_for_consts.is_none(),
+                    /*allow_uninit_and_ptr*/ self.ctfe_mode.is_none(),
                 ) {
                     // In the happy case, we needn't check anything else.
                     Ok(()) => {}
@@ -865,16 +882,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         &self,
         op: OpTy<'tcx, M::PointerTag>,
         path: Vec<PathElem>,
-        ref_tracking_for_consts: Option<
-            &mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>,
-        >,
-        may_ref_to_static: bool,
+        ref_tracking: Option<&mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>>,
+        ctfe_mode: Option<CtfeValidationMode>,
     ) -> InterpResult<'tcx> {
         trace!("validate_operand_internal: {:?}, {:?}", *op, op.layout.ty);
 
         // Construct a visitor
-        let mut visitor =
-            ValidityVisitor { path, ref_tracking_for_consts, may_ref_to_static, ecx: self };
+        let mut visitor = ValidityVisitor { path, ref_tracking, ctfe_mode, ecx: self };
 
         // Try to cast to ptr *once* instead of all the time.
         let op = self.force_op_ptr(op).unwrap_or(op);
@@ -902,23 +916,25 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
     /// `ref_tracking` is used to record references that we encounter so that they
     /// can be checked recursively by an outside driving loop.
     ///
-    /// `may_ref_to_static` controls whether references are allowed to point to statics.
+    /// `constant` controls whether this must satisfy the rules for constants:
+    /// - no pointers to statics.
+    /// - no `UnsafeCell` or non-ZST `&mut`.
     #[inline(always)]
     pub fn const_validate_operand(
         &self,
         op: OpTy<'tcx, M::PointerTag>,
         path: Vec<PathElem>,
         ref_tracking: &mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>,
-        may_ref_to_static: bool,
+        ctfe_mode: CtfeValidationMode,
     ) -> InterpResult<'tcx> {
-        self.validate_operand_internal(op, path, Some(ref_tracking), may_ref_to_static)
+        self.validate_operand_internal(op, path, Some(ref_tracking), Some(ctfe_mode))
     }
 
     /// This function checks the data at `op` to be runtime-valid.
     /// `op` is assumed to cover valid memory if it is an indirect operand.
     /// It will error if the bits at the destination do not match the ones described by the layout.
     #[inline(always)]
     pub fn validate_operand(&self, op: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx> {
-        self.validate_operand_internal(op, vec![], None, false)
+        self.validate_operand_internal(op, vec![], None, None)
     }
 }
diff --git a/compiler/rustc_mir/src/transform/const_prop.rs b/compiler/rustc_mir/src/transform/const_prop.rs
@@ -9,7 +9,6 @@ use rustc_hir::def::DefKind;
 use rustc_hir::HirId;
 use rustc_index::bit_set::BitSet;
 use rustc_index::vec::IndexVec;
-use rustc_middle::mir::interpret::{InterpResult, Scalar};
 use rustc_middle::mir::visit::{
     MutVisitor, MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor,
 };
@@ -28,9 +27,10 @@ use rustc_trait_selection::traits;
 
 use crate::const_eval::ConstEvalErr;
 use crate::interpret::{
-    self, compile_time_machine, truncate, AllocId, Allocation, ConstValue, Frame, ImmTy, Immediate,
-    InterpCx, LocalState, LocalValue, MemPlace, Memory, MemoryKind, OpTy, Operand as InterpOperand,
-    PlaceTy, Pointer, ScalarMaybeUninit, StackPopCleanup,
+    self, compile_time_machine, truncate, AllocId, Allocation, ConstValue, CtfeValidationMode,
+    Frame, ImmTy, Immediate, InterpCx, InterpResult, LocalState, LocalValue, MemPlace, Memory,
+    MemoryKind, OpTy, Operand as InterpOperand, PlaceTy, Pointer, Scalar, ScalarMaybeUninit,
+    StackPopCleanup,
 };
 use crate::transform::MirPass;
 
@@ -805,8 +805,9 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
             value,
             vec![],
             // FIXME: is ref tracking too expensive?
+            // FIXME: what is the point of ref tracking if we do not even check the tracked refs?
             &mut interpret::RefTracking::empty(),
-            /*may_ref_to_static*/ true,
+            CtfeValidationMode::Regular,
         ) {
             trace!("validation error, attempt failed: {:?}", e);
             return;
diff --git a/src/test/ui/consts/miri_unleashed/mutable_references_err.rs b/src/test/ui/consts/miri_unleashed/mutable_references_err.rs
@@ -13,7 +13,7 @@ unsafe impl Sync for Meh {}
 
 // the following will never be ok! no interior mut behind consts, because
 // all allocs interned here will be marked immutable.
-const MUH: Meh = Meh { //~ ERROR: mutable memory (`UnsafeCell`) is not allowed in constant
+const MUH: Meh = Meh { //~ ERROR: it is undefined behavior to use this value
     x: &UnsafeCell::new(42),
 };
 
@@ -24,7 +24,7 @@ unsafe impl Sync for Synced {}
 
 // Make sure we also catch this behind a type-erased `dyn Trait` reference.
 const SNEAKY: &dyn Sync = &Synced { x: UnsafeCell::new(42) };
-//~^ ERROR: mutable memory (`UnsafeCell`) is not allowed in constant
+//~^ ERROR: it is undefined behavior to use this value
 
 // Make sure we also catch mutable references.
 const BLUNT: &mut i32 = &mut 42;
diff --git a/src/test/ui/consts/miri_unleashed/mutable_references_err.stderr b/src/test/ui/consts/miri_unleashed/mutable_references_err.stderr
@@ -1,16 +1,20 @@
-error: mutable memory (`UnsafeCell`) is not allowed in constant
+error[E0080]: it is undefined behavior to use this value
   --> $DIR/mutable_references_err.rs:16:1
    |
 LL | / const MUH: Meh = Meh {
 LL | |     x: &UnsafeCell::new(42),
 LL | | };
-   | |__^
+   | |__^ type validation failed: encountered `UnsafeCell` in a `const` at .x.<deref>
+   |
+   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
 
-error: mutable memory (`UnsafeCell`) is not allowed in constant
+error[E0080]: it is undefined behavior to use this value
   --> $DIR/mutable_references_err.rs:26:1
    |
 LL | const SNEAKY: &dyn Sync = &Synced { x: UnsafeCell::new(42) };
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered `UnsafeCell` in a `const` at .<deref>.<dyn-downcast>.x
+   |
+   = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
 
 error: mutable memory (`&mut`) is not allowed in constant
   --> $DIR/mutable_references_err.rs:30:1
@@ -38,3 +42,4 @@ LL | const BLUNT: &mut i32 = &mut 42;
 
 error: aborting due to 3 previous errors; 1 warning emitted
 
+For more information about this error, try `rustc --explain E0080`.
