From 2f14b350960af0354608c041d7df501c44dc5640 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Mon, 29 Jan 2018 19:08:02 -0800 Subject: [PATCH] Prevent compiler optimization that could cause local var values to change in multithreaded environments, in some places (#16089) --- src/vm/syncblk.cpp | 4 ++-- src/vm/syncblk.h | 30 ++++++++++++++++++++++++++++-- src/vm/syncblk.inl | 14 +++++++------- src/vm/synch.cpp | 16 ++++++++-------- src/vm/synch.h | 10 ++++++++++ 5 files changed, 55 insertions(+), 19 deletions(-) diff --git a/src/vm/syncblk.cpp b/src/vm/syncblk.cpp index 11151a3..eba84e9 100644 --- a/src/vm/syncblk.cpp +++ b/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 diff --git a/src/vm/syncblk.h b/src/vm/syncblk.h index ab32fc7..e699c31 100644 --- a/src/vm/syncblk.h +++ b/src/vm/syncblk.h @@ -386,6 +386,12 @@ private: } public: + LockState VolatileLoadWithoutBarrier() const + { + WRAPPER_NO_CONTRACT; + return ::VolatileLoadWithoutBarrier(&m_state); + } + LockState VolatileLoad() const { WRAPPER_NO_CONTRACT; @@ -423,7 +429,27 @@ private: 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 @@ public: 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 diff --git a/src/vm/syncblk.inl b/src/vm/syncblk.inl index 697afe3..617e240 100644 --- a/src/vm/syncblk.inl +++ b/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) diff --git a/src/vm/synch.cpp b/src/vm/synch.cpp index 1dd0382..7a081d2 100644 --- a/src/vm/synch.cpp +++ b/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; diff --git a/src/vm/synch.h b/src/vm/synch.h index 30e6c30..1da2107 100644 --- a/src/vm/synch.h +++ b/src/vm/synch.h @@ -219,6 +219,12 @@ private: return *this; } + Counts VolatileLoadWithoutBarrier() const + { + LIMITED_METHOD_CONTRACT; + return ::VolatileLoadWithoutBarrier(&data); + } + Counts CompareExchange(Counts toCounts, Counts fromCounts) { LIMITED_METHOD_CONTRACT; @@ -264,7 +270,11 @@ public: 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) -- 2.7.4