Remove thread pointer from AwareLock (#112715)

Author: eduardo-vp

src/coreclr/vm/syncblk.cpp

@@ -1815,23 +1815,20 @@ BOOL ObjHeader::GetThreadOwningMonitorLock(DWORD *pThreadId, DWORD *pAcquisition
             SyncBlock* psb = g_pSyncTable[(int)index].m_SyncBlock;
 
             _ASSERTE(psb->GetMonitor() != NULL);
-            Thread* pThread = psb->GetMonitor()->GetHoldingThread();
-            // If the lock is orphaned during sync block creation, pThread would be assigned -1.
-            // Otherwise pThread would point to the owning thread if there was one or NULL if there wasn't.
-            if (pThread == NULL || pThread == (Thread*) -1)
+            DWORD holdingThreadId = psb->GetMonitor()->GetHoldingThreadId();
+            // If the lock is orphaned during sync block creation, holdingThreadId would be assigned -1.
+            // Otherwise it would be the id of the holding thread if there is one or 0 if there isn't.
+            if (holdingThreadId == 0 || holdingThreadId == (DWORD)-1)
             {
                 *pThreadId = 0;
                 *pAcquisitionCount = 0;
                 return FALSE;
             }
             else
             {
-                // However, the lock might get orphaned after the sync block is created.
-                // Therefore accessing pThread is not safe and pThread->GetThreadId() shouldn't be used.
-                // The thread id can be obtained from the monitor, which would be the id of the thread that owned the lock.
-                // Notice this id now could have been reused for a different thread,
-                // but this way we avoid crashing the process and orphaned locks shouldn't be expected to work correctly anyway.
-                *pThreadId = psb->GetMonitor()->GetHoldingThreadId();
+                // Notice this id now could have been reused for a different thread (in case the lock was orphaned),
+                // but orphaned locks shouldn't be expected to work correctly anyway.
+                *pThreadId = holdingThreadId;
                 *pAcquisitionCount = psb->GetMonitor()->GetRecursionLevel();
                 return TRUE;
             }
@@ -2203,7 +2200,6 @@ SyncBlock *ObjHeader::GetSyncBlock()
                             if (pThread == NULL)
                             {
                                 // The lock is orphaned.
-                                pThread = (Thread*) -1;
                                 threadId = -1;
                                 osThreadId = (SIZE_T)-1;
                             }
@@ -2213,7 +2209,7 @@ SyncBlock *ObjHeader::GetSyncBlock()
                                 osThreadId = pThread->GetOSThreadId64();
                             }
 
-                            syncBlock->InitState(recursionLevel + 1, pThread, threadId, osThreadId);
+                            syncBlock->InitState(recursionLevel + 1, threadId, osThreadId);
                         }
                     }
                     else if ((bits & BIT_SBLK_IS_HASHCODE) != 0)
@@ -2334,6 +2330,17 @@ void ObjHeader::PulseAll()
 //
 // ***************************************************************************
 
+#endif // !DACCESS_COMPILE
+
+// the DAC needs to have this method available
+PTR_Thread AwareLock::GetHoldingThread()
+{
+    LIMITED_METHOD_CONTRACT;
+    return g_pThinLockThreadIdDispenser->IdToThreadWithValidation(m_HoldingThreadId);
+}
+
+#ifndef DACCESS_COMPILE
+
 void AwareLock::AllocLockSemEvent()
 {
     CONTRACTL
@@ -2371,12 +2378,11 @@ void AwareLock::Enter()
 
     Thread *pCurThread = GetThread();
     LockState state = m_lockState.VolatileLoadWithoutBarrier();
-    if (!state.IsLocked() || m_HoldingThread != pCurThread)
+    if (!state.IsLocked() || m_HoldingThreadId != pCurThread->GetThreadId())
     {
         if (m_lockState.InterlockedTryLock_Or_RegisterWaiter(this, state))
         {
             // We get here if we successfully acquired the mutex.
-            m_HoldingThread = pCurThread;
             m_HoldingThreadId = pCurThread->GetThreadId();
             m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
             m_Recursion = 1;
@@ -2433,14 +2439,13 @@ BOOL AwareLock::TryEnter(INT32 timeOut)
     }
 
     LockState state = m_lockState.VolatileLoadWithoutBarrier();
-    if (!state.IsLocked() || m_HoldingThread != pCurThread)
+    if (!state.IsLocked() || m_HoldingThreadId != pCurThread->GetThreadId())
     {
         if (timeOut == 0
                 ? m_lockState.InterlockedTryLock(state)
                 : m_lockState.InterlockedTryLock_Or_RegisterWaiter(this, state))
         {
             // We get here if we successfully acquired the mutex.
-            m_HoldingThread = pCurThread;
             m_HoldingThreadId = pCurThread->GetThreadId();
             m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
             m_Recursion = 1;
@@ -2720,7 +2725,6 @@ BOOL AwareLock::EnterEpilogHelper(Thread* pCurThread, INT32 timeOut)
         return false;
     }
 
-    m_HoldingThread = pCurThread;
     m_HoldingThreadId = pCurThread->GetThreadId();
     m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
     m_Recursion = 1;
@@ -2783,7 +2787,7 @@ LONG AwareLock::LeaveCompletely()
 BOOL AwareLock::OwnedByCurrentThread()
 {
     WRAPPER_NO_CONTRACT;
-    return (GetThread() == m_HoldingThread);
+    return (GetThread()->GetThreadId() == m_HoldingThreadId);
 }
 
 // ***************************************************************************

src/coreclr/vm/syncblk.h

@@ -435,7 +435,6 @@ class AwareLock
     LockState m_lockState;
 
     ULONG           m_Recursion;
-    PTR_Thread      m_HoldingThread;
     DWORD           m_HoldingThreadId;
     SIZE_T          m_HoldingOSThreadId;
 
@@ -456,10 +455,6 @@ class AwareLock
     // Only SyncBlocks can create AwareLocks.  Hence this private constructor.
     AwareLock(DWORD indx)
         : m_Recursion(0),
-#ifndef DACCESS_COMPILE
-// PreFAST has trouble with initializing a NULL PTR_Thread.
-          m_HoldingThread(NULL),
-#endif // DACCESS_COMPILE
           m_HoldingThreadId(0),
           m_HoldingOSThreadId(0),
           m_TransientPrecious(0),
@@ -519,12 +514,6 @@ class AwareLock
         return m_Recursion;
     }
 
-    PTR_Thread GetHoldingThread() const
-    {
-        LIMITED_METHOD_CONTRACT;
-        return m_HoldingThread;
-    }
-
     DWORD GetHoldingThreadId() const
     {
         LIMITED_METHOD_CONTRACT;
@@ -537,13 +526,12 @@ class AwareLock
     bool ShouldStopPreemptingWaiters() const;
 
 private: // friend access is required for this unsafe function
-    void InitializeToLockedWithNoWaiters(ULONG recursionLevel, PTR_Thread holdingThread, DWORD holdingThreadId, SIZE_T holdingOSThreadId)
+    void InitializeToLockedWithNoWaiters(ULONG recursionLevel, DWORD holdingThreadId, SIZE_T holdingOSThreadId)
     {
         WRAPPER_NO_CONTRACT;
 
         m_lockState.InitializeToLockedWithNoWaiters();
         m_Recursion = recursionLevel;
-        m_HoldingThread = holdingThread;
         m_HoldingThreadId = holdingThreadId;
         m_HoldingOSThreadId = holdingOSThreadId;
     }
@@ -581,6 +569,7 @@ class AwareLock
     void    AllocLockSemEvent();
     LONG    LeaveCompletely();
     BOOL    OwnedByCurrentThread();
+    PTR_Thread GetHoldingThread();
 
     void    IncrementTransientPrecious()
     {
@@ -604,14 +593,6 @@ class AwareLock
     // protect it.
     inline OBJECTREF GetOwningObject();
 
-    // Provide access to the Thread object that owns this awarelock.  This is used
-    // to provide a host to find out owner of a lock.
-    inline PTR_Thread GetOwningThread()
-    {
-        LIMITED_METHOD_CONTRACT;
-        return m_HoldingThread;
-    }
-
     static int GetOffsetOfHoldingOSThreadId()
     {
         LIMITED_METHOD_CONTRACT;
@@ -1279,10 +1260,10 @@ class SyncBlock
     // This should ONLY be called when initializing a SyncBlock (i.e. ONLY from
     // ObjHeader::GetSyncBlock()), otherwise we'll have a race condition.
     // </NOTE>
-    void InitState(ULONG recursionLevel, PTR_Thread holdingThread, DWORD holdingThreadId, SIZE_T holdingOSThreadId)
+    void InitState(ULONG recursionLevel, DWORD holdingThreadId, SIZE_T holdingOSThreadId)
     {
         WRAPPER_NO_CONTRACT;
-        m_Monitor.InitializeToLockedWithNoWaiters(recursionLevel, holdingThread, holdingThreadId, holdingOSThreadId);
+        m_Monitor.InitializeToLockedWithNoWaiters(recursionLevel, holdingThreadId, holdingOSThreadId);
     }
 
 #if defined(ENABLE_CONTRACTS_IMPL)

src/coreclr/vm/syncblk.inl

@@ -478,14 +478,13 @@ FORCEINLINE bool AwareLock::TryEnterHelper(Thread* pCurThread)
 
     if (m_lockState.InterlockedTryLock())
     {
-        m_HoldingThread = pCurThread;
         m_HoldingThreadId = pCurThread->GetThreadId();
         m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
         m_Recursion = 1;
         return true;
     }
 
-    if (GetOwningThread() == pCurThread) /* monitor is held, but it could be a recursive case */
+    if (GetHoldingThreadId() == pCurThread->GetThreadId()) /* monitor is held, but it could be a recursive case */
     {
         m_Recursion++;
         return true;
@@ -505,7 +504,7 @@ FORCEINLINE AwareLock::EnterHelperResult AwareLock::TryEnterBeforeSpinLoopHelper
 
     // Check the recursive case once before the spin loop. If it's not the recursive case in the beginning, it will not
     // be in the future, so the spin loop can avoid checking the recursive case.
-    if (!state.IsLocked() || GetOwningThread() != pCurThread)
+    if (!state.IsLocked() || GetHoldingThreadId() != pCurThread->GetThreadId())
     {
         if (m_lockState.InterlockedTrySetShouldNotPreemptWaitersIfNecessary(this, state))
         {
@@ -525,7 +524,6 @@ FORCEINLINE AwareLock::EnterHelperResult AwareLock::TryEnterBeforeSpinLoopHelper
         }
 
         // Lock was acquired and the spinner was not registered
-        m_HoldingThread = pCurThread;
         m_HoldingThreadId = pCurThread->GetThreadId();
         m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
         m_Recursion = 1;
@@ -558,7 +556,6 @@ FORCEINLINE AwareLock::EnterHelperResult AwareLock::TryEnterInsideSpinLoopHelper
     }
 
     // Lock was acquired and spinner was unregistered
-    m_HoldingThread = pCurThread;
     m_HoldingThreadId = pCurThread->GetThreadId();
     m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
     m_Recursion = 1;
@@ -582,7 +579,6 @@ FORCEINLINE bool AwareLock::TryEnterAfterSpinLoopHelper(Thread *pCurThread)
     }
 
     // Spinner was unregistered and the lock was acquired
-    m_HoldingThread = pCurThread;
     m_HoldingThreadId = pCurThread->GetThreadId();
     m_HoldingOSThreadId = pCurThread->GetOSThreadId64();
     m_Recursion = 1;
@@ -687,7 +683,7 @@ FORCEINLINE AwareLock::LeaveHelperAction AwareLock::LeaveHelper(Thread* pCurThre
         MODE_ANY;
     } CONTRACTL_END;
 
-    if (m_HoldingThread != pCurThread)
+    if (m_HoldingThreadId != pCurThread->GetThreadId())
         return AwareLock::LeaveHelperAction_Error;
 
     _ASSERTE(m_lockState.VolatileLoadWithoutBarrier().IsLocked());
@@ -702,7 +698,6 @@ FORCEINLINE AwareLock::LeaveHelperAction AwareLock::LeaveHelper(Thread* pCurThre
 
     if (--m_Recursion == 0)
     {
-        m_HoldingThread = NULL;
         m_HoldingThreadId = 0;
         m_HoldingOSThreadId = 0;
 

src/coreclr/vm/threads.h

@@ -4428,19 +4428,19 @@ class IdDispenser
         return result;
     }
 
-    Thread *IdToThreadWithValidation(DWORD id)
+    PTR_Thread IdToThreadWithValidation(DWORD id)
     {
         WRAPPER_NO_CONTRACT;
 
         CrstHolder ch(&m_Crst);
 
-        Thread *result = NULL;
+        PTR_Thread result = NULL;
         if (id <= m_highestId)
             result = m_idToThread[id];
         // m_idToThread may have Thread*, or the next free slot
-        if ((size_t)result <= m_idToThreadCapacity)
+        if (dac_cast<size_t>(result) <= m_idToThreadCapacity)
             result = NULL;
-        _ASSERTE(result == NULL || ((size_t)result & 0x3) == 0 || ((Thread*)result)->GetThreadId() == id);
+        _ASSERTE(result == NULL || (dac_cast<size_t>(result) & 0x3) == 0 || ((Thread*)result)->GetThreadId() == id);
         return result;
     }
 };