Merge pull request #20072 from hrydgard/debugger-threading-improvements

UI threading improvements

Author: hrydgard

Common/System/System.h

@@ -100,6 +100,9 @@ enum class SystemRequestType {
 	RUN_CALLBACK_IN_WNDPROC,
 };
 
+// Run a closure on the main thread. Used to safely implement UI that runs on another thread.
+void System_RunOnMainThread(std::function<void()> func);
+
 // Implementations are supposed to process the request, and post the response to the g_RequestManager (see Message.h).
 // This is not to be used directly by applications, instead use the g_RequestManager to make the requests.
 // This can return false if it's known that the platform doesn't support the request, the app is supposed to handle

Core/Config.cpp

@@ -2015,7 +2015,7 @@ void Config::ResetControlLayout() {
 	g_Config.fRightStickHeadScale = 1.0f;
 }
 
-void Config::GetReportingInfo(UrlEncoder &data) {
+void Config::GetReportingInfo(UrlEncoder &data) const {
 	for (size_t i = 0; i < numSections; ++i) {
 		const std::string prefix = std::string("config.") + sections[i].section;
 		for (size_t j = 0; j < sections[i].settingsCount; j++) {

Core/Config.h

@@ -632,7 +632,7 @@ struct Config {
 
 	void ResetControlLayout();
 
-	void GetReportingInfo(UrlEncoder &data);
+	void GetReportingInfo(UrlEncoder &data) const;
 
 	bool IsPortrait() const;
 	int NextValidBackend();

Core/Core.cpp

@@ -108,7 +108,6 @@ const char *BreakReasonToString(BreakReason reason) {
 	case BreakReason::SavestateCrash: return "savestate.crash";
 	case BreakReason::MemoryBreakpoint: return "memory.breakpoint";
 	case BreakReason::CpuBreakpoint: return "cpu.breakpoint";
-	case BreakReason::BreakpointUpdate: return "cpu.breakpoint.update";
 	case BreakReason::MemoryAccess: return "memory.access";  // ???
 	case BreakReason::JitBranchDebug: return "jit.branchdebug";
 	case BreakReason::RABreak: return "ra.break";

Core/Core.h

@@ -61,7 +61,6 @@ enum class BreakReason {
 	SavestateCrash,
 	MemoryBreakpoint,
 	CpuBreakpoint,
-	BreakpointUpdate,
 	MemoryAccess,  // ???
 	JitBranchDebug,
 	BreakOnBoot,

Core/Debugger/Breakpoints.cpp

@@ -32,7 +32,7 @@
 
 BreakpointManager g_breakpoints;
 
-void MemCheck::Log(u32 addr, bool write, int size, u32 pc, const char *reason) {
+void MemCheck::Log(u32 addr, bool write, int size, u32 pc, const char *reason) const {
 	if (result & BREAK_ACTION_LOG) {
 		const char *type = write ? "Write" : "Read";
 		if (logFormat.empty()) {
@@ -71,11 +71,9 @@ BreakAction MemCheck::Action(u32 addr, bool write, int size, u32 pc, const char
 }
 
 // Note: must lock while calling this.
-size_t BreakpointManager::FindBreakpoint(u32 addr, bool matchTemp, bool temp)
-{
+size_t BreakpointManager::FindBreakpoint(u32 addr, bool matchTemp, bool temp) {
 	size_t found = INVALID_BREAKPOINT;
-	for (size_t i = 0; i < breakPoints_.size(); ++i)
-	{
+	for (size_t i = 0; i < breakPoints_.size(); ++i) {
 		const auto &bp = breakPoints_[i];
 		if (bp.addr == addr && (!matchTemp || bp.temporary == temp))
 		{
@@ -90,10 +88,8 @@ size_t BreakpointManager::FindBreakpoint(u32 addr, bool matchTemp, bool temp)
 	return found;
 }
 
-size_t BreakpointManager::FindMemCheck(u32 start, u32 end)
-{
-	for (size_t i = 0; i < memChecks_.size(); ++i)
-	{
+size_t BreakpointManager::FindMemCheck(u32 start, u32 end) {
+	for (size_t i = 0; i < memChecks_.size(); ++i) {
 		if (memChecks_[i].start == start && memChecks_[i].end == end)
 			return i;
 	}
@@ -155,14 +151,11 @@ int BreakpointManager::AddBreakPoint(u32 addr, bool temp) {
 
 		breakPoints_.push_back(pt);
 		anyBreakPoints_ = true;
-		guard.unlock();
 		Update(addr);
-
 		return (int)breakPoints_.size() - 1;
 	} else if (!breakPoints_[bp].IsEnabled()) {
 		breakPoints_[bp].result |= BREAK_ACTION_PAUSE;
 		breakPoints_[bp].hasCond = false;
-		guard.unlock();
 		Update(addr);
 		return (int)bp;
 	} else {
@@ -183,7 +176,6 @@ void BreakpointManager::RemoveBreakPoint(u32 addr) {
 			breakPoints_.erase(breakPoints_.begin() + bp);
 
 		anyBreakPoints_ = !breakPoints_.empty();
-		guard.unlock();
 		Update(addr);
 	}
 }
@@ -196,8 +188,6 @@ void BreakpointManager::ChangeBreakPoint(u32 addr, bool status) {
 			breakPoints_[bp].result |= BREAK_ACTION_PAUSE;
 		else
 			breakPoints_[bp].result = BreakAction(breakPoints_[bp].result & ~BREAK_ACTION_PAUSE);
-
-		guard.unlock();
 		Update(addr);
 	}
 }
@@ -207,7 +197,6 @@ void BreakpointManager::ChangeBreakPoint(u32 addr, BreakAction result) {
 	size_t bp = FindBreakpoint(addr);
 	if (bp != INVALID_BREAKPOINT) {
 		breakPoints_[bp].result = result;
-		guard.unlock();
 		Update(addr);
 	}
 }
@@ -220,7 +209,6 @@ void BreakpointManager::ClearAllBreakPoints()
 	if (!breakPoints_.empty())
 	{
 		breakPoints_.clear();
-		guard.unlock();
 		Update();
 	}
 }
@@ -230,20 +218,14 @@ void BreakpointManager::ClearTemporaryBreakPoints()
 	if (!anyBreakPoints_)
 		return;
 	std::unique_lock<std::mutex> guard(breakPointsMutex_);
-
-	bool update = false;
 	for (int i = (int)breakPoints_.size()-1; i >= 0; --i)
 	{
 		if (breakPoints_[i].temporary)
 		{
 			breakPoints_.erase(breakPoints_.begin() + i);
-			update = true;
+			Update();
 		}
 	}
-
-	guard.unlock();
-	if (update)
-		Update();
 }
 
 void BreakpointManager::ChangeBreakPointAddCond(u32 addr, const BreakPointCond &cond)
@@ -254,38 +236,32 @@ void BreakpointManager::ChangeBreakPointAddCond(u32 addr, const BreakPointCond &
 	{
 		breakPoints_[bp].hasCond = true;
 		breakPoints_[bp].cond = cond;
-		guard.unlock();
 		Update(addr);
 	}
 }
 
-void BreakpointManager::ChangeBreakPointRemoveCond(u32 addr)
-{
+void BreakpointManager::ChangeBreakPointRemoveCond(u32 addr) {
 	std::unique_lock<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr);
-	if (bp != INVALID_BREAKPOINT)
-	{
+	if (bp != INVALID_BREAKPOINT) {
 		breakPoints_[bp].hasCond = false;
-		guard.unlock();
 		Update(addr);
 	}
 }
 
-BreakPointCond *BreakpointManager::GetBreakPointCondition(u32 addr)
-{
+BreakPointCond *BreakpointManager::GetBreakPointCondition(u32 addr) {
 	std::lock_guard<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr);
 	if (bp != INVALID_BREAKPOINT && breakPoints_[bp].hasCond)
 		return &breakPoints_[bp].cond;
-	return NULL;
+	return nullptr;
 }
 
 void BreakpointManager::ChangeBreakPointLogFormat(u32 addr, const std::string &fmt) {
 	std::unique_lock<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr, true, false);
 	if (bp != INVALID_BREAKPOINT) {
 		breakPoints_[bp].logFormat = fmt;
-		guard.unlock();
 		Update(addr);
 	}
 }
@@ -338,18 +314,18 @@ int BreakpointManager::AddMemCheck(u32 start, u32 end, MemCheckCondition cond, B
 
 		memChecks_.push_back(check);
 		bool hadAny = anyMemChecks_.exchange(true);
-		if (!hadAny)
+		if (!hadAny) {
 			MemBlockOverrideDetailed();
-		guard.unlock();
+		}
 		Update();
 		return (int)memChecks_.size() - 1;
 	} else {
 		memChecks_[mc].cond = (MemCheckCondition)(memChecks_[mc].cond | cond);
 		memChecks_[mc].result = (BreakAction)(memChecks_[mc].result | result);
 		bool hadAny = anyMemChecks_.exchange(true);
-		if (!hadAny)
+		if (!hadAny) {
 			MemBlockOverrideDetailed();
-		guard.unlock();
+		}
 		Update();
 		return (int)mc;
 	}
@@ -366,7 +342,6 @@ void BreakpointManager::RemoveMemCheck(u32 start, u32 end)
 		bool hadAny = anyMemChecks_.exchange(!memChecks_.empty());
 		if (hadAny)
 			MemBlockReleaseDetailed();
-		guard.unlock();
 		Update();
 	}
 }
@@ -379,7 +354,6 @@ void BreakpointManager::ChangeMemCheck(u32 start, u32 end, MemCheckCondition con
 	{
 		memChecks_[mc].cond = cond;
 		memChecks_[mc].result = result;
-		guard.unlock();
 		Update();
 	}
 }
@@ -394,7 +368,6 @@ void BreakpointManager::ClearAllMemChecks()
 		bool hadAny = anyMemChecks_.exchange(false);
 		if (hadAny)
 			MemBlockReleaseDetailed();
-		guard.unlock();
 		Update();
 	}
 }
@@ -406,7 +379,6 @@ void BreakpointManager::ChangeMemCheckAddCond(u32 start, u32 end, const BreakPoi
 	if (mc != INVALID_MEMCHECK) {
 		memChecks_[mc].hasCondition = true;
 		memChecks_[mc].condition = cond;
-		guard.unlock();
 		// No need to update jit for a condition add/remove, they're not baked in.
 		Update(-1);
 	}
@@ -417,7 +389,6 @@ void BreakpointManager::ChangeMemCheckRemoveCond(u32 start, u32 end) {
 	size_t mc = FindMemCheck(start, end);
 	if (mc != INVALID_MEMCHECK) {
 		memChecks_[mc].hasCondition = false;
-		guard.unlock();
 		// No need to update jit for a condition add/remove, they're not baked in.
 		Update(-1);
 	}
@@ -436,7 +407,6 @@ void BreakpointManager::ChangeMemCheckLogFormat(u32 start, u32 end, const std::s
 	size_t mc = FindMemCheck(start, end);
 	if (mc != INVALID_MEMCHECK) {
 		memChecks_[mc].logFormat = fmt;
-		guard.unlock();
 		Update();
 	}
 }
@@ -620,30 +590,27 @@ std::vector<BreakPoint> BreakpointManager::GetBreakpoints() {
 	return breakPoints_;
 }
 
-void BreakpointManager::Update(u32 addr) {
-	if (MIPSComp::jit && addr != -1) {
-		bool resume = false;
-		if (Core_IsStepping() == false) {
-			Core_Break(BreakReason::BreakpointUpdate, addr);
-			Core_WaitInactive();
-			resume = true;
-		}
+void BreakpointManager::Frame() {
+	// outside the lock here, should be ok.
+	if (!needsUpdate_) {
+		return;
+	}
 
+	std::lock_guard<std::mutex> guard(breakPointsMutex_);
+	if (MIPSComp::jit && updateAddr_ != -1) {
 		// In case this is a delay slot, clear the previous instruction too.
-		if (addr != 0)
-			mipsr4k.InvalidateICache(addr - 4, 8);
+		if (updateAddr_ != 0)
+			mipsr4k.InvalidateICache(updateAddr_ - 4, 8);
 		else
 			mipsr4k.ClearJitCache();
-
-		if (resume)
-			Core_Resume();
 	}
 
-	if (anyMemChecks_ && addr != -1)
+	if (anyMemChecks_ && updateAddr_ != -1)
 		UpdateCachedMemCheckRanges();
 
 	// Redraw in order to show the breakpoint.
 	System_Notify(SystemNotification::DISASSEMBLY);
+	needsUpdate_ = false;
 }
 
 bool BreakpointManager::ValidateLogFormat(MIPSDebugInterface *cpu, const std::string &fmt) {

Core/Debugger/Breakpoints.h

@@ -103,7 +103,7 @@ struct MemCheck {
 	// Called on a copy.
 	BreakAction Action(u32 addr, bool write, int size, u32 pc, const char *reason);
 
-	void Log(u32 addr, bool write, int size, u32 pc, const char *reason);
+	void Log(u32 addr, bool write, int size, u32 pc, const char *reason) const;
 
 	bool IsEnabled() const {
 		return (result & BREAK_ACTION_PAUSE) != 0;
@@ -183,12 +183,17 @@ class BreakpointManager {
 		return anyMemChecks_;
 	}
 
-	void Update(u32 addr = 0);
+	void Frame();
 
 	bool ValidateLogFormat(MIPSDebugInterface *cpu, const std::string &fmt);
 	bool EvaluateLogFormat(MIPSDebugInterface *cpu, const std::string &fmt, std::string &result);
 
 private:
+	// Should be called under lock.
+	void Update(u32 addr = 0) {
+		needsUpdate_ = true;
+		updateAddr_ = addr;
+	}
 	size_t FindBreakpoint(u32 addr, bool matchTemp = false, bool temp = false);
 	// Finds exactly, not using a range check.
 	size_t FindMemCheck(u32 start, u32 end);
@@ -208,6 +213,9 @@ class BreakpointManager {
 	std::vector<MemCheck> memChecks_;
 	std::vector<MemCheck> memCheckRangesRead_;
 	std::vector<MemCheck> memCheckRangesWrite_;
+
+	bool needsUpdate_ = true;
+	u32 updateAddr_ = 0;
 };
 
 extern BreakpointManager g_breakpoints;

GPU/Common/GPUDebugInterface.h

@@ -205,7 +205,7 @@ struct GPUDebugVertex {
 
 class GPUDebugInterface {
 public:
-	virtual ~GPUDebugInterface() {}
+	virtual ~GPUDebugInterface() = default;
 	virtual bool GetCurrentDisplayList(DisplayList &list) = 0;
 	virtual int GetCurrentPrimCount() = 0;
 	virtual std::vector<DisplayList> ActiveDisplayLists() = 0;

GPU/Debugger/Breakpoints.cpp

@@ -42,7 +42,7 @@ const static u8 textureRelatedCmds[] = {
 	GE_CMD_TEXFLUSH, GE_CMD_TEXSYNC,
 };
 
-void GPUBreakpoints::Init() {
+GPUBreakpoints::GPUBreakpoints() {
 	ClearAllBreakpoints();
 
 	nonTextureCmds.clear();
@@ -268,20 +268,20 @@ bool GPUBreakpoints::IsRenderTargetBreakpoint(u32 addr) {
 	return breakRenderTargets.find(addr) != breakRenderTargets.end();
 }
 
-bool GPUBreakpoints::IsOpBreakpoint(u32 op, bool &temp) {
+bool GPUBreakpoints::IsOpBreakpoint(u32 op, bool &temp) const {
 	return IsCmdBreakpoint(op >> 24, temp);
 }
 
-bool GPUBreakpoints::IsOpBreakpoint(u32 op) {
+bool GPUBreakpoints::IsOpBreakpoint(u32 op) const {
 	return IsCmdBreakpoint(op >> 24);
 }
 
-bool GPUBreakpoints::IsCmdBreakpoint(u8 cmd, bool &temp) {
+bool GPUBreakpoints::IsCmdBreakpoint(u8 cmd, bool &temp) const {
 	temp = breakCmdsTemp[cmd];
 	return breakCmds[cmd];
 }
 
-bool GPUBreakpoints::IsCmdBreakpoint(u8 cmd) {
+bool GPUBreakpoints::IsCmdBreakpoint(u8 cmd) const {
 	return breakCmds[cmd];
 }