diff --git a/doc/notes/re-entrancy.md b/doc/notes/re-entrancy.md new file mode 100644 index 00000000..cd52719e --- /dev/null +++ b/doc/notes/re-entrancy.md @@ -0,0 +1,144 @@ +## wrenInterpret() + +You can already call out to a foreign method or constructor from within an +execution that was started using `wrenInterpret()`, so I think that's fine. +`wrenInterpret()` doesn't use the API stack at all. + +## wrenCall() + +Normally, when using `wrenCall()` to start executing some code, the API slots +are at the very bottom of the fiber's stack and the fiber has no other +callframes until execution begins. + +When a foreign method or constructor is called, there *are* callframes on the +fiber's stack. There must be, because that's where the arguments to the foreign +method are. + +So, if you `wrenCall()`, which eventually calls a foreign method, the same fiber +will be used for the API twice. This is currently broken. The reason it's broken +is that `callForeign()` and `createForeign()` store the old apiStack pointer +(the one used for the initial `wrenCall()`) in a local variable and then restore +it when the foreign call completes. If a GC or stack grow occurs in the middle +of that, we end up restoring a bad pointer. + +But I don't think we need to preserve apiStack for the `wrenCall()` anyway. As +soon as the user calls `wrenCall()` and it starts running, we no longer need to +track the number of slots allocated for the API. All that matters is that the +one return value is available at the end. + +I think this means it *should* be fairly easy to support: + + wrenCall() -> wren code -> foreign method + +## Foreign calls + +The interesting one is whether you can call `wrenInterpret()` or `wrenCall()` +from within a foreign method. If we're going to allow re-entrancy at all, it +would be nice to completely support it. I do think there are practical uses +for this. + +Calling `wrenInterpret()` should already work, though I don't think it's tested. + +Calling `wrenCall()` is probably broken. It will try to re-use the slots that +are already set up for the foreign call and then who knows what happens if you +start to execute. + +I think a key part of the problem is that we implicitly create or reuse the API +stack as soon as you start messing with slots. So if there already happens to +be an API stack -- because you're in the middle of a foreign method -- it will +incorrectly reuse it when you start preparing for the `wrenCall()`. + +An obvious fix is to add a new function like `wrenPrepareCall()` that explicitly +creates a new API stack -- really a new fiber -- for you to use. We still have +to figure out how to keep track of the current API stack and fiber for the +foreign call so that we can return to it. + +**TODO: more thinking here...** + +If I can figure this out, it means we can do: + + foreign method -> C code -> wrenCall() + +## Nested foreign calls + +If we compose the above it leads to the question of whether you can have +multiple nested foreign calls in-progress at the same time. Can you have a C +stack like: + + wrenCall() + runInterpreter() + foreignCall() + wrenCall() + runInterpreter() + foreignCall() + ... + +This does *not* mean there is a single Wren stack that contains multiple +foreign calls. Since each `wrenCall()` begins a new fiber, any given Wren stack +can only ever have a single foreign API call at the top of the stack. I think +that's a good invariant. + +I believe we should support the above. This means that the core +`runInterpreter()` C function is itself re-entrant. So far, I've always assumed +it would not be, so it probably breaks some assumptions. I'll have to think +through. The main thing that could be problematic is the local variables inside +`runInterpreter()`, but I believe `STORE_FRAME()` and `LOAD_FRAME()` take care +of those. We just need to make sure they get called before any re-entrancy can +happen. That probably means calling them before we invoke a foreign method. + +I'll have to write some tests and see what blows up for this. + +## Calling re-entrant fibers + +Where it gets really confusing is how re-entrant calls interact with fibers. +For example, say you: + + wrenCall() -> creates Fiber #1 + runInterpreter() -> runs Fiber #1 + some Wren code stores current fiber in a variable + foreignCall() + wrenCall() -> creates Fiber #2 + runInterpreter() -> runs Fiber #2 + some Wren code calls or transfers to Fiber #1 + +What happens in this scenario? We definitely want to prevent it. We already +detect and prevent the case where you call a fiber that's already called in the +current *Wren* stack, so we should be able to do something in the above case +too. + +Now that I think about it, you can probably already get yourself in a weird +state if you grab the root fiber and call it. Yeah, I justed tested. This: + + var root = Fiber.current + Fiber.new { + root.call() + System.print(1) + }.call() + System.print(2) + +Segfaults the VM. :( It actually dies when the called child fiber *returns*. The +root call successfully continues executing the root fiber (which is super +weird). Then that completes and control returns to the spawned fiber. Then +*that* completes and tries to return control to the root fiber, but the root is +already done, and it blows up. So the above prints "2" then "1" then dies. + +(If either of the `call()` calls are change to `transfer()`, the script runs +without any problems because then it never tries to unwind back through the +root fiber which already completed.) + +To fix this, when `runInterpreter()` begins executing a root fiber (either from +`wrenCall()` or `wrenInterpret()`), we need to mark it in some way so that it +can't be called or transferred to. + +## Suspending during re-entrancy + +Maybe the weird conceptual case is when you suspend a fiber while there are +multiple re-entrant calls to `runInterpreter()` on the C stack. Ideall, they +would all magically return, but that's obviously not feasible. + +I guess what will/should happen is that just the innermost one suspends. It's +up to the host to handle that fact. I need to think about this more, add some +tests, and work through it. + +I think we'll probably want to add another WrenInterpretResult case for +suspension so that the host can tell that's what happened. diff --git a/src/vm/wren_core.c b/src/vm/wren_core.c index 445038b9..ff037f9b 100644 --- a/src/vm/wren_core.c +++ b/src/vm/wren_core.c @@ -88,8 +88,14 @@ static bool runFiber(WrenVM* vm, ObjFiber* fiber, Value* args, bool isCall, if (isCall) { + // You can't call a called fiber, but you can transfer directly to it, + // which is why this check is gated on `isCall`. This way, after resuming a + // suspended fiber, it will run and then return to the fiber that called it + // and so on. if (fiber->caller != NULL) RETURN_ERROR("Fiber has already been called."); + if (fiber->state == FIBER_ROOT) RETURN_ERROR("Cannot call root fiber."); + // Remember who ran it. fiber->caller = vm->fiber; } @@ -181,7 +187,7 @@ DEF_PRIMITIVE(fiber_try) runFiber(vm, AS_FIBER(args[0]), args, true, false, "try"); // If we're switching to a valid fiber to try, remember that we're trying it. - if (IS_NULL(vm->fiber->error)) vm->fiber->callerIsTrying = true; + if (IS_NULL(vm->fiber->error)) vm->fiber->state = FIBER_TRY; return false; } @@ -192,7 +198,7 @@ DEF_PRIMITIVE(fiber_yield) // Unhook this fiber from the one that called it. current->caller = NULL; - current->callerIsTrying = false; + current->state = FIBER_OTHER; if (vm->fiber != NULL) { @@ -210,7 +216,7 @@ DEF_PRIMITIVE(fiber_yield1) // Unhook this fiber from the one that called it. current->caller = NULL; - current->callerIsTrying = false; + current->state = FIBER_OTHER; if (vm->fiber != NULL) { diff --git a/src/vm/wren_value.c b/src/vm/wren_value.c index 1d517045..f856cd91 100644 --- a/src/vm/wren_value.c +++ b/src/vm/wren_value.c @@ -171,7 +171,7 @@ ObjFiber* wrenNewFiber(WrenVM* vm, ObjClosure* closure) fiber->openUpvalues = NULL; fiber->caller = NULL; fiber->error = NULL_VAL; - fiber->callerIsTrying = false; + fiber->state = FIBER_OTHER; if (closure != NULL) { diff --git a/src/vm/wren_value.h b/src/vm/wren_value.h index 38c496dc..bdc6ed19 100644 --- a/src/vm/wren_value.h +++ b/src/vm/wren_value.h @@ -293,6 +293,24 @@ typedef struct Value* stackStart; } CallFrame; +// Tracks how this fiber has been invoked, aside from the ways that can be +// detected from the state of other fields in the fiber. +typedef enum +{ + // The fiber is being run from another fiber using a call to `try()`. + FIBER_TRY, + + // The fiber was directly invoked by `runInterpreter()`. This means it's the + // initial fiber used by a call to `wrenCall()` or `wrenInterpret()`. + FIBER_ROOT, + + // The fiber is invoked some other way. If [caller] is `NULL` then the fiber + // was invoked using `call()`. If [numFrames] is zero, then the fiber has + // finished running and is done. If [numFrames] is one and that frame's `ip` + // points to the first byte of code, the fiber has not been started yet. + FIBER_OTHER, +} FiberState; + typedef struct sObjFiber { Obj obj; @@ -331,10 +349,7 @@ typedef struct sObjFiber // error object. Otherwise, it will be null. Value error; - // This will be true if the caller that called this fiber did so using "try". - // In that case, if this fiber fails with an error, the error will be given - // to the caller. - bool callerIsTrying; + FiberState state; } ObjFiber; typedef enum diff --git a/src/vm/wren_vm.c b/src/vm/wren_vm.c index 61b05427..d1a7784b 100644 --- a/src/vm/wren_vm.c +++ b/src/vm/wren_vm.c @@ -399,7 +399,7 @@ static void runtimeError(WrenVM* vm) current->error = error; // If the caller ran this fiber using "try", give it the error and stop. - if (current->callerIsTrying) + if (current->state == FIBER_TRY) { // Make the caller's try method return the error message. current->caller->stackTop[-1] = vm->fiber->error; @@ -768,12 +768,12 @@ static Value getModuleVariable(WrenVM* vm, ObjModule* module, } // The main bytecode interpreter loop. This is where the magic happens. It is -// also, as you can imagine, highly performance critical. Returns `true` if the -// fiber completed without error. +// also, as you can imagine, highly performance critical. static WrenInterpretResult runInterpreter(WrenVM* vm, register ObjFiber* fiber) { // Remember the current fiber so we can find it if a GC happens. vm->fiber = fiber; + fiber->state = FIBER_ROOT; // Hoist these into local variables. They are accessed frequently in the loop // but assigned less frequently. Keeping them in locals and updating them when diff --git a/test/api/call_wren_call_root.c b/test/api/call_wren_call_root.c new file mode 100644 index 00000000..6a6cc9d1 --- /dev/null +++ b/test/api/call_wren_call_root.c @@ -0,0 +1,29 @@ +#include +#include + +#include "wren.h" +#include "vm.h" + +void callWrenCallRootRunTests(WrenVM* vm) +{ + wrenEnsureSlots(vm, 1); + wrenGetVariable(vm, "./test/api/call_wren_call_root", "Test", 0); + WrenHandle* testClass = wrenGetSlotHandle(vm, 0); + + WrenHandle* run = wrenMakeCallHandle(vm, "run()"); + + wrenEnsureSlots(vm, 1); + wrenSetSlotHandle(vm, 0, testClass); + WrenInterpretResult result = wrenCall(vm, run); + if (result == WREN_RESULT_RUNTIME_ERROR) + { + setExitCode(70); + } + else + { + printf("Missing runtime error.\n"); + } + + wrenReleaseHandle(vm, testClass); + wrenReleaseHandle(vm, run); +} diff --git a/test/api/call_wren_call_root.h b/test/api/call_wren_call_root.h new file mode 100644 index 00000000..cc737545 --- /dev/null +++ b/test/api/call_wren_call_root.h @@ -0,0 +1,3 @@ +#include "wren.h" + +void callWrenCallRootRunTests(WrenVM* vm); diff --git a/test/api/call_wren_call_root.wren b/test/api/call_wren_call_root.wren new file mode 100644 index 00000000..5227a459 --- /dev/null +++ b/test/api/call_wren_call_root.wren @@ -0,0 +1,12 @@ +class Test { + static run() { + var root = Fiber.current + System.print("begin root") // expect: begin root + + Fiber.new { + System.print("in new fiber") // expect: in new fiber + root.call() // expect runtime error: Cannot call root fiber. + System.print("called root") + }.transfer() + } +} diff --git a/test/api/main.c b/test/api/main.c index bf8445d1..4065d73f 100644 --- a/test/api/main.c +++ b/test/api/main.c @@ -6,6 +6,7 @@ #include "benchmark.h" #include "call.h" +#include "call_wren_call_root.h" #include "error.h" #include "get_variable.h" #include "foreign_class.h" @@ -101,6 +102,10 @@ static void afterLoad(WrenVM* vm) { callRunTests(vm); } + else if (strstr(testName, "/call_wren_call_root.wren") != NULL) + { + callWrenCallRootRunTests(vm); + } else if (strstr(testName, "/reset_stack_after_call_abort.wren") != NULL) { resetStackAfterCallAbortRunTests(vm); @@ -122,5 +127,5 @@ int main(int argc, const char* argv[]) testName = argv[1]; setTestCallbacks(bindForeignMethod, bindForeignClass, afterLoad); runFile(testName); - return 0; + return getExitCode(); } diff --git a/test/core/fiber/call_root.wren b/test/core/fiber/call_root.wren new file mode 100644 index 00000000..7850e79a --- /dev/null +++ b/test/core/fiber/call_root.wren @@ -0,0 +1,8 @@ +var root = Fiber.current +System.print("begin root") // expect: begin root + +Fiber.new { + System.print("in new fiber") // expect: in new fiber + root.call() // expect runtime error: Cannot call root fiber. + System.print("called root") +}.transfer() diff --git a/util/xcode/wren.xcodeproj/project.pbxproj b/util/xcode/wren.xcodeproj/project.pbxproj index 8db3bd3c..44755f9d 100644 --- a/util/xcode/wren.xcodeproj/project.pbxproj +++ b/util/xcode/wren.xcodeproj/project.pbxproj @@ -72,6 +72,7 @@ 29DC14AB1BBA3038008A8274 /* foreign_class.c in Sources */ = {isa = PBXBuildFile; fileRef = 29D009A81B7E39A8000CE58C /* foreign_class.c */; }; 29DC14AC1BBA303D008A8274 /* slots.c in Sources */ = {isa = PBXBuildFile; fileRef = 29D009AA1B7E39A8000CE58C /* slots.c */; }; 29DC14AD1BBA3040008A8274 /* handle.c in Sources */ = {isa = PBXBuildFile; fileRef = 29D009AC1B7E39A8000CE58C /* handle.c */; }; + 29F3D08121039A630082DD88 /* call_wren_call_root.c in Sources */ = {isa = PBXBuildFile; fileRef = 29F3D08021039A630082DD88 /* call_wren_call_root.c */; }; /* End PBXBuildFile section */ /* Begin PBXCopyFilesBuildPhase section */ @@ -186,6 +187,8 @@ 29D880641DC8ECF600025364 /* reset_stack_after_call_abort.c */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.c; name = reset_stack_after_call_abort.c; path = ../../test/api/reset_stack_after_call_abort.c; sourceTree = ""; }; 29D880651DC8ECF600025364 /* reset_stack_after_call_abort.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = reset_stack_after_call_abort.h; path = ../../test/api/reset_stack_after_call_abort.h; sourceTree = ""; }; 29F384111BD19706002F84E0 /* io.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = io.h; path = ../../src/module/io.h; sourceTree = ""; }; + 29F3D07F21039A630082DD88 /* call_wren_call_root.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = call_wren_call_root.h; path = ../../test/api/call_wren_call_root.h; sourceTree = ""; }; + 29F3D08021039A630082DD88 /* call_wren_call_root.c */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.c; name = call_wren_call_root.c; path = ../../test/api/call_wren_call_root.c; sourceTree = ""; }; /* End PBXFileReference section */ /* Begin PBXFrameworksBuildPhase section */ @@ -337,6 +340,8 @@ children = ( 29932D4F1C20D8C900099DEE /* benchmark.c */, 29932D501C20D8C900099DEE /* benchmark.h */, + 29F3D08021039A630082DD88 /* call_wren_call_root.c */, + 29F3D07F21039A630082DD88 /* call_wren_call_root.h */, 29D009A61B7E3993000CE58C /* main.c */, 293D46941BB43F9900200083 /* call.c */, 293D46951BB43F9900200083 /* call.h */, @@ -527,6 +532,7 @@ 29D025E61C19CD1000A3BB28 /* os.wren.inc in Sources */, 29DC14A91BBA302F008A8274 /* wren_vm.c in Sources */, 29DC14AA1BBA3032008A8274 /* main.c in Sources */, + 29F3D08121039A630082DD88 /* call_wren_call_root.c in Sources */, 293B25581CEFD8C7005D9537 /* repl.c in Sources */, 29C80D5A1D73332A00493837 /* reset_stack_after_foreign_construct.c in Sources */, 29AD96611D0A57F800C4DFE7 /* error.c in Sources */,