Replace Win32 PAL CRITICAL_SECTION with minipal Mutex (#115858)

* Remove unused ThreadStore::CanAcquireLock().

* Create PAL critical section
Delete unused debugging APIs in PAL CS code

* Remove PAL tests for CRITICAL_SECTION
Remove any non-running tests requiring CRITICAL_SECTION
Remove unused PAL header referencing CRITICAL_SECTION (internal.h)

---------

Co-authored-by: Jan Kotas <[email protected]>

Author: AaronRobinsonMSFT

src/coreclr/debug/daccess/daccess.cpp

@@ -42,7 +42,7 @@ extern TADDR g_ClrModuleBase;
 // To include definition of IsThrowableThreadAbortException
 // #include <exstatecommon.h>
 
-CRITICAL_SECTION g_dacCritSec;
+minipal_mutex g_dacMutex;
 ClrDataAccess* g_dacImpl;
 
 EXTERN_C BOOL WINAPI DllMain2(HANDLE instance, DWORD reason, LPVOID reserved)
@@ -72,7 +72,7 @@ EXTERN_C BOOL WINAPI DllMain2(HANDLE instance, DWORD reason, LPVOID reserved)
             return FALSE;
         }
 #endif
-        InitializeCriticalSection(&g_dacCritSec);
+        minipal_mutex_init(&g_dacMutex);
 
         g_procInitialized = true;
         break;
@@ -82,7 +82,7 @@ EXTERN_C BOOL WINAPI DllMain2(HANDLE instance, DWORD reason, LPVOID reserved)
         // It's possible for this to be called without ATTACH completing (eg. if it failed)
         if (g_procInitialized)
         {
-            DeleteCriticalSection(&g_dacCritSec);
+            minipal_mutex_destroy(&g_dacMutex);
         }
         g_procInitialized = false;
         break;

src/coreclr/debug/daccess/dacdbiimpl.cpp

@@ -59,7 +59,7 @@
 
 
 
-// Global allocator for DD. Access is protected under the g_dacCritSec lock.
+// Global allocator for DD. Access is protected under the g_dacMutex lock.
 IDacDbiInterface::IAllocator * g_pAllocator = NULL;
 
 //---------------------------------------------------------------------------------------
@@ -362,7 +362,7 @@ interface IMDInternalImport* DacDbiInterfaceImpl::GetMDImport(
     const ReflectionModule * pReflectionModule,
     bool fThrowEx)
 {
-    // Since this is called from an existing DAC-primitive, we already hold the g_dacCritSec lock.
+    // Since this is called from an existing DAC-primitive, we already hold the g_dacMutex lock.
     // The lock conveniently protects our cache.
     SUPPORTS_DAC;
 

src/coreclr/debug/daccess/dacdbiimpl.h

@@ -1107,7 +1107,7 @@ class DacDbiInterfaceImpl :
 };
 
 
-// Global allocator for DD. Access is protected under the g_dacCritSec lock.
+// Global allocator for DD. Access is protected under the g_dacMutex lock.
 extern "C" IDacDbiInterface::IAllocator * g_pAllocator;
 
 
@@ -1116,7 +1116,7 @@ class DDHolder
 public:
     DDHolder(DacDbiInterfaceImpl* pContainer, bool fAllowReentrant)
     {
-        EnterCriticalSection(&g_dacCritSec);
+        minipal_mutex_enter(&g_dacMutex);
 
         // If we're not re-entrant, then assert.
         if (!fAllowReentrant)
@@ -1139,7 +1139,7 @@ class DDHolder
         g_dacImpl    = m_pOldContainer;
         g_pAllocator = m_pOldAllocator;
 
-        LeaveCriticalSection(&g_dacCritSec);
+        minipal_mutex_leave(&g_dacMutex);
     }
 
 protected:

src/coreclr/debug/daccess/dacimpl.h

@@ -13,6 +13,7 @@
 #ifndef __DACIMPL_H__
 #define __DACIMPL_H__
 
+#include <minipal/mutex.h>
 #include "gcinterface.dac.h"
 //---------------------------------------------------------------------------------------
 // Setting DAC_HASHTABLE tells the DAC to use the hand rolled hashtable for
@@ -26,7 +27,7 @@
 #include <unordered_map>
 #pragma pop_macro("return")
 #endif //DAC_HASHTABLE
-extern CRITICAL_SECTION g_dacCritSec;
+extern minipal_mutex g_dacMutex;
 
 // Convert between CLRDATA_ADDRESS and TADDR.
 // Note that CLRDATA_ADDRESS is sign-extended (for compat with Windbg and OS conventions).  Converting
@@ -3809,26 +3810,26 @@ class EnumMethodInstances
 //----------------------------------------------------------------------------
 
 #define DAC_ENTER() \
-    EnterCriticalSection(&g_dacCritSec); \
+    minipal_mutex_enter(&g_dacMutex); \
     ClrDataAccess* __prevDacImpl = g_dacImpl; \
     g_dacImpl = this;
 
 // When entering a child object we validate that
 // the process's host instance cache hasn't been flushed
 // since the child was created.
 #define DAC_ENTER_SUB(dac) \
-    EnterCriticalSection(&g_dacCritSec); \
+    minipal_mutex_enter(&g_dacMutex); \
     if (dac->m_instanceAge != m_instanceAge) \
     { \
-        LeaveCriticalSection(&g_dacCritSec); \
+        minipal_mutex_leave(&g_dacMutex); \
         return E_INVALIDARG; \
     } \
     ClrDataAccess* __prevDacImpl = g_dacImpl; \
     g_dacImpl = (dac)
 
 #define DAC_LEAVE() \
     g_dacImpl = __prevDacImpl; \
-    LeaveCriticalSection(&g_dacCritSec)
+    minipal_mutex_leave(&g_dacMutex)
 
 
 #define SOSHelperEnter() \

src/coreclr/debug/di/rspriv.h

@@ -15,7 +15,7 @@
 #include <windows.h>
 
 #include <utilcode.h>
-
+#include <minipal/mutex.h>
 
 #ifdef _DEBUG
 #define LOGGING
@@ -798,7 +798,7 @@ class RSLock
     }
 
 
-    CRITICAL_SECTION m_lock;
+    minipal_mutex m_lock;
 
 #ifdef _DEBUG
 public:
@@ -839,9 +839,8 @@ class RSLock
 typedef RSLock::RSLockHolder RSLockHolder;
 typedef RSLock::RSInverseLockHolder RSInverseLockHolder;
 
-// In the RS, we should be using RSLocks instead of raw critical sections.
-#define CRITICAL_SECTION USE_RSLOCK_INSTEAD_OF_CRITICAL_SECTION
-
+// In the RS, we should be using RSLocks instead of raw minipal_mutex.
+#define minipal_mutex USE_RSLOCK_INSTEAD_OF_MINIPAL_MUTEX
 
 /* ------------------------------------------------------------------------- *
  * Helper macros. Use the ATT_* macros below instead of these.
@@ -11200,7 +11199,7 @@ inline CordbEval * UnwrapCookieCordbEval(CordbProcess *pProc, UINT cookie)
 
 
 // We defined this at the top of the file - undef it now so that we don't pollute other files.
-#undef CRITICAL_SECTION
+#undef minipal_mutex
 
 
 #ifdef RSCONTRACTS

src/coreclr/debug/di/rspriv.inl

@@ -528,14 +528,15 @@ inline void RSLock::Init(const char * szTag, int eAttr, ERSLockLevel level)
 
     _ASSERTE(IsInit());
 
-    InitializeCriticalSection(&m_lock);
+    bool init = minipal_mutex_init(&m_lock);
+    _ASSERTE(init);
 }
 
 // Cleanup a lock.
 inline void RSLock::Destroy()
 {
     CONSISTENCY_CHECK_MSGF(IsInit(), ("RSLock '%s' not inited", m_szTag));
-    DeleteCriticalSection(&m_lock);
+    minipal_mutex_destroy(&m_lock);
 
 #ifdef _DEBUG
     m_eAttr = cLockUninit; // No longer initialized.
@@ -549,10 +550,11 @@ inline void RSLock::Lock()
 
 #ifdef RSCONTRACTS
     DbgRSThread * pThread = DbgRSThread::GetThread();
+
     pThread->NotifyTakeLock(this);
 #endif
 
-    EnterCriticalSection(&m_lock);
+    minipal_mutex_enter(&m_lock);
 #ifdef _DEBUG
     m_tidOwner = ::GetCurrentThreadId();
     m_count++;
@@ -583,7 +585,7 @@ inline void RSLock::Unlock()
     pThread->NotifyReleaseLock(this);
 #endif
 
-    LeaveCriticalSection(&m_lock);
+    minipal_mutex_leave(&m_lock);
 }
 
 template <class T>

src/coreclr/debug/inc/dbgtransportsession.h

@@ -8,9 +8,9 @@
 #ifndef RIGHT_SIDE_COMPILE
 #include <utilcode.h>
 #include <crst.h>
-
 #endif // !RIGHT_SIDE_COMPILE
 
+#include <minipal/mutex.h>
 #include <minipal/guid.h>
 
 #if defined(FEATURE_DBGIPC_TRANSPORT_VM) || defined(FEATURE_DBGIPC_TRANSPORT_DI)
@@ -271,7 +271,7 @@ inline UINT32 DBGIPC_HTONL(UINT32 x)
 
 // Lock abstraction (we can't use the same lock implementation on LS and RS since we really want a Crst on the
 // LS and this isn't available in the RS environment).
-class DbgTransportLock
+class DbgTransportLock final
 {
 public:
     void Init();
@@ -281,12 +281,32 @@ class DbgTransportLock
 
 private:
 #ifdef RIGHT_SIDE_COMPILE
-    CRITICAL_SECTION    m_sLock;
+    minipal_mutex       m_sLock;
 #else // RIGHT_SIDE_COMPILE
     CrstExplicitInit    m_sLock;
 #endif // RIGHT_SIDE_COMPILE
 };
 
+class TransportLockHolder final
+{
+    DbgTransportLock& _lock;
+public:
+    TransportLockHolder(DbgTransportLock& lock)
+        : _lock(lock)
+    {
+        _lock.Enter();
+    }
+    ~TransportLockHolder()
+    {
+        _lock.Leave();
+    }
+
+    TransportLockHolder(TransportLockHolder const&) = delete;
+    TransportLockHolder& operator=(TransportLockHolder const&) = delete;
+    TransportLockHolder(TransportLockHolder&& other) = delete;
+    TransportLockHolder&& operator=(TransportLockHolder&&) = delete;
+};
+
 // The transport has only one queue for IPC events, but each IPC event can be marked as one of two types.
 // The transport will signal the handle corresponding to the type of each IPC event.  (See
 // code:DbgTransportSession::GetIPCEventReadyEvent and code:DbgTransportSession::GetDebugEventReadyEvent.)
@@ -555,26 +575,6 @@ class DbgTransportSession
         }
     };
 
-    // Holder class used to take a transport lock in a given scope and automatically release it once that
-    // scope is exited.
-    class TransportLockHolder
-    {
-    public:
-        TransportLockHolder(DbgTransportLock *pLock)
-        {
-            m_pLock = pLock;
-            m_pLock->Enter();
-        }
-
-        ~TransportLockHolder()
-        {
-            m_pLock->Leave();
-        }
-
-    private:
-        DbgTransportLock   *m_pLock;
-    };
-
 #ifdef _DEBUG
     // Store statistics for various session activities that will be useful for performance analysis and tracking
     // down bugs.