Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Miri: move ModifiedStatic to ConstEval errors #70241

Merged
merged 6 commits into from
Mar 24, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions src/librustc/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -453,9 +453,6 @@ pub enum UnsupportedOpInfo {
ReadForeignStatic(DefId),
/// Could not find MIR for a function.
NoMirFor(DefId),
/// Modified a static during const-eval.
/// FIXME: move this to `ConstEvalErrKind` through a machine hook.
ModifiedStatic,
/// Encountered a pointer where we needed raw bytes.
ReadPointerAsBytes,
/// Encountered raw bytes where we needed a pointer.
Expand All @@ -471,12 +468,6 @@ impl fmt::Debug for UnsupportedOpInfo {
write!(f, "tried to read from foreign (extern) static {:?}", did)
}
NoMirFor(did) => write!(f, "could not load MIR for {:?}", did),
ModifiedStatic => write!(
f,
"tried to modify a static's initial value from another static's \
initializer"
),

ReadPointerAsBytes => write!(f, "unable to turn pointer into raw bytes",),
ReadBytesAsPointer => write!(f, "unable to turn bytes into a pointer"),
}
Expand Down
1 change: 1 addition & 0 deletions src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,7 @@ pub struct GlobalCtxt<'tcx> {
/// Stores the value of constants (and deduplicates the actual memory)
allocation_interner: ShardedHashMap<&'tcx Allocation, ()>,

/// Stores memory for globals (statics/consts).
pub alloc_map: Lock<interpret::AllocMap<'tcx>>,

layout_interner: ShardedHashMap<&'tcx LayoutDetails, ()>,
Expand Down
4 changes: 4 additions & 0 deletions src/librustc_mir/const_eval/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::interpret::{ConstEvalErr, InterpErrorInfo, Machine};
pub enum ConstEvalErrKind {
NeedsRfc(String),
ConstAccessesStatic,
ModifiedGlobal,
AssertFailure(AssertKind<u64>),
Panic { msg: Symbol, line: u32, col: u32, file: Symbol },
}
Expand All @@ -33,6 +34,9 @@ impl fmt::Display for ConstEvalErrKind {
write!(f, "\"{}\" needs an rfc before being allowed inside constants", msg)
}
ConstAccessesStatic => write!(f, "constant accesses static"),
ModifiedGlobal => {
write!(f, "modifying a static's initial value from another static's initializer")
}
AssertFailure(ref msg) => write!(f, "{:?}", msg),
Panic { msg, line, col, file } => {
write!(f, "the evaluated program panicked at '{}', {}:{}:{}", msg, file, line, col)
Expand Down
23 changes: 16 additions & 7 deletions src/librustc_mir/const_eval/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@ use std::hash::Hash;
use rustc_data_structures::fx::FxHashMap;

use rustc::mir::AssertMessage;
use rustc_span::source_map::Span;
use rustc_ast::ast::Mutability;
use rustc_span::symbol::Symbol;
use rustc_span::{def_id::DefId, Span};

use crate::interpret::{
self, AllocId, Allocation, GlobalId, ImmTy, InterpCx, InterpResult, Memory, MemoryKind, OpTy,
Expand Down Expand Up @@ -167,7 +168,7 @@ impl interpret::MayLeak for ! {
}

impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter {
type MemoryKinds = !;
type MemoryKind = !;
type PointerTag = ();
type ExtraFnVal = !;

Expand All @@ -177,7 +178,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter {

type MemoryMap = FxHashMap<AllocId, (MemoryKind<!>, Allocation)>;

const STATIC_KIND: Option<!> = None; // no copying of statics allowed
const GLOBAL_KIND: Option<!> = None; // no copying of globals allowed
Comment on lines -180 to +181
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the comment mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, "from tcx" was the missing part, at a glance this looks like it's talking about let x = STATIC; which confused me.


// We do not check for alignment to avoid having to carry an `Align`
// in `ConstValue::ByRef`.
Expand Down Expand Up @@ -317,7 +318,7 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter {
}

#[inline(always)]
fn tag_static_base_pointer(_memory_extra: &MemoryExtra, _id: AllocId) -> Self::PointerTag {}
fn tag_global_base_pointer(_memory_extra: &MemoryExtra, _id: AllocId) -> Self::PointerTag {}

fn box_alloc(
_ecx: &mut InterpCx<'mir, 'tcx, Self>,
Expand Down Expand Up @@ -345,11 +346,19 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for CompileTimeInterpreter {
Ok(())
}

fn before_access_static(
fn before_access_global(
memory_extra: &MemoryExtra,
_allocation: &Allocation,
alloc_id: AllocId,
allocation: &Allocation,
def_id: Option<DefId>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename this to static_def_id? (in a future PR, since this one is in a rollup)

is_write: bool,
) -> InterpResult<'tcx> {
if memory_extra.can_access_statics {
if is_write && allocation.mutability == Mutability::Not {
Err(err_ub!(WriteToReadOnly(alloc_id)).into())
} else if is_write {
Err(ConstEvalErrKind::ModifiedGlobal.into())
} else if memory_extra.can_access_statics || def_id.is_none() {
// `def_id.is_none()` indicates this is not a static, but a const or so.
Ok(())
} else {
Err(ConstEvalErrKind::ConstAccessesStatic.into())
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_mir/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
/// This represents a *direct* access to that memory, as opposed to access
/// through a pointer that was created by the program.
#[inline(always)]
pub fn tag_static_base_pointer(&self, ptr: Pointer) -> Pointer<M::PointerTag> {
self.memory.tag_static_base_pointer(ptr)
pub fn tag_global_base_pointer(&self, ptr: Pointer) -> Pointer<M::PointerTag> {
self.memory.tag_global_base_pointer(ptr)
}

#[inline(always)]
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_mir/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use super::{AllocId, Allocation, InterpCx, MPlaceTy, Machine, MemoryKind, Scalar
pub trait CompileTimeMachine<'mir, 'tcx> = Machine<
'mir,
'tcx,
MemoryKinds = !,
MemoryKind = !,
PointerTag = (),
ExtraFnVal = !,
FrameExtra = (),
Expand Down Expand Up @@ -104,7 +104,7 @@ fn intern_shallow<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>>(
MemoryKind::Stack | MemoryKind::Vtable | MemoryKind::CallerLocation => {}
}
// 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
// read-only memory, and also by Miri when evaluating other globals that
// access this one.
if mode == InternMode::Static {
// When `ty` is `None`, we assume no interior mutability.
Expand Down
37 changes: 21 additions & 16 deletions src/librustc_mir/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use std::hash::Hash;

use rustc::mir;
use rustc::ty::{self, Ty};
use rustc_span::Span;
use rustc_span::{def_id::DefId, Span};

use super::{
AllocId, Allocation, AllocationExtra, Frame, ImmTy, InterpCx, InterpResult, Memory, MemoryKind,
Expand Down Expand Up @@ -79,7 +79,7 @@ pub trait AllocMap<K: Hash + Eq, V> {
/// and some use case dependent behaviour can instead be applied.
pub trait Machine<'mir, 'tcx>: Sized {
/// Additional memory kinds a machine wishes to distinguish from the builtin ones
type MemoryKinds: ::std::fmt::Debug + MayLeak + Eq + 'static;
type MemoryKind: ::std::fmt::Debug + MayLeak + Eq + 'static;

/// Tag tracked alongside every pointer. This is used to implement "Stacked Borrows"
/// <https://www.ralfj.de/blog/2018/08/07/stacked-borrows.html>.
Expand All @@ -105,16 +105,17 @@ pub trait Machine<'mir, 'tcx>: Sized {
/// Memory's allocation map
type MemoryMap: AllocMap<
AllocId,
(MemoryKind<Self::MemoryKinds>, Allocation<Self::PointerTag, Self::AllocExtra>),
(MemoryKind<Self::MemoryKind>, Allocation<Self::PointerTag, Self::AllocExtra>),
> + Default
+ Clone;

/// The memory kind to use for copied statics -- or None if statics should not be mutated
/// and thus any such attempt will cause a `ModifiedStatic` error to be raised.
/// The memory kind to use for copied global memory (held in `tcx`) --
/// or None if such memory should not be mutated and thus any such attempt will cause
/// a `ModifiedStatic` error to be raised.
/// Statics are copied under two circumstances: When they are mutated, and when
/// `tag_allocation` or `find_foreign_static` (see below) returns an owned allocation
/// `tag_allocation` (see below) returns an owned allocation
/// that is added to the memory so that the work is not done twice.
const STATIC_KIND: Option<Self::MemoryKinds>;
const GLOBAL_KIND: Option<Self::MemoryKind>;

/// Whether memory accesses should be alignment-checked.
const CHECK_ALIGN: bool;
Expand Down Expand Up @@ -207,11 +208,15 @@ pub trait Machine<'mir, 'tcx>: Sized {
Ok(())
}

/// Called before a `Static` value is accessed.
/// Called before a global allocation is accessed.
/// `def_id` is `Some` if this is the "lazy" allocation of a static.
#[inline]
fn before_access_static(
fn before_access_global(
_memory_extra: &Self::MemoryExtra,
_alloc_id: AllocId,
_allocation: &Allocation,
_def_id: Option<DefId>,
_is_write: bool,
) -> InterpResult<'tcx> {
Ok(())
}
Expand All @@ -231,10 +236,10 @@ pub trait Machine<'mir, 'tcx>: Sized {
/// it contains (in relocations) tagged. The way we construct allocations is
/// to always first construct it without extra and then add the extra.
/// This keeps uniform code paths for handling both allocations created by CTFE
/// for statics, and allocations created by Miri during evaluation.
/// for globals, and allocations created by Miri during evaluation.
///
/// `kind` is the kind of the allocation being tagged; it can be `None` when
/// it's a static and `STATIC_KIND` is `None`.
/// it's a global and `GLOBAL_KIND` is `None`.
///
/// This should avoid copying if no work has to be done! If this returns an owned
/// allocation (because a copy had to be done to add tags or metadata), machine memory will
Expand All @@ -243,20 +248,20 @@ pub trait Machine<'mir, 'tcx>: Sized {
///
/// Also return the "base" tag to use for this allocation: the one that is used for direct
/// accesses to this allocation. If `kind == STATIC_KIND`, this tag must be consistent
/// with `tag_static_base_pointer`.
/// with `tag_global_base_pointer`.
fn init_allocation_extra<'b>(
memory_extra: &Self::MemoryExtra,
id: AllocId,
alloc: Cow<'b, Allocation>,
kind: Option<MemoryKind<Self::MemoryKinds>>,
kind: Option<MemoryKind<Self::MemoryKind>>,
) -> (Cow<'b, Allocation<Self::PointerTag, Self::AllocExtra>>, Self::PointerTag);

/// Return the "base" tag for the given *static* allocation: the one that is used for direct
/// accesses to this static/const/fn allocation. If `id` is not a static allocation,
/// Return the "base" tag for the given *global* allocation: the one that is used for direct
/// accesses to this static/const/fn allocation. If `id` is not a global allocation,
/// this will return an unusable tag (i.e., accesses will be UB)!
///
/// Expects `id` to be already canonical, if needed.
fn tag_static_base_pointer(memory_extra: &Self::MemoryExtra, id: AllocId) -> Self::PointerTag;
fn tag_global_base_pointer(memory_extra: &Self::MemoryExtra, id: AllocId) -> Self::PointerTag;

/// Executes a retagging operation
#[inline]
Expand Down
Loading