Enable contracts for new holders and refactor Crst initialization

src/coreclr/debug/ee/debugger.cpp

@@ -11762,7 +11762,7 @@ void Debugger::GetAndSendTransitionStubInfo(CORDB_ADDRESS_TYPE *stubAddress)
     // If its not a stub, then maybe its an address in mscoree?
     if (result == false)
     {
-        result = (IsIPInModule(GetClrModuleBase(), (PCODE)stubAddress) == TRUE);
+        result = (g_pEEInterface->IsIPInModule(GetClrModuleBase(), (PCODE)stubAddress) == TRUE);
     }
 
     // This is a synchronous event (reply required)

src/coreclr/gc/env/gcenv.sync.h

@@ -26,9 +26,9 @@ class CrstStatic
 #endif
 
 public:
-    bool InitNoThrow(CrstType eType, CrstFlags eFlags = CRST_DEFAULT)
+    void Init(CrstType eType, CrstFlags eFlags = CRST_DEFAULT)
     {
-        return m_cs.Initialize();
+        (void)m_cs.Initialize();
     }
 
     void Destroy()

src/coreclr/gc/handletable.cpp

@@ -136,12 +136,7 @@ HHANDLETABLE HndCreateHandleTable(const uint32_t *pTypeFlags, uint32_t uTypeCoun
     // We need to allow CRST_UNSAFE_SAMELEVEL, because
     // during AD unload, we need to move some TableSegment from unloaded domain to default domain.
     // We need to take both locks for the two HandleTable's to avoid racing with concurrent gc thread.
-    if (!pTable->Lock.InitNoThrow(CrstHandleTable, CrstFlags(CRST_REENTRANCY | CRST_UNSAFE_ANYMODE | CRST_DEBUGGER_THREAD | CRST_UNSAFE_SAMELEVEL)))
-    {
-        SegmentFree(pTable->pSegmentList);
-        delete [] (uint8_t*)pTable;
-        return NULL;
-    }
+    pTable->Lock.Init(CrstHandleTable, CrstFlags(CRST_REENTRANCY | CRST_UNSAFE_ANYMODE | CRST_DEBUGGER_THREAD | CRST_UNSAFE_SAMELEVEL));
 
     // remember how many types we are supporting
     pTable->uTypeCount = uTypeCount;

src/coreclr/inc/contract.h

@@ -188,18 +188,16 @@
 #pragma warning(disable:4189) //local variable is initialized but not referenced
 #endif
 
-
-// We only enable contracts in _DEBUG builds
-#if defined(_DEBUG) && !defined(DISABLE_CONTRACTS) && !defined(JIT_BUILD)
+#if defined(_DEBUG) && !defined(DISABLE_CONTRACTS)
 #define ENABLE_CONTRACTS_DATA
 #endif
 
-// Also, we won't enable contracts if this is a DAC build.
-#if defined(ENABLE_CONTRACTS_DATA) && !defined(DACCESS_COMPILE) && !defined(CROSS_COMPILE)
+// The DAC builds reference contract data but not implementation,
+// so we disable contracts for those builds.
+#if defined(ENABLE_CONTRACTS_DATA) && !defined(DACCESS_COMPILE)
 #define ENABLE_CONTRACTS
 #endif
 
-// Finally, only define the implementation parts of contracts if this isn't a DAC build.
 #if defined(_DEBUG_IMPL) && defined(ENABLE_CONTRACTS)
 #define ENABLE_CONTRACTS_IMPL
 #endif
@@ -1739,5 +1737,4 @@ extern Volatile<LONG> g_DbgSuppressAllocationAsserts;
     STATIC_CONTRACT_GC_TRIGGERS;        \
     STATIC_CONTRACT_MODE_PREEMPTIVE;
 
-
 #endif  // CONTRACT_H_

src/coreclr/inc/holder.h

@@ -991,16 +991,32 @@ class LifetimeHolder final
     }
 };
 
+#ifdef ENABLE_CONTRACTS_IMPL
+// Out-of-line hook that enforces the ReleaseHolder release-path contract
+// (NOTHROW / GC_TRIGGERS / MODE_PREEMPTIVE).
+//
+// Exactly one definition is linked into each binary, selected by which utilcode flavor
+// that binary uses - the two are mutually exclusive, so there is never a collision or a
+// gap:
+//   * the hosted EE (coreclr.dll / static host) provides the enforcing definition
+//   * hostless and tool builds provide a no-op stub
+void ContractReleaseValidate();
+#endif // ENABLE_CONTRACTS_IMPL
+
 template <typename TYPE>
 struct ReleaseHolderTraits final
 {
     using Type = TYPE*;
     static constexpr Type Default() { return NULL; }
     static void Free(Type value)
     {
-        STATIC_CONTRACT_NOTHROW;
-        STATIC_CONTRACT_GC_TRIGGERS;
-        STATIC_CONTRACT_MODE_PREEMPTIVE;
+    STATIC_CONTRACT_NOTHROW;
+    STATIC_CONTRACT_GC_TRIGGERS;
+    STATIC_CONTRACT_MODE_PREEMPTIVE;
+
+#ifdef ENABLE_CONTRACTS_IMPL
+    ContractReleaseValidate();
+#endif // ENABLE_CONTRACTS_IMPL
 
         if (value != NULL)
             value->Release();

src/coreclr/inc/metadata.h

@@ -1234,6 +1234,7 @@ class HENUMInternalHolder
     {
         CONTRACTL {
             THROWS;
+            GC_NOTRIGGER;
         } CONTRACTL_END;
 
         _ASSERTE(!m_fAcquired);
@@ -1251,6 +1252,7 @@ class HENUMInternalHolder
     {
         CONTRACTL {
             THROWS;
+            GC_NOTRIGGER;
         } CONTRACTL_END;
 
         _ASSERTE(!m_fAcquired);
@@ -1267,6 +1269,7 @@ class HENUMInternalHolder
     {
         CONTRACTL {
             THROWS;
+            GC_NOTRIGGER;
         } CONTRACTL_END;
 
         _ASSERTE(!m_fAcquired);
@@ -1282,6 +1285,7 @@ class HENUMInternalHolder
     {
         CONTRACTL {
             THROWS;
+            GC_NOTRIGGER;
         } CONTRACTL_END;
 
         _ASSERTE(!m_fAcquired);

src/coreclr/inc/utilcode.h

@@ -3453,8 +3453,6 @@ namespace util
 
 INDEBUG(BOOL DbgIsExecutable(LPVOID lpMem, SIZE_T length);)
 
-BOOL IsIPInModule(PTR_VOID pModuleBaseAddress, PCODE ip);
-
 namespace UtilCode
 {
     // These are type-safe versions of Interlocked[Compare]Exchange

src/coreclr/nativeaot/Runtime/Crst.h

@@ -38,7 +38,6 @@ class CrstStatic
 {
 public:
     void Init(CrstType eType, CrstFlags eFlags = CRST_DEFAULT);
-    bool InitNoThrow(CrstType eType, CrstFlags eFlags = CRST_DEFAULT) { Init(eType, eFlags); return true; }
     void Destroy();
     void Enter() { CrstStatic::Enter(this); }
     void Leave() { CrstStatic::Leave(this); }

src/coreclr/nativeaot/Runtime/GcStressControl.cpp

@@ -66,7 +66,7 @@ class GcStressControl
         volatile InitState is = (InitState) PalInterlockedCompareExchange((volatile int32_t*)(&s_initState), isIniting, isNotInited);
         if (is == isNotInited)
         {
-            s_lock.InitNoThrow(CrstGcStressControl);
+            s_lock.Init(CrstGcStressControl);
 
             if (g_pRhConfig->GetGcStressSeed())
                 s_lGcStressRNGSeed = (uint32_t)g_pRhConfig->GetGcStressSeed();

src/coreclr/nativeaot/Runtime/clrgc.enabled.cpp

@@ -43,7 +43,7 @@ BOOL g_gcEventTracingInitialized = FALSE;
 
 void InitializeGCEventLock()
 {
-    g_eventStashLock.InitNoThrow(CrstGcEvent);
+    g_eventStashLock.Init(CrstGcEvent);
 }
 
 HRESULT InitializeStandaloneGC();

src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp

@@ -627,7 +627,7 @@ ep_rt_aot_spin_lock_alloc (ep_rt_spin_lock_handle_t *spin_lock)
     // EventPipe library will intialize using thread, EventPipeBufferManager instances and will maintain these on the EventPipe library side
 
     spin_lock->lock = new (nothrow) CrstStatic ();
-    spin_lock->lock->InitNoThrow (CrstType::CrstEventPipe);
+    spin_lock->lock->Init (CrstType::CrstEventPipe);
 }
 
 void
@@ -792,7 +792,7 @@ void ep_rt_aot_init (void)
     extern CrstStatic _ep_rt_aot_config_lock;
 
     _ep_rt_aot_config_lock_handle.lock = &_ep_rt_aot_config_lock;
-    _ep_rt_aot_config_lock_handle.lock->InitNoThrow (CrstType::CrstEventPipeConfig);
+    _ep_rt_aot_config_lock_handle.lock->Init (CrstType::CrstEventPipeConfig);
 }
 
 bool ep_rt_aot_lock_acquire (ep_rt_lock_handle_t *lock)

src/coreclr/utilcode/hostimpl.cpp

@@ -78,3 +78,14 @@ void CreateCrashDumpIfEnabled(bool stackoverflow)
 {
 }
 #endif
+
+#if defined(ENABLE_CONTRACTS_IMPL) && defined(SELF_NO_HOST)
+// Hostless counterpart to the other stubs in this file: with no EE there is nothing to
+// enforce, so the ReleaseHolder release-path contract hook (declared in holder.h) is a
+// no-op here.
+// See definition in holder.h for more details.
+void ContractReleaseValidate()
+{
+    LIMITED_METHOD_CONTRACT;
+}
+#endif // ENABLE_CONTRACTS_IMPL && SELF_NO_HOST

src/coreclr/utilcode/util.cpp

@@ -2324,99 +2324,6 @@ void PutRiscV64AuipcCombo(UINT32 * pCode, INT64 offset, bool isStype)
     _ASSERTE(GetRiscV64AuipcCombo(pCode, isStype) == offset);
 }
 
-//======================================================================
-// This function returns true, if it can determine that the instruction pointer
-// refers to a code address that belongs in the range of the given image.
-BOOL IsIPInModule(PTR_VOID pModuleBaseAddress, PCODE ip)
-{
-    STATIC_CONTRACT_LEAF;
-    SUPPORTS_DAC;
-
-    struct Param
-    {
-        PTR_VOID pModuleBaseAddress;
-        PCODE ip;
-        BOOL fRet;
-    } param;
-    param.pModuleBaseAddress = pModuleBaseAddress;
-    param.ip = ip;
-    param.fRet = FALSE;
-
-// UNIXTODO: implement a proper version for PAL
-#ifdef HOST_WINDOWS
-    PAL_TRY(Param *, pParam, &param)
-    {
-        PTR_BYTE pBase = dac_cast<PTR_BYTE>(pParam->pModuleBaseAddress);
-
-        PTR_IMAGE_DOS_HEADER pDOS = NULL;
-        PTR_IMAGE_NT_HEADERS pNT  = NULL;
-        USHORT cbOptHdr;
-        PCODE baseAddr;
-
-        //
-        // First, must validate the format of the PE headers to make sure that
-        // the fields we're interested in using exist in the image.
-        //
-
-        // Validate the DOS header.
-        pDOS = PTR_IMAGE_DOS_HEADER(pBase);
-        if (pDOS->e_magic != VAL16(IMAGE_DOS_SIGNATURE) ||
-            pDOS->e_lfanew == 0)
-        {
-            goto lDone;
-        }
-
-        // Validate the NT header
-        pNT = PTR_IMAGE_NT_HEADERS(pBase + VAL32(pDOS->e_lfanew));
-
-        if (pNT->Signature != VAL32(IMAGE_NT_SIGNATURE))
-        {
-            goto lDone;
-        }
-
-        // Validate that the optional header is large enough to contain the fields
-        // we're interested, namely IMAGE_OPTIONAL_HEADER::SizeOfImage. The reason
-        // we don't just check that SizeOfOptionalHeader == IMAGE_SIZEOF_NT_OPTIONAL_HEADER
-        // is due to VSW443590, which states that the extensibility of this structure
-        // is such that it is possible to include only a portion of the optional header.
-        cbOptHdr = pNT->FileHeader.SizeOfOptionalHeader;
-
-        // Check that the magic field is contained by the optional header and set to the correct value.
-        if (cbOptHdr < (offsetof(IMAGE_OPTIONAL_HEADER, Magic) + sizeofmember(IMAGE_OPTIONAL_HEADER, Magic)) ||
-            pNT->OptionalHeader.Magic != VAL16(IMAGE_NT_OPTIONAL_HDR_MAGIC))
-        {
-            goto lDone;
-        }
-
-        // Check that the SizeOfImage is contained by the optional header.
-        if (cbOptHdr < (offsetof(IMAGE_OPTIONAL_HEADER, SizeOfImage) + sizeofmember(IMAGE_OPTIONAL_HEADER, SizeOfImage)))
-        {
-            goto lDone;
-        }
-
-        //
-        // The real check
-        //
-
-        baseAddr = dac_cast<PCODE>(pBase);
-        if ((pParam->ip < baseAddr) || (pParam->ip >= (baseAddr + VAL32(pNT->OptionalHeader.SizeOfImage))))
-        {
-            goto lDone;
-        }
-
-        pParam->fRet = TRUE;
-
-lDone: ;
-    }
-    PAL_EXCEPT (EXCEPTION_EXECUTE_HANDLER)
-    {
-    }
-    PAL_ENDTRY
-#endif // HOST_WINDOWS
-
-    return param.fRet;
-}
-
 namespace Clr
 {
 namespace Util

src/coreclr/vm/common.h

@@ -56,16 +56,17 @@ using std::min;
 
 //-----------------------------------------------------------------------------------------------------------
 
-#include "stdmacros.h"
-
 #define POISONC ((UINT_PTR)((sizeof(int *) == 4)?0xCCCCCCCCL:0xCCCCCCCCCCCCCCCCLL))
 
 #include "switches.h"
+#include "eecontract.h"
 #include "holder.h"
 #include "classnames.h"
 #include "util.hpp"
 #include "corpriv.h"
 
+#include <stdmacros.h>
+
 #include <daccess.h>
 
 typedef VPTR(class LoaderAllocator)     PTR_LoaderAllocator;
@@ -210,7 +211,6 @@ namespace Loader
 #include "cgensys.h"
 #include "ceemain.h"
 #include "hash.h"
-#include "eecontract.h"
 #include "pedecoder.h"
 #include "sstring.h"
 #include "slist.h"

src/coreclr/vm/crst.cpp

@@ -27,11 +27,12 @@ Volatile<LONG> g_ShutdownCrstUsageCount = 0;
 //-----------------------------------------------------------------
 // Initialize critical section
 //-----------------------------------------------------------------
-VOID CrstBase::InitWorker(INDEBUG_COMMA(CrstType crstType) CrstFlags flags)
+void CrstBase::InitWorker(INDEBUG_COMMA(CrstType crstType) CrstFlags flags)
 {
-    CONTRACTL {
-        THROWS;
-        WRAPPER(GC_TRIGGERS);
+    CONTRACTL
+    {
+        NOTHROW;
+        GC_NOTRIGGER;
     } CONTRACTL_END;
 
     _ASSERTE((flags & CRST_INITIALIZED) == 0);

src/coreclr/vm/crst.h

@@ -265,8 +265,7 @@ class CrstBase
     }
 
 protected:
-
-    VOID InitWorker(INDEBUG_COMMA(CrstType crstType) CrstFlags flags);
+    void InitWorker(INDEBUG_COMMA(CrstType crstType) CrstFlags flags);
 
 #ifdef _DEBUG
     void DebugInit(CrstType crstType, CrstFlags flags);
@@ -453,7 +452,7 @@ typedef DPTR(Crst) PTR_Crst;
 class CrstStatic : public CrstBase
 {
 public:
-    VOID Init(CrstType crstType, CrstFlags flags = CRST_DEFAULT)
+    void Init(CrstType crstType, CrstFlags flags = CRST_DEFAULT)
     {
         WRAPPER_NO_CONTRACT;
 
@@ -462,30 +461,6 @@ class CrstStatic : public CrstBase
         // throw away the debug-only parameter in retail
         InitWorker(INDEBUG_COMMA(crstType) flags);
     }
-
-    bool InitNoThrow(CrstType crstType, CrstFlags flags = CRST_DEFAULT)
-    {
-        CONTRACTL {
-            NOTHROW;
-        } CONTRACTL_END;
-
-        _ASSERTE((flags & CRST_INITIALIZED) == 0);
-
-        bool fSuccess = false;
-
-        EX_TRY
-        {
-            // throw away the debug-only parameter in retail
-            InitWorker(INDEBUG_COMMA(crstType) flags);
-            fSuccess = true;
-        }
-        EX_CATCH
-        {
-        }
-        EX_END_CATCH
-
-        return fSuccess;
-    }
 };
 
 /* to be used as regular variable when a explicit call to Init method is needed */