Remove no-op holder stack validation. (dotnet/coreclr#22182)

Commit migrated from https://github.com/dotnet/coreclr/commit/0a43d9c795c1903e3d31b7228e7785554e89b59f

diff --git a/src/coreclr/src/inc/holder.h b/src/coreclr/src/inc/holder.h
index 49c90b7..c346c73 100644 (file)
--- a/src/coreclr/src/inc/holder.h
+++ b/src/coreclr/src/inc/holder.h
@@ -61,17 +61,8 @@
         _NAME & operator=(_NAME const &);
 #endif
 
-// The types of stack validation we support in holders.
-enum HolderStackValidation
-{
-    HSV_NoValidation,
-    HSV_ValidateMinimumStackReq,
-    HSV_ValidateNormalStackReq,        
-};
-
 #ifdef _DEBUG
 
-
 //------------------------------------------------------------------------------------------------
 // This is used to make Visual Studio autoexp.dat work sensibly with holders again.
 // The problem is that certain codebases (particulary Fusion) implement key data structures
@@ -239,8 +230,7 @@ template
         typename TYPE,
         typename BASE,
         UINT_PTR DEFAULTVALUE = 0,
-        BOOL IS_NULL(TYPE, TYPE) = CompareDefault<TYPE>,
-        HolderStackValidation VALIDATION_TYPE = HSV_ValidateNormalStackReq
+        BOOL IS_NULL(TYPE, TYPE) = CompareDefault<TYPE>
     >
 class BaseHolder : protected BASE
 {
@@ -304,15 +294,7 @@ class BaseHolder : protected BASE
         if (m_acquired)
         {
             _ASSERTE(!IsNull());
-        
-            if (VALIDATION_TYPE != HSV_NoValidation)
-            {
-                this->DoRelease();
-            }
-            else
-            {
-                this->DoRelease();
-            }
+            this->DoRelease();
             m_acquired = FALSE;
         }
     }
@@ -347,7 +329,7 @@ class BaseHolder : protected BASE
     HIDE_GENERATED_METHODS(BaseHolder)
 };  // BaseHolder<>
 
-template <void (*ACQUIRE)(), void (*RELEASEF)(), HolderStackValidation VALIDATION_TYPE = HSV_ValidateNormalStackReq>
+template <void (*ACQUIRE)(), void (*RELEASEF)()>
 class StateHolder
 {
   private:
@@ -383,14 +365,7 @@ class StateHolder
 
         if (m_acquired)
         {
-            if (VALIDATION_TYPE != HSV_NoValidation)
-            {
-                RELEASEF();
-            }
-            else
-            {
-                RELEASEF();
-            }
+            RELEASEF();
             m_acquired = FALSE;
         }
     }
@@ -414,7 +389,7 @@ class StateHolder
 };  // class StateHolder<>
 
 // Holder for the case where the acquire function can fail.
-template <typename VALUE, BOOL (*ACQUIRE)(VALUE value), void (*RELEASEF)(VALUE value), HolderStackValidation VALIDATION_TYPE = HSV_ValidateNormalStackReq>
+template <typename VALUE, BOOL (*ACQUIRE)(VALUE value), void (*RELEASEF)(VALUE value)>
 class ConditionalStateHolder
 {
   private:
@@ -452,14 +427,7 @@ class ConditionalStateHolder
 
         if (m_acquired)
         {
-            if (VALIDATION_TYPE != HSV_NoValidation)
-            {
-                RELEASEF(m_value);
-            }
-            else
-            {
-                RELEASEF(m_value);
-            }
+            RELEASEF(m_value);
             m_acquired = FALSE;
         }
     }
@@ -501,10 +469,10 @@ class ConditionalStateHolder
 // the value it contains.
 //-----------------------------------------------------------------------------
 template <typename TYPE, typename BASE,
-          UINT_PTR DEFAULTVALUE = 0, BOOL IS_NULL(TYPE, TYPE) = CompareDefault<TYPE>, HolderStackValidation VALIDATION_TYPE = HSV_ValidateNormalStackReq> 
-class BaseWrapper : public BaseHolder<TYPE, BASE, DEFAULTVALUE, IS_NULL, VALIDATION_TYPE>
+          UINT_PTR DEFAULTVALUE = 0, BOOL IS_NULL(TYPE, TYPE) = CompareDefault<TYPE>>
+class BaseWrapper : public BaseHolder<TYPE, BASE, DEFAULTVALUE, IS_NULL>
 {
-    typedef BaseHolder<TYPE, BASE, DEFAULTVALUE, IS_NULL, VALIDATION_TYPE> BaseT;
+    typedef BaseHolder<TYPE, BASE, DEFAULTVALUE, IS_NULL> BaseT;
 
 
 #ifdef __GNUC__
@@ -731,7 +699,7 @@ FORCEINLINE void SafeArrayDoNothing(SAFEARRAY* p)
 // function pointers
 //-----------------------------------------------------------------------------
 
-template <typename TYPE, void (*ACQUIREF)(TYPE), void (*RELEASEF)(TYPE), HolderStackValidation VALIDATION_TYPE = HSV_ValidateNormalStackReq>  
+template <typename TYPE, void (*ACQUIREF)(TYPE), void (*RELEASEF)(TYPE)>
 class FunctionBase : protected HolderBase<TYPE>
 {
   protected:
@@ -748,16 +716,7 @@ class FunctionBase : protected HolderBase<TYPE>
 
     void DoRelease()
     {
-        // <TODO> Consider removing this stack validation since it is redundant with the
-        // one that is already being done in BaseHolder & BaseWrapper. </TODO>
-        if (VALIDATION_TYPE != HSV_NoValidation)
-        {
-            RELEASEF(this->m_value);
-        }
-        else
-        {
-            RELEASEF(this->m_value);
-        }
+        RELEASEF(this->m_value);
     }
 };  // class Function<>
 
@@ -768,17 +727,16 @@ template
         void (*RELEASEF)(TYPE),
         UINT_PTR DEFAULTVALUE = 0, 
         BOOL IS_NULL(TYPE, TYPE) = CompareDefault<TYPE>,
-        HolderStackValidation VALIDATION_TYPE = HSV_ValidateNormalStackReq,
         // For legacy compat (see EEJitManager::WriterLockHolder), where default ctor
         // causes ACQUIREF(DEFAULTVALUE), but ACQUIREF ignores the argument and
         // operates on static or global value instead.
         bool DEFAULT_CTOR_ACQUIRE = true
     >
-class Holder : public BaseHolder<TYPE, FunctionBase<TYPE, ACQUIREF, RELEASEF, VALIDATION_TYPE>,
-                                 DEFAULTVALUE, IS_NULL, VALIDATION_TYPE>
+class Holder : public BaseHolder<TYPE, FunctionBase<TYPE, ACQUIREF, RELEASEF>,
+                                 DEFAULTVALUE, IS_NULL>
 {
-    typedef BaseHolder<TYPE, FunctionBase<TYPE, ACQUIREF, RELEASEF, VALIDATION_TYPE>,
-                                 DEFAULTVALUE, IS_NULL, VALIDATION_TYPE> BaseT;
+    typedef BaseHolder<TYPE, FunctionBase<TYPE, ACQUIREF, RELEASEF>,
+                                 DEFAULTVALUE, IS_NULL> BaseT;
 
   public:
     FORCEINLINE Holder()
@@ -818,17 +776,16 @@ template
         void (*RELEASEF)(TYPE),
         UINT_PTR DEFAULTVALUE = 0,
         BOOL IS_NULL(TYPE, TYPE) = CompareDefault<TYPE>,
-        HolderStackValidation VALIDATION_TYPE = HSV_ValidateNormalStackReq,
         // For legacy compat (see EEJitManager::WriterLockHolder), where default ctor
         // causes ACQUIREF(DEFAULTVALUE), but ACQUIREF ignores the argument and
         // operates on static or global value instead.
         bool DEFAULT_CTOR_ACQUIRE = true
     >
-class Wrapper : public BaseWrapper<TYPE, FunctionBase<TYPE, ACQUIREF, RELEASEF, VALIDATION_TYPE>,
-                                   DEFAULTVALUE, IS_NULL, VALIDATION_TYPE>
+class Wrapper : public BaseWrapper<TYPE, FunctionBase<TYPE, ACQUIREF, RELEASEF>,
+                                   DEFAULTVALUE, IS_NULL>
 {
-    typedef BaseWrapper<TYPE, FunctionBase<TYPE, ACQUIREF, RELEASEF, VALIDATION_TYPE>,
-                                   DEFAULTVALUE, IS_NULL, VALIDATION_TYPE> BaseT;
+    typedef BaseWrapper<TYPE, FunctionBase<TYPE, ACQUIREF, RELEASEF>,
+                                   DEFAULTVALUE, IS_NULL> BaseT;
 
   public:
     FORCEINLINE Wrapper()
@@ -1215,7 +1172,7 @@ typedef Wrapper< bool *, BoolSet, BoolUnset > BoolFlagStateHolder;
 FORCEINLINE void CounterIncrease(RAW_KEYWORD(volatile) LONG* p) {InterlockedIncrement(p);};
 FORCEINLINE void CounterDecrease(RAW_KEYWORD(volatile) LONG* p) {InterlockedDecrement(p);};
 
-typedef Wrapper<RAW_KEYWORD(volatile) LONG*, CounterIncrease, CounterDecrease, (UINT_PTR)0, CompareDefault<RAW_KEYWORD(volatile) LONG*>, HSV_NoValidation> CounterHolder;
+typedef Wrapper<RAW_KEYWORD(volatile) LONG*, CounterIncrease, CounterDecrease, (UINT_PTR)0, CompareDefault<RAW_KEYWORD(volatile) LONG*>> CounterHolder;
 
 
 #ifndef FEATURE_PAL

diff --git a/src/coreclr/src/inc/profilepriv.inl b/src/coreclr/src/inc/profilepriv.inl
index c612b3d..ac52521 100644 (file)
--- a/src/coreclr/src/inc/profilepriv.inl
+++ b/src/coreclr/src/inc/profilepriv.inl
@@ -743,7 +743,7 @@ inline BOOL CORProfilerDisableTieredCompilation()
 // See code:ProfilingAPIUtility::InitializeProfiling#LoadUnloadCallbackSynchronization.
 // 
 typedef Wrapper<Thread *, ProfilingAPIUtility::IncEvacuationCounter, ProfilingAPIUtility::DecEvacuationCounter, 
-    (UINT_PTR)0, CompareDefault<Thread *>, HSV_NoValidation> EvacuationCounterHolder;
+    (UINT_PTR)0, CompareDefault<Thread *>> EvacuationCounterHolder;
 
 
 //---------------------------------------------------------------------------------------

diff --git a/src/coreclr/src/inc/stresslog.h b/src/coreclr/src/inc/stresslog.h
index d109778..dd8c029 100644 (file)
--- a/src/coreclr/src/inc/stresslog.h
+++ b/src/coreclr/src/inc/stresslog.h
@@ -397,7 +397,7 @@ typedef USHORT
     static StressLog theLog;    // We only have one log, and this is it
 };
 
-typedef Holder<CRITSEC_COOKIE, StressLog::Enter, StressLog::Leave, NULL, CompareDefault<CRITSEC_COOKIE>, HSV_NoValidation> StressLogLockHolder;
+typedef Holder<CRITSEC_COOKIE, StressLog::Enter, StressLog::Leave, NULL, CompareDefault<CRITSEC_COOKIE>> StressLogLockHolder;
 
 #if defined(DACCESS_COMPILE)
 inline BOOL StressLog::LogOn(unsigned facility, unsigned level)

diff --git a/src/coreclr/src/vm/crst.h b/src/coreclr/src/vm/crst.h
index d2e7075..5289dfa 100644 (file)
--- a/src/coreclr/src/vm/crst.h
+++ b/src/coreclr/src/vm/crst.h
@@ -397,13 +397,13 @@ private:
 
     // Note that the holders for CRSTs are used in extremely low stack conditions. Because of this, they 
     // aren't allowed to use more than HOLDER_CODE_MINIMUM_STACK_LIMIT pages of stack.
-    typedef DacHolder<CrstBase *, CrstBase::AcquireLock, CrstBase::ReleaseLock, 0, CompareDefault, HSV_ValidateMinimumStackReq> CrstHolderWithState;
+    typedef DacHolder<CrstBase *, CrstBase::AcquireLock, CrstBase::ReleaseLock, 0, CompareDefault> CrstHolderWithState;
 
     // We have some situations where we're already holding a lock, and we need to release and reacquire the lock across a window.
     // This is a dangerous construct because the backout code can block.
     // Generally, it's better to use a regular CrstHolder, and then use the Release() / Acquire() methods on it.
     // This just exists to convert legacy OS Critical Section patterns over to holders.
-    typedef DacHolder<CrstBase *, CrstBase::ReleaseLock, CrstBase::AcquireLock, 0, CompareDefault, HSV_ValidateMinimumStackReq> UnsafeCrstInverseHolder;
+    typedef DacHolder<CrstBase *, CrstBase::ReleaseLock, CrstBase::AcquireLock, 0, CompareDefault> UnsafeCrstInverseHolder;
 };
 
 typedef CrstBase::CrstHolder CrstHolder;

diff --git a/src/coreclr/src/vm/threads.h b/src/coreclr/src/vm/threads.h
index 61d92e2..ba22725 100644 (file)
--- a/src/coreclr/src/vm/threads.h
+++ b/src/coreclr/src/vm/threads.h
@@ -330,9 +330,7 @@ public:
     static void IncForbidSuspendThread() { }
     static void DecForbidSuspendThread() { }
 
-    // The ForbidSuspendThreadHolder is used during the initialization of the stack marker infrastructure so
-    // it can't do any backout stack validation (which is why we pass in VALIDATION_TYPE=HSV_NoValidation).
-    typedef StateHolder<Thread::IncForbidSuspendThread, Thread::DecForbidSuspendThread, HSV_NoValidation> ForbidSuspendThreadHolder;
+    typedef StateHolder<Thread::IncForbidSuspendThread, Thread::DecForbidSuspendThread> ForbidSuspendThreadHolder;
 
     static BYTE GetOffsetOfCurrentFrame()
     {
@@ -1822,9 +1820,7 @@ public:
         return m_dwForbidSuspendThread != (LONG)0;
     }
     
-    // The ForbidSuspendThreadHolder is used during the initialization of the stack marker infrastructure so
-    // it can't do any backout stack validation (which is why we pass in VALIDATION_TYPE=HSV_NoValidation).
-    typedef StateHolder<Thread::IncForbidSuspendThread, Thread::DecForbidSuspendThread, HSV_NoValidation> ForbidSuspendThreadHolder;
+    typedef StateHolder<Thread::IncForbidSuspendThread, Thread::DecForbidSuspendThread> ForbidSuspendThreadHolder;
 
 private:
     // Per thread counter to dispense hash code - kept in the thread so we don't need a lock