From 7cecb28bdf6443362a6ab20e0042ddb3b407ebec Mon Sep 17 00:00:00 2001 From: JosJuice Date: Sun, 12 Feb 2023 11:07:11 +0100 Subject: DolphinQt: Properly lock CPU before accessing emulated memory This fixes a problem I was having where using frame advance with the debugger open would frequently cause panic alerts about invalid addresses due to the CPU thread changing MSR.DR while the host thread was trying to access memory. To aid in tracking down all the places where we weren't properly locking the CPU, I've created a new type (in Core.h) that you have to pass as a reference or pointer to functions that require running as the CPU thread. --- Source/Core/DolphinQt/Debugger/CodeWidget.cpp | 76 +++++++++++++++------------ 1 file changed, 43 insertions(+), 33 deletions(-) (limited to 'Source/Core/DolphinQt/Debugger/CodeWidget.cpp') diff --git a/Source/Core/DolphinQt/Debugger/CodeWidget.cpp b/Source/Core/DolphinQt/Debugger/CodeWidget.cpp index 5f7229ce65..513a68672d 100644 --- a/Source/Core/DolphinQt/Debugger/CodeWidget.cpp +++ b/Source/Core/DolphinQt/Debugger/CodeWidget.cpp @@ -329,7 +329,10 @@ void CodeWidget::UpdateCallstack() std::vector stack; - bool success = Dolphin_Debugger::GetCallstack(Core::System::GetInstance(), stack); + const bool success = [&stack] { + Core::CPUThreadGuard guard; + return Dolphin_Debugger::GetCallstack(Core::System::GetInstance(), guard, stack); + }(); if (!success) { @@ -452,7 +455,11 @@ void CodeWidget::StepOver() if (!CPU::IsStepping()) return; - UGeckoInstruction inst = PowerPC::HostRead_Instruction(PowerPC::ppcState.pc); + const UGeckoInstruction inst = [] { + Core::CPUThreadGuard guard; + return PowerPC::HostRead_Instruction(guard, PowerPC::ppcState.pc); + }(); + if (inst.LK) { PowerPC::breakpoints.ClearAllTemporary(); @@ -485,48 +492,51 @@ void CodeWidget::StepOut() if (!CPU::IsStepping()) return; - CPU::PauseAndLock(true, false); - PowerPC::breakpoints.ClearAllTemporary(); - // Keep stepping until the next return instruction or timeout after five seconds using clock = std::chrono::steady_clock; clock::time_point timeout = clock::now() + std::chrono::seconds(5); - PowerPC::CoreMode old_mode = PowerPC::GetMode(); - PowerPC::SetMode(PowerPC::CoreMode::Interpreter); - // Loop until either the current instruction is a return instruction with no Link flag - // or a breakpoint is detected so it can step at the breakpoint. If the PC is currently - // on a breakpoint, skip it. - UGeckoInstruction inst = PowerPC::HostRead_Instruction(PowerPC::ppcState.pc); - do { - if (WillInstructionReturn(inst)) - { - PowerPC::SingleStep(); - break; - } + Core::CPUThreadGuard guard; + + PowerPC::breakpoints.ClearAllTemporary(); + + PowerPC::CoreMode old_mode = PowerPC::GetMode(); + PowerPC::SetMode(PowerPC::CoreMode::Interpreter); - if (inst.LK) + // Loop until either the current instruction is a return instruction with no Link flag + // or a breakpoint is detected so it can step at the breakpoint. If the PC is currently + // on a breakpoint, skip it. + UGeckoInstruction inst = PowerPC::HostRead_Instruction(guard, PowerPC::ppcState.pc); + do { - // Step over branches - u32 next_pc = PowerPC::ppcState.pc + 4; - do + if (WillInstructionReturn(inst)) { PowerPC::SingleStep(); - } while (PowerPC::ppcState.pc != next_pc && clock::now() < timeout && - !PowerPC::breakpoints.IsAddressBreakPoint(PowerPC::ppcState.pc)); - } - else - { - PowerPC::SingleStep(); - } + break; + } - inst = PowerPC::HostRead_Instruction(PowerPC::ppcState.pc); - } while (clock::now() < timeout && - !PowerPC::breakpoints.IsAddressBreakPoint(PowerPC::ppcState.pc)); + if (inst.LK) + { + // Step over branches + u32 next_pc = PowerPC::ppcState.pc + 4; + do + { + PowerPC::SingleStep(); + } while (PowerPC::ppcState.pc != next_pc && clock::now() < timeout && + !PowerPC::breakpoints.IsAddressBreakPoint(PowerPC::ppcState.pc)); + } + else + { + PowerPC::SingleStep(); + } - PowerPC::SetMode(old_mode); - CPU::PauseAndLock(false, false); + inst = PowerPC::HostRead_Instruction(guard, PowerPC::ppcState.pc); + } while (clock::now() < timeout && + !PowerPC::breakpoints.IsAddressBreakPoint(PowerPC::ppcState.pc)); + + PowerPC::SetMode(old_mode); + } emit Host::GetInstance()->UpdateDisasmDialog(); -- cgit v1.2.3 From 6f0266e8deb9b9d46fb6f291db696fed06c1bed0 Mon Sep 17 00:00:00 2001 From: JosJuice Date: Sun, 12 Feb 2023 12:50:28 +0100 Subject: DolphinQt: Only update call stack if paused This avoids a pseudo infinite loop where CodeWidget::UpdateCallstack would lock the CPU in order to read the call stack, causing the CPU to call Host_UpdateDisasmDialog because it's transitioning from running to pausing, causing Host::UpdateDisasmDialog to be emitted, causing CodeWidget::Update to be called, once again causing CodeWidget::UpdateCallstack to be called, repeating the cycle. Dolphin didn't go completely unresponsive during this, because Host_UpdateDisasmDialog schedules the emitting of Host::UpdateDisasmDialog to happen on another thread without blocking, but it was stopping certain operations like exiting emulation from working. --- Source/Core/DolphinQt/Debugger/CodeWidget.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'Source/Core/DolphinQt/Debugger/CodeWidget.cpp') diff --git a/Source/Core/DolphinQt/Debugger/CodeWidget.cpp b/Source/Core/DolphinQt/Debugger/CodeWidget.cpp index 513a68672d..391152abc9 100644 --- a/Source/Core/DolphinQt/Debugger/CodeWidget.cpp +++ b/Source/Core/DolphinQt/Debugger/CodeWidget.cpp @@ -322,11 +322,11 @@ void CodeWidget::Update() void CodeWidget::UpdateCallstack() { - if (Core::GetState() == Core::State::Starting) - return; - m_callstack_list->clear(); + if (Core::GetState() != Core::State::Paused) + return; + std::vector stack; const bool success = [&stack] { -- cgit v1.2.3