Merge 1b4702d32c into 50386c4e39

Jit64/JitArm64: Check Breakpoints Before FPU Availability

Source/Core/Core/PowerPC/Jit64/Jit.cpp

@@ -16,6 +16,7 @@
 #endif
 
 #include "Common/CommonTypes.h"
+#include "Common/EnumUtils.h"
 #include "Common/GekkoDisassembler.h"
 #include "Common/IOFile.h"
 #include "Common/Logging/Log.h"
@@ -1035,6 +1036,30 @@ bool Jit64::DoJit(u32 em_address, JitBlock* b, u32 nextPC)
     }
     else
     {
+      auto& cpu = m_system.GetCPU();
+      auto& power_pc = m_system.GetPowerPC();
+      if (m_enable_debugging && power_pc.GetBreakPoints().IsAddressBreakPoint(op.address) &&
+          !cpu.IsStepping())
+      {
+        gpr.Flush();
+        fpr.Flush();
+
+        MOV(32, PPCSTATE(pc), Imm32(op.address));
+        ABI_PushRegistersAndAdjustStack({}, 0);
+        ABI_CallFunctionP(PowerPC::CheckBreakPointsFromJIT, &power_pc);
+        ABI_PopRegistersAndAdjustStack({}, 0);
+        MOV(64, R(RSCRATCH), ImmPtr(cpu.GetStatePtr()));
+        CMP(32, MatR(RSCRATCH), Imm32(Common::ToUnderlying(CPU::State::Running)));
+        FixupBranch noBreakpoint = J_CC(CC_E);
+
+        Cleanup();
+        MOV(32, PPCSTATE(npc), Imm32(op.address));
+        SUB(32, PPCSTATE(downcount), Imm32(js.downcountAmount));
+        JMP(asm_routines.dispatcher_exit, Jump::Near);
+
+        SetJumpTarget(noBreakpoint);
+      }
+
       if ((opinfo->flags & FL_USE_FPU) && !js.firstFPInstructionFound)
       {
         // This instruction uses FPU - needs to add FP exception bailout
@@ -1061,30 +1086,6 @@ bool Jit64::DoJit(u32 em_address, JitBlock* b, u32 nextPC)
         js.firstFPInstructionFound = true;
       }
 
-      auto& cpu = m_system.GetCPU();
-      auto& power_pc = m_system.GetPowerPC();
-      if (m_enable_debugging && power_pc.GetBreakPoints().IsAddressBreakPoint(op.address) &&
-          !cpu.IsStepping())
-      {
-        gpr.Flush();
-        fpr.Flush();
-
-        MOV(32, PPCSTATE(pc), Imm32(op.address));
-        ABI_PushRegistersAndAdjustStack({}, 0);
-        ABI_CallFunctionP(PowerPC::CheckBreakPointsFromJIT, &power_pc);
-        ABI_PopRegistersAndAdjustStack({}, 0);
-        MOV(64, R(RSCRATCH), ImmPtr(cpu.GetStatePtr()));
-        TEST(32, MatR(RSCRATCH), Imm32(0xFFFFFFFF));
-        FixupBranch noBreakpoint = J_CC(CC_Z);
-
-        Cleanup();
-        MOV(32, PPCSTATE(npc), Imm32(op.address));
-        SUB(32, PPCSTATE(downcount), Imm32(js.downcountAmount));
-        JMP(asm_routines.dispatcher_exit, Jump::Near);
-
-        SetJumpTarget(noBreakpoint);
-      }
-
       if (bJITRegisterCacheOff)
       {
         gpr.Flush();

Source/Core/Core/PowerPC/Jit64/JitAsm.cpp

@@ -6,6 +6,7 @@
 #include <climits>
 
 #include "Common/CommonTypes.h"
+#include "Common/EnumUtils.h"
 #include "Common/JitRegister.h"
 #include "Common/x64ABI.h"
 #include "Common/x64Emitter.h"
@@ -105,8 +106,8 @@ void Jit64AsmRoutineManager::Generate()
   if (enable_debugging)
   {
     MOV(64, R(RSCRATCH), ImmPtr(system.GetCPU().GetStatePtr()));
-    TEST(32, MatR(RSCRATCH), Imm32(0xFFFFFFFF));
-    dbg_exit = J_CC(CC_NZ, Jump::Near);
+    CMP(32, MatR(RSCRATCH), Imm32(Common::ToUnderlying(CPU::State::Running)));
+    dbg_exit = J_CC(CC_NE, Jump::Near);
   }
 
   SetJumpTarget(skipToRealDispatch);
@@ -236,8 +237,8 @@ void Jit64AsmRoutineManager::Generate()
   // Check the state pointer to see if we are exiting
   // Gets checked on at the end of every slice
   MOV(64, R(RSCRATCH), ImmPtr(system.GetCPU().GetStatePtr()));
-  TEST(32, MatR(RSCRATCH), Imm32(0xFFFFFFFF));
-  J_CC(CC_Z, outerLoop);
+  CMP(32, MatR(RSCRATCH), Imm32(Common::ToUnderlying(CPU::State::Running)));
+  J_CC(CC_E, outerLoop);
 
   // Landing pad for drec space
   dispatcher_exit = GetCodePtr();

Source/Core/Core/PowerPC/JitArm64/Jit.cpp

@@ -8,6 +8,7 @@
 
 #include "Common/Arm64Emitter.h"
 #include "Common/CommonTypes.h"
+#include "Common/EnumUtils.h"
 #include "Common/Logging/Log.h"
 #include "Common/MathUtil.h"
 #include "Common/MsgHandler.h"
@@ -1239,6 +1240,37 @@ bool JitArm64::DoJit(u32 em_address, JitBlock* b, u32 nextPC)
     }
     else
     {
+      if (m_enable_debugging && !cpu.IsStepping() &&
+          m_system.GetPowerPC().GetBreakPoints().IsAddressBreakPoint(op.address))
+      {
+        FlushCarry();
+        gpr.Flush(FlushMode::All, ARM64Reg::INVALID_REG);
+        fpr.Flush(FlushMode::All, ARM64Reg::INVALID_REG);
+
+        static_assert(PPCSTATE_OFF(pc) <= 252);
+        static_assert(PPCSTATE_OFF(pc) + 4 == PPCSTATE_OFF(npc));
+
+        MOVI2R(DISPATCHER_PC, op.address);
+        STP(IndexType::Signed, DISPATCHER_PC, DISPATCHER_PC, PPC_REG, PPCSTATE_OFF(pc));
+        ABI_CallFunction(&PowerPC::CheckBreakPointsFromJIT, &m_system.GetPowerPC());
+
+        LDR(IndexType::Unsigned, ARM64Reg::W0, ARM64Reg::X0,
+            MOVPage2R(ARM64Reg::X0, cpu.GetStatePtr()));
+        static_assert(Common::ToUnderlying(CPU::State::Running) == 0);
+        FixupBranch no_breakpoint = CBZ(ARM64Reg::W0);
+
+        Cleanup();
+        if (IsProfilingEnabled())
+        {
+          ABI_CallFunction(&JitBlock::ProfileData::EndProfiling, b->profile_data.get(),
+                           js.downcountAmount);
+        }
+        DoDownCount();
+        B(dispatcher_exit);
+
+        SetJumpTarget(no_breakpoint);
+      }
+
       if ((opinfo->flags & FL_USE_FPU) && !js.firstFPInstructionFound)
       {
         // This instruction uses FPU - needs to add FP exception bailout
@@ -1268,36 +1300,6 @@ bool JitArm64::DoJit(u32 em_address, JitBlock* b, u32 nextPC)
         js.firstFPInstructionFound = true;
       }
 
-      if (m_enable_debugging && !cpu.IsStepping() &&
-          m_system.GetPowerPC().GetBreakPoints().IsAddressBreakPoint(op.address))
-      {
-        FlushCarry();
-        gpr.Flush(FlushMode::All, ARM64Reg::INVALID_REG);
-        fpr.Flush(FlushMode::All, ARM64Reg::INVALID_REG);
-
-        static_assert(PPCSTATE_OFF(pc) <= 252);
-        static_assert(PPCSTATE_OFF(pc) + 4 == PPCSTATE_OFF(npc));
-
-        MOVI2R(DISPATCHER_PC, op.address);
-        STP(IndexType::Signed, DISPATCHER_PC, DISPATCHER_PC, PPC_REG, PPCSTATE_OFF(pc));
-        ABI_CallFunction(&PowerPC::CheckBreakPointsFromJIT, &m_system.GetPowerPC());
-
-        LDR(IndexType::Unsigned, ARM64Reg::W0, ARM64Reg::X0,
-            MOVPage2R(ARM64Reg::X0, cpu.GetStatePtr()));
-        FixupBranch no_breakpoint = CBZ(ARM64Reg::W0);
-
-        Cleanup();
-        if (IsProfilingEnabled())
-        {
-          ABI_CallFunction(&JitBlock::ProfileData::EndProfiling, b->profile_data.get(),
-                           js.downcountAmount);
-        }
-        DoDownCount();
-        B(dispatcher_exit);
-
-        SetJumpTarget(no_breakpoint);
-      }
-
       if (bJITRegisterCacheOff)
       {
         FlushCarry();

Source/Core/Core/PowerPC/JitArm64/JitAsm.cpp

@@ -9,6 +9,7 @@
 #include "Common/Arm64Emitter.h"
 #include "Common/CommonTypes.h"
 #include "Common/Config/Config.h"
+#include "Common/EnumUtils.h"
 #include "Common/FloatUtils.h"
 #include "Common/JitRegister.h"
 #include "Common/MathUtil.h"
@@ -88,6 +89,7 @@ void JitArm64::GenerateAsm()
   {
     LDR(IndexType::Unsigned, ARM64Reg::W8, ARM64Reg::X8,
         MOVPage2R(ARM64Reg::X8, cpu.GetStatePtr()));
+    static_assert(Common::ToUnderlying(CPU::State::Running) == 0);
     debug_exit = CBNZ(ARM64Reg::W8);
   }
 
@@ -195,6 +197,7 @@ void JitArm64::GenerateAsm()
   // Check the state pointer to see if we are exiting
   // Gets checked on at the end of every slice
   LDR(IndexType::Unsigned, ARM64Reg::W8, ARM64Reg::X8, MOVPage2R(ARM64Reg::X8, cpu.GetStatePtr()));
+  static_assert(Common::ToUnderlying(CPU::State::Running) == 0);
   FixupBranch exit = CBNZ(ARM64Reg::W8);
 
   SetJumpTarget(to_start_of_timing_slice);

Source/Core/DolphinQt/CheatsManager.cpp

@@ -60,6 +60,16 @@ void CheatsManager::OnFrameEnd()
   if (!isVisible())
     return;
 
+  std::unique_lock<std::mutex> frameend_update_lock{m_tab_deletion_or_frameend_update_mutex,
+                                                    std::try_to_lock};
+  if (!frameend_update_lock.owns_lock())
+  {
+    // Either the currently selected CheatSearchWidget is in the process of being deleted and is
+    // unsafe to access, or try_to_lock failed spuriously. Occasional spurious failures are fine
+    // since the next FrameEnd or Pause event will update the table soon.
+    return;
+  }
+
   auto* const selected_cheat_search_widget =
       qobject_cast<CheatSearchWidget*>(m_tab_widget->currentWidget());
   if (selected_cheat_search_widget != nullptr)
@@ -67,6 +77,8 @@ void CheatsManager::OnFrameEnd()
     selected_cheat_search_widget->UpdateTableVisibleCurrentValues(
         CheatSearchWidget::UpdateSource::Auto);
   }
+
+  frameend_update_lock.unlock();
 }
 
 void CheatsManager::UpdateAllCheatSearchWidgetCurrentValues()
@@ -183,11 +195,35 @@ void CheatsManager::OnNewSessionCreated(const Cheats::CheatSearchSessionBase& se
   m_tab_widget->setCurrentIndex(tab_index);
 }
 
-void CheatsManager::OnTabCloseRequested(int index)
+void CheatsManager::BlockFrameEndEvents()
 {
-  auto* w = m_tab_widget->widget(index);
-  if (w)
-    w->deleteLater();
+  m_tab_deletion_lock.lock();
+  // The next currentChanged signal will be triggered by the automatic tab change caused by the
+  // deletion of the current tab.
+  connect(m_tab_widget, &QTabWidget::currentChanged, this, &CheatsManager::AllowFrameEndEvents);
+}
+
+void CheatsManager::AllowFrameEndEvents()
+{
+  // This function is currently connected to QTabWidget::currentChanged which would cause the
+  // semaphore to be incremented by the user changing the tab normally.
+  disconnect(m_tab_widget, &QTabWidget::currentChanged, this, &CheatsManager::AllowFrameEndEvents);
+  // It's now safe to run FrameEnd events again.
+  m_tab_deletion_lock.unlock();
+}
+
+void CheatsManager::OnTabCloseRequested(const int index)
+{
+  auto* const widget = m_tab_widget->widget(index);
+  if (widget)
+  {
+    const bool is_current_tab = index == m_tab_widget->currentIndex();
+    const bool is_cheatsearch = qobject_cast<CheatSearchWidget*>(widget);
+    if (is_current_tab && is_cheatsearch)
+      BlockFrameEndEvents();
+
+    widget->deleteLater();
+  }
 }
 
 void CheatsManager::ConnectWidgets()

Source/Core/DolphinQt/CheatsManager.h

@@ -5,6 +5,7 @@
 
 #include <functional>
 #include <memory>
+#include <mutex>
 #include <optional>
 #include <vector>
 
@@ -58,6 +59,11 @@ private:
   void OnNewSessionCreated(const Cheats::CheatSearchSessionBase& session);
   void OnTabCloseRequested(int index);
 
+  // Stops OnFrameEnd from updating the currently selected CheatSearch (which is about to be
+  // deleted). If it is already doing so, wait for it to finish first.
+  void BlockFrameEndEvents();
+  void AllowFrameEndEvents();
+
   void RefreshCodeTabs(Core::State state, bool force);
   void UpdateAllCheatSearchWidgetCurrentValues();
 
@@ -74,5 +80,10 @@ private:
   GeckoCodeWidget* m_gecko_code = nullptr;
   CheatSearchFactoryWidget* m_cheat_search_new = nullptr;
 
+  // Deleting the currently selected CheatSearch tab from the Host thread at the same time the CPU
+  // thread uses OnFrameEnd to update the table's Current Values could cause a crash.
+  std::mutex m_tab_deletion_or_frameend_update_mutex{};
+  std::unique_lock<std::mutex> m_tab_deletion_lock{m_tab_deletion_or_frameend_update_mutex,
+                                                   std::defer_lock};
   Common::EventHook m_VI_end_field_event;
 };