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/Common/Debug/CodeTrace.cpp

@@ -122,14 +122,14 @@ InstructionAttributes CodeTrace::GetInstructionAttributes(const TraceOutput& ins
   return tmp_attributes;
 }
 
-TraceOutput CodeTrace::SaveCurrentInstruction() const
+TraceOutput CodeTrace::SaveCurrentInstruction(const Core::CPUThreadGuard* guard) const
 {
   auto& system = Core::System::GetInstance();
   auto& ppc_state = system.GetPPCState();
 
   // Quickly save instruction and memory target for fast logging.
   TraceOutput output;
-  const std::string instr = PowerPC::debug_interface.Disassemble(ppc_state.pc);
+  const std::string instr = PowerPC::debug_interface.Disassemble(guard, ppc_state.pc);
   output.instruction = instr;
   output.address = ppc_state.pc;
 
@@ -139,14 +139,15 @@ TraceOutput CodeTrace::SaveCurrentInstruction() const
   return output;
 }
 
-AutoStepResults CodeTrace::AutoStepping(bool continue_previous, AutoStop stop_on)
+AutoStepResults CodeTrace::AutoStepping(const Core::CPUThreadGuard& guard, bool continue_previous,
+                                        AutoStop stop_on)
 {
   AutoStepResults results;
 
-  if (!CPU::IsStepping() || m_recording)
+  if (m_recording)
     return results;
 
-  TraceOutput pc_instr = SaveCurrentInstruction();
+  TraceOutput pc_instr = SaveCurrentInstruction(&guard);
   const InstructionAttributes instr = GetInstructionAttributes(pc_instr);
 
   // Not an instruction we should start autostepping from (ie branches).
@@ -187,7 +188,6 @@ AutoStepResults CodeTrace::AutoStepping(bool continue_previous, AutoStop stop_on
   else if (stop_on == AutoStop::Changed)
     stop_condition = HitType::ACTIVE;
 
-  CPU::PauseAndLock(true, false);
   PowerPC::breakpoints.ClearAllTemporary();
   using clock = std::chrono::steady_clock;
   clock::time_point timeout = clock::now() + std::chrono::seconds(4);
@@ -199,7 +199,7 @@ AutoStepResults CodeTrace::AutoStepping(bool continue_previous, AutoStop stop_on
   {
     PowerPC::SingleStep();
 
-    pc_instr = SaveCurrentInstruction();
+    pc_instr = SaveCurrentInstruction(&guard);
     hit = TraceLogic(pc_instr);
     results.count += 1;
   } while (clock::now() < timeout && hit < stop_condition &&
@@ -210,7 +210,6 @@ AutoStepResults CodeTrace::AutoStepping(bool continue_previous, AutoStop stop_on
     results.timed_out = true;
 
   PowerPC::SetMode(old_mode);
-  CPU::PauseAndLock(false, false);
   m_recording = false;
 
   results.reg_tracked = m_reg_autotrack;

Source/Core/Common/Debug/CodeTrace.h

@@ -10,6 +10,11 @@
 
 #include "Common/CommonTypes.h"
 
+namespace Core
+{
+class CPUThreadGuard;
+}
+
 struct InstructionAttributes
 {
   u32 address = 0;
@@ -63,11 +68,12 @@ public:
   };
 
   void SetRegTracked(const std::string& reg);
-  AutoStepResults AutoStepping(bool continue_previous = false, AutoStop stop_on = AutoStop::Always);
+  AutoStepResults AutoStepping(const Core::CPUThreadGuard& guard, bool continue_previous = false,
+                               AutoStop stop_on = AutoStop::Always);
 
 private:
   InstructionAttributes GetInstructionAttributes(const TraceOutput& line) const;
-  TraceOutput SaveCurrentInstruction() const;
+  TraceOutput SaveCurrentInstruction(const Core::CPUThreadGuard* guard) const;
   HitType TraceLogic(const TraceOutput& current_instr, bool first_hit = false);
 
   bool m_recording = false;

Source/Core/Common/Debug/MemoryPatches.cpp

@@ -23,36 +23,37 @@ MemoryPatch::MemoryPatch(u32 address_, u32 value_)
 MemoryPatches::MemoryPatches() = default;
 MemoryPatches::~MemoryPatches() = default;
 
-void MemoryPatches::SetPatch(u32 address, u32 value)
+void MemoryPatches::SetPatch(const Core::CPUThreadGuard& guard, u32 address, u32 value)
 {
   const std::size_t index = m_patches.size();
   m_patches.emplace_back(address, value);
-  Patch(index);
+  Patch(guard, index);
 }
 
-void MemoryPatches::SetPatch(u32 address, std::vector<u8> value)
+void MemoryPatches::SetPatch(const Core::CPUThreadGuard& guard, u32 address, std::vector<u8> value)
 {
-  UnsetPatch(address);
+  UnsetPatch(guard, address);
   const std::size_t index = m_patches.size();
   m_patches.emplace_back(address, std::move(value));
-  Patch(index);
+  Patch(guard, index);
 }
 
-void MemoryPatches::SetFramePatch(u32 address, u32 value)
+void MemoryPatches::SetFramePatch(const Core::CPUThreadGuard& guard, u32 address, u32 value)
 {
   const std::size_t index = m_patches.size();
   m_patches.emplace_back(address, value);
   m_patches.back().type = MemoryPatch::ApplyType::EachFrame;
-  Patch(index);
+  Patch(guard, index);
 }
 
-void MemoryPatches::SetFramePatch(u32 address, std::vector<u8> value)
+void MemoryPatches::SetFramePatch(const Core::CPUThreadGuard& guard, u32 address,
+                                  std::vector<u8> value)
 {
-  UnsetPatch(address);
+  UnsetPatch(guard, address);
   const std::size_t index = m_patches.size();
   m_patches.emplace_back(address, std::move(value));
   m_patches.back().type = MemoryPatch::ApplyType::EachFrame;
-  Patch(index);
+  Patch(guard, index);
 }
 
 const std::vector<MemoryPatch>& MemoryPatches::GetPatches() const
@@ -60,7 +61,7 @@ const std::vector<MemoryPatch>& MemoryPatches::GetPatches() const
   return m_patches;
 }
 
-void MemoryPatches::UnsetPatch(u32 address)
+void MemoryPatches::UnsetPatch(const Core::CPUThreadGuard& guard, u32 address)
 {
   const auto it = std::find_if(m_patches.begin(), m_patches.end(),
                                [address](const auto& patch) { return patch.address == address; });
@@ -69,23 +70,23 @@ void MemoryPatches::UnsetPatch(u32 address)
     return;
 
   const std::size_t index = std::distance(m_patches.begin(), it);
-  RemovePatch(index);
+  RemovePatch(guard, index);
 }
 
-void MemoryPatches::EnablePatch(std::size_t index)
+void MemoryPatches::EnablePatch(const Core::CPUThreadGuard& guard, std::size_t index)
 {
   if (m_patches[index].is_enabled == MemoryPatch::State::Enabled)
     return;
   m_patches[index].is_enabled = MemoryPatch::State::Enabled;
-  Patch(index);
+  Patch(guard, index);
 }
 
-void MemoryPatches::DisablePatch(std::size_t index)
+void MemoryPatches::DisablePatch(const Core::CPUThreadGuard& guard, std::size_t index)
 {
   if (m_patches[index].is_enabled == MemoryPatch::State::Disabled)
     return;
   m_patches[index].is_enabled = MemoryPatch::State::Disabled;
-  Patch(index);
+  Patch(guard, index);
 }
 
 bool MemoryPatches::HasEnabledPatch(u32 address) const
@@ -95,19 +96,19 @@ bool MemoryPatches::HasEnabledPatch(u32 address) const
   });
 }
 
-void MemoryPatches::RemovePatch(std::size_t index)
+void MemoryPatches::RemovePatch(const Core::CPUThreadGuard& guard, std::size_t index)
 {
-  DisablePatch(index);
+  DisablePatch(guard, index);
   UnPatch(index);
   m_patches.erase(m_patches.begin() + index);
 }
 
-void MemoryPatches::ClearPatches()
+void MemoryPatches::ClearPatches(const Core::CPUThreadGuard& guard)
 {
   const std::size_t size = m_patches.size();
   for (std::size_t index = 0; index < size; ++index)
   {
-    DisablePatch(index);
+    DisablePatch(guard, index);
     UnPatch(index);
   }
   m_patches.clear();

Source/Core/Common/Debug/MemoryPatches.h

@@ -9,6 +9,11 @@
 
 #include "Common/CommonTypes.h"
 
+namespace Core
+{
+class CPUThreadGuard;
+}
+
 namespace Common::Debug
 {
 struct MemoryPatch
@@ -40,21 +45,21 @@ public:
   MemoryPatches();
   virtual ~MemoryPatches();
 
-  void SetPatch(u32 address, u32 value);
-  void SetPatch(u32 address, std::vector<u8> value);
-  void SetFramePatch(u32 address, u32 value);
-  void SetFramePatch(u32 address, std::vector<u8> value);
+  void SetPatch(const Core::CPUThreadGuard& guard, u32 address, u32 value);
+  void SetPatch(const Core::CPUThreadGuard& guard, u32 address, std::vector<u8> value);
+  void SetFramePatch(const Core::CPUThreadGuard& guard, u32 address, u32 value);
+  void SetFramePatch(const Core::CPUThreadGuard& guard, u32 address, std::vector<u8> value);
   const std::vector<MemoryPatch>& GetPatches() const;
-  void UnsetPatch(u32 address);
-  void EnablePatch(std::size_t index);
-  void DisablePatch(std::size_t index);
+  void UnsetPatch(const Core::CPUThreadGuard& guard, u32 address);
+  void EnablePatch(const Core::CPUThreadGuard& guard, std::size_t index);
+  void DisablePatch(const Core::CPUThreadGuard& guard, std::size_t index);
   bool HasEnabledPatch(u32 address) const;
-  void RemovePatch(std::size_t index);
-  void ClearPatches();
-  virtual void ApplyExistingPatch(std::size_t index) = 0;
+  void RemovePatch(const Core::CPUThreadGuard& guard, std::size_t index);
+  void ClearPatches(const Core::CPUThreadGuard& guard);
+  virtual void ApplyExistingPatch(const Core::CPUThreadGuard& guard, std::size_t index) = 0;
 
 protected:
-  virtual void Patch(std::size_t index) = 0;
+  virtual void Patch(const Core::CPUThreadGuard& guard, std::size_t index) = 0;
   virtual void UnPatch(std::size_t index) = 0;
 
   std::vector<MemoryPatch> m_patches;

Source/Core/Common/Debug/Threads.h

@@ -11,6 +11,11 @@
 
 #include "Common/CommonTypes.h"
 
+namespace Core
+{
+class CPUThreadGuard;
+};
+
 namespace Common::Debug
 {
 struct PartialContext
@@ -41,7 +46,7 @@ public:
     LWPThread,  // devkitPro libogc thread
   };
 
-  virtual PartialContext GetContext() const = 0;
+  virtual PartialContext GetContext(const Core::CPUThreadGuard& guard) const = 0;
   virtual u32 GetAddress() const = 0;
   virtual u16 GetState() const = 0;
   virtual bool IsSuspended() const = 0;
@@ -53,8 +58,8 @@ public:
   virtual std::size_t GetStackSize() const = 0;
   virtual s32 GetErrno() const = 0;
   // Implementation specific, used to store arbitrary data
-  virtual std::string GetSpecific() const = 0;
-  virtual bool IsValid() const = 0;
+  virtual std::string GetSpecific(const Core::CPUThreadGuard& guard) const = 0;
+  virtual bool IsValid(const Core::CPUThreadGuard& guard) const = 0;
 };
 
 using Threads = std::vector<std::unique_ptr<ThreadView>>;