diff --git a/src/vm/wren_vm.c b/src/vm/wren_vm.c index c2c8e8e1..9938a3da 100644 --- a/src/vm/wren_vm.c +++ b/src/vm/wren_vm.c @@ -260,16 +260,22 @@ static ObjUpvalue* captureUpvalue(WrenVM* vm, ObjFiber* fiber, Value* local) return createdUpvalue; } -static void closeUpvalue(ObjFiber* fiber) +// Closes any open upvates that have been created for stack slots at [last] and +// above. +static void closeUpvalues(ObjFiber* fiber, Value* last) { - ObjUpvalue* upvalue = fiber->openUpvalues; - - // Move the value into the upvalue itself and point the upvalue to it. - upvalue->closed = *upvalue->value; - upvalue->value = &upvalue->closed; - - // Remove it from the open upvalue list. - fiber->openUpvalues = upvalue->next; + while (fiber->openUpvalues != NULL && + fiber->openUpvalues->value >= last) + { + ObjUpvalue* upvalue = fiber->openUpvalues; + + // Move the value into the upvalue itself and point the upvalue to it. + upvalue->closed = *upvalue->value; + upvalue->value = &upvalue->closed; + + // Remove it from the open upvalue list. + fiber->openUpvalues = upvalue->next; + } } // Looks up a foreign method in [moduleName] on [className] with [signature]. @@ -1079,7 +1085,8 @@ static WrenInterpretResult runInterpreter(WrenVM* vm, register ObjFiber* fiber) } CASE_CODE(CLOSE_UPVALUE): - closeUpvalue(fiber); + // Close the upvalue for the local if we have one. + closeUpvalues(fiber, fiber->stackTop - 1); DROP(); DISPATCH(); @@ -1089,13 +1096,8 @@ static WrenInterpretResult runInterpreter(WrenVM* vm, register ObjFiber* fiber) fiber->numFrames--; // Close any upvalues still in scope. - Value* firstValue = stackStart; - while (fiber->openUpvalues != NULL && - fiber->openUpvalues->value >= firstValue) - { - closeUpvalue(fiber); - } - + closeUpvalues(fiber, stackStart); + // If the fiber is complete, end it. if (fiber->numFrames == 0) { diff --git a/test/language/closure/unused_closure.wren b/test/language/closure/unused_closure.wren new file mode 100644 index 00000000..8be3553c --- /dev/null +++ b/test/language/closure/unused_closure.wren @@ -0,0 +1,11 @@ +// This is a regression test. There was a bug where the VM would try to close +// an upvalue even if the upvalue was never created because the codepath for +// the closure was not executed. + +{ + var a = "a" + if (false) Fn.new { a } +} + +// If we get here, we didn't segfault when a went out of scope. +System.print("ok") // expect: ok diff --git a/test/language/closure/unused_later_closure.wren b/test/language/closure/unused_later_closure.wren new file mode 100644 index 00000000..43bb58f1 --- /dev/null +++ b/test/language/closure/unused_later_closure.wren @@ -0,0 +1,19 @@ +// This is a regression test. When closing upvalues for discarded locals, it +// wouldn't make sure it discarded the upvalue for the correct stack slot. +// +// Here we create two locals that can be closed over, but only the first one +// actually is. When "b" goes out of scope, we need to make sure we don't +// prematurely close "a". +var closure + +{ + var a = "a" + + { + var b = "b" + closure = Fn.new { a } + if (false) Fn.new { b } + } + + System.print(closure.call()) // expect: a +}