From a5147aa2d9156bcd7662621eb0288ca4b312cfab Mon Sep 17 00:00:00 2001 From: Bob Nystrom Date: Fri, 8 Feb 2019 17:09:39 -0800 Subject: [PATCH] Add a limited form of re-entrant calls. This doesn't let you arbitrarily call back into the VM from within foreign methods. I'm still not sure if that's even a good idea since God knows what that would mean if you switch fibers while doing that. But this does allow the very important use case of being able to call a foreign method from within a call to wrenCall(). In other words, foreign methods need to always be leaf calls on the call stack, but the root of that stack can now come from runInterpreter() or wrenCall(). Fix #510. --- Makefile | 9 +++-- src/vm/wren_vm.c | 29 ++++++++++----- test/api/call.c | 1 + test/api/call.wren | 1 + test/api/call_calls_foreign.c | 44 +++++++++++++++++++++++ test/api/call_calls_foreign.h | 4 +++ test/api/call_calls_foreign.wren | 17 +++++++++ test/api/main.c | 8 +++++ test/api/reset_stack_after_call_abort.c | 6 ++-- util/xcode/wren.xcodeproj/project.pbxproj | 8 ++++- 10 files changed, 111 insertions(+), 16 deletions(-) create mode 100644 test/api/call_calls_foreign.c create mode 100644 test/api/call_calls_foreign.h create mode 100644 test/api/call_calls_foreign.wren diff --git a/Makefile b/Makefile index abb079f5..74f68129 100644 --- a/Makefile +++ b/Makefile @@ -76,8 +76,7 @@ clean: $(V) rm -rf lib # Run the tests against the debug build of Wren. -test: debug - $(V) $(MAKE) -f util/wren.mk MODE=debug api_test +test: api_test debug $(V) ./util/test.py $(suite) benchmark: release @@ -92,6 +91,10 @@ unit_test: $(V) $(MAKE) -f util/wren.mk MODE=debug unit_test $(V) ./build/debug/test/unit_wrend +# Build API tests. +api_test: + $(V) $(MAKE) -f util/wren.mk MODE=debug api_test + # Generate the Wren site. docs: mkdir -p build @@ -113,4 +116,4 @@ gh-pages: docs amalgamation: src/include/wren.h src/vm/*.h src/vm/*.c ./util/generate_amalgamation.py > build/wren.c -.PHONY: all amalgamation benchmark builtin clean debug docs gh-pages release test vm watchdocs ci ci_32 ci_64 +.PHONY: all amalgamation api_test benchmark builtin clean debug docs gh-pages release test vm watchdocs ci ci_32 ci_64 diff --git a/src/vm/wren_vm.c b/src/vm/wren_vm.c index d1a7784b..4c657d37 100644 --- a/src/vm/wren_vm.c +++ b/src/vm/wren_vm.c @@ -268,8 +268,8 @@ static ObjUpvalue* captureUpvalue(WrenVM* vm, ObjFiber* fiber, Value* local) return createdUpvalue; } -// Closes any open upvates that have been created for stack slots at [last] and -// above. +// Closes any open upvalues that have been created for stack slots at [last] +// and above. static void closeUpvalues(ObjFiber* fiber, Value* last) { while (fiber->openUpvalues != NULL && @@ -370,8 +370,7 @@ static void bindMethod(WrenVM* vm, int methodType, int symbol, static void callForeign(WrenVM* vm, ObjFiber* fiber, WrenForeignMethodFn foreign, int numArgs) { - // Save the current state so we can restore it when done. - Value* apiStack = vm->apiStack; + ASSERT(vm->apiStack == NULL, "Cannot already be in foreign call."); vm->apiStack = fiber->stackTop - numArgs; foreign(vm); @@ -379,7 +378,8 @@ static void callForeign(WrenVM* vm, ObjFiber* fiber, // Discard the stack slots for the arguments and temporaries but leave one // for the result. fiber->stackTop = vm->apiStack + 1; - vm->apiStack = apiStack; + + vm->apiStack = NULL; } // Handles the current fiber having aborted because of an error. @@ -624,13 +624,12 @@ static void createForeign(WrenVM* vm, ObjFiber* fiber, Value* stack) ASSERT(method->type == METHOD_FOREIGN, "Allocator should be foreign."); // Pass the constructor arguments to the allocator as well. - Value* oldApiStack = vm->apiStack; + ASSERT(vm->apiStack == NULL, "Cannot already be in foreign call."); vm->apiStack = stack; method->as.foreign(vm); - vm->apiStack = oldApiStack; - // TODO: Check that allocateForeign was called. + vm->apiStack = NULL; } void wrenFinalizeForeign(WrenVM* vm, ObjForeign* foreign) @@ -1379,12 +1378,24 @@ WrenInterpretResult wrenCall(WrenVM* vm, WrenHandle* method) ASSERT(vm->fiber->stackTop - vm->fiber->stack >= closure->fn->arity, "Stack must have enough arguments for method."); + // Clear the API stack. Now that wrenCall() has control, we no longer need + // it. We use this being non-null to tell if re-entrant calls to foreign + // methods are happening, so it's important to clear it out now so that you + // can call foreign methods from within calls to wrenCall(). + vm->apiStack = NULL; + // Discard any extra temporary slots. We take for granted that the stub // function has exactly one slot for each argument. vm->fiber->stackTop = &vm->fiber->stack[closure->fn->maxSlots]; wrenCallFunction(vm, vm->fiber, closure, 0); - return runInterpreter(vm, vm->fiber); + WrenInterpretResult result = runInterpreter(vm, vm->fiber); + + // If the call didn't abort, then set up the API stack to point to the + // beginning of the stack so the host can access the call's return value. + if (vm->fiber != NULL) vm->apiStack = vm->fiber->stack; + + return result; } WrenHandle* wrenMakeHandle(WrenVM* vm, Value value) diff --git a/test/api/call.c b/test/api/call.c index f8d5c7d7..3b0cd295 100644 --- a/test/api/call.c +++ b/test/api/call.c @@ -66,6 +66,7 @@ void callRunTests(WrenVM* vm) wrenEnsureSlots(vm, 1); wrenSetSlotHandle(vm, 0, callClass); wrenCall(vm, getValue); + printf("slots after call: %d\n", wrenGetSlotCount(vm)); WrenHandle* value = wrenGetSlotHandle(vm, 0); // Different argument types. diff --git a/test/api/call.wren b/test/api/call.wren index cf4f2017..9306c7cc 100644 --- a/test/api/call.wren +++ b/test/api/call.wren @@ -48,6 +48,7 @@ class Call { // expect: subscript 1 2 // expect: subscript set 1 2 3 +// expect: slots after call: 1 // expect: two true false // expect: two 1.2 3.4 // expect: two string another diff --git a/test/api/call_calls_foreign.c b/test/api/call_calls_foreign.c new file mode 100644 index 00000000..b2768ee8 --- /dev/null +++ b/test/api/call_calls_foreign.c @@ -0,0 +1,44 @@ +#include +#include + +#include "wren.h" + +static void api(WrenVM *vm) { + // Grow the slot array. This should trigger the stack to be moved. + wrenEnsureSlots(vm, 10); + wrenSetSlotNewList(vm, 0); + + for (int i = 1; i < 10; i++) + { + wrenSetSlotDouble(vm, i, i); + wrenInsertInList(vm, 0, -1, i); + } +} + +WrenForeignMethodFn callCallsForeignBindMethod(const char* signature) +{ + if (strcmp(signature, "static CallCallsForeign.api()") == 0) return api; + + return NULL; +} + +void callCallsForeignRunTests(WrenVM* vm) +{ + wrenEnsureSlots(vm, 1); + wrenGetVariable(vm, "./test/api/call_calls_foreign", "CallCallsForeign", 0); + WrenHandle* apiClass = wrenGetSlotHandle(vm, 0); + WrenHandle *call = wrenMakeCallHandle(vm, "call(_)"); + + wrenEnsureSlots(vm, 2); + wrenSetSlotHandle(vm, 0, apiClass); + wrenSetSlotString(vm, 1, "parameter"); + + printf("slots before %d\n", wrenGetSlotCount(vm)); + wrenCall(vm, call); + + // We should have a single slot count for the return. + printf("slots after %d\n", wrenGetSlotCount(vm)); + + wrenReleaseHandle(vm, call); + wrenReleaseHandle(vm, apiClass); +} diff --git a/test/api/call_calls_foreign.h b/test/api/call_calls_foreign.h new file mode 100644 index 00000000..2bb3f1bf --- /dev/null +++ b/test/api/call_calls_foreign.h @@ -0,0 +1,4 @@ +#include "wren.h" + +WrenForeignMethodFn callCallsForeignBindMethod(const char* signature); +void callCallsForeignRunTests(WrenVM* vm); diff --git a/test/api/call_calls_foreign.wren b/test/api/call_calls_foreign.wren new file mode 100644 index 00000000..88020188 --- /dev/null +++ b/test/api/call_calls_foreign.wren @@ -0,0 +1,17 @@ +// Regression test for https://github.com/munificent/wren/issues/510. +// +// Tests that re-entrant API calls are handled correctly. The host uses +// `wrenCall()` to invoke `CallCallsForeign.call()`. That in turn calls +// `CallCallsForeign.api()`, which goes back through the API. +class CallCallsForeign { + foreign static api() + + static call(param) { + System.print(api()) + // expect: slots before 2 + // expect: [1, 2, 3, 4, 5, 6, 7, 8, 9] + System.print(param) // expect: parameter + // expect: slots after 1 + return "result" + } +} diff --git a/test/api/main.c b/test/api/main.c index 4065d73f..ee810702 100644 --- a/test/api/main.c +++ b/test/api/main.c @@ -6,6 +6,7 @@ #include "benchmark.h" #include "call.h" +#include "call_calls_foreign.h" #include "call_wren_call_root.h" #include "error.h" #include "get_variable.h" @@ -42,6 +43,9 @@ static WrenForeignMethodFn bindForeignMethod( method = benchmarkBindMethod(fullName); if (method != NULL) return method; + method = callCallsForeignBindMethod(fullName); + if (method != NULL) return method; + method = errorBindMethod(fullName); if (method != NULL) return method; @@ -102,6 +106,10 @@ static void afterLoad(WrenVM* vm) { callRunTests(vm); } + else if (strstr(testName, "/call_calls_foreign.wren") != NULL) + { + callCallsForeignRunTests(vm); + } else if (strstr(testName, "/call_wren_call_root.wren") != NULL) { callWrenCallRootRunTests(vm); diff --git a/test/api/reset_stack_after_call_abort.c b/test/api/reset_stack_after_call_abort.c index a4d09af3..8a85cd7f 100644 --- a/test/api/reset_stack_after_call_abort.c +++ b/test/api/reset_stack_after_call_abort.c @@ -10,7 +10,7 @@ void resetStackAfterCallAbortRunTests(WrenVM* vm) WrenHandle* testClass = wrenGetSlotHandle(vm, 0); WrenHandle* abortFiber = wrenMakeCallHandle(vm, "abortFiber()"); - WrenHandle* afterConstruct = wrenMakeCallHandle(vm, "afterAbort(_,_)"); + WrenHandle* afterAbort = wrenMakeCallHandle(vm, "afterAbort(_,_)"); wrenEnsureSlots(vm, 1); wrenSetSlotHandle(vm, 0, testClass); @@ -20,9 +20,9 @@ void resetStackAfterCallAbortRunTests(WrenVM* vm) wrenSetSlotHandle(vm, 0, testClass); wrenSetSlotDouble(vm, 1, 1.0); wrenSetSlotDouble(vm, 2, 2.0); - wrenCall(vm, afterConstruct); + wrenCall(vm, afterAbort); wrenReleaseHandle(vm, testClass); wrenReleaseHandle(vm, abortFiber); - wrenReleaseHandle(vm, afterConstruct); + wrenReleaseHandle(vm, afterAbort); } diff --git a/util/xcode/wren.xcodeproj/project.pbxproj b/util/xcode/wren.xcodeproj/project.pbxproj index 44755f9d..9aabedcb 100644 --- a/util/xcode/wren.xcodeproj/project.pbxproj +++ b/util/xcode/wren.xcodeproj/project.pbxproj @@ -35,6 +35,7 @@ 2986F6D71ACF93BA00BCE26C /* wren_primitive.c in Sources */ = {isa = PBXBuildFile; fileRef = 2986F6D51ACF93BA00BCE26C /* wren_primitive.c */; }; 29932D511C20D8C900099DEE /* benchmark.c in Sources */ = {isa = PBXBuildFile; fileRef = 29932D4F1C20D8C900099DEE /* benchmark.c */; }; 29932D541C210F8D00099DEE /* lists.c in Sources */ = {isa = PBXBuildFile; fileRef = 29932D521C210F8D00099DEE /* lists.c */; }; + 2993ED1D2111FD6D000F0D49 /* call_calls_foreign.c in Sources */ = {isa = PBXBuildFile; fileRef = 2993ED1C2111FD6D000F0D49 /* call_calls_foreign.c */; }; 29A427341BDBE435001E6E22 /* wren_opt_meta.c in Sources */ = {isa = PBXBuildFile; fileRef = 29A4272E1BDBE435001E6E22 /* wren_opt_meta.c */; }; 29A427351BDBE435001E6E22 /* wren_opt_meta.c in Sources */ = {isa = PBXBuildFile; fileRef = 29A4272E1BDBE435001E6E22 /* wren_opt_meta.c */; }; 29A427361BDBE435001E6E22 /* wren_opt_meta.wren.inc in Sources */ = {isa = PBXBuildFile; fileRef = 29A427301BDBE435001E6E22 /* wren_opt_meta.wren.inc */; }; @@ -152,6 +153,8 @@ 29932D501C20D8C900099DEE /* benchmark.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = benchmark.h; path = ../../test/api/benchmark.h; sourceTree = ""; }; 29932D521C210F8D00099DEE /* lists.c */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.c; name = lists.c; path = ../../test/api/lists.c; sourceTree = ""; }; 29932D531C210F8D00099DEE /* lists.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = lists.h; path = ../../test/api/lists.h; sourceTree = ""; }; + 2993ED1B2111FD6D000F0D49 /* call_calls_foreign.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = call_calls_foreign.h; path = ../../test/api/call_calls_foreign.h; sourceTree = ""; }; + 2993ED1C2111FD6D000F0D49 /* call_calls_foreign.c */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.c; name = call_calls_foreign.c; path = ../../test/api/call_calls_foreign.c; sourceTree = ""; }; 29A4272E1BDBE435001E6E22 /* wren_opt_meta.c */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.c; name = wren_opt_meta.c; path = ../../src/optional/wren_opt_meta.c; sourceTree = ""; }; 29A4272F1BDBE435001E6E22 /* wren_opt_meta.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = wren_opt_meta.h; path = ../../src/optional/wren_opt_meta.h; sourceTree = ""; }; 29A427301BDBE435001E6E22 /* wren_opt_meta.wren.inc */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.pascal; name = wren_opt_meta.wren.inc; path = ../../src/optional/wren_opt_meta.wren.inc; sourceTree = ""; }; @@ -340,9 +343,10 @@ children = ( 29932D4F1C20D8C900099DEE /* benchmark.c */, 29932D501C20D8C900099DEE /* benchmark.h */, + 2993ED1C2111FD6D000F0D49 /* call_calls_foreign.c */, + 2993ED1B2111FD6D000F0D49 /* call_calls_foreign.h */, 29F3D08021039A630082DD88 /* call_wren_call_root.c */, 29F3D07F21039A630082DD88 /* call_wren_call_root.h */, - 29D009A61B7E3993000CE58C /* main.c */, 293D46941BB43F9900200083 /* call.c */, 293D46951BB43F9900200083 /* call.h */, 29AD965F1D0A57F800C4DFE7 /* error.c */, @@ -355,6 +359,7 @@ 29D009AD1B7E39A8000CE58C /* handle.h */, 29932D521C210F8D00099DEE /* lists.c */, 29932D531C210F8D00099DEE /* lists.h */, + 29D009A61B7E3993000CE58C /* main.c */, 29C946961C88F14F00B4A4F3 /* new_vm.c */, 29C946971C88F14F00B4A4F3 /* new_vm.h */, 29D880641DC8ECF600025364 /* reset_stack_after_call_abort.c */, @@ -527,6 +532,7 @@ 29D24DB21E82C0A2006618CC /* user_data.c in Sources */, 29DC14A61BBA3017008A8274 /* wren_primitive.c in Sources */, 29DC14A71BBA301A008A8274 /* wren_utils.c in Sources */, + 2993ED1D2111FD6D000F0D49 /* call_calls_foreign.c in Sources */, 29DC14A81BBA301D008A8274 /* wren_value.c in Sources */, 29A4273B1BDBE435001E6E22 /* wren_opt_random.wren.inc in Sources */, 29D025E61C19CD1000A3BB28 /* os.wren.inc in Sources */,