From 6b4356a060ac96a635bc6234cfdf1cbc77c870db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 9 Feb 2021 01:30:31 +0100 Subject: [PATCH 1/4] Handle exec addr errors better - don't let IgnoreBadMemoryAccesses skip dispatcher exceptions. It would then just fall through into the compiler and die. Should remove one of the "mystery" crashes from #14082. --- Core/Core.cpp | 1 + Core/Core.h | 1 + Core/MIPS/ARM64/Arm64Asm.cpp | 1 + Core/MIPS/ARM64/Arm64Jit.h | 4 ++++ Core/MIPS/JitCommon/JitCommon.h | 3 +++ Core/MIPS/x86/Asm.cpp | 2 +- Core/MIPS/x86/Jit.cpp | 5 +++++ Core/MIPS/x86/Jit.h | 6 ++++++ Core/MemFault.cpp | 17 ++++++++++++++--- pspautotests | 2 +- 10 files changed, 37 insertions(+), 5 deletions(-) diff --git a/Core/Core.cpp b/Core/Core.cpp index f3cdd9b62421..362f3a44e31a 100644 --- a/Core/Core.cpp +++ b/Core/Core.cpp @@ -398,6 +398,7 @@ const char *MemoryExceptionTypeAsString(MemoryExceptionType type) { case MemoryExceptionType::WRITE_WORD: return "Write Word"; case MemoryExceptionType::READ_BLOCK: return "Read Block"; case MemoryExceptionType::WRITE_BLOCK: return "Read/Write Block"; + case MemoryExceptionType::EXEC_ADDR: return "Bad Exec Addr"; default: return "N/A"; } diff --git a/Core/Core.h b/Core/Core.h index 21facdb735d5..735352347057 100644 --- a/Core/Core.h +++ b/Core/Core.h @@ -88,6 +88,7 @@ enum class MemoryExceptionType { WRITE_WORD, READ_BLOCK, WRITE_BLOCK, + EXEC_ADDR, }; enum class ExecExceptionType { JUMP, diff --git a/Core/MIPS/ARM64/Arm64Asm.cpp b/Core/MIPS/ARM64/Arm64Asm.cpp index b95b8e54b4d9..bca3f535e179 100644 --- a/Core/MIPS/ARM64/Arm64Asm.cpp +++ b/Core/MIPS/ARM64/Arm64Asm.cpp @@ -250,6 +250,7 @@ void Arm64Jit::GenerateFixedCode(const JitOptions &jo) { #ifdef MASKED_PSP_MEMORY ANDI2R(SCRATCH1, SCRATCH1, 0x3FFFFFFF); #endif + dispatcherFetch = GetCodePtr(); LDR(SCRATCH1, MEMBASEREG, SCRATCH1_64); LSR(SCRATCH2, SCRATCH1, 24); // or UBFX(SCRATCH2, SCRATCH1, 24, 8) ANDI2R(SCRATCH1, SCRATCH1, 0x00FFFFFF); diff --git a/Core/MIPS/ARM64/Arm64Jit.h b/Core/MIPS/ARM64/Arm64Jit.h index 17734ade594a..95affe2515b4 100644 --- a/Core/MIPS/ARM64/Arm64Jit.h +++ b/Core/MIPS/ARM64/Arm64Jit.h @@ -185,6 +185,9 @@ class Arm64Jit : public Arm64Gen::ARM64CodeBlock, public JitInterface, public MI const u8 *GetDispatcher() const override { return dispatcher; } + bool IsAtDispatchFetch(const u8 *ptr) const override { + return ptr == dispatcherFetch; + } void LinkBlock(u8 *exitPoint, const u8 *checkedEntry) override; void UnlinkBlock(u8 *checkedEntry, u32 originalAddress) override; @@ -275,6 +278,7 @@ class Arm64Jit : public Arm64Gen::ARM64CodeBlock, public JitInterface, public MI const u8 *dispatcherPCInSCRATCH1; const u8 *dispatcher; const u8 *dispatcherNoCheck; + const u8 *dispatcherFetch; const u8 *saveStaticRegisters; const u8 *loadStaticRegisters; diff --git a/Core/MIPS/JitCommon/JitCommon.h b/Core/MIPS/JitCommon/JitCommon.h index 08063f866e49..e39a8bab217b 100644 --- a/Core/MIPS/JitCommon/JitCommon.h +++ b/Core/MIPS/JitCommon/JitCommon.h @@ -123,6 +123,9 @@ namespace MIPSComp { virtual bool CodeInRange(const u8 *ptr) const = 0; virtual bool DescribeCodePtr(const u8 *ptr, std::string &name) = 0; + virtual bool IsAtDispatchFetch(const u8 *ptr) const { + return false; + } virtual const u8 *GetDispatcher() const = 0; virtual const u8 *GetCrashHandler() const = 0; virtual JitBlockCache *GetBlockCache() = 0; diff --git a/Core/MIPS/x86/Asm.cpp b/Core/MIPS/x86/Asm.cpp index a099e1044b64..84fa1e9cccfe 100644 --- a/Core/MIPS/x86/Asm.cpp +++ b/Core/MIPS/x86/Asm.cpp @@ -161,7 +161,7 @@ void Jit::GenerateFixedCode(JitOptions &jo) { #ifdef MASKED_PSP_MEMORY AND(32, R(EAX), Imm32(Memory::MEMVIEW32_MASK)); #endif - + dispatcherFetch = GetCodePtr(); #ifdef _M_IX86 _assert_msg_( Memory::base != 0, "Memory base bogus"); MOV(32, R(EAX), MDisp(EAX, (u32)Memory::base)); diff --git a/Core/MIPS/x86/Jit.cpp b/Core/MIPS/x86/Jit.cpp index 97682d5f7db2..6e77fbade4e0 100644 --- a/Core/MIPS/x86/Jit.cpp +++ b/Core/MIPS/x86/Jit.cpp @@ -274,6 +274,11 @@ void Jit::Compile(u32 em_address) { ClearCache(); } + if (!Memory::IsValidAddress(em_address)) { + Core_ExecException(em_address, em_address, ExecExceptionType::JUMP); + return; + } + BeginWrite(); int block_num = blocks.AllocateBlock(em_address); diff --git a/Core/MIPS/x86/Jit.h b/Core/MIPS/x86/Jit.h index b065d9052216..6c679547bafa 100644 --- a/Core/MIPS/x86/Jit.h +++ b/Core/MIPS/x86/Jit.h @@ -298,6 +298,11 @@ class Jit : public Gen::XCodeBlock, public JitInterface, public MIPSFrontendInte } return true; } + + bool IsAtDispatchFetch(const u8 *codePtr) const override { + return codePtr == dispatcherFetch; + } + void SaveFlags(); void LoadFlags(); @@ -321,6 +326,7 @@ class Jit : public Gen::XCodeBlock, public JitInterface, public MIPSFrontendInte const u8 *dispatcherCheckCoreState; const u8 *dispatcherNoCheck; const u8 *dispatcherInEAXNoCheck; + const u8 *dispatcherFetch; const u8 *restoreRoundingMode; const u8 *applyRoundingMode; diff --git a/Core/MemFault.cpp b/Core/MemFault.cpp index 38944bf98ba4..4db12f0ab1f3 100644 --- a/Core/MemFault.cpp +++ b/Core/MemFault.cpp @@ -41,16 +41,19 @@ namespace Memory { static int64_t g_numReportedBadAccesses = 0; const uint8_t *g_lastCrashAddress; +MemoryExceptionType g_lastMemoryExceptionType; + std::unordered_set g_ignoredAddresses; void MemFault_Init() { g_numReportedBadAccesses = 0; g_lastCrashAddress = nullptr; + g_lastMemoryExceptionType = MemoryExceptionType::NONE; g_ignoredAddresses.clear(); } bool MemFault_MayBeResumable() { - return g_lastCrashAddress != nullptr; + return g_lastCrashAddress != nullptr && g_lastMemoryExceptionType != MemoryExceptionType::EXEC_ADDR; } void MemFault_IgnoreLastCrash() { @@ -122,11 +125,15 @@ bool HandleFault(uintptr_t hostAddress, void *ctx) { std::string infoString = ""; + bool isAtDispatch = false; if (MIPSComp::jit) { std::string desc; if (MIPSComp::jit->DescribeCodePtr(codePtr, desc)) { infoString += desc + "\n"; } + if (MIPSComp::jit->IsAtDispatchFetch(codePtr)) { + isAtDispatch = true; + } } int instructionSize = 4; @@ -158,7 +165,9 @@ bool HandleFault(uintptr_t hostAddress, void *ctx) { infoString += disassembly + "\n"; } - if (success) { + if (isAtDispatch) { + type = MemoryExceptionType::EXEC_ADDR; + } else if (success) { if (info.isMemoryWrite) { type = MemoryExceptionType::WRITE_WORD; } else { @@ -168,7 +177,9 @@ bool HandleFault(uintptr_t hostAddress, void *ctx) { type = MemoryExceptionType::UNKNOWN; } - if (success && (g_Config.bIgnoreBadMemAccess || g_ignoredAddresses.find(codePtr) != g_ignoredAddresses.end())) { + g_lastMemoryExceptionType = type; + + if (success && !isAtDispatch && (g_Config.bIgnoreBadMemAccess || g_ignoredAddresses.find(codePtr) != g_ignoredAddresses.end())) { if (!info.isMemoryWrite) { // It was a read. Fill the destination register with 0. // TODO diff --git a/pspautotests b/pspautotests index 0a5453fb9f8e..1047400eaec6 160000 --- a/pspautotests +++ b/pspautotests @@ -1 +1 @@ -Subproject commit 0a5453fb9f8ef77bf81252488c218cfee3d6546a +Subproject commit 1047400eaec6bcbdb2a64d326375ef6a6617c4ac From f6b2070e479ad8591ce7597768514d98aff3416a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 9 Feb 2021 09:38:03 +0100 Subject: [PATCH 2/4] Cause the correct type of exception. Never ignore EXEC_ADDR exceptions. --- Core/Core.cpp | 21 +++++++++------------ Core/Core.h | 1 - Core/MIPS/ARM/ArmAsm.cpp | 1 + Core/MIPS/ARM/ArmJit.h | 4 ++++ Core/MemFault.cpp | 14 ++++++++++++-- 5 files changed, 26 insertions(+), 15 deletions(-) diff --git a/Core/Core.cpp b/Core/Core.cpp index 362f3a44e31a..e7d95c685a31 100644 --- a/Core/Core.cpp +++ b/Core/Core.cpp @@ -398,7 +398,6 @@ const char *MemoryExceptionTypeAsString(MemoryExceptionType type) { case MemoryExceptionType::WRITE_WORD: return "Write Word"; case MemoryExceptionType::READ_BLOCK: return "Read Block"; case MemoryExceptionType::WRITE_BLOCK: return "Read/Write Block"; - case MemoryExceptionType::EXEC_ADDR: return "Bad Exec Addr"; default: return "N/A"; } @@ -461,17 +460,15 @@ void Core_ExecException(u32 address, u32 pc, ExecExceptionType type) { const char *desc = ExecExceptionTypeAsString(type); WARN_LOG(MEMMAP, "%s: Invalid destination %08x PC %08x LR %08x", desc, address, currentMIPS->pc, currentMIPS->r[MIPS_REG_RA]); - if (!g_Config.bIgnoreBadMemAccess) { - ExceptionInfo &e = g_exceptionInfo; - e = {}; - e.type = ExceptionType::BAD_EXEC_ADDR; - e.info = ""; - e.exec_type = type; - e.address = address; - e.pc = pc; - Core_EnableStepping(true); - host->SetDebugMode(true); - } + ExceptionInfo &e = g_exceptionInfo; + e = {}; + e.type = ExceptionType::BAD_EXEC_ADDR; + e.info = ""; + e.exec_type = type; + e.address = address; + e.pc = pc; + Core_EnableStepping(true); + host->SetDebugMode(true); } void Core_Break() { diff --git a/Core/Core.h b/Core/Core.h index 735352347057..21facdb735d5 100644 --- a/Core/Core.h +++ b/Core/Core.h @@ -88,7 +88,6 @@ enum class MemoryExceptionType { WRITE_WORD, READ_BLOCK, WRITE_BLOCK, - EXEC_ADDR, }; enum class ExecExceptionType { JUMP, diff --git a/Core/MIPS/ARM/ArmAsm.cpp b/Core/MIPS/ARM/ArmAsm.cpp index 44176dc293f8..ed06c4065e5e 100644 --- a/Core/MIPS/ARM/ArmAsm.cpp +++ b/Core/MIPS/ARM/ArmAsm.cpp @@ -204,6 +204,7 @@ void ArmJit::GenerateFixedCode() { LDR(R0, CTXREG, offsetof(MIPSState, pc)); // TODO: In practice, do we ever run code from uncached space (| 0x40000000)? If not, we can remove this BIC. BIC(R0, R0, Operand2(0xC0, 4)); // &= 0x3FFFFFFF + dispatcherFetch = GetCodePtr(); LDR(R0, MEMBASEREG, R0); AND(R1, R0, Operand2(0xFF, 4)); // rotation is to the right, in 2-bit increments. BIC(R0, R0, Operand2(0xFF, 4)); diff --git a/Core/MIPS/ARM/ArmJit.h b/Core/MIPS/ARM/ArmJit.h index 50322a65bc65..b6e313891336 100644 --- a/Core/MIPS/ARM/ArmJit.h +++ b/Core/MIPS/ARM/ArmJit.h @@ -184,6 +184,9 @@ class ArmJit : public ArmGen::ARMXCodeBlock, public JitInterface, public MIPSFro const u8 *GetDispatcher() const override { return dispatcher; } + bool IsAtDispatchFetch(const u8 *ptr) const override { + return ptr == dispatcherFetch; + } void LinkBlock(u8 *exitPoint, const u8 *checkedEntry) override; void UnlinkBlock(u8 *checkedEntry, u32 originalAddress) override; @@ -311,6 +314,7 @@ class ArmJit : public ArmGen::ARMXCodeBlock, public JitInterface, public MIPSFro const u8 *dispatcherCheckCoreState; const u8 *dispatcherPCInR0; const u8 *dispatcher; + const u8 *dispatcherFetch; const u8 *dispatcherNoCheck; const u8 *restoreRoundingMode; diff --git a/Core/MemFault.cpp b/Core/MemFault.cpp index 4db12f0ab1f3..4ebec040a14b 100644 --- a/Core/MemFault.cpp +++ b/Core/MemFault.cpp @@ -53,7 +53,7 @@ void MemFault_Init() { } bool MemFault_MayBeResumable() { - return g_lastCrashAddress != nullptr && g_lastMemoryExceptionType != MemoryExceptionType::EXEC_ADDR; + return g_lastCrashAddress != nullptr; } void MemFault_IgnoreLastCrash() { @@ -166,7 +166,17 @@ bool HandleFault(uintptr_t hostAddress, void *ctx) { } if (isAtDispatch) { - type = MemoryExceptionType::EXEC_ADDR; + u32 targetAddr = currentMIPS->pc; // bad approximation +#if PPSSPP_ARCH(AMD64) + // We know which register the address is in, look in Asm.cpp. + targetAddr = context->Rax; +#endif + // TODO: Do the other archs. + Core_ExecException(targetAddr, currentMIPS->pc, ExecExceptionType::JUMP); + // Redirect execution to a crash handler that will switch to CoreState::CORE_RUNTIME_ERROR immediately. + context->CTX_PC = (uintptr_t)MIPSComp::jit->GetCrashHandler(); + ERROR_LOG(MEMMAP, "Bad execution access detected, halting: %08x (last known pc %08x, host: %p)", targetAddr, currentMIPS->pc, (void *)hostAddress); + return true; } else if (success) { if (info.isMemoryWrite) { type = MemoryExceptionType::WRITE_WORD; From 6db827093d4f64c099b9384b86ecdb51c7c90ebf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 9 Feb 2021 09:48:34 +0100 Subject: [PATCH 3/4] Buildfix --- Core/MemFault.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Core/MemFault.cpp b/Core/MemFault.cpp index 4ebec040a14b..c76fc7560bef 100644 --- a/Core/MemFault.cpp +++ b/Core/MemFault.cpp @@ -167,11 +167,11 @@ bool HandleFault(uintptr_t hostAddress, void *ctx) { if (isAtDispatch) { u32 targetAddr = currentMIPS->pc; // bad approximation -#if PPSSPP_ARCH(AMD64) + // TODO: Do the other archs and platforms. +#if PPSSPP_ARCH(AMD64) && PPSSPP_PLATFORM(WINDOWS) // We know which register the address is in, look in Asm.cpp. targetAddr = context->Rax; #endif - // TODO: Do the other archs. Core_ExecException(targetAddr, currentMIPS->pc, ExecExceptionType::JUMP); // Redirect execution to a crash handler that will switch to CoreState::CORE_RUNTIME_ERROR immediately. context->CTX_PC = (uintptr_t)MIPSComp::jit->GetCrashHandler(); From 4b67fe4947f9a99b54e7c93a8905d72e7153e6db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Rydg=C3=A5rd?= Date: Tue, 9 Feb 2021 10:17:15 +0100 Subject: [PATCH 4/4] Remove now-redundant check --- Core/MemFault.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Core/MemFault.cpp b/Core/MemFault.cpp index c76fc7560bef..db2a4d4d98c8 100644 --- a/Core/MemFault.cpp +++ b/Core/MemFault.cpp @@ -189,7 +189,7 @@ bool HandleFault(uintptr_t hostAddress, void *ctx) { g_lastMemoryExceptionType = type; - if (success && !isAtDispatch && (g_Config.bIgnoreBadMemAccess || g_ignoredAddresses.find(codePtr) != g_ignoredAddresses.end())) { + if (success && (g_Config.bIgnoreBadMemAccess || g_ignoredAddresses.find(codePtr) != g_ignoredAddresses.end())) { if (!info.isMemoryWrite) { // It was a read. Fill the destination register with 0. // TODO