fix(runtime): unwind all callers when guest code yields to the EE scheduler - #270
Open
drakolordx7 wants to merge 1 commit into
Open
drakolordx7 wants to merge 1 commit into
drakolordx7 wants to merge 1 commit into
Conversation
…eduler Guest code gives control back to the EE scheduler by returning up the host stack: generated functions `return` when eeCheckpointDue() is true (backward branches), and dispatchGuestBranch() returns false when a checkpoint is due before a call. The scheduler then resumes at ctx->pc. dispatchGuestBranch() decides whether a callee returned normally by looking at ctx->pc afterwards, and treats "pc still equals the callee's entry" as an implicit return (it sets pc to the fallthrough and reports success). A yield can leave exactly that pc: a recursive call (the nested dispatch yields with pc = the shared entry), or a loop head that is the function's first instruction. The caller then continued as if the callee had finished, while the abandoned callee's frames were still live and left stale return addresses behind, which showed up as jumps to heap addresses and random crashes in Lua-style recursive code. EeScheduler now counts unwinds (noteGuestUnwind / guestUnwindCount). The count is bumped when eeCheckpointDue() or dispatchGuestBranch() yields, when a non-call branch is handed back to the dispatcher, and when a callee returns somewhere other than its fallthrough. dispatchGuestBranch() compares the count across the callee call and propagates the unwind (returns false, pc untouched) instead of guessing from ctx->pc. Nothing has to be reset, so a stale flag cannot leak into later calls, and the state lives in the scheduler rather than a global. Two new dispatchGuestBranch tests reproduce both cases (recursive yield, yield at the callee entry); they fail on upstream main without the change. The bug was first found in a game with a debug knob that forces a yield at every Nth checkpoint: every run crashed within seconds before a fix with the same semantics (a global "unwinding" flag there) and none did in 120 s after it. This version keeps the semantics but was only checked with the unit tests above and the existing suite, not in that game. Made by drakolord and assisted with Claude Code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Generated code yields to the EE scheduler by returning up the host stack.
dispatchGuestBranchthen inferred a normal return fromctx->pc == entry, which a yield can also produce (a recursive call, or a loop head on the callee's first instruction). The caller carried on while the callee's guest frames were still live, leaving stale return addresses: jumps into heap addresses and random crashes, found in Killzone's Lua interpreter (deep recursion).EeSchedulernow counts unwinds (noteGuestUnwind/guestUnwindCount): bumped on a due checkpoint, on a branch the dispatcher cannot follow in place and on a non-local return.dispatchGuestBranchcompares the count across the callee call and propagates the unwind instead of guessing fromctx->pc. Nothing has to be reset, so a stale flag cannot leak.Two new tests reproduce both cases and fail on main;
ps2x_tests438/438. In the Killzone port a global-flag version with the same semantics went from crashing within seconds (with a forced-yield stress knob) to no crash in 120 s; this counter version has been checked with the unit tests and the existing suite. #137 (fiber scheduler) would make this unnecessary if it lands.Made by drakolord and assisted with Claude Code.