From e6eaf2083ac66ba51ef05ff59ee1e65a43decddc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 31 May 2017 17:41:33 -0700 Subject: [PATCH 1/4] interpret StorageLive & StorageDead, and check dead stack slots are not used --- src/error.rs | 3 + src/eval_context.rs | 151 +++++++++++++++++++++++++++++++------------- src/lvalue.rs | 4 +- src/step.rs | 20 ++++-- 4 files changed, 128 insertions(+), 50 deletions(-) diff --git a/src/error.rs b/src/error.rs index 7b6542bff9..702c2c4940 100644 --- a/src/error.rs +++ b/src/error.rs @@ -24,6 +24,7 @@ pub enum EvalError<'tcx> { ReadPointerAsBytes, InvalidPointerMath, ReadUndefBytes, + DeadLocal, InvalidBoolOp(mir::BinOp), Unimplemented(String), DerefFunctionPointer, @@ -83,6 +84,8 @@ impl<'tcx> Error for EvalError<'tcx> { "attempted to do math or a comparison on pointers into different allocations", EvalError::ReadUndefBytes => "attempted to read undefined bytes", + EvalError::DeadLocal => + "tried to access a dead local variable", EvalError::InvalidBoolOp(_) => "invalid boolean operation", EvalError::Unimplemented(ref msg) => msg, diff --git a/src/eval_context.rs b/src/eval_context.rs index 922a36c892..c704ab230d 100644 --- a/src/eval_context.rs +++ b/src/eval_context.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::fmt::Write; use rustc::hir::def_id::DefId; @@ -74,11 +74,12 @@ pub struct Frame<'tcx> { pub return_lvalue: Lvalue<'tcx>, /// The list of locals for this stack frame, stored in order as - /// `[arguments..., variables..., temporaries...]`. The locals are stored as `Value`s, which + /// `[arguments..., variables..., temporaries...]`. The locals are stored as `Option`s. + /// `None` represents a local that is currently dead, while a live local /// can either directly contain `PrimVal` or refer to some part of an `Allocation`. /// - /// Before being initialized, all locals are `Value::ByVal(PrimVal::Undef)`. - pub locals: Vec, + /// Before being initialized, arguments are `Value::ByVal(PrimVal::Undef)` and other locals are `None`. + pub locals: Vec>, //////////////////////////////////////////////////////////////////////////////// // Current position within the function @@ -452,10 +453,33 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { ) -> EvalResult<'tcx> { ::log_settings::settings().indentation += 1; + /// Return the set of locals that have a stroage annotation anywhere + fn collect_storage_annotations<'tcx>(mir: &'tcx mir::Mir<'tcx>) -> HashSet { + use rustc::mir::StatementKind::*; + + let mut set = HashSet::new(); + for block in mir.basic_blocks() { + for stmt in block.statements.iter() { + match stmt.kind { + StorageLive(mir::Lvalue::Local(local)) | StorageDead(mir::Lvalue::Local(local)) => { + set.insert(local); + } + _ => {} + } + } + }; + set + } + // Subtract 1 because `local_decls` includes the ReturnPointer, but we don't store a local // `Value` for that. + let annotated_locals = collect_storage_annotations(mir); let num_locals = mir.local_decls.len() - 1; - let locals = vec![Value::ByVal(PrimVal::Undef); num_locals]; + let mut locals = Vec::with_capacity(num_locals); + for i in 0..num_locals { + let local = mir::Local::new(i+1); + locals.push(if annotated_locals.contains(&local) { None } else { Some(Value::ByVal(PrimVal::Undef)) }); + } self.stack.push(Frame { mir, @@ -509,21 +533,26 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { } // deallocate all locals that are backed by an allocation for local in frame.locals { - if let Value::ByRef(ptr) = local { - trace!("deallocating local"); - self.memory.dump_alloc(ptr.alloc_id); - match self.memory.deallocate(ptr) { - // We could alternatively check whether the alloc_id is static before calling - // deallocate, but this is much simpler and is probably the rare case. - Ok(()) | Err(EvalError::DeallocatedStaticMemory) => {}, - other => return other, - } - } + self.deallocate_local(local)?; } Ok(()) } + pub fn deallocate_local(&mut self, local: Option) -> EvalResult<'tcx> { + if let Some(Value::ByRef(ptr)) = local { + trace!("deallocating local"); + self.memory.dump_alloc(ptr.alloc_id); + match self.memory.deallocate(ptr) { + // We could alternatively check whether the alloc_id is static before calling + // deallocate, but this is much simpler and is probably the rare case. + Ok(()) | Err(EvalError::DeallocatedStaticMemory) => {}, + other => return other, + } + }; + Ok(()) + } + pub fn assign_discr_and_fields< V: IntoValTyPair<'tcx>, J: IntoIterator, @@ -1047,16 +1076,17 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { Lvalue::Local { frame, local, field } => { // -1 since we don't store the return value match self.stack[frame].locals[local.index() - 1] { - Value::ByRef(ptr) => { + None => return Err(EvalError::DeadLocal), + Some(Value::ByRef(ptr)) => { assert!(field.is_none()); Lvalue::from_ptr(ptr) }, - val => { + Some(val) => { let ty = self.stack[frame].mir.local_decls[local].ty; let ty = self.monomorphize(ty, self.stack[frame].instance.substs); let substs = self.stack[frame].instance.substs; let ptr = self.alloc_ptr_with_substs(ty, substs)?; - self.stack[frame].locals[local.index() - 1] = Value::ByRef(ptr); + self.stack[frame].locals[local.index() - 1] = Some(Value::ByRef(ptr)); // it stays live self.write_value_to_ptr(val, ptr, ty)?; let lval = Lvalue::from_ptr(ptr); if let Some((field, field_ty)) = field { @@ -1139,7 +1169,8 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { *this.globals.get_mut(&cid).expect("already checked") = Global { value: val, ..dest - } + }; + Ok(()) }; self.write_value_possibly_by_val(src_val, write_dest, dest.value, dest_ty) }, @@ -1150,7 +1181,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { } Lvalue::Local { frame, local, field } => { - let dest = self.stack[frame].get_local(local, field.map(|(i, _)| i)); + let dest = self.stack[frame].get_local(local, field.map(|(i, _)| i))?; self.write_value_possibly_by_val( src_val, |this, val| this.stack[frame].set_local(local, field.map(|(i, _)| i), val), @@ -1162,7 +1193,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { } // The cases here can be a bit subtle. Read carefully! - fn write_value_possibly_by_val( + fn write_value_possibly_by_val EvalResult<'tcx>>( &mut self, src_val: Value, write_dest: F, @@ -1192,17 +1223,17 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { // source and write that into the destination without making an allocation, so // we do so here. if let Ok(Some(src_val)) = self.try_read_value(src_ptr, dest_ty) { - write_dest(self, src_val); + write_dest(self, src_val)?; } else { let dest_ptr = self.alloc_ptr(dest_ty)?; self.copy(src_ptr, dest_ptr, dest_ty)?; - write_dest(self, Value::ByRef(dest_ptr)); + write_dest(self, Value::ByRef(dest_ptr))?; } } else { // Finally, we have the simple case where neither source nor destination are // `ByRef`. We may simply copy the source value over the the destintion. - write_dest(self, src_val); + write_dest(self, src_val)?; } Ok(()) } @@ -1572,14 +1603,20 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { write!(msg, ":").unwrap(); match self.stack[frame].get_local(local, field.map(|(i, _)| i)) { - Value::ByRef(ptr) => { + Err(EvalError::DeadLocal) => { + write!(msg, " is dead").unwrap(); + } + Err(err) => { + panic!("Failed to access local: {:?}", err); + } + Ok(Value::ByRef(ptr)) => { allocs.push(ptr.alloc_id); } - Value::ByVal(val) => { + Ok(Value::ByVal(val)) => { write!(msg, " {:?}", val).unwrap(); if let PrimVal::Ptr(ptr) = val { allocs.push(ptr.alloc_id); } } - Value::ByValPair(val1, val2) => { + Ok(Value::ByValPair(val1, val2)) => { write!(msg, " ({:?}, {:?})", val1, val2).unwrap(); if let PrimVal::Ptr(ptr) = val1 { allocs.push(ptr.alloc_id); } if let PrimVal::Ptr(ptr) = val2 { allocs.push(ptr.alloc_id); } @@ -1614,9 +1651,9 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { ) -> EvalResult<'tcx> where F: FnOnce(&mut Self, Value) -> EvalResult<'tcx, Value>, { - let val = self.stack[frame].get_local(local, field); + let val = self.stack[frame].get_local(local, field)?; let new_val = f(self, val)?; - self.stack[frame].set_local(local, field, new_val); + self.stack[frame].set_local(local, field, new_val)?; // FIXME(solson): Run this when setting to Undef? (See previous version of this code.) // if let Value::ByRef(ptr) = self.stack[frame].get_local(local) { // self.memory.deallocate(ptr)?; @@ -1626,53 +1663,79 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { } impl<'tcx> Frame<'tcx> { - pub fn get_local(&self, local: mir::Local, field: Option) -> Value { + pub fn get_local(&self, local: mir::Local, field: Option) -> EvalResult<'tcx, Value> { // Subtract 1 because we don't store a value for the ReturnPointer, the local with index 0. if let Some(field) = field { - match self.locals[local.index() - 1] { - Value::ByRef(_) => bug!("can't have lvalue fields for ByRef"), - val @ Value::ByVal(_) => { + Ok(match self.locals[local.index() - 1] { + None => return Err(EvalError::DeadLocal), + Some(Value::ByRef(_)) => bug!("can't have lvalue fields for ByRef"), + Some(val @ Value::ByVal(_)) => { assert_eq!(field, 0); val }, - Value::ByValPair(a, b) => { + Some(Value::ByValPair(a, b)) => { match field { 0 => Value::ByVal(a), 1 => Value::ByVal(b), _ => bug!("ByValPair has only two fields, tried to access {}", field), } }, - } + }) } else { - self.locals[local.index() - 1] + self.locals[local.index() - 1].ok_or(EvalError::DeadLocal) } } - fn set_local(&mut self, local: mir::Local, field: Option, value: Value) { + fn set_local(&mut self, local: mir::Local, field: Option, value: Value) -> EvalResult<'tcx> { // Subtract 1 because we don't store a value for the ReturnPointer, the local with index 0. if let Some(field) = field { match self.locals[local.index() - 1] { - Value::ByRef(_) => bug!("can't have lvalue fields for ByRef"), - Value::ByVal(_) => { + None => return Err(EvalError::DeadLocal), + Some(Value::ByRef(_)) => bug!("can't have lvalue fields for ByRef"), + Some(Value::ByVal(_)) => { assert_eq!(field, 0); - self.set_local(local, None, value); + self.set_local(local, None, value)?; }, - Value::ByValPair(a, b) => { + Some(Value::ByValPair(a, b)) => { let prim = match value { Value::ByRef(_) => bug!("can't set ValPair field to ByRef"), Value::ByVal(val) => val, Value::ByValPair(_, _) => bug!("can't set ValPair field to ValPair"), }; match field { - 0 => self.set_local(local, None, Value::ByValPair(prim, b)), - 1 => self.set_local(local, None, Value::ByValPair(a, prim)), + 0 => self.set_local(local, None, Value::ByValPair(prim, b))?, + 1 => self.set_local(local, None, Value::ByValPair(a, prim))?, _ => bug!("ByValPair has only two fields, tried to access {}", field), } }, } } else { - self.locals[local.index() - 1] = value; + match self.locals[local.index() - 1] { + None => return Err(EvalError::DeadLocal), + Some(ref mut local) => { *local = value; } + } + } + return Ok(()); + } + + pub fn storage_live(&mut self, local: mir::Local) -> EvalResult<'tcx> { + trace!("{:?} is now live", local); + if self.locals[local.index() - 1].is_some() { + // The variables comes live now, but was already accessed previously, when it was still dead + return Err(EvalError::DeadLocal); + } else { + self.locals[local.index() - 1] = Some(Value::ByVal(PrimVal::Undef)); } + return Ok(()); + } + + /// Returns the old value of the local + pub fn storage_dead(&mut self, local: mir::Local) -> EvalResult<'tcx, Option> { + trace!("{:?} is now dead", local); + + let old = self.locals[local.index() - 1]; + self.locals[local.index() - 1] = None; + return Ok(old); } } diff --git a/src/lvalue.rs b/src/lvalue.rs index cad4ca9d02..a09f72134e 100644 --- a/src/lvalue.rs +++ b/src/lvalue.rs @@ -130,7 +130,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { Ok(Value::ByRef(ptr)) } Lvalue::Local { frame, local, field } => { - Ok(self.stack[frame].get_local(local, field.map(|(i, _)| i))) + Ok(self.stack[frame].get_local(local, field.map(|(i, _)| i))?) } Lvalue::Global(cid) => { Ok(self.globals.get(&cid).expect("global not cached").value) @@ -226,7 +226,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { let (base_ptr, base_extra) = match base { Lvalue::Ptr { ptr, extra } => (ptr, extra), - Lvalue::Local { frame, local, field } => match self.stack[frame].get_local(local, field.map(|(i, _)| i)) { + Lvalue::Local { frame, local, field } => match self.stack[frame].get_local(local, field.map(|(i, _)| i))? { Value::ByRef(ptr) => { assert!(field.is_none(), "local can't be ByRef and have a field offset"); (ptr, LvalueExtra::None) diff --git a/src/step.rs b/src/step.rs index cbd9871e83..1df55a8b62 100644 --- a/src/step.rs +++ b/src/step.rs @@ -126,9 +126,20 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { } } - // Miri can safely ignore these. Only translation needs it. - StorageLive(_) | - StorageDead(_) => {} + // Mark locals as dead or alive. + StorageLive(ref lvalue) | StorageDead(ref lvalue)=> { + let (frame, local) = match self.eval_lvalue(lvalue)? { + Lvalue::Local{ frame, local, field: None } if self.stack.len() == frame+1 => (frame, local), + _ => return Err(EvalError::Unimplemented("Stroage annotations must refer to locals of the topmost stack frame.".to_owned())) // FIXME maybe this should get its own error type + }; + match stmt.kind { + StorageLive(_) => self.stack[frame].storage_live(local)?, + _ => { + let old_val = self.stack[frame].storage_dead(local)?; + self.deallocate_local(old_val)?; + } + }; + } // Defined to do nothing. These are added by optimization passes, to avoid changing the // size of MIR constantly. @@ -240,7 +251,8 @@ impl<'a, 'b, 'tcx> Visitor<'tcx> for ConstantExtractor<'a, 'b, 'tcx> { constant.span, mir, Lvalue::Global(cid), - StackPopCleanup::MarkStatic(false)) + StackPopCleanup::MarkStatic(false), + ) }); } } From db6ce463fe3d40dac627378bfbaeaa700da43a5c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 1 Jun 2017 11:01:55 -0700 Subject: [PATCH 2/4] fix some nits --- src/eval_context.rs | 8 +++++--- src/lvalue.rs | 2 +- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/eval_context.rs b/src/eval_context.rs index c704ab230d..f364f829f9 100644 --- a/src/eval_context.rs +++ b/src/eval_context.rs @@ -453,7 +453,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { ) -> EvalResult<'tcx> { ::log_settings::settings().indentation += 1; - /// Return the set of locals that have a stroage annotation anywhere + /// Return the set of locals that have a storage annotation anywhere fn collect_storage_annotations<'tcx>(mir: &'tcx mir::Mir<'tcx>) -> HashSet { use rustc::mir::StatementKind::*; @@ -475,10 +475,12 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { // `Value` for that. let annotated_locals = collect_storage_annotations(mir); let num_locals = mir.local_decls.len() - 1; - let mut locals = Vec::with_capacity(num_locals); + let mut locals = vec![None; num_locals]; for i in 0..num_locals { let local = mir::Local::new(i+1); - locals.push(if annotated_locals.contains(&local) { None } else { Some(Value::ByVal(PrimVal::Undef)) }); + if !annotated_locals.contains(&local) { + locals[i] = Some(Value::ByVal(PrimVal::Undef)); + } } self.stack.push(Frame { diff --git a/src/lvalue.rs b/src/lvalue.rs index a09f72134e..ce0651bf12 100644 --- a/src/lvalue.rs +++ b/src/lvalue.rs @@ -130,7 +130,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { Ok(Value::ByRef(ptr)) } Lvalue::Local { frame, local, field } => { - Ok(self.stack[frame].get_local(local, field.map(|(i, _)| i))?) + self.stack[frame].get_local(local, field.map(|(i, _)| i)) } Lvalue::Global(cid) => { Ok(self.globals.get(&cid).expect("global not cached").value) From dd7735b722cc5de7c217012818de355f427d0bb5 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 1 Jun 2017 17:59:00 -0700 Subject: [PATCH 3/4] make StorageLive kill the current value of the local --- src/eval_context.rs | 13 +++++-------- src/step.rs | 9 ++++----- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/eval_context.rs b/src/eval_context.rs index f364f829f9..b923ef24dc 100644 --- a/src/eval_context.rs +++ b/src/eval_context.rs @@ -1720,15 +1720,12 @@ impl<'tcx> Frame<'tcx> { return Ok(()); } - pub fn storage_live(&mut self, local: mir::Local) -> EvalResult<'tcx> { + pub fn storage_live(&mut self, local: mir::Local) -> EvalResult<'tcx, Option> { trace!("{:?} is now live", local); - if self.locals[local.index() - 1].is_some() { - // The variables comes live now, but was already accessed previously, when it was still dead - return Err(EvalError::DeadLocal); - } else { - self.locals[local.index() - 1] = Some(Value::ByVal(PrimVal::Undef)); - } - return Ok(()); + + let old = self.locals[local.index() - 1]; + self.locals[local.index() - 1] = Some(Value::ByVal(PrimVal::Undef)); // StorageLive *always* kills the value that's currently stored + return Ok(old); } /// Returns the old value of the local diff --git a/src/step.rs b/src/step.rs index 1df55a8b62..aef73f8eb0 100644 --- a/src/step.rs +++ b/src/step.rs @@ -132,13 +132,12 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { Lvalue::Local{ frame, local, field: None } if self.stack.len() == frame+1 => (frame, local), _ => return Err(EvalError::Unimplemented("Stroage annotations must refer to locals of the topmost stack frame.".to_owned())) // FIXME maybe this should get its own error type }; - match stmt.kind { + let old_val = match stmt.kind { StorageLive(_) => self.stack[frame].storage_live(local)?, - _ => { - let old_val = self.stack[frame].storage_dead(local)?; - self.deallocate_local(old_val)?; - } + StorageDead(_) => self.stack[frame].storage_dead(local)?, + _ => bug!("We already checked that we are a storage stmt") }; + self.deallocate_local(old_val)?; } // Defined to do nothing. These are added by optimization passes, to avoid changing the From ec7f1d5248fe1cee904a6d8af094167ded779781 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Fri, 2 Jun 2017 06:53:52 +0200 Subject: [PATCH 4/4] Fix typo --- src/step.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/step.rs b/src/step.rs index aef73f8eb0..24a1df51f4 100644 --- a/src/step.rs +++ b/src/step.rs @@ -130,7 +130,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { StorageLive(ref lvalue) | StorageDead(ref lvalue)=> { let (frame, local) = match self.eval_lvalue(lvalue)? { Lvalue::Local{ frame, local, field: None } if self.stack.len() == frame+1 => (frame, local), - _ => return Err(EvalError::Unimplemented("Stroage annotations must refer to locals of the topmost stack frame.".to_owned())) // FIXME maybe this should get its own error type + _ => return Err(EvalError::Unimplemented("Storage annotations must refer to locals of the topmost stack frame.".to_owned())) // FIXME maybe this should get its own error type }; let old_val = match stmt.kind { StorageLive(_) => self.stack[frame].storage_live(local)?,