user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Rollup merge of #63955 - RalfJung:intern, r=oli-obk

Browse source 
Make sure interned constants are immutable

This makes sure that interning for constants (not statics) creates only immutable allocations.

Previously, the "main" allocation of `const FOO: Cell<i32> = Cell::new(0);` was marked as mutable, but I don't think we want that. It can be only copied, not written to.

Also, "leftover" allocations (behind raw pointers etc) were left mutable. I don't think we want to support that. I tried asserting that these are all already immutable (to double-check our static checks), but that failed in this one:
```rust
const NON_NULL_PTR2: NonNull<u8> = unsafe { mem::transmute(&0) };
```
Seems like maybe we want more precise mutability annotation inside Miri for locals (like `&0` here) so that this would actually become immutable to begin with?

I also factored `intern_shallow` out of the visitor so that we don't have to construct a visitor when we do not plan to visit anything. That confused me at first.
This commit is contained in:
Mazdak Farrokhzad 2019-09-16 17:09:33 +02:00 • committed by GitHub
commit f432d5030e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
5 changed files with 195 additions and 121 deletions
Download patch file Download diff file Expand all files Collapse all files

6
src/librustc_mir/const_eval.rs
View file

@ -134,9 +134,8 @@ fn eval_body_using_ecx<'mir, 'tcx>(
ecx: &mut CompileTimeEvalContext<'mir, 'tcx>,
cid: GlobalId<'tcx>,
body: &'mir mir::Body<'tcx>,
param_env: ty::ParamEnv<'tcx>,
) -> InterpResult<'tcx, MPlaceTy<'tcx>> {
debug!("eval_body_using_ecx: {:?}, {:?}", cid, param_env);
debug!("eval_body_using_ecx: {:?}, {:?}", cid, ecx.param_env);
let tcx = ecx.tcx.tcx;
let layout = ecx.layout_of(body.return_ty().subst(tcx, cid.instance.substs))?;
assert!(!layout.is_unsized());
@ -162,7 +161,6 @@ fn eval_body_using_ecx<'mir, 'tcx>(
ecx,
cid.instance.def_id(),
ret,
param_env,
)?;
debug!("eval_body_using_ecx done: {:?}", *ret);
@ -658,7 +656,7 @@ pub fn const_eval_raw_provider<'tcx>(
let res = ecx.load_mir(cid.instance.def, cid.promoted);
res.and_then(
|body| eval_body_using_ecx(&mut ecx, cid, body, key.param_env)
|body| eval_body_using_ecx(&mut ecx, cid, body)
).and_then(|place| {
Ok(RawConst {
alloc_id: place.ptr.assert_ptr().alloc_id,

261
src/librustc_mir/interpret/intern.rs
View file

@ -3,7 +3,7 @@
//! After a const evaluation has computed a value, before we destroy the const evaluator's session
//! memory, we need to extract all memory allocations to the global memory pool so they stay around.
use rustc::ty::{Ty, TyCtxt, ParamEnv, self};
use rustc::ty::{Ty, self};
use rustc::mir::interpret::{InterpResult, ErrorHandled};
use rustc::hir;
use rustc::hir::def_id::DefId;
@ -11,32 +11,29 @@ use super::validity::RefTracking;
use rustc_data_structures::fx::FxHashSet;
use syntax::ast::Mutability;
use syntax_pos::Span;
use super::{
ValueVisitor, MemoryKind, Pointer, AllocId, MPlaceTy, Scalar,
ValueVisitor, MemoryKind, AllocId, MPlaceTy, Scalar,
};
use crate::const_eval::{CompileTimeInterpreter, CompileTimeEvalContext};
struct InternVisitor<'rt, 'mir, 'tcx> {
/// previously encountered safe references
ref_tracking: &'rt mut RefTracking<(MPlaceTy<'tcx>, Mutability, InternMode)>,
/// The ectx from which we intern.
ecx: &'rt mut CompileTimeEvalContext<'mir, 'tcx>,
param_env: ParamEnv<'tcx>,
/// Previously encountered safe references.
ref_tracking: &'rt mut RefTracking<(MPlaceTy<'tcx>, Mutability, InternMode)>,
/// A list of all encountered allocations. After type-based interning, we traverse this list to
/// also intern allocations that are only referenced by a raw pointer or inside a union.
leftover_allocations: &'rt mut FxHashSet<AllocId>,
/// The root node of the value that we're looking at. This field is never mutated and only used
/// for sanity assertions that will ICE when `const_qualif` screws up.
mode: InternMode,
/// This field stores the mutability of the value *currently* being checked.
/// It is set to mutable when an `UnsafeCell` is encountered
/// When recursing across a reference, we don't recurse but store the
/// value to be checked in `ref_tracking` together with the mutability at which we are checking
/// the value.
/// When encountering an immutable reference, we treat everything as immutable that is behind
/// it.
/// When encountering a mutable reference, we determine the pointee mutability
/// taking into account the mutability of the context: `& &mut i32` is entirely immutable,
/// despite the nested mutable reference!
/// The field gets updated when an `UnsafeCell` is encountered.
mutability: Mutability,
/// A list of all encountered relocations. After type-based interning, we traverse this list to
/// also intern allocations that are only referenced by a raw pointer or inside a union.
leftover_relocations: &'rt mut FxHashSet<AllocId>,
}
#[derive(Copy, Clone, Debug, PartialEq, Hash, Eq)]
@ -45,9 +42,10 @@ enum InternMode {
/// `static`. In a `static mut` we start out as mutable and thus can also contain further `&mut`
/// that will actually be treated as mutable.
Static,
/// UnsafeCell is OK in the value of a constant, but not behind references in a constant
/// UnsafeCell is OK in the value of a constant: `const FOO = Cell::new(0)` creates
/// a new cell every time it is used.
ConstBase,
/// `UnsafeCell` ICEs
/// `UnsafeCell` ICEs.
Const,
}
@ -55,48 +53,100 @@ enum InternMode {
/// into the memory of other constants or statics
struct IsStaticOrFn;
/// Intern an allocation without looking at its children.
/// `mode` is the mode of the environment where we found this pointer.
/// `mutablity` is the mutability of the place to be interned; even if that says
/// `immutable` things might become mutable if `ty` is not frozen.
/// `ty` can be `None` if there is no potential interior mutability
/// to account for (e.g. for vtables).
fn intern_shallow<'rt, 'mir, 'tcx>(
ecx: &'rt mut CompileTimeEvalContext<'mir, 'tcx>,
leftover_allocations: &'rt mut FxHashSet<AllocId>,
mode: InternMode,
alloc_id: AllocId,
mutability: Mutability,
ty: Option<Ty<'tcx>>,
) -> InterpResult<'tcx, Option<IsStaticOrFn>> {
trace!(
"InternVisitor::intern {:?} with {:?}",
alloc_id, mutability,
);
// remove allocation
let tcx = ecx.tcx;
let memory = ecx.memory_mut();
let (kind, mut alloc) = match memory.alloc_map.remove(&alloc_id) {
Some(entry) => entry,
None => {
// Pointer not found in local memory map. It is either a pointer to the global
// map, or dangling.
// If the pointer is dangling (neither in local nor global memory), we leave it
// to validation to error. The `delay_span_bug` ensures that we don't forget such
// a check in validation.
if tcx.alloc_map.lock().get(alloc_id).is_none() {
tcx.sess.delay_span_bug(ecx.tcx.span, "tried to intern dangling pointer");
}
// treat dangling pointers like other statics
// just to stop trying to recurse into them
return Ok(Some(IsStaticOrFn));
},
};
// This match is just a canary for future changes to `MemoryKind`, which most likely need
// changes in this function.
match kind {
MemoryKind::Stack | MemoryKind::Vtable => {},
}
// Set allocation mutability as appropriate. This is used by LLVM to put things into
// read-only memory, and also by Miri when evluating other constants/statics that
// access this one.
if mode == InternMode::Static {
// When `ty` is `None`, we assume no interior mutability.
let frozen = ty.map_or(true, |ty| ty.is_freeze(
ecx.tcx.tcx,
ecx.param_env,
ecx.tcx.span,
));
// For statics, allocation mutability is the combination of the place mutability and
// the type mutability.
// The entire allocation needs to be mutable if it contains an `UnsafeCell` anywhere.
if mutability == Mutability::Immutable && frozen {
alloc.mutability = Mutability::Immutable;
} else {
// Just making sure we are not "upgrading" an immutable allocation to mutable.
assert_eq!(alloc.mutability, Mutability::Mutable);
}
} else {
// We *could* be non-frozen at `ConstBase`, for constants like `Cell::new(0)`.
// But we still intern that as immutable as the memory cannot be changed once the
// initial value was computed.
// Constants are never mutable.
assert_eq!(
mutability, Mutability::Immutable,
"Something went very wrong: mutability requested for a constant"
);
alloc.mutability = Mutability::Immutable;
};
// link the alloc id to the actual allocation
let alloc = tcx.intern_const_alloc(alloc);
leftover_allocations.extend(alloc.relocations().iter().map(|&(_, ((), reloc))| reloc));
tcx.alloc_map.lock().set_alloc_id_memory(alloc_id, alloc);
Ok(None)
}
impl<'rt, 'mir, 'tcx> InternVisitor<'rt, 'mir, 'tcx> {
/// Intern an allocation without looking at its children
fn intern_shallow(
&mut self,
ptr: Pointer,
alloc_id: AllocId,
mutability: Mutability,
ty: Option<Ty<'tcx>>,
) -> InterpResult<'tcx, Option<IsStaticOrFn>> {
trace!(
"InternVisitor::intern {:?} with {:?}",
ptr, mutability,
);
// remove allocation
let tcx = self.ecx.tcx;
let memory = self.ecx.memory_mut();
let (kind, mut alloc) = match memory.alloc_map.remove(&ptr.alloc_id) {
Some(entry) => entry,
None => {
// if the pointer is dangling (neither in local nor global memory), we leave it
// to validation to error. The `delay_span_bug` ensures that we don't forget such
// a check in validation.
if tcx.alloc_map.lock().get(ptr.alloc_id).is_none() {
tcx.sess.delay_span_bug(self.ecx.tcx.span, "tried to intern dangling pointer");
}
// treat dangling pointers like other statics
// just to stop trying to recurse into them
return Ok(Some(IsStaticOrFn));
},
};
// This match is just a canary for future changes to `MemoryKind`, which most likely need
// changes in this function.
match kind {
MemoryKind::Stack | MemoryKind::Vtable => {},
}
// Ensure llvm knows to only put this into immutable memory if the value is immutable either
// by being behind a reference or by being part of a static or const without interior
// mutability
alloc.mutability = mutability;
// link the alloc id to the actual allocation
let alloc = tcx.intern_const_alloc(alloc);
self.leftover_relocations.extend(alloc.relocations().iter().map(|&(_, ((), reloc))| reloc));
tcx.alloc_map.lock().set_alloc_id_memory(ptr.alloc_id, alloc);
Ok(None)
intern_shallow(
self.ecx,
self.leftover_allocations,
self.mode,
alloc_id,
mutability,
ty,
)
}
}
@ -119,14 +169,16 @@ for
) -> InterpResult<'tcx> {
if let Some(def) = mplace.layout.ty.ty_adt_def() {
if Some(def.did) == self.ecx.tcx.lang_items().unsafe_cell_type() {
// We are crossing over an `UnsafeCell`, we can mutate again
// We are crossing over an `UnsafeCell`, we can mutate again. This means that
// References we encounter inside here are interned as pointing to mutable
// allocations.
let old = std::mem::replace(&mut self.mutability, Mutability::Mutable);
assert_ne!(
self.mode, InternMode::Const,
"UnsafeCells are not allowed behind references in constants. This should have \
been prevented statically by const qualification. If this were allowed one \
would be able to change a constant at one use site and other use sites may \
arbitrarily decide to change, too.",
would be able to change a constant at one use site and other use sites could \
observe that mutation.",
);
let walked = self.walk_aggregate(mplace, fields);
self.mutability = old;
@ -145,12 +197,13 @@ for
// Handle trait object vtables
if let Ok(meta) = value.to_meta() {
if let ty::Dynamic(..) =
self.ecx.tcx.struct_tail_erasing_lifetimes(referenced_ty, self.param_env).sty
self.ecx.tcx.struct_tail_erasing_lifetimes(
referenced_ty, self.ecx.param_env).sty
{
if let Ok(vtable) = meta.unwrap().to_ptr() {
// explitly choose `Immutable` here, since vtables are immutable, even
// if the reference of the fat pointer is mutable
self.intern_shallow(vtable, Mutability::Immutable)?;
self.intern_shallow(vtable.alloc_id, Mutability::Immutable, None)?;
}
}
}
@ -177,7 +230,7 @@ for
(InternMode::Const, hir::Mutability::MutMutable) => {
match referenced_ty.sty {
ty::Array(_, n)
if n.eval_usize(self.ecx.tcx.tcx, self.param_env) == 0 => {}
if n.eval_usize(self.ecx.tcx.tcx, self.ecx.param_env) == 0 => {}
ty::Slice(_)
if value.to_meta().unwrap().unwrap().to_usize(self.ecx)? == 0 => {}
_ => bug!("const qualif failed to prevent mutable references"),
@ -195,21 +248,13 @@ for
(Mutability::Mutable, hir::Mutability::MutMutable) => Mutability::Mutable,
_ => Mutability::Immutable,
};
// Compute the mutability of the allocation
let intern_mutability = intern_mutability(
self.ecx.tcx.tcx,
self.param_env,
mplace.layout.ty,
self.ecx.tcx.span,
mutability,
);
// Recursing behind references changes the intern mode for constants in order to
// cause assertions to trigger if we encounter any `UnsafeCell`s.
let mode = match self.mode {
InternMode::ConstBase => InternMode::Const,
other => other,
};
match self.intern_shallow(ptr, intern_mutability)? {
match self.intern_shallow(ptr.alloc_id, mutability, Some(mplace.layout.ty))? {
// No need to recurse, these are interned already and statics may have
// cycles, so we don't want to recurse there
Some(IsStaticOrFn) => {},
@ -224,69 +269,45 @@ for
}
}
/// Figure out the mutability of the allocation.
/// Mutable if it has interior mutability *anywhere* in the type.
fn intern_mutability<'tcx>(
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
ty: Ty<'tcx>,
span: Span,
mutability: Mutability,
) -> Mutability {
let has_interior_mutability = !ty.is_freeze(tcx, param_env, span);
if has_interior_mutability {
Mutability::Mutable
} else {
mutability
}
}
pub fn intern_const_alloc_recursive(
ecx: &mut CompileTimeEvalContext<'mir, 'tcx>,
def_id: DefId,
ret: MPlaceTy<'tcx>,
// FIXME(oli-obk): can we scrap the param env? I think we can, the final value of a const eval
// must always be monomorphic, right?
param_env: ty::ParamEnv<'tcx>,
) -> InterpResult<'tcx> {
let tcx = ecx.tcx;
// this `mutability` is the mutability of the place, ignoring the type
let (mutability, base_intern_mode) = match tcx.static_mutability(def_id) {
let (base_mutability, base_intern_mode) = match tcx.static_mutability(def_id) {
Some(hir::Mutability::MutImmutable) => (Mutability::Immutable, InternMode::Static),
None => (Mutability::Immutable, InternMode::ConstBase),
// `static mut` doesn't care about interior mutability, it's mutable anyway
Some(hir::Mutability::MutMutable) => (Mutability::Mutable, InternMode::Static),
// consts, promoteds. FIXME: what about array lengths, array initializers?
None => (Mutability::Immutable, InternMode::ConstBase),
};
// type based interning
let mut ref_tracking = RefTracking::new((ret, mutability, base_intern_mode));
let leftover_relocations = &mut FxHashSet::default();
// This mutability is the combination of the place mutability and the type mutability. If either
// is mutable, `alloc_mutability` is mutable. This exists because the entire allocation needs
// to be mutable if it contains an `UnsafeCell` anywhere. The other `mutability` exists so that
// the visitor does not treat everything outside the `UnsafeCell` as mutable.
let alloc_mutability = intern_mutability(
tcx.tcx, param_env, ret.layout.ty, tcx.span, mutability,
);
// Type based interning.
// `ref_tracking` tracks typed references we have seen and still need to crawl for
// more typed information inside them.
// `leftover_allocations` collects *all* allocations we see, because some might not
// be available in a typed way. They get interned at the end.
let mut ref_tracking = RefTracking::new((ret, base_mutability, base_intern_mode));
let leftover_allocations = &mut FxHashSet::default();
// start with the outermost allocation
InternVisitor {
ref_tracking: &mut ref_tracking,
intern_shallow(
ecx,
mode: base_intern_mode,
leftover_relocations,
param_env,
mutability,
}.intern_shallow(ret.ptr.to_ptr()?, alloc_mutability)?;
leftover_allocations,
base_intern_mode,
ret.ptr.to_ptr()?.alloc_id,
base_mutability,
Some(ret.layout.ty)
)?;
while let Some(((mplace, mutability, mode), _)) = ref_tracking.todo.pop() {
let interned = InternVisitor {
ref_tracking: &mut ref_tracking,
ecx,
mode,
leftover_relocations,
param_env,
leftover_allocations,
mutability,
}.visit_value(mplace);
if let Err(error) = interned {
@ -309,15 +330,23 @@ pub fn intern_const_alloc_recursive(
// Intern the rest of the allocations as mutable. These might be inside unions, padding, raw
// pointers, ... So we can't intern them according to their type rules
let mut todo: Vec<_> = leftover_relocations.iter().cloned().collect();
let mut todo: Vec<_> = leftover_allocations.iter().cloned().collect();
while let Some(alloc_id) = todo.pop() {
if let Some((_, alloc)) = ecx.memory_mut().alloc_map.remove(&alloc_id) {
// We can't call the `intern` method here, as its logic is tailored to safe references.
// So we hand-roll the interning logic here again
if let Some((_, mut alloc)) = ecx.memory_mut().alloc_map.remove(&alloc_id) {
// We can't call the `intern_shallow` method here, as its logic is tailored to safe
// references and a `leftover_allocations` set (where we only have a todo-list here).
// So we hand-roll the interning logic here again.
if base_intern_mode != InternMode::Static {
// If it's not a static, it *must* be immutable.
// We cannot have mutable memory inside a constant.
// FIXME: ideally we would assert that they already are immutable, to double-
// check our static checks.
alloc.mutability = Mutability::Immutable;
}
let alloc = tcx.intern_const_alloc(alloc);
tcx.alloc_map.lock().set_alloc_id_memory(alloc_id, alloc);
for &(_, ((), reloc)) in alloc.relocations().iter() {
if leftover_relocations.insert(reloc) {
if leftover_allocations.insert(reloc) {
todo.push(reloc);
}
}

20
src/test/ui/consts/miri_unleashed/mutable_const.rs Normal file
View file

@ -0,0 +1,20 @@
// compile-flags: -Zunleash-the-miri-inside-of-you
#![feature(const_raw_ptr_deref)]
#![deny(const_err)]
use std::cell::UnsafeCell;
// make sure we do not just intern this as mutable
const MUTABLE_BEHIND_RAW: *mut i32 = &UnsafeCell::new(42) as *const _ as *mut _;
const MUTATING_BEHIND_RAW: () = {
// Test that `MUTABLE_BEHIND_RAW` is actually immutable, by doing this at const time.
unsafe {
*MUTABLE_BEHIND_RAW = 99 //~ WARN skipping const checks
//~^ ERROR any use of this value will cause an error
//~^^ tried to modify constant memory
}
};
fn main() {}

27
src/test/ui/consts/miri_unleashed/mutable_const.stderr Normal file
View file

@ -0,0 +1,27 @@
warning: skipping const checks
--> $DIR/mutable_const.rs:14:9
|
LL | *MUTABLE_BEHIND_RAW = 99
| ^^^^^^^^^^^^^^^^^^^^^^^^
error: any use of this value will cause an error
--> $DIR/mutable_const.rs:14:9
|
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
| | ^^^^^^^^^^^^^^^^^^^^^^^^ tried to modify constant memory
... |
LL | | }
LL | | };
| |__-
|
note: lint level defined here
--> $DIR/mutable_const.rs:4:9
|
LL | #![deny(const_err)]
| ^^^^^^^^^
error: aborting due to previous error

2
src/test/ui/consts/miri_unleashed/mutable_references_ice.stderr
View file

@ -6,7 +6,7 @@ LL | *MUH.x.get() = 99;
thread 'rustc' panicked at 'assertion failed: `(left != right)`
left: `Const`,
right: `Const`: UnsafeCells are not allowed behind references in constants. This should have been prevented statically by const qualification. If this were allowed one would be able to change a constant at one use site and other use sites may arbitrarily decide to change, too.', src/librustc_mir/interpret/intern.rs:LL:CC
right: `Const`: UnsafeCells are not allowed behind references in constants. This should have been prevented statically by const qualification. If this were allowed one would be able to change a constant at one use site and other use sites could observe that mutation.', src/librustc_mir/interpret/intern.rs:LL:CC
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
error: internal compiler error: unexpected panic