DBG: fixed a deadlock with handling of unknown breakpoints (closes issue #1546)

src/dbg/debugger.cpp

@@ -903,7 +903,8 @@ static void cbGenericBreakpoint(BP_TYPE bptype, void* ExceptionAddress = nullptr
     hActiveThread = ThreadGetHandle(((DEBUG_EVENT*)GetDebugData())->dwThreadId);
     auto CIP = GetContextDataEx(hActiveThread, UE_CIP);
     BREAKPOINT* bpPtr = nullptr;
-    SHARED_ACQUIRE(LockBreakpoints);
+    //NOTE: this locking is very tricky, make sure you understand it before modifying anything
+    EXCLUSIVE_ACQUIRE(LockBreakpoints);
     switch(bptype)
     {
     case BPNORMAL:
@@ -928,6 +929,8 @@ static void cbGenericBreakpoint(BP_TYPE bptype, void* ExceptionAddress = nullptr
     {
         if(bptype != BPDLL || !BpUpdateDllPath(reinterpret_cast<const char*>(ExceptionAddress), &bpPtr))
         {
+            // release the breakpoint lock to prevent deadlocks during the wait
+            EXCLUSIVE_RELEASE();
             dputs(QT_TRANSLATE_NOOP("DBG", "Breakpoint reached not in list!"));
             DebugUpdateGuiSetStateAsync(GetContextDataEx(hActiveThread, UE_CIP), true);
             //lock
@@ -945,8 +948,10 @@ static void cbGenericBreakpoint(BP_TYPE bptype, void* ExceptionAddress = nullptr
     // increment hit count
     InterlockedIncrement((volatile long*)&bpPtr->hitcount);
 
+    // copy the breakpoint structure and release the breakpoint lock to prevent deadlocks during the wait
     auto bp = *bpPtr;
-    SHARED_RELEASE();
+    EXCLUSIVE_RELEASE();
+
     if(bptype != BPDLL && bptype != BPEXCEPTION)
         bp.addr += ModBaseFromAddr(CIP);
     bp.active = true; //a breakpoint that has been hit is active

src/dbg/threading.h

@@ -74,8 +74,7 @@ enum SectionLock
     LockModuleHashes,
     LockFormatFunctions,
 
-    // Number of elements in this enumeration. Must always be the last
+    // Number of elements in this enumeration. Must always be the last index.
-    // index.
     LockLast
 };
 
@@ -168,22 +167,25 @@ public:
         if(m_LockCount > 0)
             Unlock();
 
-#ifdef _DEBUG
+        // The lock count should be zero after destruction.
-        // Assert that the lock count is zero on destructor
+        assert(m_LockCount == 0);
-        if(m_LockCount > 0)
-            __debugbreak();
-#endif
     }
 
     inline void Lock()
     {
         Internal::AcquireLock(LockIndex, Shared);
 
+        // We cannot recursively lock more than 255 times.
+        assert(m_LockCount < 255);
+
         m_LockCount++;
     }
 
     inline void Unlock()
     {
+        // Unlocking twice will cause undefined behaviour.
+        assert(m_LockCount != 0);
+
         m_LockCount--;
 
         Internal::ReleaseLock(LockIndex, Shared);