Prevent compiler optimization that could cause local var values to change in multithreaded environments, in some places (#16089) (#16093)

Author: kouvel

src/vm/syncblk.cpp

@@ -2888,7 +2888,7 @@ void AwareLock::Enter()
     CONTRACTL_END;
 
     Thread *pCurThread = GetThread();
-    LockState state = m_lockState;
+    LockState state = m_lockState.VolatileLoadWithoutBarrier();
     if (!state.IsLocked() || m_HoldingThread != pCurThread)
     {
         if (m_lockState.InterlockedTryLock_Or_RegisterWaiter(this, state))
@@ -2950,7 +2950,7 @@ BOOL AwareLock::TryEnter(INT32 timeOut)
         pCurThread->HandleThreadAbort();
     }
 
-    LockState state = m_lockState;
+    LockState state = m_lockState.VolatileLoadWithoutBarrier();
     if (!state.IsLocked() || m_HoldingThread != pCurThread)
     {
         if (timeOut == 0

src/vm/syncblk.h

@@ -386,6 +386,12 @@ class AwareLock
         }
 
     public:
+        LockState VolatileLoadWithoutBarrier() const
+        {
+            WRAPPER_NO_CONTRACT;
+            return ::VolatileLoadWithoutBarrier(&m_state);
+        }
+
         LockState VolatileLoad() const
         {
             WRAPPER_NO_CONTRACT;
@@ -423,7 +429,27 @@ class AwareLock
     friend class LockState;
 
 private:
+    // Take care to use 'm_lockState.VolatileLoadWithoutBarrier()` when loading this value into a local variable that will be
+    // reused. That prevents an optimization in the compiler that avoids stack-spilling a value loaded from memory and instead
+    // reloads the value from the original memory location under the assumption that it would not be changed by another thread,
+    // which can result in the local variable's value changing between reads if the memory location is modifed by another
+    // thread. This is important for patterns such as:
+    //
+    //     T x = m_x; // no barrier
+    //     if (meetsCondition(x))
+    //     {
+    //         assert(meetsCondition(x)); // This may fail!
+    //     }
+    //
+    // The code should be written like this instead:
+    //
+    //     T x = VolatileLoadWithoutBarrier(&m_x); // compile-time barrier, no run-time barrier
+    //     if (meetsCondition(x))
+    //     {
+    //         assert(meetsCondition(x)); // This will not fail
+    //     }
     LockState m_lockState;
+
     ULONG           m_Recursion;
     PTR_Thread      m_HoldingThread;
 
@@ -482,13 +508,13 @@ class AwareLock
     UINT32 GetLockState() const
     {
         WRAPPER_NO_CONTRACT;
-        return m_lockState.GetState();
+        return m_lockState.VolatileLoadWithoutBarrier().GetState();
     }
 
     bool IsUnlockedWithNoWaiters() const
     {
         WRAPPER_NO_CONTRACT;
-        return m_lockState.IsUnlockedWithNoWaiters();
+        return m_lockState.VolatileLoadWithoutBarrier().IsUnlockedWithNoWaiters();
     }
 
     UINT32 GetMonitorHeldStateVolatile() const

src/vm/syncblk.inl

@@ -11,7 +11,7 @@
 FORCEINLINE bool AwareLock::LockState::InterlockedTryLock()
 {
     WRAPPER_NO_CONTRACT;
-    return InterlockedTryLock(*this);
+    return InterlockedTryLock(VolatileLoadWithoutBarrier());
 }
 
 FORCEINLINE bool AwareLock::LockState::InterlockedTryLock(LockState state)
@@ -80,7 +80,7 @@ FORCEINLINE bool AwareLock::LockState::InterlockedUnlock()
 FORCEINLINE bool AwareLock::LockState::InterlockedTrySetShouldNotPreemptWaitersIfNecessary(AwareLock *awareLock)
 {
     WRAPPER_NO_CONTRACT;
-    return InterlockedTrySetShouldNotPreemptWaitersIfNecessary(awareLock, *this);
+    return InterlockedTrySetShouldNotPreemptWaitersIfNecessary(awareLock, VolatileLoadWithoutBarrier());
 }
 
 FORCEINLINE bool AwareLock::LockState::InterlockedTrySetShouldNotPreemptWaitersIfNecessary(
@@ -178,7 +178,7 @@ FORCEINLINE AwareLock::EnterHelperResult AwareLock::LockState::InterlockedTry_Lo
     WRAPPER_NO_CONTRACT;
 
     // This function is called from inside a spin loop, it must unregister the spinner if and only if the lock is acquired
-    LockState state = *this;
+    LockState state = VolatileLoadWithoutBarrier();
     while (true)
     {
         _ASSERTE(state.HasAnySpinners());
@@ -288,7 +288,7 @@ FORCEINLINE void AwareLock::LockState::InterlockedUnregisterWaiter()
 {
     WRAPPER_NO_CONTRACT;
 
-    LockState state = *this;
+    LockState state = VolatileLoadWithoutBarrier();
     while (true)
     {
         _ASSERTE(state.HasAnyWaiters());
@@ -319,7 +319,7 @@ FORCEINLINE bool AwareLock::LockState::InterlockedTry_LockAndUnregisterWaiterAnd
     // This function is called from the waiter's spin loop and should observe the wake signal only if the lock is taken, to
     // prevent a lock releaser from waking another waiter while one is already spinning to acquire the lock
     bool waiterStarvationStartTimeWasRecorded = false;
-    LockState state = *this;
+    LockState state = VolatileLoadWithoutBarrier();
     while (true)
     {
         _ASSERTE(state.HasAnyWaiters());
@@ -502,7 +502,7 @@ FORCEINLINE AwareLock::EnterHelperResult AwareLock::TryEnterBeforeSpinLoopHelper
         MODE_ANY;
     } CONTRACTL_END;
 
-    LockState state = m_lockState;
+    LockState state = m_lockState.VolatileLoadWithoutBarrier();
 
     // 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.
@@ -685,7 +685,7 @@ FORCEINLINE AwareLock::LeaveHelperAction AwareLock::LeaveHelper(Thread* pCurThre
     if (m_HoldingThread != pCurThread)
         return AwareLock::LeaveHelperAction_Error;
 
-    _ASSERTE(m_lockState.IsLocked());
+    _ASSERTE(m_lockState.VolatileLoadWithoutBarrier().IsLocked());
     _ASSERTE(m_Recursion >= 1);
 
 #if defined(_DEBUG) && defined(TRACK_SYNC) && !defined(CROSSGEN_COMPILE)

src/vm/synch.cpp

@@ -644,7 +644,7 @@ bool CLRLifoSemaphore::WaitForSignal(DWORD timeoutMs)
 
     _ASSERTE(timeoutMs != 0);
     _ASSERTE(m_handle != nullptr);
-    _ASSERTE(m_counts.waiterCount != (UINT16)0);
+    _ASSERTE(m_counts.VolatileLoadWithoutBarrier().waiterCount != (UINT16)0);
 
     while (true)
     {
@@ -680,7 +680,7 @@ bool CLRLifoSemaphore::WaitForSignal(DWORD timeoutMs)
         }
 
         // Unregister the waiter if this thread will not be waiting anymore, and try to acquire the semaphore
-        Counts counts = m_counts;
+        Counts counts = m_counts.VolatileLoadWithoutBarrier();
         while (true)
         {
             _ASSERTE(counts.waiterCount != (UINT16)0);
@@ -719,7 +719,7 @@ bool CLRLifoSemaphore::Wait(DWORD timeoutMs)
     _ASSERTE(m_handle != nullptr);
 
     // Acquire the semaphore or register as a waiter
-    Counts counts = m_counts;
+    Counts counts = m_counts.VolatileLoadWithoutBarrier();
     while (true)
     {
         _ASSERTE(counts.signalCount <= m_maximumSignalCount);
@@ -762,7 +762,7 @@ bool CLRLifoSemaphore::Wait(DWORD timeoutMs, UINT32 spinCount, UINT32 processorC
     }
 
     // Try to acquire the semaphore or register as a spinner
-    Counts counts = m_counts;
+    Counts counts = m_counts.VolatileLoadWithoutBarrier();
     while (true)
     {
         Counts newCounts = counts;
@@ -809,7 +809,7 @@ bool CLRLifoSemaphore::Wait(DWORD timeoutMs, UINT32 spinCount, UINT32 processorC
         ClrSleepEx(0, false);
 
         // Try to acquire the semaphore and unregister as a spinner
-        counts = m_counts;
+        counts = m_counts.VolatileLoadWithoutBarrier();
         while (true)
         {
             _ASSERTE(counts.spinnerCount != (UINT8)0);
@@ -868,7 +868,7 @@ bool CLRLifoSemaphore::Wait(DWORD timeoutMs, UINT32 spinCount, UINT32 processorC
         }
 
         // Try to acquire the semaphore and unregister as a spinner
-        counts = m_counts;
+        counts = m_counts.VolatileLoadWithoutBarrier();
         while (true)
         {
             _ASSERTE(counts.spinnerCount != (UINT8)0);
@@ -893,7 +893,7 @@ bool CLRLifoSemaphore::Wait(DWORD timeoutMs, UINT32 spinCount, UINT32 processorC
 #endif // _TARGET_ARM64_
 
     // Unregister as a spinner, and acquire the semaphore or register as a waiter
-    counts = m_counts;
+    counts = m_counts.VolatileLoadWithoutBarrier();
     while (true)
     {
         _ASSERTE(counts.spinnerCount != (UINT8)0);
@@ -934,7 +934,7 @@ void CLRLifoSemaphore::Release(INT32 releaseCount)
     _ASSERTE(m_handle != INVALID_HANDLE_VALUE);
 
     INT32 countOfWaitersToWake;
-    Counts counts = m_counts;
+    Counts counts = m_counts.VolatileLoadWithoutBarrier();
     while (true)
     {
         Counts newCounts = counts;

src/vm/synch.h

@@ -219,6 +219,12 @@ class CLRLifoSemaphore
             return *this;
         }
 
+        Counts VolatileLoadWithoutBarrier() const
+        {
+            LIMITED_METHOD_CONTRACT;
+            return ::VolatileLoadWithoutBarrier(&data);
+        }
+
         Counts CompareExchange(Counts toCounts, Counts fromCounts)
         {
             LIMITED_METHOD_CONTRACT;
@@ -264,7 +270,11 @@ class CLRLifoSemaphore
 
 private:
     BYTE __padding1[MAX_CACHE_LINE_SIZE]; // padding to ensure that m_counts gets its own cache line
+
+    // Take care to use 'm_counts.VolatileLoadWithoutBarrier()` when loading this value into a local variable that will be
+    // reused. See AwareLock::m_lockState for details.
     Counts m_counts;
+
     BYTE __padding2[MAX_CACHE_LINE_SIZE]; // padding to ensure that m_counts gets its own cache line
 
 #if defined(DEBUG)