From 7c74436acd4c5158c99e321d843130945f224877 Mon Sep 17 00:00:00 2001 From: drako <98249188+drakolordx7@users.noreply.github.com> Date: Wed, 30 Sep 2026 18:16:32 -0500 Subject: [PATCH] fix(runtime): unwind all callers when guest code yields to the EE scheduler 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. --- ps2xRuntime/include/runtime/ee_scheduler.h | 7 ++ ps2xRuntime/src/lib/ps2_runtime.cpp | 33 ++++++- ps2xTest/src/ps2_runtime_expansion_tests.cpp | 97 ++++++++++++++++++++ 3 files changed, 135 insertions(+), 2 deletions(-) diff --git a/ps2xRuntime/include/runtime/ee_scheduler.h b/ps2xRuntime/include/runtime/ee_scheduler.h index f247ff998..dda8f5216 100644 --- a/ps2xRuntime/include/runtime/ee_scheduler.h +++ b/ps2xRuntime/include/runtime/ee_scheduler.h @@ -282,6 +282,12 @@ class EeScheduler void accountCycles(uint32_t cycles) noexcept; [[nodiscard]] bool isExecutingGuest() const noexcept; + // Guest code gives control back to the scheduler by returning up the host stack (a due checkpoint, or a + // branch the dispatcher cannot follow in place). Callers of a nested guest function compare this counter + // across the call to tell such an unwind apart from a normal return. + void noteGuestUnwind() noexcept { m_guestUnwindCount.fetch_add(1u, std::memory_order_relaxed); } + [[nodiscard]] uint64_t guestUnwindCount() const noexcept { return m_guestUnwindCount.load(std::memory_order_relaxed); } + // Kernel object API. All calls except postEvent/requestStop execute on the // EE executor and therefore need no host synchronization. void setupCurrentThread(uint32_t stack, uint32_t stackSize, uint32_t gp); @@ -430,6 +436,7 @@ class EeScheduler std::thread::id m_executorThread{}; std::atomic m_running{false}; std::atomic m_guestExecuting{false}; + std::atomic m_guestUnwindCount{0u}; std::atomic m_stopRequested{false}; std::atomic m_checkpointPending{false}; uint32_t m_debugPublishCountdown = 0u; diff --git a/ps2xRuntime/src/lib/ps2_runtime.cpp b/ps2xRuntime/src/lib/ps2_runtime.cpp index 45cb9d267..0fcb2875b 100644 --- a/ps2xRuntime/src/lib/ps2_runtime.cpp +++ b/ps2xRuntime/src/lib/ps2_runtime.cpp @@ -1358,6 +1358,7 @@ bool PS2Runtime::dispatchGuestBranch(uint8_t *rdram, // this charge bounds straight-line call chains that have no local loop. if (m_eeScheduler && m_eeScheduler->checkpointDue(EeScheduler::kGuestDispatchCycles)) { + m_eeScheduler->noteGuestUnwind(); return false; } @@ -1369,6 +1370,10 @@ bool PS2Runtime::dispatchGuestBranch(uint8_t *rdram, } ctx->pc = targetPc; + if (m_eeScheduler) + { + m_eeScheduler->noteGuestUnwind(); + } return false; } @@ -1396,6 +1401,7 @@ bool PS2Runtime::dispatchGuestBranch(uint8_t *rdram, RecompiledFunction targetFn = lookupFunction(targetPc); const uint32_t entryPc = ctx->pc; + const uint64_t unwindCountBefore = m_eeScheduler ? m_eeScheduler->guestUnwindCount() : 0u; targetFn(rdram, ctx, this); if (isStopRequested() || ctx->pc == 0u) @@ -1403,12 +1409,30 @@ bool PS2Runtime::dispatchGuestBranch(uint8_t *rdram, return false; } + // A callee that yielded to the scheduler can leave ctx->pc equal to its own entry (a recursive call, or a loop + // head on its first instruction), which is indistinguishable from an implicit return. The yield is recorded by + // the scheduler instead, and every caller on the host stack has to unwind too. + if (m_eeScheduler && m_eeScheduler->guestUnwindCount() != unwindCountBefore) + { + return false; + } + if (ctx->pc == entryPc) { ctx->pc = fallthroughPc; } - return ctx->pc == fallthroughPc; + if (ctx->pc != fallthroughPc) + { + // Non-local return (longjmp, tail jump): the scheduler continues at ctx->pc. + if (m_eeScheduler) + { + m_eeScheduler->noteGuestUnwind(); + } + return false; + } + + return true; } void PS2Runtime::SignalException(R5900Context *ctx, PS2Exception exception) @@ -2211,7 +2235,12 @@ void PS2Runtime::postEeEvent(EeEvent event) bool PS2Runtime::eeCheckpointDue(uint32_t cycles) noexcept { - return m_eeScheduler->checkpointDue(cycles); + const bool due = m_eeScheduler->checkpointDue(cycles); + if (due) + { + m_eeScheduler->noteGuestUnwind(); + } + return due; } [[noreturn]] void PS2Runtime::eeWaitVSyncTicks(uint32_t ticks, uint32_t resumePc) diff --git a/ps2xTest/src/ps2_runtime_expansion_tests.cpp b/ps2xTest/src/ps2_runtime_expansion_tests.cpp index 4772f3fb7..520dfa8cc 100644 --- a/ps2xTest/src/ps2_runtime_expansion_tests.cpp +++ b/ps2xTest/src/ps2_runtime_expansion_tests.cpp @@ -166,6 +166,53 @@ namespace } } + // Makes the next scheduler checkpoint due, like a timer or vblank event arriving during guest code. + void requestSchedulerCheckpoint(PS2Runtime *runtime) + { + runtime->postEeEvent(EeEvent{EeEventType::ExternalWake, 0u, 0u}); + } + + // Generated code for a recursive call: the callee (same entry as this function) is dispatched, finds a + // checkpoint due, and leaves ctx->pc at its own entry; the caller then returns up the host stack. + void testGuestBranchRecursiveYieldHandler(uint8_t *rdram, R5900Context *ctx, PS2Runtime *runtime) + { + if (!ctx || !runtime) + { + return; + } + + requestSchedulerCheckpoint(runtime); + if (!runtime->dispatchGuestBranch(rdram, + ctx, + 0x3300u, + 0x3304u, + 0x3308u, + PS2Runtime::GuestBranchKind::DirectCall, + "test-recursive-call")) + { + return; + } + setRegU32(*ctx, 2, 0x00BAD001u); + } + + // A function whose first instruction is a loop head: the backward branch finds a checkpoint due, so the + // function returns with ctx->pc still equal to its entry. + void testGuestBranchEntryLoopYieldHandler(uint8_t *, R5900Context *ctx, PS2Runtime *runtime) + { + if (!ctx || !runtime) + { + return; + } + + requestSchedulerCheckpoint(runtime); + ctx->pc = 0x3400u; + if (runtime->eeCheckpointDue()) + { + return; + } + setRegU32(*ctx, 2, 0x00BAD002u); + } + std::atomic gGuestJumpTargetCount{0u}; void testGuestJumpTargetHandler(uint8_t *, R5900Context *, PS2Runtime *) @@ -455,6 +502,56 @@ void register_ps2_runtime_expansion_tests() "callee transfer PC should be preserved"); }); + tc.Run("dispatchGuestBranch does not treat a recursive yield as a return", [](TestCase &t) + { + PS2Runtime runtime; + runtime.registerFunction(0x3300u, &testGuestBranchRecursiveYieldHandler); + + R5900Context ctx{}; + ctx.pc = 0x2000u; + + const bool returnedToFallthrough = runtime.dispatchGuestBranch( + nullptr, + &ctx, + 0x3300u, + 0x2000u, + 0x2008u, + PS2Runtime::GuestBranchKind::DirectCall, + "test-yield-outer"); + + t.IsFalse(returnedToFallthrough, + "a yield inside a nested call must unwind every caller instead of returning to its fallthrough"); + t.Equals(ctx.pc, 0x3300u, + "the yielded callee entry should stay in ctx->pc for the scheduler to resume"); + t.Equals(::getRegU32(&ctx, 2), 0u, + "no caller should continue running after the yield"); + }); + + tc.Run("dispatchGuestBranch does not treat a yield at the callee entry as a return", [](TestCase &t) + { + PS2Runtime runtime; + runtime.registerFunction(0x3400u, &testGuestBranchEntryLoopYieldHandler); + + R5900Context ctx{}; + ctx.pc = 0x2000u; + + const bool returnedToFallthrough = runtime.dispatchGuestBranch( + nullptr, + &ctx, + 0x3400u, + 0x2000u, + 0x2008u, + PS2Runtime::GuestBranchKind::DirectCall, + "test-entry-loop-yield"); + + t.IsFalse(returnedToFallthrough, + "a checkpoint yield that leaves ctx->pc at the callee entry must not look like a return"); + t.Equals(ctx.pc, 0x3400u, + "the callee entry should stay in ctx->pc for the scheduler to resume"); + t.Equals(::getRegU32(&ctx, 2), 0u, + "the callee should not continue past the yield"); + }); + tc.Run("dispatchGuestBranch rejects missing exact targets", [](TestCase &t) { PS2Runtime runtime;