Allow constants to refer to statics

rpjohnst · rpjohnst · commit 8f868332cff8 · 2020-04-19T14:04:23.000-07:00
This relaxes the dynamic and validity checks so that constants can
construct pointers to all statics and read from immutable statics.
diff --git a/src/librustc_mir/const_eval/eval_queries.rs b/src/librustc_mir/const_eval/eval_queries.rs
@@ -1,9 +1,9 @@
 use super::{error_to_const_error, CompileTimeEvalContext, CompileTimeInterpreter, MemoryExtra};
 use crate::interpret::eval_nullary_intrinsic;
 use crate::interpret::{
-    intern_const_alloc_recursive, Allocation, ConstValue, GlobalId, Immediate, InternKind,
-    InterpCx, InterpResult, MPlaceTy, MemoryKind, OpTy, RawConst, RefTracking, Scalar,
-    ScalarMaybeUndef, StackPopCleanup,
+    intern_const_alloc_recursive, AllocId, Allocation, ConstValue, GlobalAlloc, GlobalId,
+    Immediate, InternKind, InterpCx, InterpResult, MPlaceTy, MemoryKind, OpTy, RawConst,
+    RefTracking, Scalar, ScalarMaybeUndef, StackPopCleanup,
 };
 use rustc_hir::def::DefKind;
 use rustc_middle::mir;
@@ -94,10 +94,13 @@ pub(super) fn mk_eval_cx<'mir, 'tcx>(
     )
 }
 
-pub(super) fn op_to_const<'tcx>(
+#[derive(Debug)]
+pub(super) struct RefersToStatic;
+
+pub(super) fn try_op_to_const<'tcx>(
     ecx: &CompileTimeEvalContext<'_, 'tcx>,
     op: OpTy<'tcx>,
-) -> ConstValue<'tcx> {
+) -> Result<ConstValue<'tcx>, RefersToStatic> {
     // We do not have value optimizations for everything.
     // Only scalars and slices, since they are very common.
     // Note that further down we turn scalars of undefined bits back to `ByRef`. These can result
@@ -128,10 +131,16 @@ pub(super) fn op_to_const<'tcx>(
         op.try_as_mplace(ecx)
     };
 
+    let alloc_map = ecx.tcx.alloc_map.lock();
+    let try_unwrap_memory = |alloc_id: AllocId| match alloc_map.get(alloc_id) {
+        Some(GlobalAlloc::Memory(mem)) => Ok(mem),
+        Some(GlobalAlloc::Static(_)) => Err(RefersToStatic),
+        _ => bug!("expected allocation ID {} to point to memory", alloc_id),
+    };
     let to_const_value = |mplace: MPlaceTy<'_>| match mplace.ptr {
         Scalar::Ptr(ptr) => {
-            let alloc = ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id);
-            ConstValue::ByRef { alloc, offset: ptr.offset }
+            let alloc = try_unwrap_memory(ptr.alloc_id)?;
+            Ok(ConstValue::ByRef { alloc, offset: ptr.offset })
         }
         Scalar::Raw { data, .. } => {
             assert!(mplace.layout.is_zst());
@@ -141,22 +150,20 @@ pub(super) fn op_to_const<'tcx>(
                 "this MPlaceTy must come from `try_as_mplace` being used on a zst, so we know what
                  value this integer address must have",
             );
-            ConstValue::Scalar(Scalar::zst())
+            Ok(ConstValue::Scalar(Scalar::zst()))
         }
     };
     match immediate {
-        Ok(mplace) => to_const_value(mplace),
+        Ok(mplace) => Ok(to_const_value(mplace)?),
         // see comment on `let try_as_immediate` above
         Err(imm) => match *imm {
             Immediate::Scalar(x) => match x {
-                ScalarMaybeUndef::Scalar(s) => ConstValue::Scalar(s),
-                ScalarMaybeUndef::Undef => to_const_value(op.assert_mem_place(ecx)),
+                ScalarMaybeUndef::Scalar(s) => Ok(ConstValue::Scalar(s)),
+                ScalarMaybeUndef::Undef => Ok(to_const_value(op.assert_mem_place(ecx))?),
             },
             Immediate::ScalarPair(a, b) => {
                 let (data, start) = match a.not_undef().unwrap() {
-                    Scalar::Ptr(ptr) => {
-                        (ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id), ptr.offset.bytes())
-                    }
+                    Scalar::Ptr(ptr) => (try_unwrap_memory(ptr.alloc_id)?, ptr.offset.bytes()),
                     Scalar::Raw { .. } => (
                         ecx.tcx
                             .intern_const_alloc(Allocation::from_byte_aligned_bytes(b"" as &[u8])),
@@ -166,7 +173,7 @@ pub(super) fn op_to_const<'tcx>(
                 let len = b.to_machine_usize(&ecx.tcx.tcx).unwrap();
                 let start = start.try_into().unwrap();
                 let len: usize = len.try_into().unwrap();
-                ConstValue::Slice { data, start, end: start + len }
+                Ok(ConstValue::Slice { data, start, end: start + len })
             }
         },
     }
@@ -198,17 +205,20 @@ fn validate_and_turn_into_const<'tcx>(
             }
         }
         // Now that we validated, turn this into a proper constant.
-        // Statics/promoteds are always `ByRef`, for the rest `op_to_const` decides
-        // whether they become immediates.
-        if is_static || cid.promoted.is_some() {
+        // Statics/promoteds are always `ByRef`, for the rest `try_op_to_const`
+        // decides whether they become immediates.
+        let value = if !is_static && !cid.promoted.is_some() {
+            try_op_to_const(&ecx, mplace.into())
+        } else {
+            Err(RefersToStatic)
+        };
+        Ok(value.unwrap_or_else(|RefersToStatic| {
             let ptr = mplace.ptr.assert_ptr();
-            Ok(ConstValue::ByRef {
+            ConstValue::ByRef {
                 alloc: ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id),
                 offset: ptr.offset,
-            })
-        } else {
-            Ok(op_to_const(&ecx, mplace.into()))
-        }
+            }
+        }))
     })();
 
     val.map_err(|error| {
diff --git a/src/librustc_mir/const_eval/machine.rs b/src/librustc_mir/const_eval/machine.rs
@@ -8,7 +8,6 @@ use std::hash::Hash;
 use rustc_data_structures::fx::FxHashMap;
 
 use rustc_ast::ast::Mutability;
-use rustc_hir::def_id::DefId;
 use rustc_middle::mir::AssertMessage;
 use rustc_span::symbol::Symbol;
 
@@ -353,7 +352,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter {
         memory_extra: &MemoryExtra,
         alloc_id: AllocId,
         allocation: &Allocation,
-        static_def_id: Option<DefId>,
         is_write: bool,
     ) -> InterpResult<'tcx> {
         if is_write {
@@ -368,16 +366,14 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter {
             if memory_extra.can_access_statics {
                 // Machine configuration allows us read from anything (e.g., `static` initializer).
                 Ok(())
-            } else if static_def_id.is_some() {
-                // Machine configuration does not allow us to read statics
+            } else if allocation.mutability != Mutability::Not {
+                // Machine configuration does not allow us to read mutable statics
                 // (e.g., `const` initializer).
+                // This is unsound because the content of this allocation may be different now and
+                // at run-time, so if we permit reading it now we might return the wrong value.
                 Err(ConstEvalErrKind::ConstAccessesStatic.into())
             } else {
                 // Immutable global, this read is fine.
-                // But make sure we never accept a read from something mutable, that would be
-                // unsound. The reason is that as the content of this allocation may be different
-                // now and at run-time, so if we permit reading now we might return the wrong value.
-                assert_eq!(allocation.mutability, Mutability::Not);
                 Ok(())
             }
         }
diff --git a/src/librustc_mir/const_eval/mod.rs b/src/librustc_mir/const_eval/mod.rs
@@ -42,7 +42,7 @@ pub(crate) fn const_field<'tcx>(
     let field = ecx.operand_field(down, field.index()).unwrap();
     // and finally move back to the const world, always normalizing because
     // this is not called for statics.
-    op_to_const(&ecx, field)
+    try_op_to_const(&ecx, field).unwrap()
 }
 
 pub(crate) fn const_caller_location(
@@ -81,7 +81,7 @@ pub(crate) fn destructure_const<'tcx>(
     let down = ecx.operand_downcast(op, variant).unwrap();
     let fields_iter = (0..field_count).map(|i| {
         let field_op = ecx.operand_field(down, i).unwrap();
-        let val = op_to_const(&ecx, field_op);
+        let val = try_op_to_const(&ecx, field_op).unwrap();
         ty::Const::from_value(tcx, val, field_op.layout.ty)
     });
     let fields = tcx.arena.alloc_from_iter(fields_iter);
diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs
@@ -7,7 +7,6 @@ use std::hash::Hash;
 
 use rustc_middle::mir;
 use rustc_middle::ty::{self, Ty};
-use rustc_span::def_id::DefId;
 
 use super::{
     AllocId, Allocation, AllocationExtra, Frame, ImmTy, InterpCx, InterpResult, Memory, MemoryKind,
@@ -207,13 +206,11 @@ pub trait Machine<'mir, 'tcx>: Sized {
     }
 
     /// Called before a global allocation is accessed.
-    /// `def_id` is `Some` if this is the "lazy" allocation of a static.
     #[inline]
     fn before_access_global(
         _memory_extra: &Self::MemoryExtra,
         _alloc_id: AllocId,
         _allocation: &Allocation,
-        _static_def_id: Option<DefId>,
         _is_write: bool,
     ) -> InterpResult<'tcx> {
         Ok(())
diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs
@@ -430,11 +430,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
         is_write: bool,
     ) -> InterpResult<'tcx, Cow<'tcx, Allocation<M::PointerTag, M::AllocExtra>>> {
         let alloc = tcx.alloc_map.lock().get(id);
-        let (alloc, def_id) = match alloc {
-            Some(GlobalAlloc::Memory(mem)) => {
-                // Memory of a constant or promoted or anonymous memory referenced by a static.
-                (mem, None)
-            }
+        let alloc = match alloc {
+            // Memory of a constant or promoted or anonymous memory referenced by a static.
+            Some(GlobalAlloc::Memory(mem)) => mem,
             Some(GlobalAlloc::Function(..)) => throw_ub!(DerefFunctionPointer(id)),
             None => throw_ub!(PointerUseAfterFree(id)),
             Some(GlobalAlloc::Static(def_id)) => {
@@ -466,12 +464,10 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
                     })?;
                 // Make sure we use the ID of the resolved memory, not the lazy one!
                 let id = raw_const.alloc_id;
-                let allocation = tcx.alloc_map.lock().unwrap_memory(id);
-
-                (allocation, Some(def_id))
+                tcx.alloc_map.lock().unwrap_memory(id)
             }
         };
-        M::before_access_global(memory_extra, id, alloc, def_id, is_write)?;
+        M::before_access_global(memory_extra, id, alloc, is_write)?;
         let alloc = Cow::Borrowed(alloc);
         // We got tcx memory. Let the machine initialize its "extra" stuff.
         let (alloc, tag) = M::init_allocation_extra(
diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs
@@ -411,11 +411,11 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M
                     if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
                         return Ok(());
                     }
+                    // FIXME: Statics referenced from consts are skipped.
+                    // This avoids spurious "const accesses static" errors
+                    // unrelated to validity, but is similarly unsound.
                     if !self.may_ref_to_static && self.ecx.tcx.is_static(did) {
-                        throw_validation_failure!(
-                            format_args!("a {} pointing to a static variable", kind),
-                            self.path
-                        );
+                        return Ok(());
                     }
                 }
             }
diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs
@@ -23,7 +23,7 @@ use rustc_middle::ty::layout::{HasTyCtxt, LayoutError, TyAndLayout};
 use rustc_middle::ty::subst::{InternalSubsts, Subst};
 use rustc_middle::ty::{self, ConstKind, Instance, ParamEnv, Ty, TyCtxt, TypeFoldable};
 use rustc_session::lint;
-use rustc_span::{def_id::DefId, Span};
+use rustc_span::Span;
 use rustc_target::abi::{HasDataLayout, LayoutOf, Size, TargetDataLayout};
 use rustc_trait_selection::traits;
 
@@ -274,7 +274,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine {
         _memory_extra: &(),
         _alloc_id: AllocId,
         allocation: &Allocation<Self::PointerTag, Self::AllocExtra>,
-        _static_def_id: Option<DefId>,
         is_write: bool,
     ) -> InterpResult<'tcx> {
         if is_write {
diff --git a/src/test/ui/consts/const-points-to-static.rs b/src/test/ui/consts/const-points-to-static.rs
@@ -1,10 +1,10 @@
+// check-pass
 // compile-flags: -Zunleash-the-miri-inside-of-you
 
 #![allow(dead_code)]
 
 const TEST: &u8 = &MY_STATIC;
 //~^ skipping const checks
-//~| it is undefined behavior to use this value
 
 static MY_STATIC: u8 = 4;
 
diff --git a/src/test/ui/consts/const-points-to-static.stderr b/src/test/ui/consts/const-points-to-static.stderr
@@ -1,17 +1,8 @@
 warning: skipping const checks
-  --> $DIR/const-points-to-static.rs:5:20
+  --> $DIR/const-points-to-static.rs:6:20
    |
 LL | const TEST: &u8 = &MY_STATIC;
    |                    ^^^^^^^^^
 
-error[E0080]: it is undefined behavior to use this value
-  --> $DIR/const-points-to-static.rs:5:1
-   |
-LL | const TEST: &u8 = &MY_STATIC;
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a reference pointing to a static variable
-   |
-   = 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: aborting due to previous error; 1 warning emitted
+warning: 1 warning emitted
 
-For more information about this error, try `rustc --explain E0080`.
diff --git a/src/test/ui/consts/const-prop-read-static-in-const.rs b/src/test/ui/consts/const-prop-read-static-in-const.rs
@@ -1,8 +1,9 @@
+// check-pass
 // compile-flags: -Zunleash-the-miri-inside-of-you
 
 #![allow(dead_code)]
 
-const TEST: u8 = MY_STATIC; //~ ERROR any use of this value will cause an error
+const TEST: u8 = MY_STATIC;
 //~^ skipping const checks
 
 static MY_STATIC: u8 = 4;
diff --git a/src/test/ui/consts/const-prop-read-static-in-const.stderr b/src/test/ui/consts/const-prop-read-static-in-const.stderr
@@ -1,18 +1,8 @@
 warning: skipping const checks
-  --> $DIR/const-prop-read-static-in-const.rs:5:18
+  --> $DIR/const-prop-read-static-in-const.rs:6:18
    |
 LL | const TEST: u8 = MY_STATIC;
    |                  ^^^^^^^^^
 
-error: any use of this value will cause an error
-  --> $DIR/const-prop-read-static-in-const.rs:5:18
-   |
-LL | const TEST: u8 = MY_STATIC;
-   | -----------------^^^^^^^^^-
-   |                  |
-   |                  constant accesses static
-   |
-   = note: `#[deny(const_err)]` on by default
-
-error: aborting due to previous error; 1 warning emitted
+warning: 1 warning emitted
 
diff --git a/src/test/ui/consts/miri_unleashed/const_refers_to_static.rs b/src/test/ui/consts/miri_unleashed/const_refers_to_static.rs
@@ -1,3 +1,4 @@
+// check-pass
 // compile-flags: -Zunleash-the-miri-inside-of-you
 #![warn(const_err)]
 
@@ -6,12 +7,31 @@
 use std::sync::atomic::AtomicUsize;
 use std::sync::atomic::Ordering;
 
-const REF_INTERIOR_MUT: &usize = { //~ ERROR undefined behavior to use this value
+// Dynamically okay; does not touch any mutable static data:
+
+const READ_IMMUT: &usize = {
+    static FOO: usize = 0;
+    &FOO
+    //~^ WARN skipping const checks
+};
+
+const DEREF_IMMUT: usize = *READ_IMMUT;
+
+const REF_INTERIOR_MUT: &usize = {
     static FOO: AtomicUsize = AtomicUsize::new(0);
     unsafe { &*(&FOO as *const _ as *const usize) }
     //~^ WARN skipping const checks
 };
 
+extern { static EXTERN: usize; }
+const REF_EXTERN: &usize = unsafe { &EXTERN };
+//~^ WARN skipping const checks
+
+// Not okay; uses of these consts would read or write mutable static data:
+
+const DEREF_INTERIOR_MUT: usize = *REF_INTERIOR_MUT;
+//~^ WARN any use of this value will cause an error
+
 const MUTATE_INTERIOR_MUT: usize = {
     static FOO: AtomicUsize = AtomicUsize::new(0);
     FOO.fetch_add(1, Ordering::Relaxed) //~ WARN any use of this value will cause an error
@@ -30,10 +50,7 @@ const READ_MUT: u32 = unsafe { MUTABLE }; //~ WARN any use of this value will ca
 //~^ WARN skipping const checks
 //~| WARN skipping const checks
 
-// ok some day perhaps
-const READ_IMMUT: &usize = { //~ ERROR it is undefined behavior to use this value
-    static FOO: usize = 0;
-    &FOO
-    //~^ WARN skipping const checks
-};
+const READ_EXTERN: usize = unsafe { EXTERN }; //~ WARN any use of this value will cause an error
+//~^ WARN skipping const checks
+
 fn main() {}
diff --git a/src/test/ui/consts/miri_unleashed/const_refers_to_static.stderr b/src/test/ui/consts/miri_unleashed/const_refers_to_static.stderr
diff --git a/src/test/ui/consts/miri_unleashed/use-static-via-const.rs b/src/test/ui/consts/miri_unleashed/use-static-via-const.rs
diff --git a/src/test/ui/consts/miri_unleashed/use-static-via-const.stderr b/src/test/ui/consts/miri_unleashed/use-static-via-const.stderr
diff --git a/src/test/ui/issues/issue-52060.rs b/src/test/ui/issues/issue-52060.rs
diff --git a/src/test/ui/issues/issue-52060.stderr b/src/test/ui/issues/issue-52060.stderr

Original file line number	Diff line number	Diff line change
`@@ -411,11 +411,11 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M`
`411`	`411`	`if !did.is_local() \|\| self.ecx.tcx.is_foreign_item(did) {`
`412`	`412`	`return Ok(());`
`413`	`413`	`}`
	`414`	`+ // FIXME: Statics referenced from consts are skipped.`
	`415`	`+ // This avoids spurious "const accesses static" errors`
	`416`	`+ // unrelated to validity, but is similarly unsound.`
`414`	`417`	`if !self.may_ref_to_static && self.ecx.tcx.is_static(did) {`
`415`		`- throw_validation_failure!(`
`416`		`- format_args!("a {} pointing to a static variable", kind),`
`417`		`- self.path`
`418`		`- );`
	`418`	`+ return Ok(());`
`419`	`419`	`}`
`420`	`420`	`}`
`421`	`421`	`}`