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

[const prop] Fix "alloc id without corresponding allocation" ICE #66677

Merged
merged 4 commits into from
Nov 27, 2019
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 1 addition & 1 deletion src/librustc/mir/interpret/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ impl<Tag> From<Pointer<Tag>> for Scalar<Tag> {
}
}

#[derive(Clone, Copy, Eq, PartialEq, RustcEncodable, RustcDecodable, HashStable)]
#[derive(Clone, Copy, Eq, PartialEq, RustcEncodable, RustcDecodable, HashStable, Hash)]
pub enum ScalarMaybeUndef<Tag = (), Id = AllocId> {
Scalar(Scalar<Tag, Id>),
Undef,
Expand Down
106 changes: 83 additions & 23 deletions src/librustc_mir/interpret/intern.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,34 @@
//! 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, self};
use rustc::mir::interpret::{InterpResult, ErrorHandled};
use rustc::hir;
use super::validity::RefTracking;
use rustc_data_structures::fx::FxHashSet;
use rustc::hir;
use rustc::mir::interpret::{ErrorHandled, InterpResult};
use rustc::ty::{self, Ty};
use rustc_data_structures::fx::{FxHashMap, FxHashSet};

use syntax::ast::Mutability;

use super::{
ValueVisitor, MemoryKind, AllocId, MPlaceTy, Scalar,
AllocId, Allocation, InterpCx, Machine, MemoryKind, MPlaceTy, Scalar, ValueVisitor,
};
use crate::const_eval::{CompileTimeInterpreter, CompileTimeEvalContext};

struct InternVisitor<'rt, 'mir, 'tcx> {
struct InternVisitor<'rt, 'mir, 'tcx, M>
where
M: Machine<
'mir,
'tcx,
MemoryKinds = !,
PointerTag = (),
ExtraFnVal = !,
FrameExtra = (),
MemoryExtra = (),
AllocExtra = (),
MemoryMap = FxHashMap<AllocId, (MemoryKind<!>, Allocation)>,
>,
{
/// The ectx from which we intern.
ecx: &'rt mut CompileTimeEvalContext<'mir, 'tcx>,
ecx: &'rt mut InterpCx<'mir, 'tcx, M>,
/// 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
Expand Down Expand Up @@ -58,18 +70,28 @@ struct IsStaticOrFn;
/// `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>,
fn intern_shallow<'rt, 'mir, 'tcx, M>(
ecx: &'rt mut InterpCx<'mir, 'tcx, M>,
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,
);
) -> InterpResult<'tcx, Option<IsStaticOrFn>>
where
M: Machine<
'mir,
'tcx,
MemoryKinds = !,
PointerTag = (),
ExtraFnVal = !,
FrameExtra = (),
MemoryExtra = (),
AllocExtra = (),
MemoryMap = FxHashMap<AllocId, (MemoryKind<!>, Allocation)>,
>,
{
trace!("InternVisitor::intern {:?} with {:?}", alloc_id, mutability,);
// remove allocation
let tcx = ecx.tcx;
let (kind, mut alloc) = match ecx.memory.alloc_map.remove(&alloc_id) {
Expand Down Expand Up @@ -130,7 +152,20 @@ fn intern_shallow<'rt, 'mir, 'tcx>(
Ok(None)
}

impl<'rt, 'mir, 'tcx> InternVisitor<'rt, 'mir, 'tcx> {
impl<'rt, 'mir, 'tcx, M> InternVisitor<'rt, 'mir, 'tcx, M>
where
M: Machine<
'mir,
'tcx,
MemoryKinds = !,
PointerTag = (),
ExtraFnVal = !,
FrameExtra = (),
MemoryExtra = (),
AllocExtra = (),
MemoryMap = FxHashMap<AllocId, (MemoryKind<!>, Allocation)>,
>,
{
fn intern_shallow(
&mut self,
alloc_id: AllocId,
Expand All @@ -148,15 +183,27 @@ impl<'rt, 'mir, 'tcx> InternVisitor<'rt, 'mir, 'tcx> {
}
}

impl<'rt, 'mir, 'tcx>
ValueVisitor<'mir, 'tcx, CompileTimeInterpreter<'mir, 'tcx>>
impl<'rt, 'mir, 'tcx, M>
ValueVisitor<'mir, 'tcx, M>
for
InternVisitor<'rt, 'mir, 'tcx>
InternVisitor<'rt, 'mir, 'tcx, M>
where
M: Machine<
'mir,
'tcx,
MemoryKinds = !,
PointerTag = (),
ExtraFnVal = !,
FrameExtra = (),
MemoryExtra = (),
AllocExtra = (),
MemoryMap = FxHashMap<AllocId, (MemoryKind<!>, Allocation)>,
>,
{
type V = MPlaceTy<'tcx>;

#[inline(always)]
fn ecx(&self) -> &CompileTimeEvalContext<'mir, 'tcx> {
fn ecx(&self) -> &InterpCx<'mir, 'tcx, M> {
&self.ecx
}

Expand Down Expand Up @@ -265,12 +312,25 @@ for
}
}

pub fn intern_const_alloc_recursive(
ecx: &mut CompileTimeEvalContext<'mir, 'tcx>,
pub fn intern_const_alloc_recursive<M>(
ecx: &mut InterpCx<'mir, 'tcx, M>,
// The `mutability` of the place, ignoring the type.
place_mut: Option<hir::Mutability>,
ret: MPlaceTy<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx>
where
M: Machine<
wesleywiser marked this conversation as resolved.
Show resolved Hide resolved
'mir,
'tcx,
MemoryKinds = !,
PointerTag = (),
ExtraFnVal = !,
FrameExtra = (),
MemoryExtra = (),
AllocExtra = (),
MemoryMap = FxHashMap<AllocId, (MemoryKind<!>, Allocation)>,
>,
{
let tcx = ecx.tcx;
let (base_mutability, base_intern_mode) = match place_mut {
Some(hir::Mutability::Immutable) => (Mutability::Immutable, InternMode::Static),
Expand Down
6 changes: 3 additions & 3 deletions src/librustc_mir/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use rustc_macros::HashStable;
/// operations and fat pointers. This idea was taken from rustc's codegen.
/// In particular, thanks to `ScalarPair`, arithmetic operations and casts can be entirely
/// defined on `Immediate`, and do not have to work with a `Place`.
#[derive(Copy, Clone, Debug, PartialEq, Eq, HashStable)]
#[derive(Copy, Clone, Debug, PartialEq, Eq, HashStable, Hash)]
pub enum Immediate<Tag=(), Id=AllocId> {
Scalar(ScalarMaybeUndef<Tag, Id>),
ScalarPair(ScalarMaybeUndef<Tag, Id>, ScalarMaybeUndef<Tag, Id>),
Expand Down Expand Up @@ -104,7 +104,7 @@ impl<'tcx, Tag> ::std::ops::Deref for ImmTy<'tcx, Tag> {
/// An `Operand` is the result of computing a `mir::Operand`. It can be immediate,
/// or still in memory. The latter is an optimization, to delay reading that chunk of
/// memory and to avoid having to store arbitrary-sized data here.
#[derive(Copy, Clone, Debug, PartialEq, Eq, HashStable)]
#[derive(Copy, Clone, Debug, PartialEq, Eq, HashStable, Hash)]
pub enum Operand<Tag=(), Id=AllocId> {
Immediate(Immediate<Tag, Id>),
Indirect(MemPlace<Tag, Id>),
Expand Down Expand Up @@ -134,7 +134,7 @@ impl<Tag> Operand<Tag> {
}
}

#[derive(Copy, Clone, Debug, PartialEq)]
#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
pub struct OpTy<'tcx, Tag=()> {
op: Operand<Tag>, // Keep this private, it helps enforce invariants
pub layout: TyLayout<'tcx>,
Expand Down
21 changes: 16 additions & 5 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use crate::interpret::{
self, InterpCx, ScalarMaybeUndef, Immediate, OpTy,
StackPopCleanup, LocalValue, LocalState, AllocId, Frame,
Allocation, MemoryKind, ImmTy, Pointer, Memory, PlaceTy,
Operand as InterpOperand,
Operand as InterpOperand, intern_const_alloc_recursive,
};
use crate::const_eval::error_to_const_error;
use crate::transform::{MirPass, MirSource};
Expand Down Expand Up @@ -649,20 +649,31 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
}

fn should_const_prop(&mut self, op: OpTy<'tcx>) -> bool {
if self.tcx.sess.opts.debugging_opts.mir_opt_level >= 2 {
return true;
} else if self.tcx.sess.opts.debugging_opts.mir_opt_level == 0 {
if self.tcx.sess.opts.debugging_opts.mir_opt_level == 0 {
return false;
}

match *op {
let is_scalar = match *op {
interpret::Operand::Immediate(Immediate::Scalar(ScalarMaybeUndef::Scalar(s))) =>
s.is_bits(),
interpret::Operand::Immediate(Immediate::ScalarPair(ScalarMaybeUndef::Scalar(l),
ScalarMaybeUndef::Scalar(r))) =>
l.is_bits() && r.is_bits(),
_ => false
};

if let interpret::Operand::Indirect(_) = *op {
wesleywiser marked this conversation as resolved.
Show resolved Hide resolved
if self.tcx.sess.opts.debugging_opts.mir_opt_level >= 2 {
intern_const_alloc_recursive(
&mut self.ecx,
None,
op.assert_mem_place()
).expect("failed to intern alloc");
return true;
}
}

return is_scalar;
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/test/mir-opt/const_prop/const_prop_fails_gracefully.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ fn main() {
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _4 = const Scalar(AllocId(1).0x0) : &i32;
// _3 = const Scalar(AllocId(1).0x0) : &i32;
// _2 = const Value(Scalar(AllocId(1).0x0)) : *const i32;
// _4 = const main::FOO;
// _3 = _4;
// _2 = move _3 as *const i32 (Misc);
// ...
// _1 = move _2 as usize (Misc);
// ...
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/const_prop/ref_deref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ fn main() {
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _2 = const Scalar(AllocId(0).0x0) : &i32;
// _2 = &(promoted[0]: i32);
// _1 = const 4i32;
// ...
// }
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/const_prop/reify_fn_ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ fn main() {
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _3 = const main;
// _3 = const main as fn() (Pointer(ReifyFnPointer));
// _2 = move _3 as usize (Misc);
// ...
// _1 = move _2 as *const fn() (Misc);
Expand Down
4 changes: 2 additions & 2 deletions src/test/mir-opt/const_prop/slice_len.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ fn main() {
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _4 = const Scalar(AllocId(0).0x0) : &[u32; 3];
// _3 = const Scalar(AllocId(0).0x0) : &[u32; 3];
// _4 = &(promoted[0]: [u32; 3]);
// _3 = _4;
// _2 = move _3 as &[u32] (Pointer(Unsize));
// ...
// _6 = const 1usize;
Expand Down
24 changes: 24 additions & 0 deletions src/test/ui/consts/issue-66345.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// run-pass
// compile-flags: -Z mir-opt-level=3

// Checks that the compiler does not ICE when passing references to field of by-value struct
// with -Z mir-opt-level=3

fn do_nothing(_: &()) {}

pub struct Foo {
bar: (),
}

pub fn by_value_1(foo: Foo) {
do_nothing(&foo.bar);
}

pub fn by_value_2<T>(foo: Foo) {
do_nothing(&foo.bar);
}

fn main() {
by_value_1(Foo { bar: () });
by_value_2::<()>(Foo { bar: () });
}