Debugger: Untangle the breakpoints data flow to resolve races

Tightens the data flow between the CPU and UI threads
to resolve multiple race conditions, such as:
1. Unbinding a debug interface update CB while it's in use,
    causing a possible use-after-free.
2. Binding breakpoints via the disassembly widget that would read
    a stale local variable, and bind the breakpoint to a bogus address

+ probably more subtle races that are now resolved

pcsx2-qt/Debugger/CpuWidget.cpp

@@ -64,6 +64,8 @@ CpuWidget::CpuWidget(QWidget* parent, DebugInterface& cpu)
 	connect(m_ui.registerWidget, &RegisterWidget::VMUpdate, this, &CpuWidget::reloadCPUWidgets);
 	connect(m_ui.disassemblyWidget, &DisassemblyWidget::VMUpdate, this, &CpuWidget::reloadCPUWidgets);
 
+	connect(m_ui.disassemblyWidget, &DisassemblyWidget::breakpointsChanged, this, &CpuWidget::updateBreakpoints);
+
 	connect(m_ui.breakpointList, &QTableView::customContextMenuRequested, this, &CpuWidget::onBPListContextMenu);
 	connect(m_ui.breakpointList, &QTableView::doubleClicked, this, &CpuWidget::onBPListDoubleClicked);
 
@@ -74,6 +76,8 @@ CpuWidget::CpuWidget(QWidget* parent, DebugInterface& cpu)
 		i++;
 	}
 
+	connect(&m_bpModel, &BreakpointModel::dataChanged, this, &CpuWidget::updateBreakpoints);
+
 	connect(m_ui.threadList, &QTableView::customContextMenuRequested, this, &CpuWidget::onThreadListContextMenu);
 	connect(m_ui.threadList, &QTableView::doubleClicked, this, &CpuWidget::onThreadListDoubleClick);
 
@@ -160,6 +164,16 @@ void CpuWidget::refreshDebugger()
 	}
 }
 
+void CpuWidget::reloadCPUWidgets()
+{
+	updateThreads();
+	updateStackFrames();
+
+	m_ui.registerWidget->update();
+	m_ui.disassemblyWidget->update();
+	m_ui.memoryviewWidget->update();
+}
+
 void CpuWidget::paintEvent(QPaintEvent* event)
 {
 	m_ui.registerWidget->update();
@@ -205,9 +219,9 @@ void CpuWidget::onStepInto()
 	if (info.isSyscall)
 		bpAddr = info.branchTarget; // Syscalls are always taken
 
-	Host::RunOnCPUThread([&] {
-		CBreakPoints::AddBreakPoint(m_cpu.getCpuType(), bpAddr, true);
-		m_cpu.resumeCpu();
+	Host::RunOnCPUThread([cpu = &m_cpu, bpAddr] {
+		CBreakPoints::AddBreakPoint(cpu->getCpuType(), bpAddr, true);
+		cpu->resumeCpu();
 	});
 
 	this->repaint();
@@ -224,9 +238,9 @@ void CpuWidget::onStepOut()
 	if (m_stackModel.rowCount() < 2)
 		return;
 
-	Host::RunOnCPUThread([&] {
-		CBreakPoints::AddBreakPoint(m_cpu.getCpuType(), m_stackModel.data(m_stackModel.index(1, StackModel::PC), Qt::UserRole).toUInt(), true);
-		m_cpu.resumeCpu();
+	Host::RunOnCPUThread([cpu = &m_cpu, stackModel = &m_stackModel] {
+		CBreakPoints::AddBreakPoint(cpu->getCpuType(), stackModel->data(stackModel->index(1, StackModel::PC), Qt::UserRole).toUInt(), true);
+		cpu->resumeCpu();
 	});
 
 	this->repaint();
@@ -270,9 +284,9 @@ void CpuWidget::onStepOver()
 		}
 	}
 
-	Host::RunOnCPUThread([&] {
-		CBreakPoints::AddBreakPoint(m_cpu.getCpuType(), bpAddr, true);
-		m_cpu.resumeCpu();
+	Host::RunOnCPUThread([cpu = &m_cpu, bpAddr] {
+		CBreakPoints::AddBreakPoint(cpu->getCpuType(), bpAddr, true);
+		cpu->resumeCpu();
 	});
 
 	this->repaint();

pcsx2-qt/Debugger/CpuWidget.h

@@ -6,9 +6,6 @@
 #include "ui_CpuWidget.h"
 
 #include "DebugTools/DebugInterface.h"
-#include "DebugTools/Breakpoints.h"
-#include "DebugTools/BiosDebugData.h"
-#include "DebugTools/MipsStackWalk.h"
 
 #include "Models/BreakpointModel.h"
 #include "Models/ThreadModel.h"
@@ -72,26 +69,7 @@ public slots:
 	void onModuleTreeContextMenu(QPoint pos);
 	void onModuleTreeDoubleClick(QTreeWidgetItem* item);
 	void refreshDebugger();
-	void reloadCPUWidgets()
-	{
-		if (!QtHost::IsOnUIThread())
-		{
-			const auto& updateHandler = CBreakPoints::GetUpdateHandler();
-			if (updateHandler)
-			{
-				QtHost::RunOnUIThread(updateHandler);
-			}
-			return;
-		}
-
-		updateBreakpoints();
-		updateThreads();
-		updateStackFrames();
-
-		m_ui.registerWidget->update();
-		m_ui.disassemblyWidget->update();
-		m_ui.memoryviewWidget->update();
-	};
+	void reloadCPUWidgets();
 
 	void saveBreakpointsToDebuggerSettings();
 	void saveSavedAddressesToDebuggerSettings();

pcsx2-qt/Debugger/DebuggerWindow.cpp

@@ -45,14 +45,9 @@ DebuggerWindow::DebuggerWindow(QWidget* parent)
 
 	m_ui.cpuTabs->addTab(m_cpuWidget_r5900, "R5900");
 	m_ui.cpuTabs->addTab(m_cpuWidget_r3000, "R3000");
-
-	CBreakPoints::SetUpdateHandler(std::bind(&DebuggerWindow::onBreakpointsChanged, this));
 }
 
-DebuggerWindow::~DebuggerWindow()
-{
-	CBreakPoints::SetUpdateHandler(nullptr);
-}
+DebuggerWindow::~DebuggerWindow() = default;
 
 // There is no straightforward way to set the tab text to bold in Qt
 // Sorry colour blind people, but this is the best we can do for now
@@ -132,9 +127,3 @@ void DebuggerWindow::onStepOut()
 	CpuWidget* currentCpu = static_cast<CpuWidget*>(m_ui.cpuTabs->currentWidget());
 	currentCpu->onStepOut();
 }
-
-void DebuggerWindow::onBreakpointsChanged()
-{
-	m_cpuWidget_r5900->reloadCPUWidgets();
-	m_cpuWidget_r3000->reloadCPUWidgets();
-}

pcsx2-qt/Debugger/DebuggerWindow.h

@@ -21,7 +21,6 @@ public slots:
 	void onStepInto();
 	void onStepOver();
 	void onStepOut();
-	void onBreakpointsChanged();
 
 private:
 	Ui::DebuggerWindow m_ui;

pcsx2-qt/Debugger/DisassemblyWidget.cpp

@@ -78,7 +78,7 @@ void DisassemblyWidget::contextAssembleInstruction()
 				this->m_nopedInstructions.insert({i, cpu->read32(i)});
 				cpu->write32(i, val);
 			}
-			QtHost::RunOnUIThread([this] { VMUpdate(); });
+			emit VMUpdate();
 		});
 	}
 }
@@ -91,7 +91,7 @@ void DisassemblyWidget::contextNoopInstruction()
 			this->m_nopedInstructions.insert({i, cpu->read32(i)});
 			cpu->write32(i, 0x00);
 		}
-		QtHost::RunOnUIThread([this] { VMUpdate(); });
+		emit VMUpdate();
 	});
 }
 
@@ -106,15 +106,16 @@ void DisassemblyWidget::contextRestoreInstruction()
 				this->m_nopedInstructions.erase(i);
 			}
 		}
-		QtHost::RunOnUIThread([this] { VMUpdate(); });
+		emit VMUpdate();
 	});
 }
 
 void DisassemblyWidget::contextRunToCursor()
 {
-	Host::RunOnCPUThread([&] {
-		CBreakPoints::AddBreakPoint(m_cpu->getCpuType(), m_selectedAddressStart, true);
-		m_cpu->resumeCpu();
+	const u32 selectedAddressStart = m_selectedAddressStart;
+	Host::RunOnCPUThread([cpu = m_cpu, selectedAddressStart] {
+		CBreakPoints::AddBreakPoint(cpu->getCpuType(), selectedAddressStart, true);
+		cpu->resumeCpu();
 	});
 }
 
@@ -129,13 +130,15 @@ void DisassemblyWidget::contextToggleBreakpoint()
 	if (!m_cpu->isAlive())
 		return;
 
-	if (CBreakPoints::IsAddressBreakPoint(m_cpu->getCpuType(), m_selectedAddressStart))
+	const u32 selectedAddressStart = m_selectedAddressStart;
+	const BreakPointCpu cpuType = m_cpu->getCpuType();
+	if (CBreakPoints::IsAddressBreakPoint(cpuType, selectedAddressStart))
 	{
-		Host::RunOnCPUThread([&] { CBreakPoints::RemoveBreakPoint(m_cpu->getCpuType(), m_selectedAddressStart); });
+		Host::RunOnCPUThread([cpuType, selectedAddressStart] { CBreakPoints::RemoveBreakPoint(cpuType, selectedAddressStart); });
 	}
 	else
 	{
-		Host::RunOnCPUThread([&] { CBreakPoints::AddBreakPoint(m_cpu->getCpuType(), m_selectedAddressStart); });
+		Host::RunOnCPUThread([cpuType, selectedAddressStart] { CBreakPoints::AddBreakPoint(cpuType, selectedAddressStart); });
 	}
 
 	breakpointsChanged();
@@ -280,7 +283,7 @@ void DisassemblyWidget::contextStubFunction()
 			this->m_stubbedFunctions.insert({curFuncAddress, {cpu->read32(curFuncAddress), cpu->read32(curFuncAddress + 4)}});
 			cpu->write32(curFuncAddress, 0x03E00008); // jr $ra
 			cpu->write32(curFuncAddress + 4, 0x00000000); // nop
-			QtHost::RunOnUIThread([this] { VMUpdate(); });
+			emit VMUpdate();
 		});
 	}
 	else // Stub the current opcode instead
@@ -289,7 +292,7 @@ void DisassemblyWidget::contextStubFunction()
 			this->m_stubbedFunctions.insert({m_selectedAddressStart, {cpu->read32(m_selectedAddressStart), cpu->read32(m_selectedAddressStart + 4)}});
 			cpu->write32(m_selectedAddressStart, 0x03E00008); // jr $ra
 			cpu->write32(m_selectedAddressStart + 4, 0x00000000); // nop
-			QtHost::RunOnUIThread([this] { VMUpdate(); });
+			emit VMUpdate();
 		});
 	}
 }
@@ -303,7 +306,7 @@ void DisassemblyWidget::contextRestoreFunction()
 			cpu->write32(curFuncAddress, std::get<0>(this->m_stubbedFunctions[curFuncAddress]));
 			cpu->write32(curFuncAddress + 4, std::get<1>(this->m_stubbedFunctions[curFuncAddress]));
 			this->m_stubbedFunctions.erase(curFuncAddress);
-			QtHost::RunOnUIThread([this] { VMUpdate(); });
+			emit VMUpdate();
 		});
 	}
 	else if (m_stubbedFunctions.find(m_selectedAddressStart) != m_stubbedFunctions.end())
@@ -312,7 +315,7 @@ void DisassemblyWidget::contextRestoreFunction()
 			cpu->write32(m_selectedAddressStart, std::get<0>(this->m_stubbedFunctions[m_selectedAddressStart]));
 			cpu->write32(m_selectedAddressStart + 4, std::get<1>(this->m_stubbedFunctions[m_selectedAddressStart]));
 			this->m_stubbedFunctions.erase(m_selectedAddressStart);
-			QtHost::RunOnUIThread([this] { VMUpdate(); });
+			emit VMUpdate();
 		});
 	}
 	else
@@ -551,13 +554,14 @@ void DisassemblyWidget::mouseDoubleClickEvent(QMouseEvent* event)
 		return;
 
 	const u32 selectedAddress = (static_cast<int>(event->position().y()) / m_rowHeight * 4) + m_visibleStart;
-	if (CBreakPoints::IsAddressBreakPoint(m_cpu->getCpuType(), selectedAddress))
+	const BreakPointCpu cpuType = m_cpu->getCpuType();
+	if (CBreakPoints::IsAddressBreakPoint(cpuType, selectedAddress))
 	{
-		Host::RunOnCPUThread([&] { CBreakPoints::RemoveBreakPoint(m_cpu->getCpuType(), selectedAddress); });
+		Host::RunOnCPUThread([cpuType, selectedAddress] { CBreakPoints::RemoveBreakPoint(cpuType, selectedAddress); });
 	}
 	else
 	{
-		Host::RunOnCPUThread([&] { CBreakPoints::AddBreakPoint(m_cpu->getCpuType(), selectedAddress); });
+		Host::RunOnCPUThread([cpuType, selectedAddress] { CBreakPoints::AddBreakPoint(cpuType, selectedAddress); });
 	}
 	breakpointsChanged();
 	this->repaint();

pcsx2-qt/Debugger/Models/BreakpointModel.cpp

@@ -296,6 +296,7 @@ bool BreakpointModel::setData(const QModelIndex& index, const QVariant& value, i
 					MemCheckResult(mc.result ^ MEMCHECK_BREAK));
 			});
 		}
+		emit dataChanged(index, index);
 		return true;
 	}
 	else if (role == Qt::EditRole && index.column() == BreakpointColumns::CONDITION)
@@ -336,8 +337,9 @@ bool BreakpointModel::setData(const QModelIndex& index, const QVariant& value, i
 			Host::RunOnCPUThread([cpu = m_cpu.getCpuType(), bp, cond] {
 				CBreakPoints::ChangeBreakPointAddCond(cpu, bp.addr, cond);
 			});
-			return true;
 		}
+		emit dataChanged(index, index);
+		return true;
 	}
 
 	return false;
@@ -364,6 +366,9 @@ bool BreakpointModel::removeRows(int row, int count, const QModelIndex& index)
 			});
 		}
 	}
+	const auto begin = m_breakpoints.begin() + row;
+	const auto end = begin + count;
+	m_breakpoints.erase(begin, end);
 
 	endRemoveRows();
 	return true;
@@ -407,23 +412,20 @@ bool BreakpointModel::insertBreakpointRows(int row, int count, std::vector<Break
 
 void BreakpointModel::refreshData()
 {
-	beginResetModel();
+	Host::RunOnCPUThread([this]() mutable {
 
-	m_breakpoints.clear();
+		std::vector<BreakpointMemcheck> all_breakpoints;
+		std::ranges::move(CBreakPoints::GetBreakpoints(m_cpu.getCpuType(), false), std::back_inserter(all_breakpoints));
+		std::ranges::move(CBreakPoints::GetMemChecks(m_cpu.getCpuType()), std::back_inserter(all_breakpoints));
 
-	auto breakpoints = CBreakPoints::GetBreakpoints(m_cpu.getCpuType(), false);
-	for (const auto& bp : breakpoints)
-	{
-		m_breakpoints.push_back(bp);
-	}
+		QtHost::RunOnUIThread([this, breakpoints = std::move(all_breakpoints)]() mutable {
+
+			beginResetModel();
+			m_breakpoints = std::move(breakpoints);
+			endResetModel();
+		});
 
-	auto memchecks = CBreakPoints::GetMemChecks(m_cpu.getCpuType());
-	for (const auto& mc : memchecks)
-	{
-		m_breakpoints.push_back(mc);
-	}
-
-	endResetModel();
+	});
 }
 
 void BreakpointModel::loadBreakpointFromFieldList(QStringList fields)

pcsx2/DebugTools/Breakpoints.cpp

@@ -18,7 +18,6 @@ std::vector<MemCheck*> CBreakPoints::cleanupMemChecks_;
 bool CBreakPoints::breakpointTriggered_ = false;
 BreakPointCpu CBreakPoints::breakpointTriggeredCpu_;
 bool CBreakPoints::corePaused = false;
-std::function<void()> CBreakPoints::cb_bpUpdated_;
 
 // called from the dynarec
 u32 standardizeBreakpointAddress(u32 addr)
@@ -440,7 +439,4 @@ void CBreakPoints::Update(BreakPointCpu cpu, u32 addr)
 
 	if (resume)
 		r5900Debug.resumeCpu();
-
-	if (cb_bpUpdated_)
-		cb_bpUpdated_();
 }

pcsx2/DebugTools/Breakpoints.h

@@ -4,7 +4,6 @@
 #pragma once
 
 #include <algorithm>
-#include <functional>
 #include <iterator>
 #include <vector>
 
@@ -150,10 +149,6 @@ class CBreakPoints
 	static bool GetCorePaused() { return corePaused; };
 	static void SetCorePaused(bool b) { corePaused = b; };
 
-	// This will have to do until a full fledged debugger host interface is made
-	static void SetUpdateHandler(std::function<void()> f) {cb_bpUpdated_ = std::move(f); };
-	static const std::function<void()>& GetUpdateHandler() { return cb_bpUpdated_; };
-
 private:
 	static size_t FindBreakpoint(BreakPointCpu cpu, u32 addr, bool matchTemp = false, bool temp = false);
 	// Finds exactly, not using a range check.
@@ -169,8 +164,6 @@ class CBreakPoints
 	static BreakPointCpu breakpointTriggeredCpu_;
 	static bool corePaused;
 
-	static std::function<void()> cb_bpUpdated_;
-
 	static std::vector<MemCheck> memChecks_;
 	static std::vector<MemCheck *> cleanupMemChecks_;
 };