Files
ppsspp/Core/Debugger/WebSocket/SteppingSubscriber.cpp
T
Henrik RydgårdandClaude Sonnet 5 4389f706b9 Debugger: fix a real race in cpu.stepOut/stepOver/runUntil/nextHLE from a delay slot
PrepareResume() used Core_RequestCPUStep(CPUStepType::Into, 1) to step past a
delay slot instruction before deciding whether to add a breakpoint and call
Core_Resume() - but Core_RequestCPUStep() only queues that step for
Core_ProcessStepping() to perform later (on the next iteration of the normal
stepping-mode loop). Every caller (Into's cross-thread branch, Over, Out,
RunUntil, HLE) immediately inspected currentMIPS->pc/inDelaySlot right after
PrepareResume() returned to decide what to do next - reading stale,
pre-step state, since the queued step hadn't run yet.

Worse: those callers then call Core_Resume(), which sets coreState back to
CORE_RUNNING_CPU. Core_ProcessStepping() only processes g_cpuStepCommand
when coreState is CORE_STEPPING_CPU/STEPPING_GE/RUNNING_GE, so once resumed,
the queued step is never processed at all - not just late, silently dropped,
leaving g_cpuStepCommand permanently set until the next Core_Break() resets
it. Any cpu.step*/cpu.runUntil request a client issues in that window (CPU
resumed running, breakpoint not yet hit again) hits
Core_RequestCPUStep()'s "Can't submit two steps in one host frame" guard and
is silently ignored, since none of these call sites check its return value -
this is the "step-out sometimes just doesn't do anything" flakiness reported
against this file.

PrepareResume() is only ever called from within a Core_RunOnCPUThread()
callback, so it's always already running on the CPU thread - safe to
single-step synchronously (currentMIPS->SingleStep(), matching how
Core_PerformCPUStep()'s own CPUStepType::Into case does it) instead of
queuing an async request whose completion every caller then assumes without
verifying.

Verified via UnitTest.exe all (49/49). Attempted to force a live repro via
wsdbg against a delay-slot jal in a demo ELF; wasn't able to reliably
trigger the failure window externally (by the time a client's next command
arrives, the CPU has typically already reached its next breakpoint and
Core_Break() has cleaned up the stale state first) - the race window is
real per the code trace above but appears to be narrow enough that it
mainly shows up under real usage timing (a slow-to-reach next breakpoint,
or a fast follow-up command from a script/UI), not simple synchronous
scripting. The fix is unconditionally more correct regardless: it replaces
a fire-and-forget async request every caller immediately assumed had
already completed with a direct synchronous call that actually has by the
time the next line runs.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GZq8ZtJmFY7bkX5FVkr3P9
2026-08-14 11:04:32 +02:00

324 lines
12 KiB
C++

// Copyright (c) 2018- PPSSPP Project.
// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, version 2.0 or later versions.
// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License 2.0 for more details.
// A copy of the GPL 2.0 should have been included with the program.
// If not, see http://www.gnu.org/licenses/
// Official git repository and contact information can be found at
// https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/.
#include "Common/StringUtils.h"
#include "Core/Debugger/Breakpoints.h"
#include "Core/Debugger/DisassemblyManager.h"
#include "Core/Debugger/WebSocket/SteppingSubscriber.h"
#include "Core/Debugger/WebSocket/WebSocketUtils.h"
#include "Core/Core.h"
#include "Core/HLE/HLE.h"
#include "Core/HLE/sceKernelThread.h"
#include "Core/MIPS/MIPSDebugInterface.h"
#include "Core/MIPS/MIPSStackWalk.h"
using namespace MIPSAnalyst;
struct WebSocketSteppingState : public DebuggerSubscriber {
WebSocketSteppingState() {
g_disassemblyManager.setCpu(currentDebugMIPS);
}
~WebSocketSteppingState() {
g_disassemblyManager.clear();
}
void Into(DebuggerRequest &req);
void Over(DebuggerRequest &req);
void Out(DebuggerRequest &req);
void RunUntil(DebuggerRequest &req);
void HLE(DebuggerRequest &req);
protected:
uint32_t GetNextAddress(DebugInterface *cpuDebug);
void PrepareResume();
void AddThreadCondition(uint32_t breakpointAddress, uint32_t threadID);
};
DebuggerSubscriber *WebSocketSteppingInit(DebuggerEventHandlerMap &map) {
WebSocketSteppingState *p = new WebSocketSteppingState();
map["cpu.stepInto"] = [p](DebuggerRequest &req) { p->Into(req); };
map["cpu.stepOver"] = [p](DebuggerRequest &req) { p->Over(req); };
map["cpu.stepOut"] = [p](DebuggerRequest &req) { p->Out(req); };
map["cpu.runUntil"] = [p](DebuggerRequest &req) { p->RunUntil(req); };
map["cpu.nextHLE"] = [p](DebuggerRequest &req) { p->HLE(req); };
return p;
}
static DebugInterface *CPUFromRequest(DebuggerRequest &req, uint32_t *threadID = nullptr) {
if (!req.HasParam("thread")) {
if (threadID)
*threadID = -1;
return currentDebugMIPS;
}
uint32_t uid;
if (!req.ParamU32("thread", &uid))
return nullptr;
DebugInterface *cpuDebug = KernelDebugThread((SceUID)uid);
if (!cpuDebug)
req.Fail("Thread could not be found");
if (threadID)
*threadID = uid;
return cpuDebug;
}
// Single step into the next instruction (cpu.stepInto)
//
// Parameters:
// - thread: optional number indicating the thread id to plan stepping on.
//
// No immediate response. A cpu.stepping event will be sent once complete.
//
// Note: any thread can wake the cpu when it hits the next instruction currently.
void WebSocketSteppingState::Into(DebuggerRequest &req) {
if (!currentDebugMIPS->isAlive())
return req.Fail("CPU not started");
if (!Core_IsStepping()) {
// Core_Break() is explicitly free-threaded (see Core.cpp), so no need to bounce this to the CPU
// thread - and we can't anyway, since queuing to it only makes sense once the CPU actually *is*
// stepping, which this call is what triggers in the first place.
Core_Break(BreakReason::DebugStepInto, 0);
return;
}
// Route the actual breakpoint/stepping manipulation to the CPU thread instead of poking at it directly
// from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h.
Core_RunOnCPUThread([&] {
uint32_t threadID;
DebugInterface *cpuDebug = CPUFromRequest(req, &threadID);
if (!cpuDebug)
return;
if (cpuDebug == currentDebugMIPS) {
// If the current PC is on a breakpoint, the user doesn't want to do nothing.
g_breakpoints.SetSkipFirst(currentMIPS->pc);
Core_RequestCPUStep(CPUStepType::Into, 1);
} else {
uint32_t breakpointAddress = cpuDebug->GetPC();
PrepareResume();
// Could have advanced to the breakpoint already in PrepareResume().
// Note: we need to get cpuDebug again anyway (in case we ran some HLE above.)
cpuDebug = CPUFromRequest(req);
if (cpuDebug != currentDebugMIPS) {
g_breakpoints.AddBreakPoint(breakpointAddress, true);
AddThreadCondition(breakpointAddress, threadID);
Core_Resume();
}
}
});
}
// Step over the next instruction (cpu.stepOver)
//
// Note: this jumps over function calls, but also delay slots.
//
// Parameters:
// - thread: optional number indicating the thread id to plan stepping on.
//
// No immediate response. A cpu.stepping event will be sent once complete.
//
// Note: any thread can wake the cpu when it hits the next instruction currently.
void WebSocketSteppingState::Over(DebuggerRequest &req) {
if (!currentDebugMIPS->isAlive())
return req.Fail("CPU not started");
if (!Core_IsStepping())
return req.Fail("CPU currently running (cpu.stepping first)");
// Route the actual breakpoint/stepping manipulation to the CPU thread instead of poking at it directly
// from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h.
Core_RunOnCPUThread([&] {
uint32_t threadID;
DebugInterface *cpuDebug = CPUFromRequest(req, &threadID);
if (!cpuDebug)
return;
MipsOpcodeInfo info = GetOpcodeInfo(cpuDebug, cpuDebug->GetPC());
uint32_t breakpointAddress = GetNextAddress(cpuDebug);
if (info.isBranch) {
if (info.isConditional && !info.isLinkedBranch) {
if (info.conditionMet) {
breakpointAddress = info.branchTarget;
} else {
// Skip over the delay slot.
breakpointAddress += 4;
}
} else {
if (info.isLinkedBranch) {
// jal or jalr - a function call. Skip the delay slot.
breakpointAddress += 4;
} else {
// j - for absolute branches, set the breakpoint at the branch target.
breakpointAddress = info.branchTarget;
}
}
}
PrepareResume();
// Could have advanced to the breakpoint already in PrepareResume().
cpuDebug = CPUFromRequest(req);
if (cpuDebug->GetPC() != breakpointAddress) {
g_breakpoints.AddBreakPoint(breakpointAddress, true);
if (cpuDebug != currentDebugMIPS)
AddThreadCondition(breakpointAddress, threadID);
Core_Resume();
}
});
}
// Step out of a function based on a stack walk (cpu.stepOut)
//
// Parameters:
// - thread: optional number indicating the thread id to plan stepping on.
//
// No immediate response. A cpu.stepping event will be sent once complete.
//
// Note: any thread can wake the cpu when it hits the next instruction currently.
void WebSocketSteppingState::Out(DebuggerRequest &req) {
if (!currentDebugMIPS->isAlive())
return req.Fail("CPU not started");
if (!Core_IsStepping())
return req.Fail("CPU currently running (cpu.stepping first)");
// Route the actual breakpoint/stepping manipulation to the CPU thread instead of poking at it directly
// from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h.
Core_RunOnCPUThread([&] {
uint32_t threadID;
DebugInterface *cpuDebug = CPUFromRequest(req, &threadID);
if (!cpuDebug)
return;
std::vector<DebugThreadInfo> threads = GetThreadsInfo();
uint32_t entry = cpuDebug->GetPC();
uint32_t stackTop = 0;
for (const DebugThreadInfo &th : threads) {
if ((threadID == -1 && th.isCurrent) || th.id == threadID) {
entry = th.entrypoint;
stackTop = th.initialStack;
break;
}
}
uint32_t ra = cpuDebug->GetRegValue(0, MIPS_REG_RA);
uint32_t sp = cpuDebug->GetRegValue(0, MIPS_REG_SP);
std::vector<MIPSStackWalk::StackFrame> frames = MIPSStackWalk::Walk(cpuDebug->GetPC(), ra, sp, entry, stackTop);
if (frames.size() < 2) {
return req.Fail("Could not find function call to step out into");
}
uint32_t breakpointAddress = frames[1].pc;
PrepareResume();
// Could have advanced to the breakpoint already in PrepareResume().
cpuDebug = CPUFromRequest(req);
if (cpuDebug->GetPC() != breakpointAddress) {
g_breakpoints.AddBreakPoint(breakpointAddress, true);
if (cpuDebug != currentDebugMIPS)
AddThreadCondition(breakpointAddress, threadID);
Core_Resume();
}
});
}
// Run until a certain address (cpu.runUntil)
//
// Parameters:
// - address: number parameter for destination.
//
// No immediate response. A cpu.stepping event will be sent once complete.
void WebSocketSteppingState::RunUntil(DebuggerRequest &req) {
if (!currentDebugMIPS->isAlive()) {
return req.Fail("CPU not started");
}
uint32_t address = 0;
if (!req.ParamU32("address", &address)) {
// Error already sent.
return;
}
// Route the actual breakpoint/stepping manipulation to the CPU thread instead of poking at it directly
// from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h.
Core_RunOnCPUThread([&] {
bool wasAtAddress = currentMIPS->pc == address;
PrepareResume();
// We may have arrived already if PauseResume() stepped out of a delay slot.
if (currentMIPS->pc != address || wasAtAddress) {
g_breakpoints.AddBreakPoint(address, true);
Core_Resume();
}
});
}
// Jump after the next HLE call (cpu.nextHLE)
//
// No parameters.
//
// No immediate response. A cpu.stepping event will be sent once complete.
void WebSocketSteppingState::HLE(DebuggerRequest &req) {
if (!currentDebugMIPS->isAlive()) {
return req.Fail("CPU not started");
}
// Route the actual breakpoint/stepping manipulation to the CPU thread instead of poking at it directly
// from this WebSocket handler thread - see Core_RunOnCPUThread() in Core.h.
Core_RunOnCPUThread([&] {
PrepareResume();
hleDebugBreak();
Core_Resume();
});
}
uint32_t WebSocketSteppingState::GetNextAddress(DebugInterface *cpuDebug) {
uint32_t current = g_disassemblyManager.getStartAddress(cpuDebug->GetPC());
return g_disassemblyManager.getNthNextAddress(current, 1);
}
void WebSocketSteppingState::PrepareResume() {
if (currentMIPS->inDelaySlot) {
// Delay slot instructions are never joined, so we pass 1.
//
// This must happen synchronously, not via Core_RequestCPUStep(): that only queues the
// step for Core_ProcessStepping() to perform later (on the next iteration of the normal
// stepping-mode loop), while every caller of PrepareResume() immediately inspects
// currentMIPS->pc/inDelaySlot right after this returns to decide whether to add a
// breakpoint and call Core_Resume(). Core_Resume() itself sets coreState back to
// CORE_RUNNING_CPU, which makes Core_ProcessStepping() skip its pending-step check
// entirely - so the queued step was not just late, it was silently dropped, leaving
// g_cpuStepCommand permanently set until the next Core_Break() reset it. Any stepping
// request issued by the debugger client in that window (e.g. a script or fast-clicking
// UI immediately re-stepping instead of waiting for a fresh cpu.stepping event) hit
// Core_RequestCPUStep()'s "Can't submit two steps in one host frame" guard and got
// silently ignored - the "step-out sometimes just doesn't do anything" flakiness this
// was found while tracking down. PrepareResume() is only ever called from within a
// Core_RunOnCPUThread() callback (Into/Over/Out/RunUntil/HLE below), so it's always
// already running on the CPU thread - safe to single-step directly instead of queuing.
currentMIPS->SingleStep();
} else {
// If the current PC is on a breakpoint, the user doesn't want to do nothing.
g_breakpoints.SetSkipFirst(currentMIPS->pc);
}
}
void WebSocketSteppingState::AddThreadCondition(uint32_t breakpointAddress, uint32_t threadID) {
BreakPointCond cond;
cond.debug = currentDebugMIPS;
cond.expressionString = StringFromFormat("threadid == 0x%08x", threadID);
if (initExpression(currentDebugMIPS, cond.expressionString.c_str(), cond.expression))
g_breakpoints.ChangeBreakPointAddCond(breakpointAddress, cond);
}