From c303ac001d7c2a47c12a93918d9b73aab5906da6 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 6 Dec 2016 15:41:28 +0100 Subject: [PATCH 1/5] rustup --- src/interpreter/mod.rs | 2 +- src/interpreter/terminator/intrinsics.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 1a52434f9fc..999fec73bd7 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -1798,7 +1798,7 @@ impl IntegerExt for layout::Integer { } -pub fn monomorphize_field_ty<'a, 'tcx:'a >(tcx: TyCtxt<'a, 'tcx, 'tcx>, f: ty::FieldDef<'tcx>, substs: &'tcx Substs<'tcx>) -> Ty<'tcx> { +pub fn monomorphize_field_ty<'a, 'tcx:'a >(tcx: TyCtxt<'a, 'tcx, 'tcx>, f: &ty::FieldDef, substs: &'tcx Substs<'tcx>) -> Ty<'tcx> { let substituted = &f.ty(tcx, substs); tcx.normalize_associated_type(&substituted) } diff --git a/src/interpreter/terminator/intrinsics.rs b/src/interpreter/terminator/intrinsics.rs index 706fadd0ab4..1781211c9c7 100644 --- a/src/interpreter/terminator/intrinsics.rs +++ b/src/interpreter/terminator/intrinsics.rs @@ -489,8 +489,8 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { fn field_ty( &self, param_substs: &Substs<'tcx>, - f: ty::FieldDef<'tcx>, - )-> ty::Ty<'tcx> { + f: &ty::FieldDef, + ) -> ty::Ty<'tcx> { self.tcx.normalize_associated_type(&f.ty(self.tcx, param_substs)) } } From 360ef490f47040e7baac28affb6a0ce531410f2f Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 6 Dec 2016 16:16:22 +0100 Subject: [PATCH 2/5] supply a real "caller" span to drop calls --- src/interpreter/terminator/intrinsics.rs | 6 +++++- src/interpreter/terminator/mod.rs | 6 ++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/interpreter/terminator/intrinsics.rs b/src/interpreter/terminator/intrinsics.rs index 1781211c9c7..9c7b13adf76 100644 --- a/src/interpreter/terminator/intrinsics.rs +++ b/src/interpreter/terminator/intrinsics.rs @@ -190,11 +190,15 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { }; let mut drops = Vec::new(); self.drop(lvalue, ty, &mut drops)?; + let span = { + let frame = self.frame(); + frame.mir[frame.block].terminator().source_info.span + }; // need to change the block before pushing the drop impl stack frames // we could do this for all intrinsics before evaluating the intrinsics, but if // the evaluation fails, we should not have moved forward self.goto_block(target); - return self.eval_drop_impls(drops); + return self.eval_drop_impls(drops, span); } "fabsf32" => { diff --git a/src/interpreter/terminator/mod.rs b/src/interpreter/terminator/mod.rs index e95d68a67db..0b80b633829 100644 --- a/src/interpreter/terminator/mod.rs +++ b/src/interpreter/terminator/mod.rs @@ -118,7 +118,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { let mut drops = Vec::new(); self.drop(lval, ty, &mut drops)?; self.goto_block(target); - self.eval_drop_impls(drops)?; + self.eval_drop_impls(drops, terminator.source_info.span)?; } Assert { ref cond, expected, ref msg, target, .. } => { @@ -151,12 +151,10 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { Ok(()) } - pub fn eval_drop_impls(&mut self, drops: Vec<(DefId, Value, &'tcx Substs<'tcx>)>) -> EvalResult<'tcx, ()> { - let span = self.frame().span; + pub fn eval_drop_impls(&mut self, drops: Vec<(DefId, Value, &'tcx Substs<'tcx>)>, span: Span) -> EvalResult<'tcx, ()> { // add them to the stack in reverse order, because the impl that needs to run the last // is the one that needs to be at the bottom of the stack for (drop_def_id, self_arg, substs) in drops.into_iter().rev() { - // FIXME: supply a real span let mir = self.load_mir(drop_def_id)?; trace!("substs for drop glue: {:?}", substs); self.push_stack_frame( From bfe1efcbf8a30974800ec797bc9fb7307e4b38fb Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 6 Dec 2016 18:13:11 +0100 Subject: [PATCH 3/5] stop leaking memory on closure calls --- src/interpreter/mod.rs | 35 +++++++++------------------- src/interpreter/step.rs | 5 ++-- src/interpreter/terminator/mod.rs | 38 ++++++++++++++++++++++--------- 3 files changed, 41 insertions(+), 37 deletions(-) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index 999fec73bd7..f5da33d4b37 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -82,6 +82,12 @@ pub struct Frame<'tcx> { /// Before being initialized, a local is simply marked as None. pub locals: Vec>, + /// Temporaries introduced to save stackframes + /// This is pure interpreter magic and has nothing to do with how rustc does it + /// An example is calling an FnMut closure that has been converted to a FnOnce closure + /// If they are Value::ByRef, their memory will be freed when the stackframe finishes + pub interpreter_temporaries: Vec, + //////////////////////////////////////////////////////////////////////////////// // Current position within the function //////////////////////////////////////////////////////////////////////////////// @@ -327,6 +333,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { substs: &'tcx Substs<'tcx>, return_lvalue: Lvalue<'tcx>, return_to_block: StackPopCleanup, + temporaries: Vec, ) -> EvalResult<'tcx, ()> { ::log_settings::settings().indentation += 1; @@ -341,6 +348,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { return_to_block: return_to_block, return_lvalue: return_lvalue, locals: locals, + interpreter_temporaries: temporaries, span: span, def_id: def_id, substs: substs, @@ -385,9 +393,8 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { StackPopCleanup::None => {}, } // deallocate all locals that are backed by an allocation - for (i, local) in frame.locals.into_iter().enumerate() { - if let Some(Value::ByRef(ptr)) = local { - trace!("deallocating local {}: {:?}", i + 1, ptr); + for local in frame.locals.into_iter().filter_map(|l| l).chain(frame.interpreter_temporaries) { + if let Value::ByRef(ptr) = local { self.memory.dump(ptr.alloc_id); match self.memory.deallocate(ptr) { // Any frozen memory means that it belongs to a constant or something referenced @@ -1131,27 +1138,6 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { Ok(new_lvalue) } - // FIXME(solson): This method unnecessarily allocates and should not be necessary. We can - // remove it as soon as PrimVal can represent fat pointers. - fn value_to_ptr_dont_use(&mut self, value: Value, ty: Ty<'tcx>) -> EvalResult<'tcx, Pointer> { - match value { - Value::ByRef(ptr) => Ok(ptr), - - Value::ByVal(primval) => { - let ptr = self.alloc_ptr(ty)?; - let kind = self.ty_to_primval_kind(ty)?; - self.memory.write_primval(ptr, primval, kind)?; - Ok(ptr) - } - - Value::ByValPair(a, b) => { - let ptr = self.alloc_ptr(ty)?; - self.write_pair_to_ptr(a, b, ptr, ty)?; - Ok(ptr) - } - } - } - /// ensures this Value is not a ByRef fn follow_by_ref_value(&mut self, value: Value, ty: Ty<'tcx>) -> EvalResult<'tcx, Value> { match value { @@ -1719,6 +1705,7 @@ pub fn eval_main<'a, 'tcx: 'a>( tcx.intern_substs(&[]), Lvalue::from_ptr(Pointer::zst_ptr()), StackPopCleanup::None, + Vec::new(), ).expect("could not allocate first stack frame"); loop { diff --git a/src/interpreter/step.rs b/src/interpreter/step.rs index 1d075fe0e9e..ff4f3b3aa70 100644 --- a/src/interpreter/step.rs +++ b/src/interpreter/step.rs @@ -145,7 +145,7 @@ impl<'a, 'b, 'tcx> ConstantExtractor<'a, 'b, 'tcx> { } else { StackPopCleanup::None }; - this.ecx.push_stack_frame(def_id, span, mir, substs, Lvalue::Global(cid), cleanup) + this.ecx.push_stack_frame(def_id, span, mir, substs, Lvalue::Global(cid), cleanup, Vec::new()) }); } fn try EvalResult<'tcx, ()>>(&mut self, f: F) { @@ -194,7 +194,8 @@ impl<'a, 'b, 'tcx> Visitor<'tcx> for ConstantExtractor<'a, 'b, 'tcx> { mir, this.substs, Lvalue::Global(cid), - StackPopCleanup::Freeze) + StackPopCleanup::Freeze, + Vec::new()) }); } } diff --git a/src/interpreter/terminator/mod.rs b/src/interpreter/terminator/mod.rs index 0b80b633829..8c7d06af419 100644 --- a/src/interpreter/terminator/mod.rs +++ b/src/interpreter/terminator/mod.rs @@ -164,6 +164,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { substs, Lvalue::from_ptr(Pointer::zst_ptr()), StackPopCleanup::None, + Vec::new(), )?; let mut arg_locals = self.frame().mir.args_iter(); let first = arg_locals.next().expect("drop impl has self arg"); @@ -211,11 +212,11 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { } // Only trait methods can have a Self parameter. - let (resolved_def_id, resolved_substs) = + let (resolved_def_id, resolved_substs, temporaries) = if let Some(trait_id) = self.tcx.trait_of_item(def_id) { self.trait_method(trait_id, def_id, substs, &mut args)? } else { - (def_id, substs) + (def_id, substs, Vec::new()) }; let mir = self.load_mir(resolved_def_id)?; @@ -235,6 +236,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { resolved_substs, return_lvalue, return_to_block, + temporaries, )?; let arg_locals = self.frame().mir.args_iter(); @@ -430,7 +432,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { def_id: DefId, substs: &'tcx Substs<'tcx>, args: &mut Vec<(Value, Ty<'tcx>)>, - ) -> EvalResult<'tcx, (DefId, &'tcx Substs<'tcx>)> { + ) -> EvalResult<'tcx, (DefId, &'tcx Substs<'tcx>, Vec)> { let trait_ref = ty::TraitRef::from_method(self.tcx, trait_id, substs); let trait_ref = self.tcx.normalize_associated_type(&ty::Binder(trait_ref)); @@ -442,7 +444,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { // and those from the method: let (did, substs) = find_method(self.tcx, substs, impl_did, vtable_impl.substs, mname); - Ok((did, substs)) + Ok((did, substs, Vec::new())) } traits::VtableClosure(vtable_closure) => { @@ -453,6 +455,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { let closure_kind = self.tcx.closure_kind(vtable_closure.closure_def_id); trace!("closures {:?}, {:?}", closure_kind, trait_closure_kind); self.unpack_fn_args(args)?; + let mut temporaries = Vec::new(); match (closure_kind, trait_closure_kind) { (ty::ClosureKind::Fn, ty::ClosureKind::Fn) | (ty::ClosureKind::FnMut, ty::ClosureKind::FnMut) | @@ -472,23 +475,36 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { // Interpreter magic: insert an intermediate pointer, so we can skip the // intermediate function call. - // FIXME: this is a memory leak, should probably add the pointer to the - // current stack. - let first = self.value_to_ptr_dont_use(args[0].0, args[0].1)?; - args[0].0 = Value::ByVal(PrimVal::from_ptr(first)); + let ptr = match args[0].0 { + Value::ByRef(ptr) => ptr, + Value::ByVal(primval) => { + let ptr = self.alloc_ptr(args[0].1)?; + let kind = self.ty_to_primval_kind(args[0].1)?; + self.memory.write_primval(ptr, primval, kind)?; + temporaries.push(Value::ByRef(ptr)); + ptr + }, + Value::ByValPair(a, b) => { + let ptr = self.alloc_ptr(args[0].1)?; + self.write_pair_to_ptr(a, b, ptr, args[0].1)?; + temporaries.push(Value::ByRef(ptr)); + ptr + }, + }; + args[0].0 = Value::ByVal(PrimVal::from_ptr(ptr)); args[0].1 = self.tcx.mk_mut_ptr(args[0].1); } _ => bug!("cannot convert {:?} to {:?}", closure_kind, trait_closure_kind), } - Ok((vtable_closure.closure_def_id, vtable_closure.substs.substs)) + Ok((vtable_closure.closure_def_id, vtable_closure.substs.substs, temporaries)) } traits::VtableFnPointer(vtable_fn_ptr) => { if let ty::TyFnDef(did, ref substs, _) = vtable_fn_ptr.fn_ty.sty { args.remove(0); self.unpack_fn_args(args)?; - Ok((did, substs)) + Ok((did, substs, Vec::new())) } else { bug!("VtableFnPointer did not contain a concrete function: {:?}", vtable_fn_ptr) } @@ -504,7 +520,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { let fn_ptr = self.memory.read_ptr(vtable.offset(offset))?; let (def_id, substs, _abi, sig) = self.memory.get_fn(fn_ptr.alloc_id)?; *first_ty = sig.inputs[0]; - Ok((def_id, substs)) + Ok((def_id, substs, Vec::new())) } else { Err(EvalError::VtableForArgumentlessMethod) } From 3065273601f2fe7865ffaebbdf750c41f545399c Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 7 Dec 2016 09:19:14 +0100 Subject: [PATCH 4/5] simplify the interpreter locals, since they always must be backed by an allocation --- src/interpreter/mod.rs | 15 +++++++++++---- src/interpreter/terminator/mod.rs | 6 +++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index f5da33d4b37..a8ccfae3dc7 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -86,7 +86,7 @@ pub struct Frame<'tcx> { /// This is pure interpreter magic and has nothing to do with how rustc does it /// An example is calling an FnMut closure that has been converted to a FnOnce closure /// If they are Value::ByRef, their memory will be freed when the stackframe finishes - pub interpreter_temporaries: Vec, + pub interpreter_temporaries: Vec, //////////////////////////////////////////////////////////////////////////////// // Current position within the function @@ -333,7 +333,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { substs: &'tcx Substs<'tcx>, return_lvalue: Lvalue<'tcx>, return_to_block: StackPopCleanup, - temporaries: Vec, + temporaries: Vec, ) -> EvalResult<'tcx, ()> { ::log_settings::settings().indentation += 1; @@ -393,8 +393,9 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { StackPopCleanup::None => {}, } // deallocate all locals that are backed by an allocation - for local in frame.locals.into_iter().filter_map(|l| l).chain(frame.interpreter_temporaries) { - if let Value::ByRef(ptr) = local { + for local in frame.locals.into_iter() { + if let Some(Value::ByRef(ptr)) = local { + trace!("deallocating local"); self.memory.dump(ptr.alloc_id); match self.memory.deallocate(ptr) { // Any frozen memory means that it belongs to a constant or something referenced @@ -406,6 +407,12 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { } } } + // deallocate all temporary allocations + for ptr in frame.interpreter_temporaries { + trace!("deallocating temporary allocation"); + self.memory.dump(ptr.alloc_id); + self.memory.deallocate(ptr)?; + } Ok(()) } diff --git a/src/interpreter/terminator/mod.rs b/src/interpreter/terminator/mod.rs index 8c7d06af419..837c7011125 100644 --- a/src/interpreter/terminator/mod.rs +++ b/src/interpreter/terminator/mod.rs @@ -432,7 +432,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { def_id: DefId, substs: &'tcx Substs<'tcx>, args: &mut Vec<(Value, Ty<'tcx>)>, - ) -> EvalResult<'tcx, (DefId, &'tcx Substs<'tcx>, Vec)> { + ) -> EvalResult<'tcx, (DefId, &'tcx Substs<'tcx>, Vec)> { let trait_ref = ty::TraitRef::from_method(self.tcx, trait_id, substs); let trait_ref = self.tcx.normalize_associated_type(&ty::Binder(trait_ref)); @@ -481,13 +481,13 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> { let ptr = self.alloc_ptr(args[0].1)?; let kind = self.ty_to_primval_kind(args[0].1)?; self.memory.write_primval(ptr, primval, kind)?; - temporaries.push(Value::ByRef(ptr)); + temporaries.push(ptr); ptr }, Value::ByValPair(a, b) => { let ptr = self.alloc_ptr(args[0].1)?; self.write_pair_to_ptr(a, b, ptr, args[0].1)?; - temporaries.push(Value::ByRef(ptr)); + temporaries.push(ptr); ptr }, }; From 5dd01c309fbe730d378c4a55f8d6aadfe9eabeea Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 7 Dec 2016 09:52:22 +0100 Subject: [PATCH 5/5] fix documentation --- src/interpreter/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs index a8ccfae3dc7..1b8072190ba 100644 --- a/src/interpreter/mod.rs +++ b/src/interpreter/mod.rs @@ -82,10 +82,10 @@ pub struct Frame<'tcx> { /// Before being initialized, a local is simply marked as None. pub locals: Vec>, - /// Temporaries introduced to save stackframes + /// Temporary allocations introduced to save stackframes /// This is pure interpreter magic and has nothing to do with how rustc does it /// An example is calling an FnMut closure that has been converted to a FnOnce closure - /// If they are Value::ByRef, their memory will be freed when the stackframe finishes + /// The memory will be freed when the stackframe finishes pub interpreter_temporaries: Vec, ////////////////////////////////////////////////////////////////////////////////