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
This commit is contained in:
Silent 2024-04-07 22:48:28 +02:00 committed by refractionpcsx2
parent c0a6e21599
commit 272c0369f1
8 changed files with 62 additions and 87 deletions

View File

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

View File

@ -6,9 +6,6 @@
#include "ui_CpuWidget.h" #include "ui_CpuWidget.h"
#include "DebugTools/DebugInterface.h" #include "DebugTools/DebugInterface.h"
#include "DebugTools/Breakpoints.h"
#include "DebugTools/BiosDebugData.h"
#include "DebugTools/MipsStackWalk.h"
#include "Models/BreakpointModel.h" #include "Models/BreakpointModel.h"
#include "Models/ThreadModel.h" #include "Models/ThreadModel.h"
@ -72,26 +69,7 @@ public slots:
void onModuleTreeContextMenu(QPoint pos); void onModuleTreeContextMenu(QPoint pos);
void onModuleTreeDoubleClick(QTreeWidgetItem* item); void onModuleTreeDoubleClick(QTreeWidgetItem* item);
void refreshDebugger(); void refreshDebugger();
void reloadCPUWidgets() 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 saveBreakpointsToDebuggerSettings(); void saveBreakpointsToDebuggerSettings();
void saveSavedAddressesToDebuggerSettings(); void saveSavedAddressesToDebuggerSettings();

View File

@ -45,14 +45,9 @@ DebuggerWindow::DebuggerWindow(QWidget* parent)
m_ui.cpuTabs->addTab(m_cpuWidget_r5900, "R5900"); m_ui.cpuTabs->addTab(m_cpuWidget_r5900, "R5900");
m_ui.cpuTabs->addTab(m_cpuWidget_r3000, "R3000"); m_ui.cpuTabs->addTab(m_cpuWidget_r3000, "R3000");
CBreakPoints::SetUpdateHandler(std::bind(&DebuggerWindow::onBreakpointsChanged, this));
} }
DebuggerWindow::~DebuggerWindow() DebuggerWindow::~DebuggerWindow() = default;
{
CBreakPoints::SetUpdateHandler(nullptr);
}
// There is no straightforward way to set the tab text to bold in Qt // 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 // 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()); CpuWidget* currentCpu = static_cast<CpuWidget*>(m_ui.cpuTabs->currentWidget());
currentCpu->onStepOut(); currentCpu->onStepOut();
} }
void DebuggerWindow::onBreakpointsChanged()
{
m_cpuWidget_r5900->reloadCPUWidgets();
m_cpuWidget_r3000->reloadCPUWidgets();
}

View File

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

View File

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

View File

@ -296,6 +296,7 @@ bool BreakpointModel::setData(const QModelIndex& index, const QVariant& value, i
MemCheckResult(mc.result ^ MEMCHECK_BREAK)); MemCheckResult(mc.result ^ MEMCHECK_BREAK));
}); });
} }
emit dataChanged(index, index);
return true; return true;
} }
else if (role == Qt::EditRole && index.column() == BreakpointColumns::CONDITION) 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] { Host::RunOnCPUThread([cpu = m_cpu.getCpuType(), bp, cond] {
CBreakPoints::ChangeBreakPointAddCond(cpu, bp.addr, cond); CBreakPoints::ChangeBreakPointAddCond(cpu, bp.addr, cond);
}); });
return true;
} }
emit dataChanged(index, index);
return true;
} }
return false; 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(); endRemoveRows();
return true; return true;
@ -407,23 +412,20 @@ bool BreakpointModel::insertBreakpointRows(int row, int count, std::vector<Break
void BreakpointModel::refreshData() 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); QtHost::RunOnUIThread([this, breakpoints = std::move(all_breakpoints)]() mutable {
for (const auto& bp : breakpoints)
{ beginResetModel();
m_breakpoints.push_back(bp); 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) void BreakpointModel::loadBreakpointFromFieldList(QStringList fields)

View File

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

View File

@ -4,7 +4,6 @@
#pragma once #pragma once
#include <algorithm> #include <algorithm>
#include <functional>
#include <iterator> #include <iterator>
#include <vector> #include <vector>
@ -150,10 +149,6 @@ public:
static bool GetCorePaused() { return corePaused; }; static bool GetCorePaused() { return corePaused; };
static void SetCorePaused(bool b) { corePaused = b; }; 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: private:
static size_t FindBreakpoint(BreakPointCpu cpu, u32 addr, bool matchTemp = false, bool temp = false); static size_t FindBreakpoint(BreakPointCpu cpu, u32 addr, bool matchTemp = false, bool temp = false);
// Finds exactly, not using a range check. // Finds exactly, not using a range check.
@ -169,8 +164,6 @@ private:
static BreakPointCpu breakpointTriggeredCpu_; static BreakPointCpu breakpointTriggeredCpu_;
static bool corePaused; static bool corePaused;
static std::function<void()> cb_bpUpdated_;
static std::vector<MemCheck> memChecks_; static std::vector<MemCheck> memChecks_;
static std::vector<MemCheck *> cleanupMemChecks_; static std::vector<MemCheck *> cleanupMemChecks_;
}; };