From c8f7e18ceb29d78314ceef16979ff6ce5356f4c2 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sat, 14 Sep 2019 09:22:07 -0400 Subject: [PATCH 1/7] [const-prop] Handle MIR Rvalue::Repeat --- src/librustc_mir/transform/const_prop.rs | 2 +- src/test/mir-opt/const_prop/repeat.rs | 37 ++++++++++++++++++++++++ src/test/ui/consts/const-prop-ice.rs | 1 + src/test/ui/consts/const-prop-ice.stderr | 8 ++++- 4 files changed, 46 insertions(+), 2 deletions(-) create mode 100644 src/test/mir-opt/const_prop/repeat.rs diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 49ac1de8fef64..77943e9acd527 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -436,13 +436,13 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // if this isn't a supported operation, then return None match rvalue { - Rvalue::Repeat(..) | Rvalue::Aggregate(..) | Rvalue::NullaryOp(NullOp::Box, _) | Rvalue::Discriminant(..) => return None, Rvalue::Use(_) | Rvalue::Len(_) | + Rvalue::Repeat(..) | Rvalue::Cast(..) | Rvalue::NullaryOp(..) | Rvalue::CheckedBinaryOp(..) | diff --git a/src/test/mir-opt/const_prop/repeat.rs b/src/test/mir-opt/const_prop/repeat.rs new file mode 100644 index 0000000000000..fb091ad2a3d53 --- /dev/null +++ b/src/test/mir-opt/const_prop/repeat.rs @@ -0,0 +1,37 @@ +// compile-flags: -O + +fn main() { + let x: u32 = [42; 8][2] + 0; +} + +// END RUST SOURCE +// START rustc.main.ConstProp.before.mir +// bb0: { +// ... +// _3 = [const 42u32; 8]; +// ... +// _4 = const 2usize; +// _5 = const 8usize; +// _6 = Lt(_4, _5); +// assert(move _6, "index out of bounds: the len is move _5 but the index is _4") -> bb1; +// } +// bb1: { +// _2 = _3[_4]; +// _1 = Add(move _2, const 0u32); +// ... +// return; +// } +// END rustc.main.ConstProp.before.mir +// START rustc.main.ConstProp.after.mir +// bb0: { +// ... +// _6 = const true; +// assert(const true, "index out of bounds: the len is move _5 but the index is _4") -> bb1; +// } +// bb1: { +// _2 = const 42u32; +// _1 = Add(move _2, const 0u32); +// ... +// return; +// } +// END rustc.main.ConstProp.after.mir diff --git a/src/test/ui/consts/const-prop-ice.rs b/src/test/ui/consts/const-prop-ice.rs index 13309f978b672..48c4b7da942e4 100644 --- a/src/test/ui/consts/const-prop-ice.rs +++ b/src/test/ui/consts/const-prop-ice.rs @@ -1,3 +1,4 @@ fn main() { [0; 3][3u64 as usize]; //~ ERROR the len is 3 but the index is 3 + //~| ERROR this expression will panic at runtime } diff --git a/src/test/ui/consts/const-prop-ice.stderr b/src/test/ui/consts/const-prop-ice.stderr index 4b3880198bf2d..8ecc6f4bc6b12 100644 --- a/src/test/ui/consts/const-prop-ice.stderr +++ b/src/test/ui/consts/const-prop-ice.stderr @@ -6,5 +6,11 @@ LL | [0; 3][3u64 as usize]; | = note: `#[deny(const_err)]` on by default -error: aborting due to previous error +error: this expression will panic at runtime + --> $DIR/const-prop-ice.rs:2:5 + | +LL | [0; 3][3u64 as usize]; + | ^^^^^^^^^^^^^^^^^^^^^ index out of bounds: the len is 3 but the index is 3 + +error: aborting due to 2 previous errors From a2e3ed5c054b544df6ceeb9e612d39af819f4aae Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sat, 14 Sep 2019 14:11:31 -0400 Subject: [PATCH 2/7] [const-prop] Handle MIR Rvalue::Aggregates --- src/librustc_mir/transform/const_prop.rs | 8 +++++- src/test/compile-fail/consts/const-err3.rs | 1 + src/test/mir-opt/const_prop/aggregate.rs | 25 +++++++++++++++++++ src/test/run-fail/overflowing-rsh-5.rs | 1 + src/test/run-fail/overflowing-rsh-6.rs | 1 + src/test/ui/consts/const-err2.rs | 1 + src/test/ui/consts/const-err2.stderr | 8 +++++- src/test/ui/consts/const-err3.rs | 1 + src/test/ui/consts/const-err3.stderr | 8 +++++- src/test/ui/issues/issue-54348.rs | 2 ++ src/test/ui/issues/issue-54348.stderr | 16 ++++++++++-- src/test/ui/lint/lint-exceeding-bitshifts2.rs | 2 +- .../ui/lint/lint-exceeding-bitshifts2.stderr | 8 +++++- 13 files changed, 75 insertions(+), 7 deletions(-) create mode 100644 src/test/mir-opt/const_prop/aggregate.rs diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 77943e9acd527..2119ee1b598ef 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -436,13 +436,13 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // if this isn't a supported operation, then return None match rvalue { - Rvalue::Aggregate(..) | Rvalue::NullaryOp(NullOp::Box, _) | Rvalue::Discriminant(..) => return None, Rvalue::Use(_) | Rvalue::Len(_) | Rvalue::Repeat(..) | + Rvalue::Aggregate(..) | Rvalue::Cast(..) | Rvalue::NullaryOp(..) | Rvalue::CheckedBinaryOp(..) | @@ -535,6 +535,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { return None; } } + } else if let Rvalue::Aggregate(_, operands) = rvalue { + // FIXME(wesleywiser): const eval will turn this into a `const Scalar()` that + // `SimplifyLocals` doesn't know it can remove. + if operands.len() == 0 { + return None; + } } self.use_ecx(source_info, |this| { diff --git a/src/test/compile-fail/consts/const-err3.rs b/src/test/compile-fail/consts/const-err3.rs index fc10824f0c03c..add4eef13c784 100644 --- a/src/test/compile-fail/consts/const-err3.rs +++ b/src/test/compile-fail/consts/const-err3.rs @@ -14,6 +14,7 @@ fn main() { //~^ ERROR const_err let _e = [5u8][1]; //~^ ERROR const_err + //~| ERROR this expression will panic at runtime black_box(b); black_box(c); black_box(d); diff --git a/src/test/mir-opt/const_prop/aggregate.rs b/src/test/mir-opt/const_prop/aggregate.rs new file mode 100644 index 0000000000000..0937d37be6b6e --- /dev/null +++ b/src/test/mir-opt/const_prop/aggregate.rs @@ -0,0 +1,25 @@ +// compile-flags: -O + +fn main() { + let x = (0, 1, 2).1 + 0; +} + +// END RUST SOURCE +// START rustc.main.ConstProp.before.mir +// bb0: { +// ... +// _3 = (const 0i32, const 1i32, const 2i32); +// _2 = (_3.1: i32); +// _1 = Add(move _2, const 0i32); +// ... +// } +// END rustc.main.ConstProp.before.mir +// START rustc.main.ConstProp.after.mir +// bb0: { +// ... +// _3 = (const 0i32, const 1i32, const 2i32); +// _2 = const 1i32; +// _1 = Add(move _2, const 0i32); +// ... +// } +// END rustc.main.ConstProp.after.mir diff --git a/src/test/run-fail/overflowing-rsh-5.rs b/src/test/run-fail/overflowing-rsh-5.rs index 793f495240d57..58dfc5710ae4e 100644 --- a/src/test/run-fail/overflowing-rsh-5.rs +++ b/src/test/run-fail/overflowing-rsh-5.rs @@ -2,6 +2,7 @@ // compile-flags: -C debug-assertions #![warn(exceeding_bitshifts)] +#![warn(const_err)] fn main() { let _n = 1i64 >> [64][0]; diff --git a/src/test/run-fail/overflowing-rsh-6.rs b/src/test/run-fail/overflowing-rsh-6.rs index d6b2f8dc9f9af..c2fec5e4860af 100644 --- a/src/test/run-fail/overflowing-rsh-6.rs +++ b/src/test/run-fail/overflowing-rsh-6.rs @@ -2,6 +2,7 @@ // compile-flags: -C debug-assertions #![warn(exceeding_bitshifts)] +#![warn(const_err)] #![feature(const_indexing)] fn main() { diff --git a/src/test/ui/consts/const-err2.rs b/src/test/ui/consts/const-err2.rs index ecbcc2a4b496f..e5ee90fc9f11f 100644 --- a/src/test/ui/consts/const-err2.rs +++ b/src/test/ui/consts/const-err2.rs @@ -23,6 +23,7 @@ fn main() { //~^ ERROR const_err let _e = [5u8][1]; //~^ ERROR index out of bounds + //~| ERROR this expression will panic at runtime black_box(a); black_box(b); black_box(c); diff --git a/src/test/ui/consts/const-err2.stderr b/src/test/ui/consts/const-err2.stderr index 1d84d44dc27b3..0a09a7213dabc 100644 --- a/src/test/ui/consts/const-err2.stderr +++ b/src/test/ui/consts/const-err2.stderr @@ -34,5 +34,11 @@ error: index out of bounds: the len is 1 but the index is 1 LL | let _e = [5u8][1]; | ^^^^^^^^ -error: aborting due to 5 previous errors +error: this expression will panic at runtime + --> $DIR/const-err2.rs:24:14 + | +LL | let _e = [5u8][1]; + | ^^^^^^^^ index out of bounds: the len is 1 but the index is 1 + +error: aborting due to 6 previous errors diff --git a/src/test/ui/consts/const-err3.rs b/src/test/ui/consts/const-err3.rs index a9cf04cda7a5a..89373f99f75c2 100644 --- a/src/test/ui/consts/const-err3.rs +++ b/src/test/ui/consts/const-err3.rs @@ -23,6 +23,7 @@ fn main() { //~^ ERROR const_err let _e = [5u8][1]; //~^ ERROR const_err + //~| ERROR this expression will panic at runtime black_box(a); black_box(b); black_box(c); diff --git a/src/test/ui/consts/const-err3.stderr b/src/test/ui/consts/const-err3.stderr index 0602707be7040..42de247c8f7e0 100644 --- a/src/test/ui/consts/const-err3.stderr +++ b/src/test/ui/consts/const-err3.stderr @@ -34,5 +34,11 @@ error: index out of bounds: the len is 1 but the index is 1 LL | let _e = [5u8][1]; | ^^^^^^^^ -error: aborting due to 5 previous errors +error: this expression will panic at runtime + --> $DIR/const-err3.rs:24:14 + | +LL | let _e = [5u8][1]; + | ^^^^^^^^ index out of bounds: the len is 1 but the index is 1 + +error: aborting due to 6 previous errors diff --git a/src/test/ui/issues/issue-54348.rs b/src/test/ui/issues/issue-54348.rs index 68d838054776e..e7221e2cbb1e1 100644 --- a/src/test/ui/issues/issue-54348.rs +++ b/src/test/ui/issues/issue-54348.rs @@ -1,5 +1,7 @@ fn main() { [1][0u64 as usize]; [1][1.5 as usize]; //~ ERROR index out of bounds + //~| ERROR this expression will panic at runtime [1][1u64 as usize]; //~ ERROR index out of bounds + //~| ERROR this expression will panic at runtime } diff --git a/src/test/ui/issues/issue-54348.stderr b/src/test/ui/issues/issue-54348.stderr index fa77bd6fd7797..79320ef4f31c7 100644 --- a/src/test/ui/issues/issue-54348.stderr +++ b/src/test/ui/issues/issue-54348.stderr @@ -6,11 +6,23 @@ LL | [1][1.5 as usize]; | = note: `#[deny(const_err)]` on by default +error: this expression will panic at runtime + --> $DIR/issue-54348.rs:3:5 + | +LL | [1][1.5 as usize]; + | ^^^^^^^^^^^^^^^^^ index out of bounds: the len is 1 but the index is 1 + error: index out of bounds: the len is 1 but the index is 1 - --> $DIR/issue-54348.rs:4:5 + --> $DIR/issue-54348.rs:5:5 | LL | [1][1u64 as usize]; | ^^^^^^^^^^^^^^^^^^ -error: aborting due to 2 previous errors +error: this expression will panic at runtime + --> $DIR/issue-54348.rs:5:5 + | +LL | [1][1u64 as usize]; + | ^^^^^^^^^^^^^^^^^^ index out of bounds: the len is 1 but the index is 1 + +error: aborting due to 4 previous errors diff --git a/src/test/ui/lint/lint-exceeding-bitshifts2.rs b/src/test/ui/lint/lint-exceeding-bitshifts2.rs index 69b627355b801..2c213daddd752 100644 --- a/src/test/ui/lint/lint-exceeding-bitshifts2.rs +++ b/src/test/ui/lint/lint-exceeding-bitshifts2.rs @@ -8,7 +8,7 @@ fn main() { let n = 1u8 << (4+3); let n = 1u8 << (4+4); //~ ERROR: attempt to shift left with overflow let n = 1i64 >> [63][0]; - let n = 1i64 >> [64][0]; // should be linting, needs to wait for const propagation + let n = 1i64 >> [64][0]; //~ ERROR: attempt to shift right with overflow #[cfg(target_pointer_width = "32")] const BITS: usize = 32; diff --git a/src/test/ui/lint/lint-exceeding-bitshifts2.stderr b/src/test/ui/lint/lint-exceeding-bitshifts2.stderr index cb96982a78930..d9c76d233d03e 100644 --- a/src/test/ui/lint/lint-exceeding-bitshifts2.stderr +++ b/src/test/ui/lint/lint-exceeding-bitshifts2.stderr @@ -10,6 +10,12 @@ note: lint level defined here LL | #![deny(exceeding_bitshifts, const_err)] | ^^^^^^^^^^^^^^^^^^^ +error: attempt to shift right with overflow + --> $DIR/lint-exceeding-bitshifts2.rs:11:15 + | +LL | let n = 1i64 >> [64][0]; + | ^^^^^^^^^^^^^^^ + error: attempt to shift left with overflow --> $DIR/lint-exceeding-bitshifts2.rs:17:15 | @@ -22,5 +28,5 @@ error: attempt to shift left with overflow LL | let n = 1_usize << BITS; | ^^^^^^^^^^^^^^^ -error: aborting due to 3 previous errors +error: aborting due to 4 previous errors From 4d89031e801e03009e74a3028007dd387e859717 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sun, 15 Sep 2019 00:05:19 -0400 Subject: [PATCH 3/7] [const-prop] Handle MIR Rvalue::Discriminant --- src/librustc_mir/transform/const_prop.rs | 4 +- src/test/mir-opt/const_prop/discriminant.rs | 53 +++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 src/test/mir-opt/const_prop/discriminant.rs diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 2119ee1b598ef..fb2cdce2d7aef 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -436,13 +436,13 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // if this isn't a supported operation, then return None match rvalue { - Rvalue::NullaryOp(NullOp::Box, _) | - Rvalue::Discriminant(..) => return None, + Rvalue::NullaryOp(NullOp::Box, _) => return None, Rvalue::Use(_) | Rvalue::Len(_) | Rvalue::Repeat(..) | Rvalue::Aggregate(..) | + Rvalue::Discriminant(..) | Rvalue::Cast(..) | Rvalue::NullaryOp(..) | Rvalue::CheckedBinaryOp(..) | diff --git a/src/test/mir-opt/const_prop/discriminant.rs b/src/test/mir-opt/const_prop/discriminant.rs new file mode 100644 index 0000000000000..07bbd9202b940 --- /dev/null +++ b/src/test/mir-opt/const_prop/discriminant.rs @@ -0,0 +1,53 @@ +// compile-flags: -O + +fn main() { + let x = (if let Some(true) = Some(true) { 42 } else { 10 }) + 0; +} + +// END RUST SOURCE +// START rustc.main.ConstProp.before.mir +// bb0: { +// ... +// _3 = std::option::Option::::Some(const true,); +// _4 = discriminant(_3); +// switchInt(move _4) -> [1isize: bb3, otherwise: bb2]; +// } +// bb1: { +// _2 = const 42i32; +// goto -> bb4; +// } +// bb2: { +// _2 = const 10i32; +// goto -> bb4; +// } +// bb3: { +// switchInt(((_3 as Some).0: bool)) -> [false: bb2, otherwise: bb1]; +// } +// bb4: { +// _1 = Add(move _2, const 0i32); +// ... +// } +// END rustc.main.ConstProp.before.mir +// START rustc.main.ConstProp.after.mir +// bb0: { +// ... +// _3 = const Scalar(0x01) : std::option::Option; +// _4 = const 1isize; +// switchInt(const 1isize) -> [1isize: bb3, otherwise: bb2]; +// } +// bb1: { +// _2 = const 42i32; +// goto -> bb4; +// } +// bb2: { +// _2 = const 10i32; +// goto -> bb4; +// } +// bb3: { +// switchInt(const true) -> [false: bb2, otherwise: bb1]; +// } +// bb4: { +// _1 = Add(move _2, const 0i32); +// ... +// } +// END rustc.main.ConstProp.after.mir From 2d22063e4c6fbdb61ce2a2dd9ce50d12fe9f0e9b Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sun, 15 Sep 2019 12:03:52 -0400 Subject: [PATCH 4/7] [const-prop] Handle MIR Rvalue::Box --- src/librustc_mir/transform/const_prop.rs | 19 +------- src/test/mir-opt/const_prop/boxes.rs | 56 ++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 18 deletions(-) create mode 100644 src/test/mir-opt/const_prop/boxes.rs diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index fb2cdce2d7aef..984938d00b2f0 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -8,7 +8,7 @@ use rustc::hir::def::DefKind; use rustc::hir::def_id::DefId; use rustc::mir::{ AggregateKind, Constant, Location, Place, PlaceBase, Body, Operand, Rvalue, - Local, NullOp, UnOp, StatementKind, Statement, LocalKind, + Local, UnOp, StatementKind, Statement, LocalKind, TerminatorKind, Terminator, ClearCrossCrate, SourceInfo, BinOp, SourceScope, SourceScopeLocalData, LocalDecl, BasicBlock, }; @@ -434,23 +434,6 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ) -> Option> { let span = source_info.span; - // if this isn't a supported operation, then return None - match rvalue { - Rvalue::NullaryOp(NullOp::Box, _) => return None, - - Rvalue::Use(_) | - Rvalue::Len(_) | - Rvalue::Repeat(..) | - Rvalue::Aggregate(..) | - Rvalue::Discriminant(..) | - Rvalue::Cast(..) | - Rvalue::NullaryOp(..) | - Rvalue::CheckedBinaryOp(..) | - Rvalue::Ref(..) | - Rvalue::UnaryOp(..) | - Rvalue::BinaryOp(..) => { } - } - // perform any special checking for specific Rvalue types if let Rvalue::UnaryOp(op, arg) = rvalue { trace!("checking UnaryOp(op = {:?}, arg = {:?})", op, arg); diff --git a/src/test/mir-opt/const_prop/boxes.rs b/src/test/mir-opt/const_prop/boxes.rs new file mode 100644 index 0000000000000..cf134dadf2789 --- /dev/null +++ b/src/test/mir-opt/const_prop/boxes.rs @@ -0,0 +1,56 @@ +// compile-flags: -O +// ignore-emscripten compiled with panic=abort by default +// ignore-wasm32 +// ignore-wasm64 + +#![feature(box_syntax)] + +// Note: this test verifies that we, in fact, do not const prop `box` + +fn main() { + let x = *(box 42) + 0; +} + +// END RUST SOURCE +// START rustc.main.ConstProp.before.mir +// bb0: { +// ... +// _4 = Box(i32); +// (*_4) = const 42i32; +// _3 = move _4; +// ... +// _2 = (*_3); +// _1 = Add(move _2, const 0i32); +// ... +// drop(_3) -> [return: bb2, unwind: bb1]; +// } +// bb1 (cleanup): { +// resume; +// } +// bb2: { +// ... +// _0 = (); +// ... +// } +// END rustc.main.ConstProp.before.mir +// START rustc.main.ConstProp.after.mir +// bb0: { +// ... +// _4 = Box(i32); +// (*_4) = const 42i32; +// _3 = move _4; +// ... +// _2 = (*_3); +// _1 = Add(move _2, const 0i32); +// ... +// drop(_3) -> [return: bb2, unwind: bb1]; +// } +// bb1 (cleanup): { +// resume; +// } +// bb2: { +// ... +// _0 = (); +// ... +// } +// END rustc.main.ConstProp.after.mir From 8cf6c23d66ce4d80d6d76291c10cbbe6cab9cb00 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sun, 15 Sep 2019 12:08:09 -0400 Subject: [PATCH 5/7] Cleanup const_prop() some --- src/librustc_mir/transform/const_prop.rs | 147 ++++++++++++----------- 1 file changed, 75 insertions(+), 72 deletions(-) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 984938d00b2f0..abb880fd62419 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -118,7 +118,7 @@ impl<'tcx> MirPass<'tcx> for ConstProp { struct ConstPropMachine; impl<'mir, 'tcx> interpret::Machine<'mir, 'tcx> for ConstPropMachine { - type MemoryKinds= !; + type MemoryKinds = !; type PointerTag = (); type ExtraFnVal = !; @@ -435,79 +435,80 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { let span = source_info.span; // perform any special checking for specific Rvalue types - if let Rvalue::UnaryOp(op, arg) = rvalue { - trace!("checking UnaryOp(op = {:?}, arg = {:?})", op, arg); - let overflow_check = self.tcx.sess.overflow_checks(); - - self.use_ecx(source_info, |this| { - // We check overflow in debug mode already - // so should only check in release mode. - if *op == UnOp::Neg && !overflow_check { - let ty = arg.ty(&this.local_decls, this.tcx); - - if ty.is_integral() { - let arg = this.ecx.eval_operand(arg, None)?; - let prim = this.ecx.read_immediate(arg)?; - // Need to do overflow check here: For actual CTFE, MIR - // generation emits code that does this before calling the op. - if prim.to_bits()? == (1 << (prim.layout.size.bits() - 1)) { - throw_panic!(OverflowNeg) + match rvalue { + Rvalue::UnaryOp(UnOp::Neg, arg) => { + trace!("checking UnaryOp(op = Neg, arg = {:?})", arg); + let overflow_check = self.tcx.sess.overflow_checks(); + + self.use_ecx(source_info, |this| { + // We check overflow in debug mode already + // so should only check in release mode. + if !overflow_check { + let ty = arg.ty(&this.local_decls, this.tcx); + + if ty.is_integral() { + let arg = this.ecx.eval_operand(arg, None)?; + let prim = this.ecx.read_immediate(arg)?; + // Need to do overflow check here: For actual CTFE, MIR + // generation emits code that does this before calling the op. + if prim.to_bits()? == (1 << (prim.layout.size.bits() - 1)) { + throw_panic!(OverflowNeg) + } } } - } - Ok(()) - })?; - } else if let Rvalue::BinaryOp(op, left, right) = rvalue { - trace!("checking BinaryOp(op = {:?}, left = {:?}, right = {:?})", op, left, right); - - let r = self.use_ecx(source_info, |this| { - this.ecx.read_immediate(this.ecx.eval_operand(right, None)?) - })?; - if *op == BinOp::Shr || *op == BinOp::Shl { - let left_bits = place_layout.size.bits(); - let right_size = r.layout.size; - let r_bits = r.to_scalar().and_then(|r| r.to_bits(right_size)); - if r_bits.ok().map_or(false, |b| b >= left_bits as u128) { - let source_scope_local_data = match self.source_scope_local_data { - ClearCrossCrate::Set(ref data) => data, - ClearCrossCrate::Clear => return None, - }; - let dir = if *op == BinOp::Shr { - "right" - } else { - "left" - }; - let hir_id = source_scope_local_data[source_info.scope].lint_root; - self.tcx.lint_hir( - ::rustc::lint::builtin::EXCEEDING_BITSHIFTS, - hir_id, - span, - &format!("attempt to shift {} with overflow", dir)); - return None; - } + Ok(()) + })?; } - self.use_ecx(source_info, |this| { - let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?; - let (_, overflow, _ty) = this.ecx.overflowing_binary_op(*op, l, r)?; - - // We check overflow in debug mode already - // so should only check in release mode. - if !this.tcx.sess.overflow_checks() && overflow { - let err = err_panic!(Overflow(*op)).into(); - return Err(err); + + Rvalue::BinaryOp(op, left, right) => { + trace!("checking BinaryOp(op = {:?}, left = {:?}, right = {:?})", op, left, right); + + let r = self.use_ecx(source_info, |this| { + this.ecx.read_immediate(this.ecx.eval_operand(right, None)?) + })?; + if *op == BinOp::Shr || *op == BinOp::Shl { + let left_bits = place_layout.size.bits(); + let right_size = r.layout.size; + let r_bits = r.to_scalar().and_then(|r| r.to_bits(right_size)); + if r_bits.ok().map_or(false, |b| b >= left_bits as u128) { + let source_scope_local_data = match self.source_scope_local_data { + ClearCrossCrate::Set(ref data) => data, + ClearCrossCrate::Clear => return None, + }; + let dir = if *op == BinOp::Shr { + "right" + } else { + "left" + }; + let hir_id = source_scope_local_data[source_info.scope].lint_root; + self.tcx.lint_hir( + ::rustc::lint::builtin::EXCEEDING_BITSHIFTS, + hir_id, + span, + &format!("attempt to shift {} with overflow", dir)); + return None; + } } + self.use_ecx(source_info, |this| { + let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?; + let (_, overflow, _ty) = this.ecx.overflowing_binary_op(*op, l, r)?; + + // We check overflow in debug mode already + // so should only check in release mode. + if !this.tcx.sess.overflow_checks() && overflow { + let err = err_panic!(Overflow(*op)).into(); + return Err(err); + } - Ok(()) - })?; - } else if let Rvalue::Ref(_, _, place) = rvalue { - trace!("checking Ref({:?})", place); - // FIXME(wesleywiser) we don't currently handle the case where we try to make a ref - // from a function argument that hasn't been assigned to in this function. - if let Place { - base: PlaceBase::Local(local), - projection: box [] - } = place { + Ok(()) + })?; + } + + Rvalue::Ref(_, _, Place { base: PlaceBase::Local(local), projection: box [] }) => { + trace!("checking Ref({:?})", place); + // FIXME(wesleywiser) we don't currently handle the case where we try to make a ref + // from a function argument that hasn't been assigned to in this function. let alive = if let LocalValue::Live(_) = self.ecx.frame().locals[*local].value { true @@ -518,12 +519,14 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { return None; } } - } else if let Rvalue::Aggregate(_, operands) = rvalue { - // FIXME(wesleywiser): const eval will turn this into a `const Scalar()` that - // `SimplifyLocals` doesn't know it can remove. - if operands.len() == 0 { + + Rvalue::Aggregate(_, operands) if operands.len() == 0 => { + // FIXME(wesleywiser): const eval will turn this into a `const Scalar()` that + // `SimplifyLocals` doesn't know it can remove. return None; } + + _ => { } } self.use_ecx(source_info, |this| { From b71ea80172136fe4b597aef1001ca72a8588fe15 Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Wed, 9 Oct 2019 06:08:46 -0400 Subject: [PATCH 6/7] Don't ICE when evaluating writes to uninhabited enum variants --- src/librustc/mir/interpret/error.rs | 7 +++-- src/librustc_mir/interpret/operand.rs | 8 +++--- src/librustc_mir/interpret/place.rs | 16 +++++++---- src/librustc_mir/interpret/validity.rs | 2 +- .../write-to-uninhabited-enum-variant.rs | 28 +++++++++++++++++++ 5 files changed, 48 insertions(+), 13 deletions(-) create mode 100644 src/test/ui/consts/const-eval/write-to-uninhabited-enum-variant.rs diff --git a/src/librustc/mir/interpret/error.rs b/src/librustc/mir/interpret/error.rs index ac99ccd45eafe..d918b9ee67347 100644 --- a/src/librustc/mir/interpret/error.rs +++ b/src/librustc/mir/interpret/error.rs @@ -363,6 +363,8 @@ pub enum UndefinedBehaviorInfo { UbExperimental(String), /// Unreachable code was executed. Unreachable, + /// An enum discriminant was set to a value which was outside the range of valid values. + InvalidDiscriminant(ScalarMaybeUndef), } impl fmt::Debug for UndefinedBehaviorInfo { @@ -373,6 +375,8 @@ impl fmt::Debug for UndefinedBehaviorInfo { write!(f, "{}", msg), Unreachable => write!(f, "entered unreachable code"), + InvalidDiscriminant(val) => + write!(f, "encountered invalid enum discriminant {}", val), } } } @@ -400,7 +404,6 @@ pub enum UnsupportedOpInfo<'tcx> { InvalidMemoryAccess, InvalidFunctionPointer, InvalidBool, - InvalidDiscriminant(ScalarMaybeUndef), PointerOutOfBounds { ptr: Pointer, msg: CheckInAllocMsg, @@ -485,8 +488,6 @@ impl fmt::Debug for UnsupportedOpInfo<'tcx> { write!(f, "incorrect alloc info: expected size {} and align {}, \ got size {} and align {}", size.bytes(), align.bytes(), size2.bytes(), align2.bytes()), - InvalidDiscriminant(val) => - write!(f, "encountered invalid enum discriminant {}", val), InvalidMemoryAccess => write!(f, "tried to access memory through an invalid pointer"), DanglingPointerDeref => diff --git a/src/librustc_mir/interpret/operand.rs b/src/librustc_mir/interpret/operand.rs index 4bdd71f9602ac..4d9be55945e02 100644 --- a/src/librustc_mir/interpret/operand.rs +++ b/src/librustc_mir/interpret/operand.rs @@ -647,7 +647,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let bits_discr = raw_discr .not_undef() .and_then(|raw_discr| self.force_bits(raw_discr, discr_val.layout.size)) - .map_err(|_| err_unsup!(InvalidDiscriminant(raw_discr.erase_tag())))?; + .map_err(|_| err_ub!(InvalidDiscriminant(raw_discr.erase_tag())))?; let real_discr = if discr_val.layout.ty.is_signed() { // going from layout tag type to typeck discriminant type // requires first sign extending with the discriminant layout @@ -677,7 +677,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { _ => bug!("tagged layout for non-adt non-generator"), }.ok_or_else( - || err_unsup!(InvalidDiscriminant(raw_discr.erase_tag())) + || err_ub!(InvalidDiscriminant(raw_discr.erase_tag())) )?; (real_discr, index.0) }, @@ -689,7 +689,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let variants_start = niche_variants.start().as_u32(); let variants_end = niche_variants.end().as_u32(); let raw_discr = raw_discr.not_undef().map_err(|_| { - err_unsup!(InvalidDiscriminant(ScalarMaybeUndef::Undef)) + err_ub!(InvalidDiscriminant(ScalarMaybeUndef::Undef)) })?; match raw_discr.to_bits_or_ptr(discr_val.layout.size, self) { Err(ptr) => { @@ -697,7 +697,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let ptr_valid = niche_start == 0 && variants_start == variants_end && !self.memory.ptr_may_be_null(ptr); if !ptr_valid { - throw_unsup!(InvalidDiscriminant(raw_discr.erase_tag().into())) + throw_ub!(InvalidDiscriminant(raw_discr.erase_tag().into())) } (dataful_variant.as_u32() as u128, dataful_variant) }, diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 903eb3c1c44b9..0289c52fd3744 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -1031,9 +1031,13 @@ where variant_index: VariantIdx, dest: PlaceTy<'tcx, M::PointerTag>, ) -> InterpResult<'tcx> { + let variant_scalar = Scalar::from_u32(variant_index.as_u32()).into(); + match dest.layout.variants { layout::Variants::Single { index } => { - assert_eq!(index, variant_index); + if index != variant_index { + throw_ub!(InvalidDiscriminant(variant_scalar)); + } } layout::Variants::Multiple { discr_kind: layout::DiscriminantKind::Tag, @@ -1041,7 +1045,9 @@ where discr_index, .. } => { - assert!(dest.layout.ty.variant_range(*self.tcx).unwrap().contains(&variant_index)); + if !dest.layout.ty.variant_range(*self.tcx).unwrap().contains(&variant_index) { + throw_ub!(InvalidDiscriminant(variant_scalar)); + } let discr_val = dest.layout.ty.discriminant_for_variant(*self.tcx, variant_index).unwrap().val; @@ -1064,9 +1070,9 @@ where discr_index, .. } => { - assert!( - variant_index.as_usize() < dest.layout.ty.ty_adt_def().unwrap().variants.len(), - ); + if !variant_index.as_usize() < dest.layout.ty.ty_adt_def().unwrap().variants.len() { + throw_ub!(InvalidDiscriminant(variant_scalar)); + } if variant_index != dataful_variant { let variants_start = niche_variants.start().as_u32(); let variant_index_relative = variant_index.as_u32() diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 853fcb1beabf5..3444fb60f333b 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -344,7 +344,7 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> match self.walk_value(op) { Ok(()) => Ok(()), Err(err) => match err.kind { - err_unsup!(InvalidDiscriminant(val)) => + err_ub!(InvalidDiscriminant(val)) => throw_validation_failure!( val, self.path, "a valid enum discriminant" ), diff --git a/src/test/ui/consts/const-eval/write-to-uninhabited-enum-variant.rs b/src/test/ui/consts/const-eval/write-to-uninhabited-enum-variant.rs new file mode 100644 index 0000000000000..cccb7879fc0fb --- /dev/null +++ b/src/test/ui/consts/const-eval/write-to-uninhabited-enum-variant.rs @@ -0,0 +1,28 @@ +// run-pass + +#![allow(dead_code)] + +enum Empty { } +enum Test1 { + A(u8), + B(Empty), +} +enum Test2 { + A(u8), + B(Empty), + C, +} + +fn bar() -> Option { + None +} + +fn main() { + if let Some(x) = bar() { + Test1::B(x); + } + + if let Some(x) = bar() { + Test2::B(x); + } +} From fd20dbed004c5c84fe846e04255608dc78b60a0d Mon Sep 17 00:00:00 2001 From: Wesley Wiser Date: Sun, 13 Oct 2019 13:48:26 -0400 Subject: [PATCH 7/7] Improve comments and structure of `ConstProp::const_prop()` Per code review feedback --- src/librustc_mir/transform/const_prop.rs | 82 +++++++++++++++--------- 1 file changed, 50 insertions(+), 32 deletions(-) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index abb880fd62419..f0c0e57344388 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -434,26 +434,32 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ) -> Option> { let span = source_info.span; - // perform any special checking for specific Rvalue types + let overflow_check = self.tcx.sess.overflow_checks(); + + // 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 + // - In this case, we will do some validation and then fall through to the + // end of the function which evals the assignment. + // 2. Working around bugs in other parts of the compiler + // - In this case, we'll return `None` from this function to stop evaluation. match rvalue { - Rvalue::UnaryOp(UnOp::Neg, arg) => { + // Additional checking: if overflow checks are disabled (which is usually the case in + // release mode), then we need to do additional checking here to give lints to the user + // if an overflow would occur. + Rvalue::UnaryOp(UnOp::Neg, arg) if !overflow_check => { trace!("checking UnaryOp(op = Neg, arg = {:?})", arg); - let overflow_check = self.tcx.sess.overflow_checks(); self.use_ecx(source_info, |this| { - // We check overflow in debug mode already - // so should only check in release mode. - if !overflow_check { - let ty = arg.ty(&this.local_decls, this.tcx); - - if ty.is_integral() { - let arg = this.ecx.eval_operand(arg, None)?; - let prim = this.ecx.read_immediate(arg)?; - // Need to do overflow check here: For actual CTFE, MIR - // generation emits code that does this before calling the op. - if prim.to_bits()? == (1 << (prim.layout.size.bits() - 1)) { - throw_panic!(OverflowNeg) - } + let ty = arg.ty(&this.local_decls, this.tcx); + + if ty.is_integral() { + let arg = this.ecx.eval_operand(arg, None)?; + let prim = this.ecx.read_immediate(arg)?; + // Need to do overflow check here: For actual CTFE, MIR + // generation emits code that does this before calling the op. + if prim.to_bits()? == (1 << (prim.layout.size.bits() - 1)) { + throw_panic!(OverflowNeg) } } @@ -461,6 +467,8 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { })?; } + // Additional checking: check for overflows on integer binary operations and report + // them to the user as lints. Rvalue::BinaryOp(op, left, right) => { trace!("checking BinaryOp(op = {:?}, left = {:?}, right = {:?})", op, left, right); @@ -490,25 +498,34 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { return None; } } - self.use_ecx(source_info, |this| { - let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?; - let (_, overflow, _ty) = this.ecx.overflowing_binary_op(*op, l, r)?; - - // We check overflow in debug mode already - // so should only check in release mode. - if !this.tcx.sess.overflow_checks() && overflow { - let err = err_panic!(Overflow(*op)).into(); - return Err(err); - } - Ok(()) - })?; + // If overflow checking is enabled (like in debug mode by default), + // then we'll already catch overflow when we evaluate the `Assert` statement + // in MIR. However, if overflow checking is disabled, then there won't be any + // `Assert` statement and so we have to do additional checking here. + if !overflow_check { + self.use_ecx(source_info, |this| { + let l = this.ecx.read_immediate(this.ecx.eval_operand(left, None)?)?; + let (_, overflow, _ty) = this.ecx.overflowing_binary_op(*op, l, r)?; + + if overflow { + let err = err_panic!(Overflow(*op)).into(); + return Err(err); + } + + Ok(()) + })?; + } } + // Work around: avoid ICE in miri. + // FIXME(wesleywiser) we don't currently handle the case where we try to make a ref + // from a function argument that hasn't been assigned to in this function. The main + // issue is if an arg is a fat-pointer, miri `expects()` to be able to read the value + // of that pointer to get size info. However, since this is `ConstProp`, that argument + // doesn't actually have a backing value and so this causes an ICE. Rvalue::Ref(_, _, Place { base: PlaceBase::Local(local), projection: box [] }) => { trace!("checking Ref({:?})", place); - // FIXME(wesleywiser) we don't currently handle the case where we try to make a ref - // from a function argument that hasn't been assigned to in this function. let alive = if let LocalValue::Live(_) = self.ecx.frame().locals[*local].value { true @@ -520,9 +537,10 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { } } + // Work around: avoid extra unnecessary locals. + // FIXME(wesleywiser): const eval will turn this into a `const Scalar()` that + // `SimplifyLocals` doesn't know it can remove. Rvalue::Aggregate(_, operands) if operands.len() == 0 => { - // FIXME(wesleywiser): const eval will turn this into a `const Scalar()` that - // `SimplifyLocals` doesn't know it can remove. return None; }