From 71bb5cd74e48b2a21ed869b7d68f86538a27016f Mon Sep 17 00:00:00 2001 From: jumbatm Date: Thu, 2 Jan 2020 21:18:56 +1000 Subject: [PATCH 1/3] Add test case which demonstrates current miscomp. --- src/test/ui/mir/issue67529.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 src/test/ui/mir/issue67529.rs diff --git a/src/test/ui/mir/issue67529.rs b/src/test/ui/mir/issue67529.rs new file mode 100644 index 0000000000000..7e0b37be2e91e --- /dev/null +++ b/src/test/ui/mir/issue67529.rs @@ -0,0 +1,14 @@ +// Tests for miscompilation due to const propagation, as described in #67529. This used to result +// in an assertion error, because d.a was replaced with a new allocation that was never +// initialized. +// +// run-pass +// compile-flags: -Zmir-opt-level=2 +struct Baz { + a: T, +} + +fn main() { + let d: Baz<[i32; 4]> = Baz { a: [1, 2, 3, 4] }; + assert_eq!([1, 2, 3, 4], d.a); +} From 3c5bdfae3ffe141d7e7248a7bb0eac58236820d0 Mon Sep 17 00:00:00 2001 From: jumbatm Date: Fri, 3 Jan 2020 12:25:37 +1000 Subject: [PATCH 2/3] Fix a few comment and logging typos --- src/librustc_mir/transform/const_prop.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 1d5a643484a73..bbf57226ad5a1 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -643,7 +643,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { value: Const<'tcx>, source_info: SourceInfo, ) { - trace!("attepting to replace {:?} with {:?}", rval, value); + trace!("attempting to replace {:?} with {:?}", rval, value); if let Err(e) = self.ecx.validate_operand( value, vec![], @@ -654,8 +654,9 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { return; } - // FIXME> figure out what tho do when try_read_immediate fails + // FIXME: figure out what tho do when try_read_immediate fails let imm = self.use_ecx(source_info, |this| this.ecx.try_read_immediate(value)); + debug!("Read value {:?} as immediate {:?}", value, imm); if let Some(Ok(imm)) = imm { match *imm { @@ -670,7 +671,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { ScalarMaybeUndef::Scalar(one), ScalarMaybeUndef::Scalar(two), ) => { - // Found a value represented as a pair. For now only do cont-prop if type of + // Found a value represented as a pair. For now only do const-prop if type of // Rvalue is also a pair with two scalars. The more general case is more // complicated to implement so we'll do it later. let ty = &value.layout.ty.kind; From ed3a601213633dbd402d8eb825367d779cd9ca1d Mon Sep 17 00:00:00 2001 From: jumbatm Date: Wed, 8 Jan 2020 18:48:44 +1000 Subject: [PATCH 3/3] Don't const prop refs with uninitialised bases. --- src/librustc_mir/transform/const_prop.rs | 26 ++++++++++++++---------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index bbf57226ad5a1..f2aea6746863e 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -10,7 +10,7 @@ use rustc::mir::visit::{ }; use rustc::mir::{ read_only, AggregateKind, BasicBlock, BinOp, Body, BodyAndCache, ClearCrossCrate, Constant, - Local, LocalDecl, LocalKind, Location, Operand, Place, ReadOnlyBodyAndCache, Rvalue, + Local, LocalDecl, LocalKind, Location, Operand, Place, PlaceRef, ReadOnlyBodyAndCache, Rvalue, SourceInfo, SourceScope, SourceScopeData, Statement, StatementKind, Terminator, TerminatorKind, UnOp, RETURN_PLACE, }; @@ -602,20 +602,24 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { // (e.g. for CTFE) it can never happen. But here in const_prop // unknown data is uninitialized, so if e.g. a function argument is unsized // and has a reference taken, we get an ICE. + // + // Additionally, to evaluate a Ref into a place to const prop, we must ensure that the + // underlying base data is initialized before we evaluate the rvalue, or we will end up + // propagating an allocation which will never be initialized. Rvalue::Ref(_, _, place_ref) => { trace!("checking Ref({:?})", place_ref); - if let Some(local) = place_ref.as_local() { - let alive = if let LocalValue::Live(_) = self.ecx.frame().locals[local].value { - true - } else { - false - }; + let PlaceRef { local, .. } = place_ref.as_ref(); - if !alive { - trace!("skipping Ref({:?}) to uninitialized local", place); - return None; - } + let alive = if let LocalValue::Live(_) = self.ecx.frame().locals[*local].value { + true + } else { + false + }; + + if !alive { + trace!("skipping Ref({:?}) to uninitialized local", place); + return None; } }