Merge pull request #17269 from unknownbrackets/debugger-minor

Debugger: Avoid unaligned reads in expressions

Core/Debugger/DisassemblyManager.cpp

@@ -364,7 +364,7 @@ u32 DisassemblyManager::getNthPreviousAddress(u32 address, int n)
 		analyze(address-127,128);
 	}
 	
-	return address-n*4;
+	return (address - n * 4) & ~3;
 }
 
 u32 DisassemblyManager::getNthNextAddress(u32 address, int n)
@@ -396,7 +396,7 @@ u32 DisassemblyManager::getNthNextAddress(u32 address, int n)
 		analyze(address);
 	}
 
-	return address+n*4;
+	return (address + n * 4) & ~3;
 }
 
 DisassemblyManager::~DisassemblyManager() {

Core/MIPS/MIPSDebugInterface.cpp

@@ -162,17 +162,20 @@ public:
 	bool getMemoryValue(uint32_t address, int size, uint32_t& dest, char* error) override {
 		// We allow, but ignore, bad access.
 		// If we didn't, log/condition statements that reference registers couldn't be configured.
-		bool valid = Memory::IsValidRange(address, size);
+		uint32_t valid = Memory::ValidSize(address, size);
+		uint8_t buf[4]{};
+		if (valid != 0)
+			memcpy(buf, Memory::GetPointerUnchecked(address), valid);
 
 		switch (size) {
 		case 1:
-			dest = valid ? Memory::Read_U8(address) : 0;
+			dest = buf[0];
 			return true;
 		case 2:
-			dest = valid ? Memory::Read_U16(address) : 0;
+			dest = (buf[1] << 8) | buf[0];
 			return true;
 		case 4:
-			dest = valid ? Memory::Read_U32(address) : 0;
+			dest = (buf[3] << 24) | (buf[2] << 16) | (buf[1] << 8) | buf[0];
 			return true;
 		}
 
@@ -196,9 +199,10 @@ const char *MIPSDebugInterface::disasm(unsigned int address, unsigned int align)
 	return mojs;
 }
 
-unsigned int MIPSDebugInterface::readMemory(unsigned int address)
-{
-	return Memory::Read_Instruction(address).encoding;
+unsigned int MIPSDebugInterface::readMemory(unsigned int address) {
+	if (Memory::IsValidRange(address, 4))
+		return Memory::ReadUnchecked_Instruction(address).encoding;
+	return 0;
 }
 
 bool MIPSDebugInterface::isAlive()

Core/MemMap.h

@@ -331,7 +331,7 @@ inline u32 ValidSize(const u32 address, const u32 requested_size) {
 }
 
 inline bool IsValidRange(const u32 address, const u32 size) {
-	return IsValidAddress(address) && ValidSize(address, size) == size;
+	return ValidSize(address, size) == size;
 }
 
 }  // namespace Memory

GPU/Common/GPUDebugInterface.cpp

@@ -929,17 +929,20 @@ ExpressionType GEExpressionFunctions::getFieldType(GECmdFormat fmt, GECmdField f
 bool GEExpressionFunctions::getMemoryValue(uint32_t address, int size, uint32_t &dest, char *error) {
 	// We allow, but ignore, bad access.
 	// If we didn't, log/condition statements that reference registers couldn't be configured.
-	bool valid = Memory::IsValidRange(address, size);
+	uint32_t valid = Memory::ValidSize(address, size);
+	uint8_t buf[4]{};
+	if (valid != 0)
+		memcpy(buf, Memory::GetPointerUnchecked(address), valid);
 
 	switch (size) {
 	case 1:
-		dest = valid ? Memory::Read_U8(address) : 0;
+		dest = buf[0];
 		return true;
 	case 2:
-		dest = valid ? Memory::Read_U16(address) : 0;
+		dest = (buf[1] << 8) | buf[0];
 		return true;
 	case 4:
-		dest = valid ? Memory::Read_U32(address) : 0;
+		dest = (buf[3] << 24) | (buf[2] << 16) | (buf[1] << 8) | buf[0];
 		return true;
 	}
 

Windows/Debugger/CtrlDisAsmView.cpp

@@ -909,6 +909,8 @@ void CtrlDisAsmView::onMouseDown(WPARAM wParam, LPARAM lParam, int button)
 }
 
 void CtrlDisAsmView::CopyInstructions(u32 startAddr, u32 endAddr, CopyInstructionsMode mode) {
+	_assert_msg_((startAddr & 3) == 0, "readMemory() can't handle unaligned reads");
+
 	if (mode != CopyInstructionsMode::DISASM) {
 		int instructionSize = debugger->getInstructionSize(0);
 		int count = (endAddr - startAddr) / instructionSize;

Windows/Debugger/CtrlMemView.cpp

@@ -218,6 +218,8 @@ void CtrlMemView::onPaint(WPARAM wParam, LPARAM lParam) {
 		}
 	};
 
+	_assert_msg_(((windowStart_ | rowSize_) & 3) == 0, "readMemory() can't handle unaligned reads");
+
 	// draw one extra row that may be partially visible
 	for (int i = 0; i < visibleRows_ + 1; i++) {
 		int rowY = rowHeight_ * i;
@@ -236,8 +238,8 @@ void CtrlMemView::onPaint(WPARAM wParam, LPARAM lParam) {
 			uint32_t words[4];
 			uint8_t bytes[16];
 		} memory;
-		bool valid = debugger_ != nullptr && debugger_->isAlive() && Memory::IsValidAddress(address);
-		for (int i = 0; valid && i < 4; ++i) {
+		int valid = debugger_ != nullptr && debugger_->isAlive() ? Memory::ValidSize(address, 16) / 4 : 0;
+		for (int i = 0; i < valid; ++i) {
 			memory.words[i] = debugger_->readMemory(address + i * 4);
 		}
 

Windows/Debugger/Debugger_Lists.cpp

@@ -887,20 +887,23 @@ bool CtrlWatchList::WindowMessage(UINT msg, WPARAM wParam, LPARAM lParam, LRESUL
 }
 
 void CtrlWatchList::GetColumnText(wchar_t *dest, int row, int col) {
-	uint32_t value = 0;
-	float valuef = 0.0f;
+	const auto &watch = watches_[row];
 	switch (col) {
 	case WL_NAME:
-		wcsncpy(dest, ConvertUTF8ToWString(watches_[row].name).c_str(), 255);
+		wcsncpy(dest, ConvertUTF8ToWString(watch.name).c_str(), 255);
 		dest[255] = 0;
 		break;
 	case WL_EXPRESSION:
-		wcsncpy(dest, ConvertUTF8ToWString(watches_[row].originalExpression).c_str(), 255);
+		wcsncpy(dest, ConvertUTF8ToWString(watch.originalExpression).c_str(), 255);
 		dest[255] = 0;
 		break;
 	case WL_VALUE:
-		if (cpu_->parseExpression(watches_[row].expression, value)) {
-			switch (watches_[row].format) {
+		if (watch.evaluateFailed) {
+			wcscpy(dest, L"(failed to evaluate)");
+		} else {
+			const uint32_t &value = watch.currentValue;
+			float valuef = 0.0f;
+			switch (watch.format) {
 			case WatchFormat::HEX:
 				wsprintf(dest, L"0x%08X", value);
 				break;
@@ -912,14 +915,14 @@ void CtrlWatchList::GetColumnText(wchar_t *dest, int row, int col) {
 				swprintf_s(dest, 255, L"%f", valuef);
 				break;
 			case WatchFormat::STR:
-				if (Memory::IsValidAddress(value))
-					swprintf_s(dest, 255, L"%.255S", Memory::GetCharPointer(value));
-				else
+				if (Memory::IsValidAddress(value)) {
+					uint32_t len = Memory::ValidSize(value, 255);
+					swprintf_s(dest, 255, L"%.*S", len, Memory::GetCharPointer(value));
+				} else {
 					wsprintf(dest, L"(0x%08X)", value);
+				}
 				break;
 			}
-		} else {
-			wcscpy(dest, L"(failed to evaluate)");
 		}
 		break;
 	}