Hook up the new (bare-bones) ImGui debug window to GPU stepping, fix stepping.

Core/Core.cpp

@@ -322,6 +322,11 @@ void Core_ProcessStepping(MIPSDebugInterface *cpu) {
 
 // Free-threaded (hm, possibly except tracing).
 void Core_Break(const char *reason, u32 relatedAddress) {
+	if (coreState != CORE_RUNNING_CPU) {
+		ERROR_LOG(Log::CPU, "Core_Break ony works in the CORE_RUNNING_CPU state");
+		return;
+	}
+
 	// Stop the tracer
 	{
 		std::lock_guard<std::mutex> lock(g_stepMutex);

Core/Core.h

@@ -46,7 +46,8 @@ enum class CPUStepType {
 
 // Async, called from gui
 void Core_Break(const char *reason, u32 relatedAddress = 0);
-// void Core_Step(CPUStepType type);  // CPUStepType::None not allowed
+
+// Resumes execution. Works both when stepping the CPU and the GE.
 void Core_Resume();
 
 // This should be called externally.

Core/Debugger/DisassemblyManager.cpp

@@ -39,8 +39,7 @@ std::recursive_mutex DisassemblyManager::entriesLock_;
 DebugInterface* DisassemblyManager::cpu;
 int DisassemblyManager::maxParamChars = 29;
 
-bool isInInterval(u32 start, u32 size, u32 value)
+bool isInInterval(u32 start, u32 size, u32 value) {
-{
 	return start <= value && value <= (start+size-1);
 }
 
@@ -196,7 +195,6 @@ void DisassemblyManager::analyze(u32 address, u32 size = 1024)
 		if (!PSP_IsInited())
 			return;
 
-		auto memLock = Memory::Lock();
 		std::lock_guard<std::recursive_mutex> guard(entriesLock_);
 		auto it = findDisassemblyEntry(entries, address, false);
 		if (it != entries.end())
@@ -285,12 +283,9 @@ std::vector<BranchLine> DisassemblyManager::getBranchLines(u32 start, u32 size)
 
 void DisassemblyManager::getLine(u32 address, bool insertSymbols, DisassemblyLineInfo &dest, DebugInterface *cpuDebug)
 {
-	// This is here really to avoid lock ordering issues.
-	auto memLock = Memory::Lock();
 	std::lock_guard<std::recursive_mutex> guard(entriesLock_);
 	auto it = findDisassemblyEntry(entries,address,false);
-	if (it == entries.end())
+	if (it == entries.end()) {
-	{
 		analyze(address);
 		it = findDisassemblyEntry(entries,address,false);
 	}
@@ -319,7 +314,6 @@ void DisassemblyManager::getLine(u32 address, bool insertSymbols, DisassemblyLin
 
 u32 DisassemblyManager::getStartAddress(u32 address)
 {
-	auto memLock = Memory::Lock();
 	std::lock_guard<std::recursive_mutex> guard(entriesLock_);
 	auto it = findDisassemblyEntry(entries,address,false);
 	if (it == entries.end())
@@ -337,15 +331,13 @@ u32 DisassemblyManager::getStartAddress(u32 address)
 
 u32 DisassemblyManager::getNthPreviousAddress(u32 address, int n)
 {
-	auto memLock = Memory::Lock();
 	std::lock_guard<std::recursive_mutex> guard(entriesLock_);
 	while (Memory::IsValidAddress(address))
 	{
 		auto it = findDisassemblyEntry(entries,address,false);
 		if (it == entries.end())
 			break;
-		while (it != entries.end())
+		while (it != entries.end()) {
-		{
 			DisassemblyEntry* entry = it->second;
 			int oldLineNum = entry->getLineNum(address,true);
 			if (n <= oldLineNum)
@@ -366,7 +358,6 @@ u32 DisassemblyManager::getNthPreviousAddress(u32 address, int n)
 
 u32 DisassemblyManager::getNthNextAddress(u32 address, int n)
 {
-	auto memLock = Memory::Lock();
 	std::lock_guard<std::recursive_mutex> guard(entriesLock_);
 	while (Memory::IsValidAddress(address))
 	{
@@ -401,7 +392,6 @@ DisassemblyManager::~DisassemblyManager() {
 
 void DisassemblyManager::clear()
 {
-	auto memLock = Memory::Lock();
 	std::lock_guard<std::recursive_mutex> guard(entriesLock_);
 	for (auto it = entries.begin(); it != entries.end(); it++)
 	{
@@ -412,7 +402,6 @@ void DisassemblyManager::clear()
 
 DisassemblyFunction::DisassemblyFunction(u32 _address, u32 _size): address(_address), size(_size)
 {
-	auto memLock = Memory::Lock();
 	if (!PSP_IsInited())
 		return;
 
@@ -426,7 +415,6 @@ DisassemblyFunction::~DisassemblyFunction() {
 
 void DisassemblyFunction::recheck()
 {
-	auto memLock = Memory::Lock();
 	if (!PSP_IsInited())
 		return;
 
@@ -888,7 +876,6 @@ bool DisassemblyMacro::disassemble(u32 address, DisassemblyLineInfo &dest, bool
 
 DisassemblyData::DisassemblyData(u32 _address, u32 _size, DataType _type): address(_address), size(_size), type(_type)
 {
-	auto memLock = Memory::Lock();
 	if (!PSP_IsInited())
 		return;
 
@@ -898,7 +885,6 @@ DisassemblyData::DisassemblyData(u32 _address, u32 _size, DataType _type): addre
 
 void DisassemblyData::recheck()
 {
-	auto memLock = Memory::Lock();
 	if (!PSP_IsInited())
 		return;
 

GPU/Common/GPUDebugInterface.h

@@ -209,7 +209,7 @@ public:
 		return DisassembleOp(pc, Memory::Read_U32(pc));
 	}
 	virtual GPUDebugOp DisassembleOp(u32 pc, u32 op) = 0;
-	virtual std::vector<GPUDebugOp> DissassembleOpRange(u32 startpc, u32 endpc) = 0;
+	virtual std::vector<GPUDebugOp> DisassembleOpRange(u32 startpc, u32 endpc) = 0;
 
 	// Enter/exit stepping mode.  Mainly for better debug stats on time taken.
 	virtual void NotifySteppingEnter() = 0;

GPU/Common/TextureCacheCommon.cpp

@@ -3101,7 +3101,7 @@ void TextureCacheCommon::DrawImGuiDebug(uint64_t &selectedTextureId) const {
 		ImGui::Image(texId, ImVec2(128, 128));
 	}
 
-	if (!secondCache_.size()) {
+	if (!secondCache_.empty()) {
 		ImGui::Text("Secondary Cache (%d): TODO", (int)secondCache_.size());
 		// TODO
 	}

GPU/Debugger/Debugger.cpp

@@ -39,6 +39,7 @@ static int thisFlipNum = 0;
 bool g_drawNotified = false;
 
 static double lastStepTime = -1.0;
+static uint32_t g_skipPcOnce = 0;
 
 static std::vector<std::pair<int, int>> restrictPrimRanges;
 static std::string restrictPrimRule;
@@ -97,17 +98,6 @@ void SetBreakCount(int c, bool relative) {
 	}
 }
 
-static bool IsBreakpoint(u32 pc, u32 op) {
-	if (breakNext == BreakNext::OP) {
-		return true;
-	} else if (breakNext == BreakNext::COUNT) {
-		return primsThisFrame == breakAtCount;
-	} else if (hasBreakpoints) {
-		return GPUBreakpoints::IsBreakpoint(pc, op);
-	}
-	return false;
-}
-
 NotifyResult NotifyCommand(u32 pc) {
 	if (!active) {
 		_dbg_assert_(false);
@@ -143,7 +133,23 @@ NotifyResult NotifyCommand(u32 pc) {
 		}
 	}
 
-	if (IsBreakpoint(pc, op)) {
+	bool isBreakpoint = false;
+	if (breakNext == BreakNext::OP) {
+		isBreakpoint = true;
+	} else if (breakNext == BreakNext::COUNT) {
+		isBreakpoint = primsThisFrame == breakAtCount;
+	} else if (hasBreakpoints) {
+		isBreakpoint = GPUBreakpoints::IsBreakpoint(pc, op);
+	}
+
+	if (isBreakpoint && pc == g_skipPcOnce) {
+		INFO_LOG(Log::G3D, "Skipping break at %08x (last break was here)", g_skipPcOnce);
+		g_skipPcOnce = 0;
+		return process ? NotifyResult::Execute : NotifyResult::Skip;
+	}
+	g_skipPcOnce = 0;
+
+	if (isBreakpoint) {
 		GPUBreakpoints::ClearTempBreakpoints();
 
 		if (coreState == CORE_POWERDOWN || !gpuDebug) {
@@ -158,6 +164,8 @@ NotifyResult NotifyCommand(u32 pc) {
 		} else {
 			NOTICE_LOG(Log::G3D, "Waiting at %08x, %s", pc, info.desc.c_str());
 		}
+
+		g_skipPcOnce = pc;
 		return NotifyResult::Break;  // new. caller will call GPUStepping::EnterStepping().
 	}
 

GPU/Debugger/Playback.cpp

@@ -338,7 +338,7 @@ void DumpExecute::SyncStall() {
 	gpu->UpdateStall(execListID, execListPos, &runList);
 	if (runList) {
 		DLResult result = gpu->ProcessDLQueue();
-		_dbg_assert_(result == DLResult::Done || result == DLResult::Pause);
+		_dbg_assert_(result == DLResult::Done || result == DLResult::Stall);
 	}
 	s64 listTicks = gpu->GetListTicks(execListID);
 	if (listTicks != -1) {

GPU/GPUCommon.cpp

@@ -1637,7 +1637,7 @@ GPUDebugOp GPUCommon::DisassembleOp(u32 pc, u32 op) {
 	return info;
 }
 
-std::vector<GPUDebugOp> GPUCommon::DissassembleOpRange(u32 startpc, u32 endpc) {
+std::vector<GPUDebugOp> GPUCommon::DisassembleOpRange(u32 startpc, u32 endpc) {
 	char buffer[1024];
 	std::vector<GPUDebugOp> result;
 	GPUDebugOp info;
@@ -1995,12 +1995,32 @@ bool GPUCommon::DescribeCodePtr(const u8 *ptr, std::string &name) {
 }
 
 void GPUCommon::DrawImGuiDebugger() {
+	// Proof of concept
+	if (ImGui::Button("Run")) {
+		Core_Resume();
+	}
+	ImGui::SameLine();
+	if (ImGui::Button("Next Tex")) {
+		GPUDebug::SetBreakNext(GPUDebug::BreakNext::TEX);
+	}
+	ImGui::SameLine();
+	if (ImGui::Button("Next Prim")) {
+		GPUDebug::SetBreakNext(GPUDebug::BreakNext::PRIM);
+	}
+	ImGui::SameLine();
+	if (ImGui::Button("Single step")) {
+		GPUDebug::SetBreakNext(GPUDebug::BreakNext::OP);
+	}
+
 	// First, let's list any active display lists.
 	ImGui::Text("Next list ID: %d", nextListID);
 	for (auto index : dlQueue) {
 		const auto &list = dls[index];
-		ImGui::Text("List %d", list.id);
+		char title[64];
-		ImGui::Text("pc: %08x (start: %08x)", list.pc, list.startpc);
+		snprintf(title, sizeof(title), "List %d", list.id);
-		ImGui::Text("bbox: %d", (int)list.bboxResult);
+		if (ImGui::CollapsingHeader(title, ImGuiTreeNodeFlags_DefaultOpen)) {
+			ImGui::Text("PC: %08x (start: %08x)", list.pc, list.startpc);
+			ImGui::Text("BBOX result: %d", (int)list.bboxResult);
+		}
 	}
 }

GPU/GPUCommon.h

@@ -141,12 +141,6 @@ namespace Draw {
 class DrawContext;
 }
 
-enum class DLResult {
-	Done,
-	Error,
-	Pause,  // used for stepping, breakpoints
-};
-
 enum DrawType {
 	DRAW_UNKNOWN,
 	DRAW_PRIM,
@@ -355,7 +349,7 @@ public:
 	void ResetListState(int listID, DisplayListState state) override;
 
 	GPUDebugOp DisassembleOp(u32 pc, u32 op) override;
-	std::vector<GPUDebugOp> DissassembleOpRange(u32 startpc, u32 endpc) override;
+	std::vector<GPUDebugOp> DisassembleOpRange(u32 startpc, u32 endpc) override;
 
 	void NotifySteppingEnter() override;
 	void NotifySteppingExit() override;

GPU/GPUDefinitions.h

@@ -17,6 +17,11 @@
 
 #pragma once
 
+// X11, sigh.
+#ifdef None
+#undef None
+#endif
+
 enum DisplayListStatus {
 	// The list has been completed
 	PSP_GE_LIST_COMPLETED = 0,

UI/ImDebugger/ImDebugger.cpp

@@ -132,6 +132,7 @@ static const char *ThreadStatusToString(u32 status) {
 }
 
 void DrawThreadView(ImConfig &cfg) {
+	ImGui::SetNextWindowSize(ImVec2(420, 300), ImGuiCond_FirstUseEver);
 	if (!ImGui::Begin("Threads", &cfg.threadsOpen)) {
 		ImGui::End();
 		return;
@@ -213,6 +214,7 @@ static void RecurseFileSystem(IFileSystem *fs, std::string path) {
 }
 
 static void DrawFilesystemBrowser(ImConfig &cfg) {
+	ImGui::SetNextWindowSize(ImVec2(420, 500), ImGuiCond_FirstUseEver);
 	if (!ImGui::Begin("File System", &cfg.filesystemBrowserOpen)) {
 		ImGui::End();
 		return;
@@ -738,7 +740,6 @@ ImDebugger::~ImDebugger() {
 	cfg_.SaveConfig(ConfigPath());
 }
 
-
 void ImDebugger::Frame(MIPSDebugInterface *mipsDebug, GPUDebugInterface *gpuDebug) {
 	// Snapshot the coreState to avoid inconsistency.
 	const CoreState coreState = ::coreState;
@@ -753,15 +754,17 @@ void ImDebugger::Frame(MIPSDebugInterface *mipsDebug, GPUDebugInterface *gpuDebu
 
 	if (ImGui::BeginMainMenuBar()) {
 		if (ImGui::BeginMenu("Debug")) {
-			if (coreState == CoreState::CORE_STEPPING_CPU) {
+			switch (coreState) {
+			case CoreState::CORE_STEPPING_CPU:
 				if (ImGui::MenuItem("Run")) {
 					Core_Resume();
 				}
-				// used to have the step commands here, but they belong in the disassembly window.
+				break;
-			} else {
+			case CoreState::CORE_RUNNING_CPU:
 				if (ImGui::MenuItem("Break")) {
 					Core_Break("Menu:Break");
 				}
+				break;
 			}
 			ImGui::Separator();
 			ImGui::MenuItem("Ignore bad memory accesses", nullptr, &g_Config.bIgnoreBadMemAccess);
@@ -821,7 +824,7 @@ void ImDebugger::Frame(MIPSDebugInterface *mipsDebug, GPUDebugInterface *gpuDebu
 			ImGui::EndMenu();
 		}
 		if (ImGui::BeginMenu("Graphics")) {
-			ImGui::MenuItem("Ge Debugger", nullptr, &cfg_.geDebuggerOpen);
+			ImGui::MenuItem("GE Debugger", nullptr, &cfg_.geDebuggerOpen);
 			ImGui::MenuItem("Display Output", nullptr, &cfg_.displayOpen);
 			ImGui::MenuItem("Textures", nullptr, &cfg_.texturesOpen);
 			ImGui::MenuItem("Framebuffers", nullptr, &cfg_.framebuffersOpen);
@@ -858,7 +861,7 @@ void ImDebugger::Frame(MIPSDebugInterface *mipsDebug, GPUDebugInterface *gpuDebu
 	}
 
 	if (cfg_.disasmOpen) {
-		disasm_.Draw(mipsDebug, &cfg_.disasmOpen, coreState);
+		disasm_.Draw(mipsDebug, cfg_, coreState);
 	}
 
 	if (cfg_.regsOpen) {
@@ -926,14 +929,14 @@ void ImDebugger::Frame(MIPSDebugInterface *mipsDebug, GPUDebugInterface *gpuDebu
 	}
 }
 
-void ImDisasmWindow::Draw(MIPSDebugInterface *mipsDebug, bool *open, CoreState coreState) {
+void ImDisasmWindow::Draw(MIPSDebugInterface *mipsDebug, ImConfig &cfg, CoreState coreState) {
 	char title[256];
 	snprintf(title, sizeof(title), "%s - Disassembly", "Allegrex MIPS");
 
 	disasmView_.setDebugger(mipsDebug);
 
 	ImGui::SetNextWindowSize(ImVec2(520, 600), ImGuiCond_FirstUseEver);
-	if (!ImGui::Begin(title, open, ImGuiWindowFlags_NoNavInputs)) {
+	if (!ImGui::Begin(title, &cfg.disasmOpen, ImGuiWindowFlags_NoNavInputs)) {
 		ImGui::End();
 		return;
 	}
@@ -951,7 +954,12 @@ void ImDisasmWindow::Draw(MIPSDebugInterface *mipsDebug, bool *open, CoreState c
 	}
 
 	if (coreState == CORE_STEPPING_GE || coreState == CORE_RUNNING_GE) {
-		ImGui::Text("!!! Currently stepping the Ge. See that window (when implemented)");
+		ImGui::Text("!!! Currently stepping the GE");
+		ImGui::SameLine();
+		if (ImGui::SmallButton("Open Ge debugger")) {
+			cfg.geDebuggerOpen = true;
+			ImGui::SetWindowFocus("GE Debugger");
+		}
 	}
 
 	ImGui::BeginDisabled(coreState != CORE_STEPPING_CPU);

UI/ImDebugger/ImDebugger.h

@@ -26,11 +26,12 @@
 
 class MIPSDebugInterface;
 class GPUDebugInterface;
+struct ImConfig;
 
 // Corresponds to the CDisasm dialog
 class ImDisasmWindow {
 public:
-	void Draw(MIPSDebugInterface *mipsDebug, bool *open, CoreState coreState);
+	void Draw(MIPSDebugInterface *mipsDebug, ImConfig &cfg, CoreState coreState);
 	ImDisasmView &View() {
 		return disasmView_;
 	}

UI/ImDebugger/ImDisasmView.cpp

@@ -102,7 +102,6 @@ static std::string trimString(std::string input) {
 
 void ImDisasmView::assembleOpcode(u32 address, const std::string &defaultText) {
 	/*
-	auto memLock = Memory::Lock();
 	if (!Core_IsStepping()) {
 		MessageBox(wnd, L"Cannot change code while the core is running!", L"Error", MB_OK);
 		return;
@@ -315,7 +314,6 @@ void ImDisasmView::drawArguments(ImDrawList *drawList, Rect rc, const Disassembl
 }
 
 void ImDisasmView::Draw(ImDrawList *drawList) {
-	auto memLock = Memory::Lock();
 	if (!debugger->isAlive()) {
 		return;
 	}
@@ -922,7 +920,6 @@ void ImDisasmView::PopupMenu() {
 }
 
 void ImDisasmView::updateStatusBarText() {
-	auto memLock = Memory::Lock();
 	if (!PSP_IsInited())
 		return;
 
@@ -1047,8 +1044,6 @@ void ImDisasmView::SearchNext(bool forward) {
 		return;
 	}
 
-	auto memLock = Memory::Lock();
-
 	// Note: Search will replace matchAddress_ with the current address.
 	u32 searchAddress = manager.getNthNextAddress(matchAddress_, 1);
 
@@ -1110,7 +1105,6 @@ void ImDisasmView::SearchNext(bool forward) {
 }
 
 std::string ImDisasmView::disassembleRange(u32 start, u32 size) {
-	auto memLock = Memory::Lock();
 	std::string result;
 
 	// gather all branch targets without labels

UI/ImDebugger/ImGe.cpp

@@ -9,6 +9,7 @@
 #include "Core/HW/Display.h"
 
 void DrawFramebuffersWindow(ImConfig &cfg, FramebufferManagerCommon *framebufferManager) {
+	ImGui::SetNextWindowSize(ImVec2(520, 600), ImGuiCond_FirstUseEver);
 	if (!ImGui::Begin("Framebuffers", &cfg.framebuffersOpen)) {
 		ImGui::End();
 		return;
@@ -20,6 +21,7 @@ void DrawFramebuffersWindow(ImConfig &cfg, FramebufferManagerCommon *framebuffer
 }
 
 void DrawTexturesWindow(ImConfig &cfg, TextureCacheCommon *textureCache) {
+	ImGui::SetNextWindowSize(ImVec2(520, 600), ImGuiCond_FirstUseEver);
 	if (!ImGui::Begin("Textures", &cfg.texturesOpen)) {
 		ImGui::End();
 		return;
@@ -31,6 +33,7 @@ void DrawTexturesWindow(ImConfig &cfg, TextureCacheCommon *textureCache) {
 }
 
 void DrawDisplayWindow(ImConfig &cfg, FramebufferManagerCommon *framebufferManager) {
+	ImGui::SetNextWindowSize(ImVec2(520, 600), ImGuiCond_FirstUseEver);
 	if (!ImGui::Begin("Display", &cfg.displayOpen)) {
 		ImGui::End();
 		return;
@@ -59,6 +62,7 @@ void DrawDisplayWindow(ImConfig &cfg, FramebufferManagerCommon *framebufferManag
 
 // Note: This is not exclusively graphics.
 void DrawDebugStatsWindow(ImConfig &cfg) {
+	ImGui::SetNextWindowSize(ImVec2(300, 500), ImGuiCond_FirstUseEver);
 	if (!ImGui::Begin("Debug Stats", &cfg.debugStatsOpen)) {
 		ImGui::End();
 		return;
@@ -71,7 +75,8 @@ void DrawDebugStatsWindow(ImConfig &cfg) {
 
 // Stub
 void DrawGeDebuggerWindow(ImConfig &cfg) {
-	if (!ImGui::Begin("Debug Stats", &cfg.geDebuggerOpen)) {
+	ImGui::SetNextWindowSize(ImVec2(520, 600), ImGuiCond_FirstUseEver);
+	if (!ImGui::Begin("GE Debugger", &cfg.geDebuggerOpen)) {
 		ImGui::End();
 		return;
 	}

Windows/Debugger/Debugger_Disasm.cpp

@@ -407,7 +407,6 @@ BOOL CDisasm::DlgProc(UINT message, WPARAM wParam, LPARAM lParam) {
 
 						// If the current PC is on a breakpoint, the user doesn't want to do nothing.
 						breakpoints_->SetSkipFirst(currentMIPS->pc);
-
 						Core_Resume();
 					}
 				}

Windows/GEDebugger/CtrlDisplayListView.cpp

@@ -180,7 +180,7 @@ void CtrlDisplayListView::onPaint(WPARAM wParam, LPARAM lParam)
 	
 	HICON breakPoint = (HICON)LoadIcon(GetModuleHandle(0),(LPCWSTR)IDI_STOP);
 
-	auto disasm = gpuDebug->DissassembleOpRange(windowStart, windowStart + (visibleRows + 2) * instructionSize);
+	auto disasm = gpuDebug->DisassembleOpRange(windowStart, windowStart + (visibleRows + 2) * instructionSize);
 
 	for (int i = 0; i < visibleRows+2; i++)
 	{