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.

Sign up for GitHub

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

Jump to bottom

Check whether locals are too large instead of whether accesses into them are too large #74821

Merged
merged 7 commits into from
Aug 7, 2020
Merged

Check whether locals are too large instead of whether accesses into them are too large #74821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c3454c0
Check whether locals are too large instead of whether accesses into t…
oli-obk Jul 27, 2020
b26a7d5
Stop propagating to locals that were marks as unpropagatable.
oli-obk Jul 27, 2020
9e21004
Update ui tests
oli-obk Jul 27, 2020
8d5f2bd
Add test ensuring that we don't propagate large arrays
oli-obk Jul 28, 2020
dc0408e
Update clippy ui test.
oli-obk Jul 29, 2020
f7a1e64
Update tests after rebase
oli-obk Jul 29, 2020
1864a97
Improve the diagnostics around misspelled mir dump filenames
oli-obk Jul 29, 2020
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
130 changes: 70 additions & 60 deletions src/librustc_mir/transform/const_prop.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ use crate::interpret::{
};
use crate::transform::{MirPass, MirSource};

/// The maximum number of bytes that we'll allocate space for a return value.
/// The maximum number of bytes that we'll allocate space for a local or the return value.
/// Needed for #66397, because otherwise we eval into large places and that can cause OOM or just
/// Severely regress performance.
const MAX_ALLOC_LIMIT: u64 = 1024;

/// Macro for machine-specific `InterpError` without allocation.
Expand Down Expand Up @@ -155,14 +157,19 @@ struct ConstPropMachine<'mir, 'tcx> {
written_only_inside_own_block_locals: FxHashSet<Local>,
/// Locals that need to be cleared after every block terminates.
only_propagate_inside_block_locals: BitSet<Local>,
can_const_prop: IndexVec<Local, ConstPropMode>,
}

impl<'mir, 'tcx> ConstPropMachine<'mir, 'tcx> {
fn new(only_propagate_inside_block_locals: BitSet<Local>) -> Self {
fn new(
only_propagate_inside_block_locals: BitSet<Local>,
can_const_prop: IndexVec<Local, ConstPropMode>,
) -> Self {
Self {
stack: Vec::new(),
written_only_inside_own_block_locals: Default::default(),
only_propagate_inside_block_locals,
can_const_prop,
}
}
}
Expand Down Expand Up @@ -241,6 +248,9 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>
local: Local,
) -> InterpResult<'tcx, Result<&'a mut LocalValue<Self::PointerTag>, MemPlace<Self::PointerTag>>>
{
if ecx.machine.can_const_prop[local] == ConstPropMode::NoPropagation {
throw_machine_stop_str!("tried to write to a local that is marked as not propagatable")
}
if frame == 0 && ecx.machine.only_propagate_inside_block_locals.contains(local) {
ecx.machine.written_only_inside_own_block_locals.insert(local);
}
Expand Down Expand Up @@ -285,7 +295,6 @@ impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine<'mir, 'tcx>
struct ConstPropagator<'mir, 'tcx> {
ecx: InterpCx<'mir, 'tcx, ConstPropMachine<'mir, 'tcx>>,
tcx: TyCtxt<'tcx>,
can_const_prop: IndexVec<Local, ConstPropMode>,
param_env: ParamEnv<'tcx>,
// FIXME(eddyb) avoid cloning these two fields more than once,
// by accessing them through `ecx` instead.
Expand Down Expand Up @@ -331,7 +340,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
let param_env = tcx.param_env(def_id).with_reveal_all();

let span = tcx.def_span(def_id);
let can_const_prop = CanConstProp::check(body);
// FIXME: `CanConstProp::check` computes the layout of all locals, return those layouts
// so we can write them to `ecx.frame_mut().locals.layout, reducing the duplication in
// `layout_of` query invocations.
let can_const_prop = CanConstProp::check(tcx, param_env, body);
let mut only_propagate_inside_block_locals = BitSet::new_empty(can_const_prop.len());
for (l, mode) in can_const_prop.iter_enumerated() {
if *mode == ConstPropMode::OnlyInsideOwnBlock {
Expand All @@ -342,7 +354,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
tcx,
span,
param_env,
ConstPropMachine::new(only_propagate_inside_block_locals),
ConstPropMachine::new(only_propagate_inside_block_locals, can_const_prop),
(),
);

Expand All @@ -368,7 +380,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
ecx,
tcx,
param_env,
can_const_prop,
// FIXME(eddyb) avoid cloning these two fields more than once,
// by accessing them through `ecx` instead.
source_scopes: body.source_scopes.clone(),
Expand Down Expand Up @@ -612,15 +623,9 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
fn const_prop(
&mut self,
rvalue: &Rvalue<'tcx>,
place_layout: TyAndLayout<'tcx>,
source_info: SourceInfo,
place: Place<'tcx>,
) -> Option<()> {
// #66397: Don't try to eval into large places as that can cause an OOM
if place_layout.size >= Size::from_bytes(MAX_ALLOC_LIMIT) {
return None;
}

// Perform any special handling for specific Rvalue types.
// Generally, checks here fall into one of two categories:
// 1. Additional checking to provide useful lints to the user
Expand Down Expand Up @@ -893,7 +898,11 @@ struct CanConstProp {

impl CanConstProp {
/// Returns true if `local` can be propagated
fn check(body: &Body<'_>) -> IndexVec<Local, ConstPropMode> {
fn check(
tcx: TyCtxt<'tcx>,
param_env: ParamEnv<'tcx>,
body: &Body<'tcx>,
) -> IndexVec<Local, ConstPropMode> {
let mut cpv = CanConstProp {
can_const_prop: IndexVec::from_elem(ConstPropMode::FullConstProp, &body.local_decls),
found_assignment: BitSet::new_empty(body.local_decls.len()),
Expand All @@ -903,6 +912,16 @@ impl CanConstProp {
),
};
for (local, val) in cpv.can_const_prop.iter_enumerated_mut() {
let ty = body.local_decls[local].ty;
match tcx.layout_of(param_env.and(ty)) {
Ok(layout) if layout.size < Size::from_bytes(MAX_ALLOC_LIMIT) => {}
// Either the layout fails to compute, then we can't use this local anyway
// or the local is too large, then we don't want to.
_ => {
*val = ConstPropMode::NoPropagation;
continue;
}
}
// Cannot use args at all
// Cannot use locals because if x < y { y - x } else { x - y } would
// lint for x != y
Expand Down Expand Up @@ -1018,61 +1037,52 @@ impl<'mir, 'tcx> MutVisitor<'tcx> for ConstPropagator<'mir, 'tcx> {
let source_info = statement.source_info;
self.source_info = Some(source_info);
if let StatementKind::Assign(box (place, ref mut rval)) = statement.kind {
let place_ty: Ty<'tcx> = place.ty(&self.local_decls, self.tcx).ty;
if let Ok(place_layout) = self.tcx.layout_of(self.param_env.and(place_ty)) {
let can_const_prop = self.can_const_prop[place.local];
if let Some(()) = self.const_prop(rval, place_layout, source_info, place) {
// This will return None if the above `const_prop` invocation only "wrote" a
// type whose creation requires no write. E.g. a generator whose initial state
// consists solely of uninitialized memory (so it doesn't capture any locals).
if let Some(value) = self.get_const(place) {
if self.should_const_prop(value) {
trace!("replacing {:?} with {:?}", rval, value);
self.replace_with_const(rval, value, source_info);
if can_const_prop == ConstPropMode::FullConstProp
|| can_const_prop == ConstPropMode::OnlyInsideOwnBlock
{
trace!("propagated into {:?}", place);
}
let can_const_prop = self.ecx.machine.can_const_prop[place.local];
if let Some(()) = self.const_prop(rval, source_info, place) {
// This will return None if the above `const_prop` invocation only "wrote" a
// type whose creation requires no write. E.g. a generator whose initial state
// consists solely of uninitialized memory (so it doesn't capture any locals).
if let Some(value) = self.get_const(place) {
if self.should_const_prop(value) {
trace!("replacing {:?} with {:?}", rval, value);
self.replace_with_const(rval, value, source_info);
if can_const_prop == ConstPropMode::FullConstProp
|| can_const_prop == ConstPropMode::OnlyInsideOwnBlock
{
trace!("propagated into {:?}", place);
}
}
match can_const_prop {
ConstPropMode::OnlyInsideOwnBlock => {
trace!(
"found local restricted to its block. \
}
match can_const_prop {
ConstPropMode::OnlyInsideOwnBlock => {
trace!(
"found local restricted to its block. \
Will remove it from const-prop after block is finished. Local: {:?}",
place.local
);
}
ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => {
trace!("can't propagate into {:?}", place);
if place.local != RETURN_PLACE {
Self::remove_const(&mut self.ecx, place.local);
}
place.local
);
}
ConstPropMode::OnlyPropagateInto | ConstPropMode::NoPropagation => {
trace!("can't propagate into {:?}", place);
if place.local != RETURN_PLACE {
Self::remove_const(&mut self.ecx, place.local);
}
ConstPropMode::FullConstProp => {}
}
} else {
// Const prop failed, so erase the destination, ensuring that whatever happens
// from here on, does not know about the previous value.
// This is important in case we have
// ```rust
// let mut x = 42;
// x = SOME_MUTABLE_STATIC;
// // x must now be undefined
// ```
// FIXME: we overzealously erase the entire local, because that's easier to
// implement.
trace!(
"propagation into {:?} failed.
Nuking the entire site from orbit, it's the only way to be sure",
place,
);
Self::remove_const(&mut self.ecx, place.local);
ConstPropMode::FullConstProp => {}
}
} else {
// Const prop failed, so erase the destination, ensuring that whatever happens
// from here on, does not know about the previous value.
// This is important in case we have
// ```rust
// let mut x = 42;
// x = SOME_MUTABLE_STATIC;
// // x must now be undefined
// ```
// FIXME: we overzealously erase the entire local, because that's easier to
// implement.
trace!(
"cannot propagate into {:?}, because the type of the local is generic.",
"propagation into {:?} failed.
Nuking the entire site from orbit, it's the only way to be sure",
place,
);
Self::remove_const(&mut self.ecx, place.local);
Expand Down
85 changes: 85 additions & 0 deletions src/test/mir-opt/const_prop/large_array_index.main.ConstProp.diff.32bit
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
- // MIR for `main` before ConstProp
+ // MIR for `main` after ConstProp

fn main() -> () {
let mut _0: (); // return place in scope 0 at $DIR/large_array_index.rs:4:11: 4:11
let _1: u8; // in scope 0 at $DIR/large_array_index.rs:6:9: 6:10
let mut _2: [u8; 5000]; // in scope 0 at $DIR/large_array_index.rs:6:17: 6:29
let _3: usize; // in scope 0 at $DIR/large_array_index.rs:6:30: 6:31
let mut _4: usize; // in scope 0 at $DIR/large_array_index.rs:6:17: 6:32
let mut _5: bool; // in scope 0 at $DIR/large_array_index.rs:6:17: 6:32
scope 1 {
debug x => _1; // in scope 1 at $DIR/large_array_index.rs:6:9: 6:10
}

bb0: {
StorageLive(_1); // scope 0 at $DIR/large_array_index.rs:6:9: 6:10
StorageLive(_2); // scope 0 at $DIR/large_array_index.rs:6:17: 6:29
_2 = [const 0_u8; 5000]; // scope 0 at $DIR/large_array_index.rs:6:17: 6:29
// ty::Const
// + ty: u8
// + val: Value(Scalar(0x00))
// mir::Constant
// + span: $DIR/large_array_index.rs:6:18: 6:22
// + literal: Const { ty: u8, val: Value(Scalar(0x00)) }
StorageLive(_3); // scope 0 at $DIR/large_array_index.rs:6:30: 6:31
_3 = const 2_usize; // scope 0 at $DIR/large_array_index.rs:6:30: 6:31
// ty::Const
// + ty: usize
// + val: Value(Scalar(0x00000002))
// mir::Constant
// + span: $DIR/large_array_index.rs:6:30: 6:31
// + literal: Const { ty: usize, val: Value(Scalar(0x00000002)) }
_4 = const 5000_usize; // scope 0 at $DIR/large_array_index.rs:6:17: 6:32
// ty::Const
// + ty: usize
// + val: Value(Scalar(0x00001388))
// mir::Constant
// + span: $DIR/large_array_index.rs:6:17: 6:32
// + literal: Const { ty: usize, val: Value(Scalar(0x00001388)) }
- _5 = Lt(_3, _4); // scope 0 at $DIR/large_array_index.rs:6:17: 6:32
- assert(move _5, "index out of bounds: the len is {} but the index is {}", move _4, _3) -> bb1; // scope 0 at $DIR/large_array_index.rs:6:17: 6:32
+ _5 = const true; // scope 0 at $DIR/large_array_index.rs:6:17: 6:32
+ // ty::Const
+ // + ty: bool
+ // + val: Value(Scalar(0x01))
+ // mir::Constant
+ // + span: $DIR/large_array_index.rs:6:17: 6:32
+ // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
+ assert(const true, "index out of bounds: the len is {} but the index is {}", const 5000_usize, const 2_usize) -> bb1; // scope 0 at $DIR/large_array_index.rs:6:17: 6:32
+ // ty::Const
+ // + ty: bool
+ // + val: Value(Scalar(0x01))
+ // mir::Constant
+ // + span: $DIR/large_array_index.rs:6:17: 6:32
+ // + literal: Const { ty: bool, val: Value(Scalar(0x01)) }
+ // ty::Const
+ // + ty: usize
+ // + val: Value(Scalar(0x00001388))
+ // mir::Constant
+ // + span: $DIR/large_array_index.rs:6:17: 6:32
+ // + literal: Const { ty: usize, val: Value(Scalar(0x00001388)) }
+ // ty::Const
+ // + ty: usize
+ // + val: Value(Scalar(0x00000002))
+ // mir::Constant
+ // + span: $DIR/large_array_index.rs:6:17: 6:32
+ // + literal: Const { ty: usize, val: Value(Scalar(0x00000002)) }
}

bb1: {
_1 = _2[_3]; // scope 0 at $DIR/large_array_index.rs:6:17: 6:32
StorageDead(_3); // scope 0 at $DIR/large_array_index.rs:6:32: 6:33
StorageDead(_2); // scope 0 at $DIR/large_array_index.rs:6:32: 6:33
_0 = const (); // scope 0 at $DIR/large_array_index.rs:4:11: 7:2
// ty::Const
// + ty: ()
// + val: Value(Scalar(<ZST>))
// mir::Constant
// + span: $DIR/large_array_index.rs:4:11: 7:2
// + literal: Const { ty: (), val: Value(Scalar(<ZST>)) }
StorageDead(_1); // scope 0 at $DIR/large_array_index.rs:7:1: 7:2
return; // scope 0 at $DIR/large_array_index.rs:7:2: 7:2
}
}

Loading