From b1f0112cef93e8adcd6aedb675187db407842fbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Sat, 8 Aug 2026 11:16:36 +0200 Subject: [PATCH] Debugger: Remove opcode-fusion display and fix cpu step size units DisassemblyManager used to fuse lui+addiu/load/store into single pseudo- instructions ("li", fused loads/stores) for display. This only applied to a handful of opcodes, complicated DisassemblyManager, and was the root cause of a stepping bug: Core_PerformCPUStep's Into/Over cases treated stepSize as a byte count, while the WebSocket cpu.stepInto handler computed it as an instruction count (needed to step over a whole fused macro in one go) - so a plain, non-fused stepInto silently executed zero instructions. Removed the fusion logic entirely (DisassemblyMacro, DISTYPE_MACRO) - every disassembly line is now exactly one 4-byte instruction. With that, "how many instructions does this line span" is always 1, so the getInstructionSizeAt() byte-size queries in the legacy Windows and ImGui debuggers are gone too; step requests just pass 1. Core_RequestCPUStep's stepSize is now consistently in instructions everywhere. Also fixes the PPSSPPHeadless build, broken since 0ed1f3e added OpenWebDebugger() (which calls System_LaunchUrl) without a headless stub. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Hqm11k99viLfbJm2MkH4BH --- Core/Core.cpp | 8 +- Core/Core.h | 1 + Core/Debugger/DisassemblyManager.cpp | 155 ------------------ Core/Debugger/DisassemblyManager.h | 29 +--- Core/Debugger/WebSocket/DisasmSubscriber.cpp | 2 - .../Debugger/WebSocket/SteppingSubscriber.cpp | 8 +- UI/ImDebugger/ImDisasmView.cpp | 23 +-- UI/ImDebugger/ImDisasmView.h | 1 - Windows/Debugger/CtrlDisAsmView.cpp | 10 +- Windows/Debugger/CtrlDisAsmView.h | 1 - Windows/Debugger/Debugger_Disasm.cpp | 3 +- headless/Headless.cpp | 1 + 12 files changed, 16 insertions(+), 226 deletions(-) diff --git a/Core/Core.cpp b/Core/Core.cpp index e8c0000a01..21a3dfa7fc 100644 --- a/Core/Core.cpp +++ b/Core/Core.cpp @@ -274,8 +274,7 @@ bool Core_RequestCPUStep(CPUStepType type, int stepSize) { } // Handles more advanced step types (used by the debugger). -// stepSize is to support stepping through compound instructions like fused lui+ladd (li). -// Yes, our disassembler does support those. +// stepSize is always in instructions (4 bytes each), never bytes. // Doesn't return the new address, as that's just mips->getPC(). // Internal use. static void Core_PerformCPUStep(MIPSDebugInterface *cpu, CPUStepType stepType, int stepSize) { @@ -283,10 +282,9 @@ static void Core_PerformCPUStep(MIPSDebugInterface *cpu, CPUStepType stepType, i case CPUStepType::Into: { u32 currentPc = cpu->GetPC(); - u32 newAddress = currentPc + stepSize; // If the current PC is on a breakpoint, the user still wants the step to happen. g_breakpoints.SetSkipFirst(currentPc); - for (int i = 0; i < (int)(newAddress - currentPc) / 4; i++) { + for (int i = 0; i < stepSize; i++) { currentMIPS->SingleStep(); } break; @@ -294,7 +292,7 @@ static void Core_PerformCPUStep(MIPSDebugInterface *cpu, CPUStepType stepType, i case CPUStepType::Over: { u32 currentPc = cpu->GetPC(); - u32 breakpointAddress = currentPc + stepSize; + u32 breakpointAddress = currentPc + stepSize * 4; g_breakpoints.SetSkipFirst(currentPc); MIPSAnalyst::MipsOpcodeInfo info = MIPSAnalyst::GetOpcodeInfo(cpu, cpu->GetPC()); diff --git a/Core/Core.h b/Core/Core.h index 1272e1ce2d..1ed94874f0 100644 --- a/Core/Core.h +++ b/Core/Core.h @@ -84,6 +84,7 @@ BreakReason Core_BreakReason(); // This should be called externally. // Can fail if another step type was requested this frame. +// stepSize is always in instructions (4 bytes each), never bytes - see Core_PerformCPUStep in Core.cpp. bool Core_RequestCPUStep(CPUStepType stepType, int stepSize); bool Core_NextFrame(); diff --git a/Core/Debugger/DisassemblyManager.cpp b/Core/Debugger/DisassemblyManager.cpp index 39e03307ab..39266230fc 100644 --- a/Core/Debugger/DisassemblyManager.cpp +++ b/Core/Debugger/DisassemblyManager.cpp @@ -595,26 +595,6 @@ void DisassemblyFunction::load() { generateBranchLines(); - // gather all branch targets - std::set branchTargets; - { - std::lock_guard guard(lock_); - for (size_t i = 0; i < lines.size(); i++) - { - switch (lines[i].type) - { - case LINE_DOWN: - branchTargets.insert(lines[i].second); - break; - case LINE_UP: - branchTargets.insert(lines[i].first); - break; - default: - break; - } - } - } - DebugInterface *cpu = g_disassemblyManager.getCpu(); u32 funcPos = address; u32 funcEnd = address+size; @@ -655,7 +635,6 @@ void DisassemblyFunction::load() } MIPSAnalyst::MipsOpcodeInfo opInfo = MIPSAnalyst::GetOpcodeInfo(cpu,funcPos); - u32 opAddress = funcPos; funcPos += 4; // skip branches and their delay slots @@ -665,70 +644,6 @@ void DisassemblyFunction::load() continue; } - // lui - if (MIPS_GET_OP(opInfo.encodedOpcode) == 0x0F && funcPos < funcEnd && funcPos != nextData) - { - MIPSOpcode next = Memory::Read_Instruction(funcPos); - MIPSInfo nextInfo = MIPSGetInfo(next); - - u32 immediate = ((opInfo.encodedOpcode & 0xFFFF) << 16) + (s16)(next.encoding & 0xFFFF); - int rt = MIPS_GET_RT(opInfo.encodedOpcode); - - int nextRs = MIPS_GET_RS(next.encoding); - int nextRt = MIPS_GET_RT(next.encoding); - - // both rs and rt of the second op have to match rt of the first, - // otherwise there may be hidden consequences if the macro is displayed. - // also, don't create a macro if something branches into the middle of it - if (nextRs == rt && nextRt == rt && branchTargets.find(funcPos) == branchTargets.end()) - { - DisassemblyMacro* macro = NULL; - switch (MIPS_GET_OP(next.encoding)) - { - case 0x09: // addiu - macro = new DisassemblyMacro(opAddress); - macro->setMacroLi(immediate,rt); - funcPos += 4; - break; - case 0x20: // lb - case 0x21: // lh - case 0x23: // lw - case 0x24: // lbu - case 0x25: // lhu - case 0x28: // sb - case 0x29: // sh - case 0x2B: // sw - macro = new DisassemblyMacro(opAddress); - - int dataSize = MIPSGetMemoryAccessSize(next); - if (dataSize == 0) { - delete macro; - return; - } - - macro->setMacroMemory(MIPSGetName(next),immediate,rt,dataSize); - funcPos += 4; - break; - } - - if (macro != NULL) - { - if (opcodeSequenceStart != opAddress) - addOpcodeSequence(opcodeSequenceStart,opAddress); - - std::lock_guard guard(lock_); - entries[opAddress] = macro; - for (int i = 0; i < macro->getNumLines(); i++) - { - lineAddresses.push_back(macro->getLineAddress(i)); - } - - opcodeSequenceStart = funcPos; - continue; - } - } - } - // just a normal opcode } @@ -801,76 +716,6 @@ void DisassemblyOpcode::getBranchLines(u32 start, u32 size, std::vectorGetLabelString(immediate); - if (!addressSymbol.empty() && insertSymbols) { - snprintf(buffer, sizeof(buffer), "%s,%s", MIPSDebugInterface::GetRegName(0, rt).c_str(), addressSymbol.c_str()); - } else { - snprintf(buffer, sizeof(buffer), "%s,0x%08X", MIPSDebugInterface::GetRegName(0, rt).c_str(), immediate); - } - - dest.params = buffer; - - dest.info.hasRelevantAddress = true; - dest.info.relevantAddress = immediate; - break; - case MACRO_MEMORYIMM: - dest.name = name; - - addressSymbol = g_symbolMap->GetLabelString(immediate); - if (!addressSymbol.empty() && insertSymbols) { - snprintf(buffer, sizeof(buffer), "%s,%s", MIPSDebugInterface::GetRegName(0, rt).c_str(), addressSymbol.c_str()); - } else { - snprintf(buffer, sizeof(buffer), "%s,0x%08X", MIPSDebugInterface::GetRegName(0, rt).c_str(), immediate); - } - - dest.params = buffer; - - dest.info.isDataAccess = true; - dest.info.dataAddress = immediate; - dest.info.dataSize = dataSize; - - dest.info.hasRelevantAddress = true; - dest.info.relevantAddress = immediate; - break; - default: - return false; - } - - dest.totalSize = getTotalSize(); - return true; -} - DisassemblyData::DisassemblyData(u32 _address, u32 _size, DataType _type): address(_address), size(_size), type(_type) { _dbg_assert_(PSP_GetBootState() == BootState::Complete); diff --git a/Core/Debugger/DisassemblyManager.h b/Core/Debugger/DisassemblyManager.h index ecfde48f81..7c8fc8dc17 100644 --- a/Core/Debugger/DisassemblyManager.h +++ b/Core/Debugger/DisassemblyManager.h @@ -32,7 +32,7 @@ typedef u64 HashType; typedef u32 HashType; #endif -enum DisassemblyLineType { DISTYPE_OPCODE, DISTYPE_MACRO, DISTYPE_DATA, DISTYPE_OTHER }; +enum DisassemblyLineType { DISTYPE_OPCODE, DISTYPE_DATA, DISTYPE_OTHER }; struct DisassemblyLineInfo { @@ -117,33 +117,6 @@ private: }; -class DisassemblyMacro: public DisassemblyEntry -{ -public: - DisassemblyMacro(u32 _address): address(_address) { } - - void setMacroLi(u32 _immediate, u8 _rt); - void setMacroMemory(std::string_view _name, u32 _immediate, u8 _rt, int _dataSize); - - void recheck() override { }; - int getNumLines() override { return 1; }; - int getLineNum(u32 address, bool findStart) override { return 0; }; - u32 getLineAddress(int line) override { return address; }; - u32 getTotalSize() override { return numOpcodes * 4; }; - bool disassemble(u32 address, DisassemblyLineInfo& dest, bool insertSymbols, DebugInterface *cpuDebug) override; -private: - enum MacroType { MACRO_LI, MACRO_MEMORYIMM }; - - MacroType type; - std::string name; - u32 immediate; - u32 address; - u32 numOpcodes; - u8 rt; - int dataSize; -}; - - class DisassemblyData: public DisassemblyEntry { public: diff --git a/Core/Debugger/WebSocket/DisasmSubscriber.cpp b/Core/Debugger/WebSocket/DisasmSubscriber.cpp index 2758183b8a..5d877de3e1 100644 --- a/Core/Debugger/WebSocket/DisasmSubscriber.cpp +++ b/Core/Debugger/WebSocket/DisasmSubscriber.cpp @@ -78,8 +78,6 @@ void WebSocketDisasmState::WriteDisasmLine(JsonWriter &json, const DisassemblyLi json.pushDict(); if (l.type == DISTYPE_OPCODE) json.writeString("type", "opcode"); - else if (l.type == DISTYPE_MACRO) - json.writeString("type", "macro"); else if (l.type == DISTYPE_DATA) json.writeString("type", "data"); else if (l.type == DISTYPE_OTHER) diff --git a/Core/Debugger/WebSocket/SteppingSubscriber.cpp b/Core/Debugger/WebSocket/SteppingSubscriber.cpp index 04c2824455..9e6b3d58a5 100644 --- a/Core/Debugger/WebSocket/SteppingSubscriber.cpp +++ b/Core/Debugger/WebSocket/SteppingSubscriber.cpp @@ -44,7 +44,6 @@ struct WebSocketSteppingState : public DebuggerSubscriber { protected: uint32_t GetNextAddress(DebugInterface *cpuDebug); - int GetNextInstructionCount(DebugInterface *cpuDebug); void PrepareResume(); void AddThreadCondition(uint32_t breakpointAddress, uint32_t threadID); }; @@ -103,8 +102,7 @@ void WebSocketSteppingState::Into(DebuggerRequest &req) { // If the current PC is on a breakpoint, the user doesn't want to do nothing. g_breakpoints.SetSkipFirst(currentMIPS->pc); - int c = GetNextInstructionCount(cpuDebug); - Core_RequestCPUStep(CPUStepType::Into, c); + Core_RequestCPUStep(CPUStepType::Into, 1); } else { uint32_t breakpointAddress = cpuDebug->GetPC(); PrepareResume(); @@ -267,10 +265,6 @@ uint32_t WebSocketSteppingState::GetNextAddress(DebugInterface *cpuDebug) { return g_disassemblyManager.getNthNextAddress(current, 1); } -int WebSocketSteppingState::GetNextInstructionCount(DebugInterface *cpuDebug) { - return (GetNextAddress(cpuDebug) - cpuDebug->GetPC()) / 4; -} - void WebSocketSteppingState::PrepareResume() { if (currentMIPS->inDelaySlot) { // Delay slot instructions are never joined, so we pass 1. diff --git a/UI/ImDebugger/ImDisasmView.cpp b/UI/ImDebugger/ImDisasmView.cpp index 1fd8ca1a7f..a72461953a 100644 --- a/UI/ImDebugger/ImDisasmView.cpp +++ b/UI/ImDebugger/ImDisasmView.cpp @@ -465,7 +465,7 @@ void ImDisasmView::FollowBranch() { DisassemblyLineInfo line; g_disassemblyManager.getLine(curAddress_, true, line, debugger_); - if (line.type == DISTYPE_OPCODE || line.type == DISTYPE_MACRO) { + if (line.type == DISTYPE_OPCODE) { if (line.info.isBranch) { jumpStack_.push_back(curAddress_); gotoAddr(line.info.branchTarget); @@ -890,7 +890,7 @@ void ImDisasmView::updateStatusBarText() { g_disassemblyManager.getLine(curAddress_, true, line, debugger_); text[0] = 0; - if (line.type == DISTYPE_OPCODE || line.type == DISTYPE_MACRO) { + if (line.type == DISTYPE_OPCODE) { if (line.info.hasRelevantAddress && IsLikelyStringAt(line.info.relevantAddress)) { snprintf(text, sizeof(text), "[%08X] = \"%s\"", line.info.relevantAddress, Memory::GetCharPointer(line.info.relevantAddress)); } @@ -1162,13 +1162,6 @@ void ImDisasmView::scrollStepping(u32 newPc) { } } -u32 ImDisasmView::getInstructionSizeAt(u32 address) { - u32 start = g_disassemblyManager.getStartAddress(address); - u32 next = g_disassemblyManager.getNthNextAddress(start, 1); - return next - address; -} - - void ImDisasmWindow::Draw(MIPSDebugInterface *mipsDebug, ImConfig &cfg, ImControl &control, CoreState coreState) { disasmView_.setDebugger(mipsDebug); @@ -1181,12 +1174,10 @@ void ImDisasmWindow::Draw(MIPSDebugInterface *mipsDebug, ImConfig &cfg, ImContro if (ImGui::IsWindowFocused()) { // Process stepping keyboard shortcuts. if (ImGui::IsKeyPressed(ImGuiKey_F10)) { - u32 stepSize = disasmView_.getInstructionSizeAt(mipsDebug->GetPC()); - Core_RequestCPUStep(CPUStepType::Over, stepSize); + Core_RequestCPUStep(CPUStepType::Over, 1); } if (ImGui::IsKeyPressed(ImGuiKey_F11)) { - u32 stepSize = disasmView_.getInstructionSizeAt(mipsDebug->GetPC()); - Core_RequestCPUStep(CPUStepType::Into, stepSize); + Core_RequestCPUStep(CPUStepType::Into, 1); } } @@ -1219,8 +1210,7 @@ void ImDisasmWindow::Draw(MIPSDebugInterface *mipsDebug, ImConfig &cfg, ImContro ImGui::SameLine(); if (ImGui::RepeatButtonShift("Into")) { - u32 stepSize = disasmView_.getInstructionSizeAt(mipsDebug->GetPC()); - Core_RequestCPUStep(CPUStepType::Into, stepSize); + Core_RequestCPUStep(CPUStepType::Into, 1); } if (ImGui::IsItemHovered()) { ImGui::SetTooltip("F11"); @@ -1228,8 +1218,7 @@ void ImDisasmWindow::Draw(MIPSDebugInterface *mipsDebug, ImConfig &cfg, ImContro ImGui::SameLine(); if (ImGui::SmallButton("Over")) { - u32 stepSize = disasmView_.getInstructionSizeAt(mipsDebug->GetPC()); - Core_RequestCPUStep(CPUStepType::Over, stepSize); + Core_RequestCPUStep(CPUStepType::Over, 1); } if (ImGui::IsItemHovered()) { ImGui::SetTooltip("F10"); diff --git a/UI/ImDebugger/ImDisasmView.h b/UI/ImDebugger/ImDisasmView.h index f70f7291f4..573288e858 100644 --- a/UI/ImDebugger/ImDisasmView.h +++ b/UI/ImDebugger/ImDisasmView.h @@ -58,7 +58,6 @@ public: } void scrollStepping(u32 newPc); - u32 getInstructionSizeAt(u32 address); // not const because it might have to analyze. void gotoAddr(unsigned int addr) { if (positionLocked_ != 0) diff --git a/Windows/Debugger/CtrlDisAsmView.cpp b/Windows/Debugger/CtrlDisAsmView.cpp index 8f328112c6..6d78ed67f8 100644 --- a/Windows/Debugger/CtrlDisAsmView.cpp +++ b/Windows/Debugger/CtrlDisAsmView.cpp @@ -602,7 +602,7 @@ void CtrlDisAsmView::followBranch() DisassemblyLineInfo line; g_disassemblyManager.getLine(curAddress, true, line, debugger); - if (line.type == DISTYPE_OPCODE || line.type == DISTYPE_MACRO) + if (line.type == DISTYPE_OPCODE) { if (line.info.isBranch) { @@ -1116,7 +1116,7 @@ void CtrlDisAsmView::updateStatusBarText() g_disassemblyManager.getLine(curAddress,true,line, debugger); text[0] = 0; - if (line.type == DISTYPE_OPCODE || line.type == DISTYPE_MACRO) + if (line.type == DISTYPE_OPCODE) { if (line.info.hasRelevantAddress && IsLikelyStringAt(line.info.relevantAddress)) { snprintf(text, sizeof(text), "[%08X] = \"%s\"", line.info.relevantAddress, Memory::GetCharPointer(line.info.relevantAddress)); @@ -1350,9 +1350,3 @@ void CtrlDisAsmView::scrollStepping(u32 newPc) } } -u32 CtrlDisAsmView::getInstructionSizeAt(u32 address) -{ - u32 start = g_disassemblyManager.getStartAddress(address); - u32 next = g_disassemblyManager.getNthNextAddress(start,1); - return next - address; -} diff --git a/Windows/Debugger/CtrlDisAsmView.h b/Windows/Debugger/CtrlDisAsmView.h index 2f661ec659..727fd55573 100644 --- a/Windows/Debugger/CtrlDisAsmView.h +++ b/Windows/Debugger/CtrlDisAsmView.h @@ -119,7 +119,6 @@ public: } void scrollStepping(u32 newPc); - u32 getInstructionSizeAt(u32 address); void gotoAddr(unsigned int addr) { diff --git a/Windows/Debugger/Debugger_Disasm.cpp b/Windows/Debugger/Debugger_Disasm.cpp index ef577edf23..e0c294e772 100644 --- a/Windows/Debugger/Debugger_Disasm.cpp +++ b/Windows/Debugger/Debugger_Disasm.cpp @@ -206,8 +206,7 @@ void CDisasm::step(CPUStepType stepType) { ptr->setDontRedraw(true); lastTicks_ = CoreTiming::GetTicks(); - u32 stepSize = ptr->getInstructionSizeAt(cpu->GetPC()); - Core_RequestCPUStep(stepType, stepSize); + Core_RequestCPUStep(stepType, 1); } void CDisasm::runToLine() { diff --git a/headless/Headless.cpp b/headless/Headless.cpp index a449d6b7b7..af52010547 100644 --- a/headless/Headless.cpp +++ b/headless/Headless.cpp @@ -92,6 +92,7 @@ bool System_AudioRecordingState() { return false; } void NativeFrame(GraphicsContext *graphicsContext) { } void NativeResized() { } +void System_LaunchUrl(LaunchUrlType urlType, std::string_view url) {} std::string System_GetProperty(SystemProperty prop) { return ""; } std::vector System_GetPropertyStringVec(SystemProperty prop) { return std::vector(); } int64_t System_GetPropertyInt(SystemProperty prop) {