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

Various const eval and pattern matching ICE fixes #67192

Merged
merged 22 commits into from
Dec 27, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
1e40681
Don't ICE on the use of integer addresses for ZST constants in patter…
oli-obk Dec 13, 2019
b5b5258
Retire `to_ptr` which should already have no users but still kept get…
oli-obk Dec 13, 2019
bb1ecee
Simplify `force_allocation_maybe_sized`
oli-obk Dec 13, 2019
13694de
Comment on a few odd things that we should look at
oli-obk Dec 13, 2019
a0bd1a6
Prevent an ICE on invalid transmutes
oli-obk Dec 13, 2019
0e969b7
Interning even happens when validation of a constant fails
oli-obk Dec 14, 2019
a7a011d
Immediately evaluate and validate constants when we want them as oper…
oli-obk Dec 20, 2019
6b651b1
Add regression test for ZST statics being allowed to "read" from them…
oli-obk Dec 20, 2019
41d5818
Explain ParamEnv::reveal_all usage
oli-obk Dec 22, 2019
8a88ff1
Comments should start capitalized and end in a period
oli-obk Dec 22, 2019
cb8d1c3
Explain what we are doing with parameter environments for statics
oli-obk Dec 22, 2019
6937ca2
Explain the currently necessary existance of `TransmuteSizeDiff`
oli-obk Dec 22, 2019
72ebce0
Remove unintended noisy log statement
oli-obk Dec 22, 2019
0e3fafa
Typo
oli-obk Dec 22, 2019
9520551
Explain why `const_eval` is ok here
oli-obk Dec 22, 2019
1acbf4b
Early abort instead of building up zero sized values
oli-obk Dec 22, 2019
20c1b3f
Add a `const_eval` helper to `InterpCx`
oli-obk Dec 22, 2019
1531c39
Documentation nit
oli-obk Dec 22, 2019
b476344
Reintroduce the recursion comment
oli-obk Dec 22, 2019
aaffe12
Use the targetted const eval functions
oli-obk Dec 23, 2019
12a4c2c
Fix rebase fallout
oli-obk Dec 23, 2019
f65a91e
Make ui test bitwidth independent
oli-obk Dec 24, 2019
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
6 changes: 6 additions & 0 deletions src/librustc/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,7 @@ pub enum UnsupportedOpInfo<'tcx> {
HeapAllocNonPowerOfTwoAlignment(u64),
ReadFromReturnPointer,
PathNotFound(Vec<String>),
TransmuteSizeDiff(Ty<'tcx>, Ty<'tcx>),
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't add new "unclassified" error kinds. This one looks like UB to me?

But, how can it even be possible to get around the check rustc is already doing that the size must match...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This happens for array lengths, which are evaluated before typeck prevents const eval from running.... and now I'm thinking I can maybe just abort if typeck tables have errors. I'll try to remove this variant again, but I'd rather not make such a profound change to how we do early const eval in this PR

Copy link
Member

Choose a reason for hiding this comment

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

I think we should never run Miri on untypechecked code. That would mean we'd lose tons and tons of useful invariants -- Miri relies on code being well-typed in a lot of places. This would be an endless source of ICEs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, this is an issue since 1.0. I'll try to address it again, I think the query system may be up to it nowadays.

}

impl fmt::Debug for UnsupportedOpInfo<'tcx> {
Expand Down Expand Up @@ -460,6 +461,11 @@ impl fmt::Debug for UnsupportedOpInfo<'tcx> {
passing data of type {:?}",
callee_ty, caller_ty
),
TransmuteSizeDiff(from_ty, to_ty) => write!(
f,
"tried to transmute from {:?} to {:?}, but their sizes differed",
from_ty, to_ty
),
FunctionRetMismatch(caller_ty, callee_ty) => write!(
f,
"tried to call a function with return type {:?} \
Expand Down
9 changes: 2 additions & 7 deletions src/librustc/mir/interpret/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -367,8 +367,9 @@ impl<'tcx, Tag> Scalar<Tag> {
}

/// Do not call this method! Use either `assert_ptr` or `force_ptr`.
/// This method is intentionally private, do not make it public.
#[inline]
pub fn to_ptr(self) -> InterpResult<'tcx, Pointer<Tag>> {
fn to_ptr(self) -> InterpResult<'tcx, Pointer<Tag>> {
match self {
Scalar::Raw { data: 0, .. } => throw_unsup!(InvalidNullPointerUsage),
Scalar::Raw { .. } => throw_unsup!(ReadBytesAsPointer),
Expand Down Expand Up @@ -544,12 +545,6 @@ impl<'tcx, Tag> ScalarMaybeUndef<Tag> {
}
}

/// Do not call this method! Use either `assert_ptr` or `force_ptr`.
#[inline(always)]
pub fn to_ptr(self) -> InterpResult<'tcx, Pointer<Tag>> {
self.not_undef()?.to_ptr()
}

/// Do not call this method! Use either `assert_bits` or `force_bits`.
#[inline(always)]
pub fn to_bits(self, target_size: Size) -> InterpResult<'tcx, u128> {
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/ty/relate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -537,8 +537,8 @@ pub fn super_relate_consts<R: TypeRelation<'tcx>>(
Ok(ConstValue::Scalar(a_val))
} else if let ty::FnPtr(_) = a.ty.kind {
let alloc_map = tcx.alloc_map.lock();
let a_instance = alloc_map.unwrap_fn(a_val.to_ptr().unwrap().alloc_id);
let b_instance = alloc_map.unwrap_fn(b_val.to_ptr().unwrap().alloc_id);
let a_instance = alloc_map.unwrap_fn(a_val.assert_ptr().alloc_id);
let b_instance = alloc_map.unwrap_fn(b_val.assert_ptr().alloc_id);
if a_instance == b_instance {
Ok(ConstValue::Scalar(a_val))
} else {
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_mir/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ pub(super) fn op_to_const<'tcx>(
};
let val = match immediate {
Ok(mplace) => {
let ptr = mplace.ptr.to_ptr().unwrap();
let ptr = mplace.ptr.assert_ptr();
let alloc = ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id);
ConstValue::ByRef { alloc, offset: ptr.offset }
}
Expand All @@ -133,7 +133,7 @@ pub(super) fn op_to_const<'tcx>(
// comes from a constant so it can happen have `Undef`, because the indirect
// memory that was read had undefined bytes.
let mplace = op.assert_mem_place();
let ptr = mplace.ptr.to_ptr().unwrap();
let ptr = mplace.ptr.assert_ptr();
let alloc = ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id);
ConstValue::ByRef { alloc, offset: ptr.offset }
}
Expand Down Expand Up @@ -176,7 +176,7 @@ fn validate_and_turn_into_const<'tcx>(
// Statics/promoteds are always `ByRef`, for the rest `op_to_const` decides
// whether they become immediates.
if is_static || cid.promoted.is_some() {
let ptr = mplace.ptr.to_ptr()?;
let ptr = mplace.ptr.assert_ptr();
Ok(tcx.mk_const(ty::Const {
val: ty::ConstKind::Value(ConstValue::ByRef {
alloc: ecx.tcx.alloc_map.lock().unwrap_memory(ptr.alloc_id),
Expand Down
50 changes: 40 additions & 10 deletions src/librustc_mir/hair/pattern/_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ use syntax_pos::{Span, DUMMY_SP};
use arena::TypedArena;

use smallvec::{smallvec, SmallVec};
use std::borrow::Cow;
use std::cmp::{self, max, min, Ordering};
use std::convert::TryInto;
use std::fmt;
Expand All @@ -260,11 +261,12 @@ use std::ops::RangeInclusive;
use std::u128;

pub fn expand_pattern<'a, 'tcx>(cx: &MatchCheckCtxt<'a, 'tcx>, pat: Pat<'tcx>) -> Pat<'tcx> {
LiteralExpander { tcx: cx.tcx }.fold_pattern(&pat)
LiteralExpander { tcx: cx.tcx, param_env: cx.param_env }.fold_pattern(&pat)
}

struct LiteralExpander<'tcx> {
tcx: TyCtxt<'tcx>,
param_env: ty::ParamEnv<'tcx>,
}

impl LiteralExpander<'tcx> {
Expand All @@ -284,9 +286,23 @@ impl LiteralExpander<'tcx> {
debug!("fold_const_value_deref {:?} {:?} {:?}", val, rty, crty);
match (val, &crty.kind, &rty.kind) {
// the easy case, deref a reference
(ConstValue::Scalar(Scalar::Ptr(p)), x, y) if x == y => {
let alloc = self.tcx.alloc_map.lock().unwrap_memory(p.alloc_id);
ConstValue::ByRef { alloc, offset: p.offset }
(ConstValue::Scalar(p), x, y) if x == y => {
match p {
Scalar::Ptr(p) => {
let alloc = self.tcx.alloc_map.lock().unwrap_memory(p.alloc_id);
ConstValue::ByRef { alloc, offset: p.offset }
}
Scalar::Raw { .. } => {
let layout = self.tcx.layout_of(self.param_env.and(rty)).unwrap();
if layout.is_zst() {
// Deref of a reference to a ZST is a nop.
ConstValue::Scalar(Scalar::zst())
} else {
// FIXME(oli-obk): this is reachable for `const FOO: &&&u32 = &&&42;`
bug!("cannot deref {:#?}, {} -> {}", val, crty, rty);
}
}
}
}
// unsize array to slice if pattern is array but match value or other patterns are slice
(ConstValue::Scalar(Scalar::Ptr(p)), ty::Array(t, n), ty::Slice(u)) => {
Expand Down Expand Up @@ -2348,16 +2364,30 @@ fn specialize_one_pattern<'p, 'tcx>(
// just integers. The only time they should be pointing to memory
// is when they are subslices of nonzero slices.
let (alloc, offset, n, ty) = match value.ty.kind {
ty::Array(t, n) => match value.val {
ty::ConstKind::Value(ConstValue::ByRef { offset, alloc, .. }) => {
(alloc, offset, n.eval_usize(cx.tcx, cx.param_env), t)
ty::Array(t, n) => {
let n = n.eval_usize(cx.tcx, cx.param_env);
// Shortcut for `n == 0` where no matter what `alloc` and `offset` we produce,
// the result would be exactly what we early return here.
if n == 0 {
if ctor_wild_subpatterns.len() as u64 == 0 {
return Some(PatStack::from_slice(&[]));
} else {
return None;
}
}
_ => span_bug!(pat.span, "array pattern is {:?}", value,),
},
match value.val {
ty::ConstKind::Value(ConstValue::ByRef { offset, alloc, .. }) => {
(Cow::Borrowed(alloc), offset, n, t)
}
_ => span_bug!(pat.span, "array pattern is {:?}", value,),
}
}
ty::Slice(t) => {
match value.val {
ty::ConstKind::Value(ConstValue::Slice { data, start, end }) => {
(data, Size::from_bytes(start as u64), (end - start) as u64, t)
let offset = Size::from_bytes(start as u64);
let n = (end - start) as u64;
(Cow::Borrowed(data), offset, n, t)
}
ty::ConstKind::Value(ConstValue::ByRef { .. }) => {
// FIXME(oli-obk): implement `deref` for `ConstValue`
Expand Down
6 changes: 6 additions & 0 deletions src/librustc_mir/hair/pattern/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -993,6 +993,12 @@ pub fn compare_const_vals<'tcx>(
return fallback();
}

// Early return for equal constants (so e.g. references to ZSTs can be compared, even if they
// are just integer addresses).
if a.val == b.val {
return from_bool(true);
}

let a_bits = a.try_eval_bits(tcx, param_env, ty);
let b_bits = b.try_eval_bits(tcx, param_env, ty);

Expand Down
36 changes: 29 additions & 7 deletions src/librustc_mir/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ use rustc_macros::HashStable;
use syntax::source_map::{self, Span, DUMMY_SP};

use super::{
Immediate, MPlaceTy, Machine, MemPlace, Memory, Operand, Place, PlaceTy, ScalarMaybeUndef,
StackPopInfo,
Immediate, MPlaceTy, Machine, MemPlace, Memory, OpTy, Operand, Place, PlaceTy,
ScalarMaybeUndef, StackPopInfo,
};

pub struct InterpCx<'mir, 'tcx, M: Machine<'mir, 'tcx>> {
Expand Down Expand Up @@ -118,7 +118,7 @@ pub struct LocalState<'tcx, Tag = (), Id = AllocId> {
}

/// Current value of a local variable
#[derive(Clone, PartialEq, Eq, Debug, HashStable)] // Miri debug-prints these
#[derive(Copy, Clone, PartialEq, Eq, Debug, HashStable)] // Miri debug-prints these
pub enum LocalValue<Tag = (), Id = AllocId> {
/// This local is not currently alive, and cannot be used at all.
Dead,
Expand Down Expand Up @@ -743,7 +743,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// FIXME: should we tell the user that there was a local which was never written to?
if let LocalValue::Live(Operand::Indirect(MemPlace { ptr, .. })) = local {
trace!("deallocating local");
let ptr = ptr.to_ptr()?;
// All locals have a backing allocation, even if the allocation is empty
// due to the local having ZST type.
let ptr = ptr.assert_ptr();
if log_enabled!(::log::Level::Trace) {
self.memory.dump_alloc(ptr.alloc_id);
}
Expand All @@ -752,13 +754,33 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Ok(())
}

pub(super) fn const_eval(
&self,
gid: GlobalId<'tcx>,
) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
let val = if self.tcx.is_static(gid.instance.def_id()) {
self.tcx.const_eval_poly(gid.instance.def_id())?
} else if let Some(promoted) = gid.promoted {
self.tcx.const_eval_promoted(gid.instance, promoted)?
} else {
self.tcx.const_eval_instance(self.param_env, gid.instance, Some(self.tcx.span))?
};
// Even though `ecx.const_eval` is called from `eval_const_to_op` we can never have a
// recursion deeper than one level, because the `tcx.const_eval` above is guaranteed to not
// return `ConstValue::Unevaluated`, which is the only way that `eval_const_to_op` will call
// `ecx.const_eval`.
self.eval_const_to_op(val, None)
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
}

pub fn const_eval_raw(
&self,
gid: GlobalId<'tcx>,
) -> InterpResult<'tcx, MPlaceTy<'tcx, M::PointerTag>> {
// FIXME(oli-obk): make this check an assertion that it's not a static here
// FIXME(RalfJ, oli-obk): document that `Place::Static` can never be anything but a static
// and `ConstValue::Unevaluated` can never be a static
// For statics we pick `ParamEnv::reveal_all`, because statics don't have generics
// and thus don't care about the parameter environment. While we could just use
// `self.param_env`, that would mean we invoke the query to evaluate the static
// with different parameter environments, thus causing the static to be evaluated
// multiple times.
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
let param_env = if self.tcx.is_static(gid.instance.def_id()) {
ty::ParamEnv::reveal_all()
} else {
Expand Down
19 changes: 14 additions & 5 deletions src/librustc_mir/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,14 +187,21 @@ impl<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx
if let ty::Ref(_, referenced_ty, mutability) = ty.kind {
let value = self.ecx.read_immediate(mplace.into())?;
let mplace = self.ecx.ref_to_mplace(value)?;
// Handle trait object vtables
// Handle trait object vtables.
if let ty::Dynamic(..) =
self.ecx.tcx.struct_tail_erasing_lifetimes(referenced_ty, self.ecx.param_env).kind
{
if let Ok(vtable) = mplace.meta.unwrap().to_ptr() {
// explitly choose `Immutable` here, since vtables are immutable, even
// if the reference of the fat pointer is mutable
// Validation has already errored on an invalid vtable pointer so we can safely not
// do anything if this is not a real pointer.
if let Scalar::Ptr(vtable) = mplace.meta.unwrap() {
// Explicitly choose `Immutable` here, since vtables are immutable, even
// if the reference of the fat pointer is mutable.
self.intern_shallow(vtable.alloc_id, Mutability::Not, None)?;
} else {
self.ecx().tcx.sess.delay_span_bug(
syntax_pos::DUMMY_SP,
"vtables pointers cannot be integer pointers",
);
}
}
// Check if we have encountered this pointer+layout combination before.
Expand Down Expand Up @@ -280,7 +287,9 @@ pub fn intern_const_alloc_recursive<M: CompileTimeMachine<'mir, 'tcx>>(
ecx,
leftover_allocations,
base_intern_mode,
ret.ptr.to_ptr()?.alloc_id,
// The outermost allocation must exist, because we allocated it with
// `Memory::allocate`.
ret.ptr.assert_ptr().alloc_id,
base_mutability,
Some(ret.layout.ty),
)?;
Expand Down
7 changes: 3 additions & 4 deletions src/librustc_mir/interpret/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
use rustc::hir::def_id::DefId;
use rustc::mir::{
self,
interpret::{ConstValue, InterpResult, Scalar},
interpret::{ConstValue, GlobalId, InterpResult, Scalar},
BinOp,
};
use rustc::ty;
Expand Down Expand Up @@ -118,9 +118,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
| sym::size_of
| sym::type_id
| sym::type_name => {
let val =
self.tcx.const_eval_instance(self.param_env, instance, Some(self.tcx.span))?;
let val = self.eval_const_to_op(val, None)?;
let gid = GlobalId { instance, promoted: None };
let val = self.const_eval(gid)?;
self.copy_op(val, dest)?;
}

Expand Down
10 changes: 9 additions & 1 deletion src/librustc_mir/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,15 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
ty::ConstKind::Param(_) => throw_inval!(TooGeneric),
ty::ConstKind::Unevaluated(def_id, substs) => {
let instance = self.resolve(def_id, substs)?;
return Ok(OpTy::from(self.const_eval_raw(GlobalId { instance, promoted: None })?));
// We use `const_eval` here and `const_eval_raw` elsewhere in mir interpretation.
// The reason we use `const_eval_raw` everywhere else is to prevent cycles during
// validation, because validation automatically reads through any references, thus
// potentially requiring the current static to be evaluated again. This is not a
// problem here, because we are building an operand which means an actual read is
// happening.
// FIXME(oli-obk): eliminate all the `const_eval_raw` usages when we get rid of
// `StaticKind` once and for all.
return self.const_eval(GlobalId { instance, promoted: None });
}
ty::ConstKind::Infer(..)
| ty::ConstKind::Bound(..)
Expand Down
31 changes: 11 additions & 20 deletions src/librustc_mir/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -923,12 +923,14 @@ where
return self.copy_op(src, dest);
}
// We still require the sizes to match.
assert!(
src.layout.size == dest.layout.size,
"Size mismatch when transmuting!\nsrc: {:#?}\ndest: {:#?}",
src,
dest
);
if src.layout.size != dest.layout.size {
// FIXME: This should be an assert instead of an error, but if we transmute within an
// array length computation, `typeck` may not have yet been run and errored out. In fact
// most likey we *are* running `typeck` right now. Investigate whether we can bail out
// on `typeck_tables().has_errors` at all const eval entry points.
debug!("Size mismatch when transmuting!\nsrc: {:#?}\ndest: {:#?}", src, dest);
throw_unsup!(TransmuteSizeDiff(src.layout.ty, dest.layout.ty));
Copy link
Member

Choose a reason for hiding this comment

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

delay_span_bug? Or can typeck still succeed when hitting this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that we can't possibly continue evaluation here, there's no useful operation to do. So we need to error out. I'd rather fix this correctly in a follow up where I enforce that we don't even try to evaluate if we have typeck errors

}
// Unsized copies rely on interpreting `src.meta` with `dest.layout`, we want
// to avoid that here.
assert!(
Expand Down Expand Up @@ -974,31 +976,20 @@ where
let (mplace, size) = match place.place {
Place::Local { frame, local } => {
match self.stack[frame].locals[local].access_mut()? {
Ok(local_val) => {
Ok(&mut local_val) => {
// We need to make an allocation.
// FIXME: Consider not doing anything for a ZST, and just returning
// a fake pointer? Are we even called for ZST?

// We cannot hold on to the reference `local_val` while allocating,
// but we can hold on to the value in there.
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
let old_val =
if let LocalValue::Live(Operand::Immediate(value)) = *local_val {
Some(value)
} else {
None
};

// We need the layout of the local. We can NOT use the layout we got,
// that might e.g., be an inner field of a struct with `Scalar` layout,
// that has different alignment than the outer field.
// We also need to support unsized types, and hence cannot use `allocate`.
let local_layout = self.layout_of_local(&self.stack[frame], local, None)?;
// We also need to support unsized types, and hence cannot use `allocate`.
let (size, align) = self
.size_and_align_of(meta, local_layout)?
.expect("Cannot allocate for non-dyn-sized type");
let ptr = self.memory.allocate(size, align, MemoryKind::Stack);
let mplace = MemPlace { ptr: ptr.into(), align, meta };
if let Some(value) = old_val {
if let LocalValue::Live(Operand::Immediate(value)) = local_val {
// Preserve old value.
// We don't have to validate as we can assume the local
// was already valid for its type.
Expand Down
Loading