From 776120de05fc9f26d1af5da92d8b25ec26648365 Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Sun, 28 Aug 2022 06:07:20 +0300 Subject: fix(tvix/eval): instantiate *new* closures from blueprints each time The previous closure refactoring introduced a bug in which the same closure object would get mutated constantly for each instance of a closure, which is incorrect behaviour. This commit instead introduces an explicit new Value variant for the internal "blueprint" that the compiler generates (essentially just the lambda) and uses this variant to construct the closure at runtime. If the blueprint ever leaks out to a user somehow that is a critical bug and tvix-eval will panic. As a ~treat~ test for this, the fibonacci function is being used as it is a self-recursive closure (i.e. different instantiations of the same "blueprint") getting called with different values and it's good to have it around. Change-Id: I485de675e9bb0c599ed7d5dc0f001eb34ab4c15f Reviewed-on: https://cl.tvl.fyi/c/depot/+/6323 Tested-by: BuildkiteCI Reviewed-by: sterni --- tvix/eval/src/vm.rs | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) (limited to 'tvix/eval/src/vm.rs') diff --git a/tvix/eval/src/vm.rs b/tvix/eval/src/vm.rs index 5d5cb57dba..7b3de64065 100644 --- a/tvix/eval/src/vm.rs +++ b/tvix/eval/src/vm.rs @@ -385,15 +385,19 @@ impl VM { } OpCode::OpClosure(idx) => { - let value = self.chunk().constant(idx).clone(); - self.push(value.clone()); + let blueprint = match self.chunk().constant(idx) { + Value::Blueprint(lambda) => lambda.clone(), + _ => panic!("compiler bug: non-blueprint in blueprint slot"), + }; + + let closure = Closure::new(blueprint); + self.push(Value::Closure(closure.clone())); - // This refers to the same Rc, and from this point - // on internally mutates the closure objects - // upvalues. The closure is already in its stack - // slot, which means that it can capture itself as - // an upvalue for self-recursion. - let closure = value.to_closure()?; + // From this point on we internally mutate the + // closure object's upvalues. The closure is + // already in its stack slot, which means that it + // can capture itself as an upvalue for + // self-recursion. debug_assert!( closure.upvalue_count() > 0, @@ -536,6 +540,6 @@ pub fn run_lambda(lambda: Lambda) -> EvalResult { with_stack: vec![], }; - vm.call(Closure::new(lambda), 0); + vm.call(Closure::new(Rc::new(lambda)), 0); vm.run() } -- cgit 1.4.1