summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/vm/syncblk.cpp4
-rw-r--r--src/vm/syncblk.h30
-rw-r--r--src/vm/syncblk.inl14
-rw-r--r--src/vm/synch.cpp16
-rw-r--r--src/vm/synch.h10
5 files changed, 55 insertions, 19 deletions
diff --git a/src/vm/syncblk.cpp b/src/vm/syncblk.cpp
index 11151a32c5..eba84e9237 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 ab32fc76aa..e699c31716 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 697afe369b..617e2409c0 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 1dd038237a..7a081d22b2 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 30e6c30672..1da2107b8d 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)
generated by cgit v1.2.3 (git 2.25.1) at 2024-06-06 00:55:47 +0000