Merge pull request #22709 from hrydgard/ir-interpreter-faults

IR interpreter: Halt with a memory exception on a bad guest access

Core/MIPS/IR/IRInterpreter.cpp

@@ -16,6 +16,7 @@
 #include "Core/HLE/HLE.h"
 #include "Core/HLE/ReplaceTables.h"
 #include "Core/HW/MMIO.h"
+#include "Core/MemFault.h"
 #include "Core/MemMap.h"
 #include "Core/MIPS/MIPS.h"
 #include "Core/MIPS/MIPSTables.h"
@@ -178,6 +179,8 @@ void IRRunKernelStore(MIPSState *mips, u32 addr, u32 value, u32 op) {
 
 u32 IRInterpret(MIPSState *mips, const IRInst *inst, const IRInst *chainArena) {
 	const Memory::AddressSpace &space = *mips->space;
+	// Lets a bad guest access halt the core. Calls out to HLE and the like leave this scope.
+	Memory::IRInterpreterFaultScope faultScope(true);
 #if IR_THREADED_DISPATCH
 	static const void *dispatch[256];
 	static bool dispatchReady = false;
@@ -1448,7 +1451,10 @@ u32 IRInterpret(MIPSState *mips, const IRInst *inst, const IRInst *chainArena) {
 			MIPSOpcode op(inst->constant);
 			// HLE runs in the host's normal mode, see ApplyHostRoundingMode.
 			RestoreHostRoundingMode();
-			CallSyscall(mips, op);
+			{
+				Memory::IRInterpreterFaultScope outside(false);
+				CallSyscall(mips, op);
+			}
 			ApplyHostRoundingMode(mips);
 			if (coreState != CORE_RUNNING_CPU) {
 				CoreTiming::ForceCheck(mips);
@@ -1461,7 +1467,10 @@ u32 IRInterpret(MIPSState *mips, const IRInst *inst, const IRInst *chainArena) {
 			// If we get here, the syscall is invalid.
 			u32 pc = inst->constant;
 			RestoreHostRoundingMode();
-			CallSyscallUnresolvedAtPC(mips, pc);
+			{
+				Memory::IRInterpreterFaultScope outside(false);
+				CallSyscallUnresolvedAtPC(mips, pc);
+			}
 			ApplyHostRoundingMode(mips);
 			if (coreState != CORE_RUNNING_CPU) {
 				// hm, what's this for?
@@ -1476,7 +1485,10 @@ u32 IRInterpret(MIPSState *mips, const IRInst *inst, const IRInst *chainArena) {
 		IR_CASE(Interpret)  // SLOW fallback. Can be made faster. Ideally should be removed but may be useful for debugging.
 		{
 			MIPSOpcode op(inst->constant);
-			MIPSInterpret(mips, op);
+			{
+				Memory::IRInterpreterFaultScope outside(false);
+				MIPSInterpret(mips, op);
+			}
 			IR_NEXT;
 		}
 
@@ -1485,7 +1497,11 @@ u32 IRInterpret(MIPSState *mips, const IRInst *inst, const IRInst *chainArena) {
 			int funcIndex = inst->constant;
 			const ReplacementTableEntry *f = GetReplacementFunc(funcIndex);
 			RestoreHostRoundingMode();
-			int cycles = f->replaceFunc(mips);
+			int cycles;
+			{
+				Memory::IRInterpreterFaultScope outside(false);
+				cycles = f->replaceFunc(mips);
+			}
 			ApplyHostRoundingMode(mips);
 			mips->r[inst->dest] = cycles < 0 ? -1 : 0;
 			mips->downcount -= cycles < 0 ? -cycles : cycles;

Core/MemFault.cpp

@@ -58,6 +58,7 @@ static int64_t g_numReportedBadAccesses = 0;
 const uint8_t *g_lastCrashAddress;
 MemoryExceptionType g_lastMemoryExceptionType;
 static bool inCrashHandler = false;
+volatile bool g_inIRInterpreter = false;
 
 std::unordered_set<const uint8_t *> g_ignoredAddresses;
 
@@ -126,10 +127,11 @@ bool HandleFault(uintptr_t hostAddress, void *ctx) {
 
 	// TODO: Check that codePtr is within the current JIT space.
 	bool inJitSpace = mipsr4k.jit && mipsr4k.jit->CodeInRange(codePtr);
-	if (!inJitSpace) {
-		// This is a crash in non-jitted code. Not something we want to handle here, ignore.
-		// Actually, we could handle crashes from the IR interpreter here, although recovering the call stack
-		// might be tricky...
+	// The IR interpreter's own accesses to PSP memory. We can't redirect those to a crash handler, but we can
+	// step over the faulting instruction and stop the core.
+	const bool inIRInterpreter = !inJitSpace && g_inIRInterpreter;
+	if (!inJitSpace && !inIRInterpreter) {
+		// This is a crash in other non-jitted code. Not something we want to handle here, ignore.
 		inCrashHandler = false;
 		return false;
 	}
@@ -239,6 +241,18 @@ bool HandleFault(uintptr_t hostAddress, void *ctx) {
 		type = MemoryExceptionType::UNKNOWN;
 	}
 
+	if (inIRInterpreter && !success) {
+		// Without the instruction's size we can't step over it.
+		inCrashHandler = false;
+		return false;
+	}
+	if (inIRInterpreter && coreState == CORE_RUNTIME_ERROR) {
+		// Already reported, the interpreter just hasn't reached the end of the block yet.
+		context->CTX_PC += info.instructionSize;
+		inCrashHandler = false;
+		return true;
+	}
+
 	g_lastMemoryExceptionType = type;
 
 	bool handled = true;
@@ -276,6 +290,17 @@ bool HandleFault(uintptr_t hostAddress, void *ctx) {
 		// just need to clean it up.
 		Core_MemoryException(guestAddress, info.OperandSizeInBytes(), approximatePC, type, infoString, true);
 
+		if (inIRInterpreter) {
+			// Step over the access and stop the core, like the JITs' crash handlers do. The interpreter
+			// leaves at the next block exit, which checks the downcount. Resuming isn't possible, the guest
+			// has already run on past the access.
+			context->CTX_PC += info.instructionSize;
+			coreState = CORE_RUNTIME_ERROR;
+			currentMIPS->downcount = -1;
+			inCrashHandler = false;
+			return true;
+		}
+
 		// There's a small chance we can resume from this type of crash.
 		g_lastCrashAddress = codePtr;
 

Core/MemFault.h

@@ -20,6 +20,23 @@ void MemFault_IgnoreLastCrash();
 // just leave it as-is.
 bool HandleFault(uintptr_t hostAddress, void *context);
 
+// Set while IRInterpret runs its own loads and stores. A fault in those can be stepped over, which the
+// IR interpreter does to halt cleanly instead of crashing. Only the CPU thread runs it.
+extern volatile bool g_inIRInterpreter;
+
+class IRInterpreterFaultScope {
+public:
+	explicit IRInterpreterFaultScope(bool inInterpreter) : prev_(g_inIRInterpreter) {
+		g_inIRInterpreter = inInterpreter;
+	}
+	~IRInterpreterFaultScope() {
+		g_inIRInterpreter = prev_;
+	}
+
+private:
+	bool prev_;
+};
+
 }
 
 // Stack walk utility function, walks from the current state. Useful in the debugger and crash report screens etc.