From f23c82071a84f38a51189e115ca409f5f57cad53 Mon Sep 17 00:00:00 2001 From: Bob Nystrom Date: Sat, 21 Jul 2018 10:02:29 -0700 Subject: [PATCH] Don't allow calling the root fiber. The VM used to not detect this case. It meant you could get into a situation where another fiber's caller had completed. Then, when it tried to resume that fiber, the VM would crash because there was nothing to resume to. This is part of thinking through all the cases around re-entrancy. Added some notes for that too. --- doc/notes/re-entrancy.md | 144 ++++++++++++++++++++++ src/vm/wren_core.c | 12 +- src/vm/wren_value.c | 2 +- src/vm/wren_value.h | 23 +++- src/vm/wren_vm.c | 6 +- test/api/call_wren_call_root.c | 29 +++++ test/api/call_wren_call_root.h | 3 + test/api/call_wren_call_root.wren | 12 ++ test/api/main.c | 7 +- test/core/fiber/call_root.wren | 8 ++ util/xcode/wren.xcodeproj/project.pbxproj | 6 + 11 files changed, 240 insertions(+), 12 deletions(-) create mode 100644 doc/notes/re-entrancy.md create mode 100644 test/api/call_wren_call_root.c create mode 100644 test/api/call_wren_call_root.h create mode 100644 test/api/call_wren_call_root.wren create mode 100644 test/core/fiber/call_root.wren 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 */,