Debugger: Fixed issues & improved performance with new breakpoint logic

Core/BaseMapper.cpp

@@ -1058,6 +1058,29 @@ void BaseMapper::SetMemoryValue(DebugMemoryType memoryType, uint32_t address, ui
 	}
 }
 
+void BaseMapper::GetAbsoluteAddressAndType(uint32_t relativeAddr, AddressTypeInfo* info)
+{
+	if(relativeAddr < 0x2000) {
+		info->Address = relativeAddr;
+		info->Type = AddressType::InternalRam;
+	} else {
+		uint8_t *prgAddr = _prgPages[relativeAddr >> 8] + (uint8_t)relativeAddr;
+		if(prgAddr >= _prgRom && prgAddr < _prgRom + _prgSize) {
+			info->Address = (uint32_t)(prgAddr - _prgRom);
+			info->Type = AddressType::PrgRom;
+		} else if(prgAddr >= _workRam && prgAddr < _workRam + _workRamSize) {
+			info->Address = (uint32_t)(prgAddr - _workRam);
+			info->Type = AddressType::WorkRam;
+		} else if(prgAddr >= _saveRam && prgAddr < _saveRam + _saveRamSize) {
+			info->Address = (uint32_t)(prgAddr - _saveRam);
+			info->Type = AddressType::SaveRam;
+		} else {
+			info->Address = -1;
+			info->Type = AddressType::InternalRam;
+		}
+	}
+}
+
 int32_t BaseMapper::ToAbsoluteAddress(uint16_t addr)
 {
 	uint8_t *prgAddr = _prgPages[addr >> 8] + (uint8_t)addr;

Core/BaseMapper.h

@@ -215,6 +215,8 @@ public:
 
 	uint32_t CopyMemory(DebugMemoryType type, uint8_t* buffer);
 	void WriteMemory(DebugMemoryType type, uint8_t* buffer);
+
+	void GetAbsoluteAddressAndType(uint32_t relativeAddr, AddressTypeInfo *info);
 	int32_t ToAbsoluteAddress(uint16_t addr);
 	int32_t ToAbsoluteSaveRamAddress(uint16_t addr);
 	int32_t ToAbsoluteWorkRamAddress(uint16_t addr);

Core/Breakpoint.cpp

@@ -10,12 +10,8 @@ Breakpoint::~Breakpoint()
 {
 }
 
-bool Breakpoint::Matches(uint32_t memoryAddr, AddressTypeInfo &info, MemoryOperationType opType)
+bool Breakpoint::Matches(uint32_t memoryAddr, AddressTypeInfo &info)
 {
-	if(!_processDummyReadWrites && (opType == MemoryOperationType::DummyRead || opType == MemoryOperationType::DummyWrite)) {
-		return false;
-	}
-
 	if(_startAddr == -1) {
 		return true;
 	}
@@ -72,11 +68,13 @@ bool Breakpoint::HasBreakpointType(BreakpointType type)
 {
 	switch(type) {
 		case BreakpointType::Global: return (_type == BreakpointTypeFlags::Global);
-		case BreakpointType::Execute: return (_type & BreakpointTypeFlags::Execute) == BreakpointTypeFlags::Execute;
+		case BreakpointType::Execute: return (_type & BreakpointTypeFlags::Execute) != 0;
-		case BreakpointType::ReadRam: return (_type & BreakpointTypeFlags::ReadRam) == BreakpointTypeFlags::ReadRam;
+		case BreakpointType::ReadRam: return (_type & BreakpointTypeFlags::ReadRam) != 0;
-		case BreakpointType::WriteRam: return (_type & BreakpointTypeFlags::WriteRam) == BreakpointTypeFlags::WriteRam;
+		case BreakpointType::WriteRam: return (_type & BreakpointTypeFlags::WriteRam) != 0;
-		case BreakpointType::ReadVram: return (_type & BreakpointTypeFlags::ReadVram) == BreakpointTypeFlags::ReadVram;
+		case BreakpointType::ReadVram: return (_type & BreakpointTypeFlags::ReadVram) != 0;
-		case BreakpointType::WriteVram: return (_type & BreakpointTypeFlags::WriteVram) == BreakpointTypeFlags::WriteVram;
+		case BreakpointType::WriteVram: return (_type & BreakpointTypeFlags::WriteVram) != 0;
+		case BreakpointType::DummyReadRam: return (_type & BreakpointTypeFlags::ReadRam) != 0 && _processDummyReadWrites;
+		case BreakpointType::DummyWriteRam: return (_type & BreakpointTypeFlags::WriteRam) != 0 && _processDummyReadWrites;
 	}
 	return false;
 }

Core/Breakpoint.h

@@ -19,7 +19,7 @@ public:
 	Breakpoint();
 	~Breakpoint();
 
-	bool Matches(uint32_t memoryAddr, AddressTypeInfo &info, MemoryOperationType opType);
+	bool Matches(uint32_t memoryAddr, AddressTypeInfo &info);
 	bool Matches(uint32_t memoryAddr, PpuAddressTypeInfo &info);
 	bool HasBreakpointType(BreakpointType type);
 	string GetCondition();

Core/CPU.cpp

@@ -186,13 +186,6 @@ void CPU::BRK() {
 
 void CPU::MemoryWrite(uint16_t addr, uint8_t value, MemoryOperationType operationType)
 {
-	_cpuWrite = true;;
-	_writeAddr = addr;
-	IncCycleCount();
-	while(_dmcDmaRunning) {
-		IncCycleCount();
-	}
-
 #ifdef DUMMYCPU
 	if(operationType == MemoryOperationType::Write || operationType == MemoryOperationType::DummyWrite) {
 		_writeAddresses[_writeCounter] = addr;
@@ -201,17 +194,34 @@ void CPU::MemoryWrite(uint16_t addr, uint8_t value, MemoryOperationType operatio
 		_writeCounter++;
 	}
 #else
+	_cpuWrite = true;;
+	_writeAddr = addr;
+	IncCycleCount();
+	while(_dmcDmaRunning) {
+		IncCycleCount();
+	}
+
 	_memoryManager->Write(addr, value, operationType);
-#endif
 
 	//DMA DMC might have started after a write to $4015, stall CPU if needed
 	while(_dmcDmaRunning) {
 		IncCycleCount();
 	}
 	_cpuWrite = false;
+#endif
 }
 
 uint8_t CPU::MemoryRead(uint16_t addr, MemoryOperationType operationType) {
+#ifdef DUMMYCPU
+	uint8_t value = _memoryManager->DebugRead(addr);
+	if(operationType == MemoryOperationType::Read || operationType == MemoryOperationType::DummyRead) {
+		_readAddresses[_readCounter] = addr;
+		_readValue[_readCounter] = value;
+		_isDummyRead[_readCounter] = operationType == MemoryOperationType::DummyRead;
+		_readCounter++;
+	}
+	return value;
+#else 
 	IncCycleCount();
 	while(_dmcDmaRunning) {
 		//Stall CPU until we can process a DMC read
@@ -220,24 +230,11 @@ uint8_t CPU::MemoryRead(uint16_t addr, MemoryOperationType operationType) {
 			//Reads are only performed every other cycle? This fixes "dma_2007_read" test
 			//This behavior causes the $4016/7 data corruption when a DMC is running.
 			//When reading $4016/7, only the last read counts (because this only occurs to low-to-high transitions, i.e once in this case)
-			#ifdef DUMMYCPU
-			_memoryManager->DebugRead(addr);
-			#else
 			_memoryManager->Read(addr);
-			#endif
 		}
 		IncCycleCount();
 	}
 
-#ifdef DUMMYCPU
-	if(operationType == MemoryOperationType::Read || operationType == MemoryOperationType::DummyRead) {
-		_readAddresses[_readCounter] = addr;
-		_isDummyRead[_readCounter] = operationType == MemoryOperationType::DummyRead;
-		_readCounter++;
-	}
-
-	return _memoryManager->DebugRead(addr);
-#else 
 	if(operationType == MemoryOperationType::ExecOpCode) {
 		_state.DebugPC = _state.PC;
 	}
@@ -310,9 +307,7 @@ void CPU::IncCycleCount()
 		}
 	}
 
-	#ifndef DUMMYCPU
 	_console->ProcessCpuClock();
-	#endif
 
 	if(!_spriteDmaTransfer && !_dmcDmaRunning) {
 		//IRQ flags are ignored during Sprite DMA - fixes irq_and_dma

Core/CPU.h

@@ -63,6 +63,7 @@ private:
 
 	uint32_t _readCounter = 0;
 	uint16_t _readAddresses[10];
+	uint8_t _readValue[10];
 	bool _isDummyRead[10];
 #endif
 
@@ -863,9 +864,10 @@ public:
 		isDummyWrite = _isDummyWrite[index];
 	}
 
-	void GetReadAddr(uint32_t index, uint16_t &addr, bool &isDummyRead)
+	void GetReadAddr(uint32_t index, uint16_t &addr, uint8_t &value, bool &isDummyRead)
 	{
 		addr = _readAddresses[index];
+		value = _readValue[index];
 		isDummyRead = _isDummyRead[index];
 	}
 #endif

Core/Debugger.cpp

@@ -283,6 +283,8 @@ void Debugger::SetBreakpoints(Breakpoint breakpoints[], uint32_t length)
 		_hasBreakpoint[i] = false;
 	}
 
+	_bpDummyCpuRequired = false;
+
 	_bpExpEval.reset(new ExpressionEvaluator(this));
 	for(uint32_t j = 0; j < length; j++) {
 		Breakpoint &bp = breakpoints[j];
@@ -297,6 +299,11 @@ void Debugger::SetBreakpoints(Breakpoint breakpoints[], uint32_t length)
 					_breakpointRpnList[i].push_back(success ? data : ExpressionData());
 				}
 
+				if(bp.IsEnabled()) {
+					bool isReadWriteBp = i == BreakpointType::ReadVram || i == BreakpointType::ReadRam || i == BreakpointType::WriteVram || i == BreakpointType::WriteRam || i == BreakpointType::DummyReadRam || i == BreakpointType::DummyWriteRam;
+					_bpDummyCpuRequired |= isReadWriteBp;
+				}
+
 				_hasBreakpoint[i] = true;
 			}
 		}
@@ -320,6 +327,8 @@ bool Debugger::ProcessBreakpoints(BreakpointType type, OperationInfo &operationI
 		case BreakpointType::Execute:
 		case BreakpointType::ReadRam:
 		case BreakpointType::WriteRam:
+		case BreakpointType::DummyReadRam:
+		case BreakpointType::DummyWriteRam:
 			GetAbsoluteAddressAndType(operationInfo.Address, &info);
 			break;
 
@@ -359,7 +368,7 @@ bool Debugger::ProcessBreakpoints(BreakpointType type, OperationInfo &operationI
 
 		if(
 			type == BreakpointType::Global ||
-			(!isPpuBreakpoint && breakpoint.Matches(operationInfo.Address, info, operationInfo.OperationType)) ||
+			(!isPpuBreakpoint && breakpoint.Matches(operationInfo.Address, info)) ||
 			(isPpuBreakpoint && breakpoint.Matches(operationInfo.Address, ppuInfo))
 		) {
 			if(!breakpoint.HasCondition()) {
@@ -401,6 +410,13 @@ void Debugger::ProcessAllBreakpoints(OperationInfo &operationInfo, AddressTypeIn
 		ProcessBreakpoints(BreakpointType::Execute, operationInfo, true, true);
 	}
 
+	bool checkUninitReads = _enableBreakOnUninitRead && CheckFlag(DebuggerFlags::BreakOnUninitMemoryRead);
+
+	if(!checkUninitReads && !_bpDummyCpuRequired) {
+		//Nothing to do, no read/write breakpoints are active and don't need to check uninit reads
+		return;
+	}
+
 	_dummyCpu->SetDummyState(_cpu.get());
 	_dummyCpu->Exec();
 
@@ -408,9 +424,10 @@ void Debugger::ProcessAllBreakpoints(OperationInfo &operationInfo, AddressTypeIn
 	uint32_t readCount = _dummyCpu->GetReadCount();
 	if(readCount > 0) {
 		uint16_t addr;
+		uint8_t value;
 		bool isDummyRead;
 		for(uint32_t i = 0; i < readCount; i++) {
-			_dummyCpu->GetReadAddr(i, addr, isDummyRead);
+			_dummyCpu->GetReadAddr(i, addr, value, isDummyRead);
 
 			OperationInfo info;
 
@@ -433,7 +450,7 @@ void Debugger::ProcessAllBreakpoints(OperationInfo &operationInfo, AddressTypeIn
 
 				info.Value = state.PPU.MemoryReadBuffer;
 			} else {
-				if(_enableBreakOnUninitRead && CheckFlag(DebuggerFlags::BreakOnUninitMemoryRead)) {
+				if(!isDummyRead && checkUninitReads) {
 					//Break on uninit memory read
 					if(_memoryAccessCounter->IsAddressUninitialized(addressInfo)) {
 						Step(1);
@@ -442,34 +459,52 @@ void Debugger::ProcessAllBreakpoints(OperationInfo &operationInfo, AddressTypeIn
 					}
 				}
 
-				info.Value = _memoryManager->DebugRead(addr);
+				info.Value = value;
 			}
 
-			if(_hasBreakpoint[BreakpointType::ReadRam]) {
+			info.Address = addr;
-				info.Address = addr;
+			if(isDummyRead) {
-				info.OperationType = isDummyRead ? MemoryOperationType::DummyRead : MemoryOperationType::Read;
+				if(_hasBreakpoint[BreakpointType::DummyReadRam]) {
-				if(ProcessBreakpoints(BreakpointType::ReadRam, info, true, false)) {
+					info.OperationType = MemoryOperationType::DummyRead;
-					return;
+					if(ProcessBreakpoints(BreakpointType::DummyReadRam, info, true, false)) {
+						return;
+					}
+				}
+			} else {
+				if(_hasBreakpoint[BreakpointType::ReadRam]) {
+					info.OperationType = MemoryOperationType::Read;
+					if(ProcessBreakpoints(BreakpointType::ReadRam, info, true, false)) {
+						return;
+					}
 				}
 			}
 		}
 	}
 	
 	uint32_t writeCount = _dummyCpu->GetWriteCount();
-	if(writeCount > 0 && (_hasBreakpoint[BreakpointType::WriteRam] || _hasBreakpoint[BreakpointType::WriteVram])) {
+	if(writeCount > 0) {
 		uint16_t addr;
 		uint8_t value;
 		bool isDummyWrite;
 		for(uint32_t i = 0; i < writeCount; i++) {
 			_dummyCpu->GetWriteAddrValue(i, addr, value, isDummyWrite);
 
-			if(_hasBreakpoint[BreakpointType::WriteRam]) {
+			OperationInfo info;
-				OperationInfo info;
+			info.Address = addr;
-				info.Address = addr;
+			info.Value = value;
-				info.Value = value;
+			if(isDummyWrite) {
-				info.OperationType = isDummyWrite ? MemoryOperationType::DummyWrite : MemoryOperationType::Write;
+				if(_hasBreakpoint[BreakpointType::DummyWriteRam]) {
-				if(ProcessBreakpoints(BreakpointType::WriteRam, info, true, false)) {
+					info.OperationType = MemoryOperationType::DummyWrite;
-					return;
+					if(ProcessBreakpoints(BreakpointType::DummyWriteRam, info, true, false)) {
+						return;
+					}
+				}
+			} else {
+				if(_hasBreakpoint[BreakpointType::WriteRam]) {
+					info.OperationType = MemoryOperationType::Write;
+					if(ProcessBreakpoints(BreakpointType::WriteRam, info, true, false)) {
+						return;
+					}
 				}
 			}
 
@@ -801,10 +836,10 @@ bool Debugger::ProcessRamOperation(MemoryOperationType type, uint16_t &addr, uin
 	if(_breakOnFirstCycle) {
 		if(type == MemoryOperationType::ExecOpCode && !breakDone) {
 			ProcessAllBreakpoints(operationInfo, addressInfo);
+		} else if(_hasBreakpoint[breakpointType]) {
+			//Process marked breakpoints
+			ProcessBreakpoints(breakpointType, operationInfo, false, true);
 		}
-
-		//Process marked breakpoints
-		ProcessBreakpoints(breakpointType, operationInfo, false, true);
 	} else {
 		if(_hasBreakpoint[breakpointType]) {
 			ProcessBreakpoints(breakpointType, operationInfo, !breakDone, true);
@@ -907,9 +942,9 @@ void Debugger::ProcessVramReadOperation(MemoryOperationType type, uint16_t addr,
 	int32_t absoluteAddr = _mapper->ToAbsoluteChrAddress(addr);
 	_codeDataLogger->SetFlag(absoluteAddr, type == MemoryOperationType::Read ? CdlChrFlags::Read : CdlChrFlags::Drawn);
 
-	if(!_breakOnFirstCycle && _hasBreakpoint[BreakpointType::ReadVram]) {
+	if(_hasBreakpoint[BreakpointType::ReadVram]) {
 		OperationInfo operationInfo{ addr, value, type };
-		ProcessBreakpoints(BreakpointType::ReadVram, operationInfo);
+		ProcessBreakpoints(BreakpointType::ReadVram, operationInfo, !_breakOnFirstCycle, true);
 	}
 
 	ProcessPpuOperation(addr, value, MemoryOperationType::Read);
@@ -917,9 +952,9 @@ void Debugger::ProcessVramReadOperation(MemoryOperationType type, uint16_t addr,
 
 void Debugger::ProcessVramWriteOperation(uint16_t addr, uint8_t &value)
 {
-	if(!_breakOnFirstCycle && _hasBreakpoint[BreakpointType::WriteVram]) {
+	if(_hasBreakpoint[BreakpointType::WriteVram]) {
 		OperationInfo operationInfo{ addr, value, MemoryOperationType::Write };
-		ProcessBreakpoints(BreakpointType::WriteVram, operationInfo);
+		ProcessBreakpoints(BreakpointType::WriteVram, operationInfo, !_breakOnFirstCycle, true);
 	}
 
 	ProcessPpuOperation(addr, value, MemoryOperationType::Write);
@@ -1231,35 +1266,7 @@ void Debugger::AllowResume()
 
 void Debugger::GetAbsoluteAddressAndType(uint32_t relativeAddr, AddressTypeInfo* info)
 {
-	if(relativeAddr < 0x2000) {
+	return _mapper->GetAbsoluteAddressAndType(relativeAddr, info);
-		info->Address = relativeAddr;
-		info->Type = AddressType::InternalRam;
-		return;
-	}
-
-	int32_t addr = _mapper->ToAbsoluteAddress(relativeAddr);
-	if(addr >= 0) {
-		info->Address = addr;
-		info->Type = AddressType::PrgRom;
-		return;
-	}
-	
-	addr = _mapper->ToAbsoluteWorkRamAddress(relativeAddr);
-	if(addr >= 0) {
-		info->Address = addr;
-		info->Type = AddressType::WorkRam;
-		return;
-	}
-
-	addr = _mapper->ToAbsoluteSaveRamAddress(relativeAddr);
-	if(addr >= 0) {
-		info->Address = addr;
-		info->Type = AddressType::SaveRam;
-		return;
-	}
-
-	info->Address = -1;
-	info->Type = AddressType::InternalRam;
 }
 
 void Debugger::GetPpuAbsoluteAddressAndType(uint32_t relativeAddr, PpuAddressTypeInfo* info)

Core/Debugger.h

@@ -38,7 +38,7 @@ enum class CdlStripFlag;
 class Debugger
 {
 private:
-	static constexpr int BreakpointTypeCount = 6;
+	static constexpr int BreakpointTypeCount = 8;
 
 	//Must be static to be thread-safe when switching game
 	static string _disassemblerOutput;
@@ -61,6 +61,7 @@ private:
 	shared_ptr<BaseMapper> _mapper;
 	
 	shared_ptr<DummyCpu> _dummyCpu;
+	bool _bpDummyCpuRequired;
 	bool _breakOnFirstCycle;
 
 	bool _hasScript;
@@ -169,6 +170,8 @@ public:
 	
 	void SetBreakpoints(Breakpoint breakpoints[], uint32_t length);
 
+	void ProcessMarkedBreakpoints(BreakpointType type, OperationInfo &operationInfo);
+	
 	shared_ptr<LabelManager> GetLabelManager();
 
 	void GetFunctionEntryPoints(int32_t* entryPoints, int32_t maxCount);

Core/DebuggerTypes.h

@@ -10,6 +10,8 @@ enum BreakpointType
 	WriteRam = 3,
 	ReadVram = 4,
 	WriteVram = 5,
+	DummyReadRam = 6,
+	DummyWriteRam = 7
 };
 
 enum class DebuggerFlags

GUI.NET/Debugger/frmDebugger.cs

@@ -507,13 +507,7 @@ namespace Mesen.GUI.Debugger
 					if(breakpointId >= 0 && breakpointId < breakpoints.Count) {
 						Breakpoint bp = breakpoints[breakpointId];
 						if(bpType != BreakpointType.Global) {
-							string prefix = "";
+							message += ": " + ResourceHelper.GetEnumText(bpType) + " ($" + bpAddress.ToString("X4") + ")";
-							if(bpType == BreakpointType.ReadRam || bpType == BreakpointType.WriteRam) {
-								if(memOpType == InteropMemoryOperationType.DummyRead || memOpType == InteropMemoryOperationType.DummyWrite) {
-									prefix = "(Dummy) ";
-								}
-							}
-							message += ": " + prefix + ResourceHelper.GetEnumText(bpType) + " ($" + bpAddress.ToString("X4") + ")";
 						}
 						if(!string.IsNullOrWhiteSpace(bp.Condition)) {
 							string cond = bp.Condition.Trim();

GUI.NET/Dependencies/resources.en.xml

@@ -1194,6 +1194,8 @@
 			<Value ID="WriteRam">CPU Write</Value>
 			<Value ID="ReadVram">PPU Read</Value>
 			<Value ID="WriteVram">PPU Write</Value>
+			<Value ID="DummyReadRam">(Dummy) CPU Read</Value>
+			<Value ID="DummyWriteRam">(Dummy) CPU Write</Value>
 		</Enum>
 		<Enum ID="BreakSource">
 			<Value ID="Breakpoint">Breakpoint</Value>

GUI.NET/InteropEmu.cs

@@ -2113,6 +2113,8 @@ namespace Mesen.GUI
 		WriteRam = 3,
 		ReadVram = 4,
 		WriteVram = 5,
+		DummyReadRam = 6,
+		DummyWriteRam = 7
 	}
 
 	[Flags]

PGOHelper/PGOHelper.cpp

@@ -56,6 +56,7 @@ extern "C" {
 	void __stdcall Release();
 	void __stdcall Stop();
 	void __stdcall DebugInitialize();
+	void __stdcall DebugSetFlags(uint32_t flags);
 }
 
 vector<string> GetFilesInFolder(string rootFolder, std::unordered_set<string> extensions)
@@ -104,6 +105,7 @@ int main(int argc, char* argv[])
 			SetVideoFilter(filterTypes[i % 13]);
 			LoadROM(testRoms[i].c_str(), "");
 			DebugInitialize();
+			DebugSetFlags(0x10000 /*DebuggerFlags::BreakOnFirstCycle*/);
 		}
 		std::this_thread::sleep_for(std::chrono::duration<int, std::milli>(5000));
 		Stop();