Debugger: Add thread safety to breakpoints.

Core/Debugger/Breakpoints.cpp

@@ -16,6 +16,7 @@
 // https://github.com/hrydgard/ppsspp and http://www.ppsspp.org/.
 
 #include <cstdio>
+#include <mutex>
 
 #include "Common/Log.h"
 #include "Core/Core.h"
@@ -27,9 +28,11 @@
 #include "Core/MIPS/JitCommon/JitCommon.h"
 #include "Core/CoreTiming.h"
 
+static std::mutex breakPointsMutex_;
 std::vector<BreakPoint> CBreakPoints::breakPoints_;
 u32 CBreakPoints::breakSkipFirstAt_ = 0;
 u64 CBreakPoints::breakSkipFirstTicks_ = 0;
+static std::mutex memCheckMutex_;
 std::vector<MemCheck> CBreakPoints::memChecks_;
 std::vector<MemCheck *> CBreakPoints::cleanupMemChecks_;
 
@@ -45,16 +48,21 @@ void MemCheck::Log(u32 addr, bool write, int size, u32 pc) {
 	}
 }
 
-BreakAction MemCheck::Action(u32 addr, bool write, int size, u32 pc)
-{
+BreakAction MemCheck::Apply(u32 addr, bool write, int size, u32 pc) {
 	int mask = write ? MEMCHECK_WRITE : MEMCHECK_READ;
-	if (cond & mask)
-	{
+	if (cond & mask) {
 		++numHits;
+		return result;
+	}
 
+	return BREAK_ACTION_IGNORE;
+}
+
+BreakAction MemCheck::Action(u32 addr, bool write, int size, u32 pc) {
+	int mask = write ? MEMCHECK_WRITE : MEMCHECK_READ;
+	if (cond & mask) {
 		Log(addr, write, size, pc);
-		if (result & BREAK_ACTION_PAUSE)
-		{
+		if (result & BREAK_ACTION_PAUSE) {
 			Core_EnableStepping(true);
 			host->SetDebugMode(true);
 		}
@@ -65,38 +73,46 @@ BreakAction MemCheck::Action(u32 addr, bool write, int size, u32 pc)
 	return BREAK_ACTION_IGNORE;
 }
 
-void MemCheck::JitBefore(u32 addr, bool write, int size, u32 pc)
-{
+void MemCheck::JitBeforeApply(u32 addr, bool write, int size, u32 pc) {
 	int mask = MEMCHECK_WRITE | MEMCHECK_WRITE_ONCHANGE;
-	if (write && (cond & mask) == mask)
-	{
+	if (write && (cond & mask) == mask) {
 		lastAddr = addr;
 		lastPC = pc;
 		lastSize = size;
+	} else {
+		lastAddr = 0;
+		Apply(addr, write, size, pc);
+	}
+}
 
+void MemCheck::JitBeforeAction(u32 addr, bool write, int size, u32 pc) {
+	if (lastAddr) {
 		// We have to break to find out if it changed.
 		Core_EnableStepping(true);
-	}
-	else
-	{
-		lastAddr = 0;
+	} else {
 		Action(addr, write, size, pc);
 	}
 }
 
-void MemCheck::JitCleanup()
-{
+bool MemCheck::JitApplyChanged() {
 	if (lastAddr == 0 || lastPC == 0)
-		return;
+		return false;
 
 	// Here's the tricky part: would this have changed memory?
 	// Note that it did not actually get written.
 	bool changed = MIPSAnalyst::OpWouldChangeMemory(lastPC, lastAddr, lastSize);
 	if (changed)
-	{
 		++numHits;
+	return changed;
+}
+
+void MemCheck::JitCleanup(bool changed)
+{
+	if (lastAddr == 0 || lastPC == 0)
+		return;
+
+	if (changed)
 		Log(lastAddr, true, lastSize, lastPC);
-	}
 
 	// Resume if it should not have gone to stepping, or if it did not change.
 	if ((!(result & BREAK_ACTION_PAUSE) || !changed) && coreState == CORE_STEPPING)
@@ -108,6 +124,7 @@ void MemCheck::JitCleanup()
 		host->SetDebugMode(true);
 }
 
+// Note: must lock while calling this.
 size_t CBreakPoints::FindBreakpoint(u32 addr, bool matchTemp, bool temp)
 {
 	size_t found = INVALID_BREAKPOINT;
@@ -140,12 +157,14 @@ size_t CBreakPoints::FindMemCheck(u32 start, u32 end)
 
 bool CBreakPoints::IsAddressBreakPoint(u32 addr)
 {
+	std::lock_guard<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr);
 	return bp != INVALID_BREAKPOINT && breakPoints_[bp].result != BREAK_ACTION_IGNORE;
 }
 
 bool CBreakPoints::IsAddressBreakPoint(u32 addr, bool* enabled)
 {
+	std::lock_guard<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr);
 	if (bp == INVALID_BREAKPOINT) return false;
 	if (enabled != nullptr)
@@ -155,12 +174,14 @@ bool CBreakPoints::IsAddressBreakPoint(u32 addr, bool* enabled)
 
 bool CBreakPoints::IsTempBreakPoint(u32 addr)
 {
+	std::lock_guard<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr, true, true);
 	return bp != INVALID_BREAKPOINT;
 }
 
 bool CBreakPoints::RangeContainsBreakPoint(u32 addr, u32 size)
 {
+	std::lock_guard<std::mutex> guard(breakPointsMutex_);
 	const u32 end = addr + size;
 	for (const auto &bp : breakPoints_)
 	{
@@ -173,6 +194,7 @@ bool CBreakPoints::RangeContainsBreakPoint(u32 addr, u32 size)
 
 void CBreakPoints::AddBreakPoint(u32 addr, bool temp)
 {
+	std::unique_lock<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr, true, temp);
 	if (bp == INVALID_BREAKPOINT)
 	{
@@ -182,18 +204,21 @@ void CBreakPoints::AddBreakPoint(u32 addr, bool temp)
 		pt.addr = addr;
 
 		breakPoints_.push_back(pt);
+		guard.unlock();
 		Update(addr);
 	}
 	else if (!breakPoints_[bp].IsEnabled())
 	{
 		breakPoints_[bp].result |= BREAK_ACTION_PAUSE;
 		breakPoints_[bp].hasCond = false;
+		guard.unlock();
 		Update(addr);
 	}
 }
 
 void CBreakPoints::RemoveBreakPoint(u32 addr)
 {
+	std::unique_lock<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr);
 	if (bp != INVALID_BREAKPOINT)
 	{
@@ -204,12 +229,14 @@ void CBreakPoints::RemoveBreakPoint(u32 addr)
 		if (bp != INVALID_BREAKPOINT)
 			breakPoints_.erase(breakPoints_.begin() + bp);
 
+		guard.unlock();
 		Update(addr);
 	}
 }
 
 void CBreakPoints::ChangeBreakPoint(u32 addr, bool status)
 {
+	std::unique_lock<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr);
 	if (bp != INVALID_BREAKPOINT)
 	{
@@ -217,31 +244,38 @@ void CBreakPoints::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);
 	}
 }
 
 void CBreakPoints::ChangeBreakPoint(u32 addr, BreakAction result)
 {
+	std::unique_lock<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr);
 	if (bp != INVALID_BREAKPOINT)
 	{
 		breakPoints_[bp].result = result;
+		guard.unlock();
 		Update(addr);
 	}
 }
 
 void CBreakPoints::ClearAllBreakPoints()
 {
+	std::unique_lock<std::mutex> guard(breakPointsMutex_);
 	if (!breakPoints_.empty())
 	{
 		breakPoints_.clear();
+		guard.unlock();
 		Update();
 	}
 }
 
 void CBreakPoints::ClearTemporaryBreakPoints()
 {
+	std::unique_lock<std::mutex> guard(breakPointsMutex_);
 	if (breakPoints_.empty())
 		return;
 
@@ -254,34 +288,40 @@ void CBreakPoints::ClearTemporaryBreakPoints()
 			update = true;
 		}
 	}
-	
+
+	guard.unlock();
 	if (update)
 		Update();
 }
 
 void CBreakPoints::ChangeBreakPointAddCond(u32 addr, const BreakPointCond &cond)
 {
+	std::unique_lock<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr, true, false);
 	if (bp != INVALID_BREAKPOINT)
 	{
 		breakPoints_[bp].hasCond = true;
 		breakPoints_[bp].cond = cond;
+		guard.unlock();
 		Update(addr);
 	}
 }
 
 void CBreakPoints::ChangeBreakPointRemoveCond(u32 addr)
 {
+	std::unique_lock<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr, true, false);
 	if (bp != INVALID_BREAKPOINT)
 	{
 		breakPoints_[bp].hasCond = false;
+		guard.unlock();
 		Update(addr);
 	}
 }
 
 BreakPointCond *CBreakPoints::GetBreakPointCondition(u32 addr)
 {
+	std::lock_guard<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr, true, false);
 	if (bp != INVALID_BREAKPOINT && breakPoints_[bp].hasCond)
 		return &breakPoints_[bp].cond;
@@ -289,38 +329,44 @@ BreakPointCond *CBreakPoints::GetBreakPointCondition(u32 addr)
 }
 
 void CBreakPoints::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);
 	}
 }
 
 BreakAction CBreakPoints::ExecBreakPoint(u32 addr) {
+	std::unique_lock<std::mutex> guard(breakPointsMutex_);
 	size_t bp = FindBreakpoint(addr, false);
 	if (bp != INVALID_BREAKPOINT) {
-		if (breakPoints_[bp].hasCond) {
+		BreakPoint info = breakPoints_[bp];
+		guard.unlock();
+
+		if (info.hasCond) {
 			// Evaluate the breakpoint and abort if necessary.
 			auto cond = CBreakPoints::GetBreakPointCondition(currentMIPS->pc);
 			if (cond && !cond->Evaluate())
 				return BREAK_ACTION_IGNORE;
 		}
 
-		if (breakPoints_[bp].result & BREAK_ACTION_LOG) {
-			if (breakPoints_[bp].logFormat.empty()) {
+		if (info.result & BREAK_ACTION_LOG) {
+			if (info.logFormat.empty()) {
 				NOTICE_LOG(JIT, "BKP PC=%08x (%s)", addr, g_symbolMap->GetDescription(addr).c_str());
 			} else {
 				std::string formatted;
-				CBreakPoints::EvaluateLogFormat(currentDebugMIPS, breakPoints_[bp].logFormat, formatted);
+				CBreakPoints::EvaluateLogFormat(currentDebugMIPS, info.logFormat, formatted);
 				NOTICE_LOG(JIT, "BKP PC=%08x: %s", addr, formatted.c_str());
 			}
 		}
-		if (breakPoints_[bp].result & BREAK_ACTION_PAUSE) {
+		if (info.result & BREAK_ACTION_PAUSE) {
 			Core_EnableStepping(true);
 			host->SetDebugMode(true);
 		}
 
-		return breakPoints_[bp].result;
+		return info.result;
 	}
 
 	return BREAK_ACTION_IGNORE;
@@ -328,6 +374,7 @@ BreakAction CBreakPoints::ExecBreakPoint(u32 addr) {
 
 void CBreakPoints::AddMemCheck(u32 start, u32 end, MemCheckCondition cond, BreakAction result)
 {
+	std::unique_lock<std::mutex> guard(memCheckMutex_);
 	// This will ruin any pending memchecks.
 	cleanupMemChecks_.clear();
 
@@ -341,18 +388,21 @@ void CBreakPoints::AddMemCheck(u32 start, u32 end, MemCheckCondition cond, Break
 		check.result = result;
 
 		memChecks_.push_back(check);
+		guard.unlock();
 		Update();
 	}
 	else
 	{
 		memChecks_[mc].cond = (MemCheckCondition)(memChecks_[mc].cond | cond);
 		memChecks_[mc].result = (BreakAction)(memChecks_[mc].result | result);
+		guard.unlock();
 		Update();
 	}
 }
 
 void CBreakPoints::RemoveMemCheck(u32 start, u32 end)
 {
+	std::unique_lock<std::mutex> guard(memCheckMutex_);
 	// This will ruin any pending memchecks.
 	cleanupMemChecks_.clear();
 
@@ -360,37 +410,44 @@ void CBreakPoints::RemoveMemCheck(u32 start, u32 end)
 	if (mc != INVALID_MEMCHECK)
 	{
 		memChecks_.erase(memChecks_.begin() + mc);
+		guard.unlock();
 		Update();
 	}
 }
 
 void CBreakPoints::ChangeMemCheck(u32 start, u32 end, MemCheckCondition cond, BreakAction result)
 {
+	std::unique_lock<std::mutex> guard(memCheckMutex_);
 	size_t mc = FindMemCheck(start, end);
 	if (mc != INVALID_MEMCHECK)
 	{
 		memChecks_[mc].cond = cond;
 		memChecks_[mc].result = result;
+		guard.unlock();
 		Update();
 	}
 }
 
 void CBreakPoints::ClearAllMemChecks()
 {
+	std::unique_lock<std::mutex> guard(memCheckMutex_);
 	// This will ruin any pending memchecks.
 	cleanupMemChecks_.clear();
 
 	if (!memChecks_.empty())
 	{
 		memChecks_.clear();
+		guard.unlock();
 		Update();
 	}
 }
 
 void CBreakPoints::ChangeMemCheckLogFormat(u32 start, u32 end, const std::string &fmt) {
+	std::unique_lock<std::mutex> guard(memCheckMutex_);
 	size_t mc = FindMemCheck(start, end);
 	if (mc != INVALID_MEMCHECK) {
 		memChecks_[mc].logFormat = fmt;
+		guard.unlock();
 		Update();
 	}
 }
@@ -401,8 +458,12 @@ static inline u32 NotCached(u32 val)
 	return val & ~0x40000000;
 }
 
-MemCheck *CBreakPoints::GetMemCheck(u32 address, int size)
-{
+MemCheck *CBreakPoints::GetMemCheck(u32 address, int size) {
+	std::lock_guard<std::mutex> guard(memCheckMutex_);
+	return GetMemCheckLocked(address, size);
+}
+
+MemCheck *CBreakPoints::GetMemCheckLocked(u32 address, int size) {
 	std::vector<MemCheck>::iterator iter;
 	for (iter = memChecks_.begin(); iter != memChecks_.end(); ++iter)
 	{
@@ -425,9 +486,14 @@ MemCheck *CBreakPoints::GetMemCheck(u32 address, int size)
 
 BreakAction CBreakPoints::ExecMemCheck(u32 address, bool write, int size, u32 pc)
 {
-	auto check = GetMemCheck(address, size);
-	if (check)
-		return check->Action(address, write, size, pc);
+	std::unique_lock<std::mutex> guard(memCheckMutex_);
+	auto check = GetMemCheckLocked(address, size);
+	if (check) {
+		check->Apply(address, write, size, pc);
+		auto copy = *check;
+		guard.unlock();
+		return copy.Action(address, write, size, pc);
+	}
 	return BREAK_ACTION_IGNORE;
 }
 
@@ -443,15 +509,23 @@ BreakAction CBreakPoints::ExecOpMemCheck(u32 address, u32 pc)
 	}
 
 	bool write = MIPSAnalyst::IsOpMemoryWrite(pc);
-	auto check = GetMemCheck(address, size);
+	std::unique_lock<std::mutex> guard(memCheckMutex_);
+	auto check = GetMemCheckLocked(address, size);
 	if (check) {
 		int mask = MEMCHECK_WRITE | MEMCHECK_WRITE_ONCHANGE;
+		bool apply = false;
 		if (write && (check->cond & mask) == mask) {
 			if (MIPSAnalyst::OpWouldChangeMemory(pc, address, size)) {
-				return check->Action(address, write, size, pc);
+				apply = true;
 			}
 		} else {
-			return check->Action(address, write, size, pc);
+			apply = true;
+		}
+		if (apply) {
+			check->Apply(address, write, size, pc);
+			auto copy = *check;
+			guard.unlock();
+			return copy.Action(address, write, size, pc);
 		}
 	}
 	return BREAK_ACTION_IGNORE;
@@ -459,18 +533,28 @@ BreakAction CBreakPoints::ExecOpMemCheck(u32 address, u32 pc)
 
 void CBreakPoints::ExecMemCheckJitBefore(u32 address, bool write, int size, u32 pc)
 {
-	auto check = GetMemCheck(address, size);
+	std::unique_lock<std::mutex> guard(memCheckMutex_);
+	auto check = GetMemCheckLocked(address, size);
 	if (check) {
-		check->JitBefore(address, write, size, pc);
+		check->JitBeforeApply(address, write, size, pc);
+		auto copy = *check;
+		guard.unlock();
+		copy.JitBeforeAction(address, write, size, pc);
+		guard.lock();
 		cleanupMemChecks_.push_back(check);
 	}
 }
 
 void CBreakPoints::ExecMemCheckJitCleanup()
 {
+	std::unique_lock<std::mutex> guard(memCheckMutex_);
 	for (auto it = cleanupMemChecks_.begin(), end = cleanupMemChecks_.end(); it != end; ++it) {
 		auto check = *it;
-		check->JitCleanup();
+		bool changed = check->JitApplyChanged();
+		auto copy = *check;
+		guard.unlock();
+		copy.JitCleanup(changed);
+		guard.lock();
 	}
 	cleanupMemChecks_.clear();
 }
@@ -489,6 +573,7 @@ u32 CBreakPoints::CheckSkipFirst()
 }
 
 const std::vector<MemCheck> CBreakPoints::GetMemCheckRanges(bool write) {
+	std::lock_guard<std::mutex> guard(memCheckMutex_);
 	std::vector<MemCheck> ranges = memChecks_;
 	for (const auto &check : memChecks_) {
 		if (!(check.cond & MEMCHECK_READ) && !write)
@@ -509,16 +594,19 @@ const std::vector<MemCheck> CBreakPoints::GetMemCheckRanges(bool write) {
 
 const std::vector<MemCheck> CBreakPoints::GetMemChecks()
 {
+	std::lock_guard<std::mutex> guard(memCheckMutex_);
 	return memChecks_;
 }
 
 const std::vector<BreakPoint> CBreakPoints::GetBreakpoints()
 {
+	std::lock_guard<std::mutex> guard(breakPointsMutex_);
 	return breakPoints_;
 }
 
 bool CBreakPoints::HasMemChecks()
 {
+	std::lock_guard<std::mutex> guard(memCheckMutex_);
 	return !memChecks_.empty();
 }