Enable contracts for new holders and refactor Crst initialization

[AaronRobinsonMSFT]

Summary

Enables EE contracts for the holder infrastructure and refactors related lock initialization so that a single, consistent contract vocabulary is used across the VM, GC, and NativeAOT runtimes. Contracts are additionally hardened so the hosted contract machinery is only enabled when code is compiled into or linked against coreclr or the static host.

Details

Contract enablement and host gating (inc/contract.h)

• Simplify the macros that gate ENABLE_CONTRACTS_DATA / ENABLE_CONTRACTS / ENABLE_CONTRACTS_IMPL. Contract data no longer depends on JIT_BUILD / CROSS_COMPILE.
• ENABLE_CONTRACTS is now additionally gated on !DBI_COMPILE && !SELF_NO_HOST (alongside the existing !DACCESS_COMPILE), so the hosted contract implementation is only enabled when code is compiled into or linked against coreclr or the static host. DAC/DBI builds continue to reference contract data but not the implementation.
• Move the EEContract vocabulary (GC_TRIGGERS, MODE_COOPERATIVE / MODE_PREEMPTIVE / MODE_ANY, etc.) directly into contract.h and delete vm/eecontract.h; the separate ENABLE_EE_CONTRACTS define is removed.

Holders (inc/holder.h)

• ReleaseHolderTraits::Free now uses a full CONTRACTL block (NOTHROW / GC_TRIGGERS / MODE_PREEMPTIVE) when the EE contract vocabulary is available, falling back to the static contracts otherwise.
• The release-path contract validation is factored out of line into ContractReleaseValidate() (declared in holder.h), with a hosted implementation in vm/util.cpp and a hostless no-op stub in utilcode/hostimpl.cpp.

Standalone IL tools (ilasm / ildasm)

Make the hostless ilasm / ildasm tool chain consistently SELF_NO_HOST so it no longer requires the hosted contract implementation at link time:

• Add SELF_NO_HOST to the tool-only _ppdb metadata libraries (mdcompiler_ppdb, mdruntime_ppdb, mdruntimerw_ppdb) and to mscorpe.
• Add a ceefgen_nohost flavor for ilasm; coreclr keeps the contract-enabled ceefgen.
• Switch ildasm from the contract-enabled _wks metadata flavor to the hostless _ppdb flavor.

Crst initialization

• Remove CrstStatic::InitNoThrow and make Init (and CrstBase::InitWorker) NOTHROW / GC_NOTRIGGER. All callers in the VM, GC, and NativeAOT are updated to call Init. gcenv.sync.h and NativeAOT Crst.h drop their InitNoThrow shims accordingly.

IsIPInModule

• Move the implementation out of the generic utilcode/util.cpp (which cannot depend on EE contracts) into vm/util.cpp, declare it in vm/util.hpp, and expose it to the debugger through the EE debugger interface (IEEDbgInterface::IsIPInModule) instead of the free function.

Metadata (inc/metadata.h)

• Add GC_NOTRIGGER to several lock-acquire contracts.

Include ordering (vm/common.h)

• Pull in contract.h before holder.h so the EE contract vocabulary is visible to the holder definitions.

Note

The legacy fault-contract annotation removal (INJECT_FAULT, FORBID_FAULT / STATIC_CONTRACT_FORBID_FAULT, FAULT_NOT_FATAL, and the FaultViolation / FaultNotFatal machinery) has been split into a separate follow-up PR to keep this change focused; it will be opened after this PR merges.

Note

This PR description was generated with the assistance of GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR updates CoreCLR’s contracts and lock initialization patterns to use a more consistent EE contract vocabulary across VM/GC/NativeAOT, and relocates IsIPInModule into the VM while routing debugger usage through EEDebugInterface.

Changes:

• Refactors CrstStatic initialization to remove InitNoThrow and update call sites across VM/GC/NativeAOT to call Init.
• Introduces ENABLE_EE_CONTRACTS (when contracts implementation is enabled) and uses it to upgrade holder contracts in holder.h.
• Moves IsIPInModule from utilcode/util.cpp into vm/util.cpp, adds a VM declaration, and exposes it via EEDebugInterface.

Show a summary per file

File	Description
src/coreclr/vm/util.hpp	Declares VM-level IsIPInModule.
src/coreclr/vm/util.cpp	Adds VM implementation of IsIPInModule.
src/coreclr/vm/eventtrace.cpp	Switches CrstStatic init from InitNoThrow to Init.
src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h	Switches EventPipe lock init to Init.
src/coreclr/vm/eedbginterfaceimpl.h	Adds IsIPInModule to the EE debugger interface implementation.
src/coreclr/vm/eedbginterfaceimpl.cpp	Implements EEDbgInterfaceImpl::IsIPInModule forwarding to the helper.
src/coreclr/vm/eedbginterface.h	Adds IsIPInModule to EEDebugInterface.
src/coreclr/vm/eecontract.h	Introduces ENABLE_EE_CONTRACTS under ENABLE_CONTRACTS_IMPL.
src/coreclr/vm/crst.h	Removes CrstStatic::InitNoThrow and standardizes on Init.
src/coreclr/vm/crst.cpp	Changes CrstBase::InitWorker contract and initialization behavior.
src/coreclr/vm/common.h	Adjusts include ordering so EE contract vocabulary is visible to holders.
src/coreclr/utilcode/util.cpp	Removes the old IsIPInModule implementation from utilcode.
src/coreclr/nativeaot/Runtime/GcStressControl.cpp	Updates NativeAOT lock init to Init.
src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp	Updates NativeAOT EventPipe locks to Init.
src/coreclr/nativeaot/Runtime/Crst.h	Removes NativeAOT InitNoThrow shim.
src/coreclr/nativeaot/Runtime/clrgc.enabled.cpp	Updates GC event lock init to Init.
src/coreclr/inc/utilcode.h	Removes utilcode declaration of IsIPInModule.
src/coreclr/inc/metadata.h	Adds GC_NOTRIGGER to several lock-acquire contract blocks.
src/coreclr/inc/holder.h	Uses full CONTRACTL under ENABLE_EE_CONTRACTS for ReleaseHolderTraits::Free.
src/coreclr/inc/contract.h	Simplifies gating for ENABLE_CONTRACTS_DATA / ENABLE_CONTRACTS (DAC excluded from impl).
src/coreclr/gc/handletable.cpp	Switches handle table lock init to Init and removes failure cleanup path.
src/coreclr/gc/env/gcenv.sync.h	Changes GC env shim CrstStatic init API to Init and drops status return.
src/coreclr/debug/ee/debugger.cpp	Routes debugger’s module-IP check through g_pEEInterface->IsIPInModule.

Copilot's findings

Comments suppressed due to low confidence (1)

src/coreclr/vm/crst.cpp:42

• CrstBase::InitWorker relies on _ASSERTE(suc) after minipal_mutex_init. In retail builds _ASSERTE compiles away, so a failed mutex init would still mark the Crst initialized and proceed with an uninitialized m_lock, leading to undefined behavior later. This needs an all-build failure path (fail-fast) rather than a debug-only assert.

    _ASSERTE((flags & CRST_INITIALIZED) == 0);

    bool suc = minipal_mutex_init(&m_lock._mtx);
    _ASSERTE(suc);

• Files reviewed: 23/23 changed files
• Comments generated: 1

[azure-pipelines]

Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

Copilot's findings

Suppressed comments (4)

src/coreclr/vm/crst.cpp:42

• CrstBase::InitWorker ignores minipal_mutex_init failure in retail builds (only _ASSERTE checks it). If initialization fails (e.g., OOM), we proceed with an uninitialized mutex, which can lead to undefined behavior later. This is especially problematic now that callers no longer have an InitNoThrow fallback path.

    _ASSERTE((flags & CRST_INITIALIZED) == 0);

    bool suc = minipal_mutex_init(&m_lock._mtx);
    _ASSERTE(suc);

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

• CrstStatic::Init now discards the CLRCriticalSection::Initialize() return value, removing even the prior ability to detect initialization failure in debug builds. If Initialize() can fail, this can leave the GC lock in an unusable state with no signal to callers.

    void Init(CrstType eType, CrstFlags eFlags = CRST_DEFAULT)
    {
        (void)m_cs.Initialize();
    }

src/coreclr/inc/holder.h:1019

• The body of ReleaseHolderTraits::Free has inconsistent indentation (contract lines and ContractReleaseValidate() are not indented to match the rest of the function body). This makes the block harder to read and stands out from the surrounding style in holder.h.

    {
    STATIC_CONTRACT_NOTHROW;
    STATIC_CONTRACT_GC_TRIGGERS;
    STATIC_CONTRACT_MODE_PREEMPTIVE;

#ifdef ENABLE_CONTRACTS_IMPL
    ContractReleaseValidate();
#endif // ENABLE_CONTRACTS_IMPL

src/coreclr/vm/eedbginterface.h:136

• New virtual methods on these VM<->debugger interfaces should be appended at the end to preserve vtable slot ordering (per established CoreCLR convention). IsIPInModule is currently inserted in the middle of the interface, which increases ABI risk for components that may be built against a different header version.

    virtual BOOL IsManagedNativeCode(const BYTE *address) = 0;

#endif // #ifndef DACCESS_COMPILE

    virtual BOOL IsIPInModule(PTR_VOID pModuleBaseAddress, PCODE ip) = 0;

    virtual PCODE GetNativeCodeStartAddress(PCODE address) = 0;

    virtual MethodDesc *GetNativeCodeMethodDesc(const PCODE address) = 0;

• Files reviewed: 24/24 changed files
• Comments generated: 0 new

[jkotas]

Remove legacy fault contract annotations

Can this be in a separate PR? It makes it impossible to review the actual changes

[Copilot]

Copilot's findings

Suppressed comments (4)

src/coreclr/inc/holder.h:1018

• The ContractReleaseValidate(); call inside ReleaseHolderTraits::Free is indented less than the rest of the function body, which looks accidental and makes the new contract hook easy to miss.

    ContractReleaseValidate();

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

• CrstStatic::Init ignores the bool return from CLRCriticalSection::Initialize() (which wraps minipal_mutex_init). If initialization can fail (e.g., pthread attr/mutex init failures), this will silently leave an uninitialized lock, leading to undefined behavior on first use. Consider either fail-fast here or keep an init API that propagates failure to callers.

    void Init(CrstType eType, CrstFlags eFlags = CRST_DEFAULT)
    {
        (void)m_cs.Initialize();
    }

src/coreclr/inc/holder.h:1015

• ReleaseHolderTraits::Free's contract lines are no longer indented to match the surrounding code block, making the new hook harder to read and inconsistent with the rest of holder.h.

This issue also appears on line 1018 of the same file.

    STATIC_CONTRACT_NOTHROW;
    STATIC_CONTRACT_GC_TRIGGERS;
    STATIC_CONTRACT_MODE_PREEMPTIVE;

src/coreclr/vm/crst.cpp:36

• CrstBase::InitWorker is now annotated as NOTHROW / GC_NOTRIGGER, but the implementation calls minipal_mutex_init (which can fail on Unix and can raise on Windows via InitializeCriticalSection). Unless the body is hardened to never throw/fail, these contracts are likely inaccurate and can cause contract asserts or hide real failure behavior.

• Files reviewed: 201/202 changed files
• Comments generated: 0 new

[Copilot]

Review details

• Files reviewed: 61/65 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (2)

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

• CLRCriticalSection::Initialize() returns false on failure, but the result is ignored here. In release builds that means the lock can be used uninitialized, leading to undefined behavior. Please either check the return value and fail-fast, or keep a boolean return from Init so callers can unwind/propagate failure.

    void Init(CrstType eType, CrstFlags eFlags = CRST_DEFAULT)
    {
        (void)m_cs.Initialize();
    }

src/coreclr/vm/crst.cpp:36

• This method’s contract was tightened to NOTHROW/GC_NOTRIGGER, but the implementation calls minipal_mutex_init (later in the method) which can fail (returns false on Unix). In non-debug builds the current _ASSERTE-only check is compiled out, so we can mark the Crst initialized and proceed with an uninitialized mutex. Please handle initialization failure (e.g., fail-fast) before setting CRST_INITIALIZED.

• Files reviewed: 60/64 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (2)

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

• CrstStatic::Init now discards CLRCriticalSection::Initialize()'s bool return value. Initialize() can fail (it wraps minipal_mutex_init), and ignoring the result can leave the mutex uninitialized, leading to undefined behavior on the first Enter(). If the design goal is that Init is NOTHROW and non-failable, it should fail-fast on initialization failure (or alternatively keep a bool-returning InitNoThrow and preserve the existing caller cleanup paths).

    void Init(CrstType eType, CrstFlags eFlags = CRST_DEFAULT)
    {
        (void)m_cs.Initialize();
    }

src/coreclr/vm/reflectclasswriter.cpp:24

• The contract for RefClassWriter::Init was broadened from NOTHROW/GC_NOTRIGGER/MODE_ANY to STANDARD_VM_CHECK (THROWS/GC_TRIGGERS/MODE_PREEMPTIVE). This weakens contract accuracy and changes the documented GC-mode requirement, which can hide contract violations in callers and can also trip debug contract checks if this is ever invoked in cooperative mode (the previous comment explicitly called out MODE_ANY as intentional). Prefer keeping the precise contract here rather than using STANDARD_VM_CHECK.

• Files reviewed: 60/64 changed files
• Comments generated: 0 new
• Review effort level: Lite

[jkotas]

LGTM!

[Copilot]

Review details

Suppressed comments (2)

src/coreclr/vm/crst.cpp:42

• CrstBase::InitWorker is now declared NOTHROW/GC_NOTRIGGER, but the underlying lock initialization can fail: on Windows, minipal_mutex_init calls InitializeCriticalSection (which may raise on OOM), and on Unix it returns false on pthread init failure. Currently the failure is only asserted in debug and then ignored in release builds, which can lead to using an uninitialized mutex.

Consider converting init failures into a deterministic fail-fast (or restoring a throwing/checked path) so callers don't proceed with a partially-initialized Crst.
src/coreclr/nativeaot/Runtime/eventpipe/ep-rt-aot.cpp:631

• ep_rt_aot_spin_lock_alloc uses new (nothrow) but then unconditionally dereferences the result. If allocation fails, this will null-deref immediately and later acquire/release paths also assume spin_lock->lock is non-null.

Either allocate with a throwing/fail-fast allocator, or explicitly fail fast on null here to keep the failure mode deterministic.

    spin_lock->lock = new (nothrow) CrstStatic ();
    spin_lock->lock->Init (CrstType::CrstEventPipe);
}

• Files reviewed: 60/64 changed files
• Comments generated: 0 new
• Review effort level: Lite

[jkotas]

Conflict

[Copilot]

Review details

Suppressed comments (3)

src/coreclr/vm/crst.cpp:36

• CrstBase::InitWorker is now annotated NOTHROW/GC_NOTRIGGER, but it calls minipal_mutex_init() which can fail (returns false on pthread path; and InitializeCriticalSection can raise on Windows). The current code only _ASSERTE(suc), which compiles out in retail and can leave m_lock uninitialized while still marking the Crst initialized.
  src/coreclr/gc/env/gcenv.sync.h:32
• CrstStatic::Init discards the return value from CLRCriticalSection::Initialize(), which can fail. In retail builds this can result in using an uninitialized mutex with no diagnostic. Consider keeping the boolean return (as before) so callers can decide how to handle init failure (or at least assert/fail-fast).

    void Init(CrstType eType, CrstFlags eFlags = CRST_DEFAULT)
    {
        (void)m_cs.Initialize();
    }

src/coreclr/vm/eedbginterface.h:136

• EEDebugInterface is a cross-component C++ vtable interface. Adding a new virtual method in the middle changes vtable slot indices for all following methods; the established pattern in the runtime is to append new interface methods at the end to preserve slot ordering and reduce risk. Please move IsIPInModule to the end of the interface (and update EEDbgInterfaceImpl accordingly).

• Files reviewed: 60/64 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (4)

src/coreclr/vm/crst.cpp:36

• InitWorker is now annotated NOTHROW/GC_NOTRIGGER, but it calls minipal_mutex_init: on Unix it can return false, and on Windows it can raise an exception (InitializeCriticalSection). The current _ASSERTE-based handling is debug-only, so retail builds could proceed with an uninitialized mutex, and the NOTHROW contract can be violated.
  src/coreclr/gc/env/gcenv.sync.h:32
• CrstStatic::Init now discards CLRCriticalSection::Initialize()'s return value. On Unix, Initialize can fail (minipal_mutex_init returns false), and ignoring it can leave the lock uninitialized in retail builds, leading to undefined behavior later. Consider restoring a failure signal (e.g., return bool) or fail-fast here with a mechanism available at this layer.

    void Init(CrstType eType, CrstFlags eFlags = CRST_DEFAULT)
    {
        (void)m_cs.Initialize();
    }

src/coreclr/vm/eedbginterface.h:134

• New virtual methods on runtime/EE interfaces should be appended at the end to preserve vtable slot ordering. Adding IsIPInModule here (mid-interface) shifts all subsequent slots and risks ABI breaks for any separately-built consumers.
  src/coreclr/utilcode/hostimpl.cpp:91
• This hostless ContractReleaseValidate stub is guarded by ENABLE_CONTRACTS_IMPL && SELF_NO_HOST, but contract.h only defines ENABLE_CONTRACTS_IMPL when SELF_NO_HOST is not defined. As written, this block is effectively dead and makes it harder to reason about which binaries are expected to provide the symbol.

#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

• Files reviewed: 61/65 changed files
• Comments generated: 0 new
• Review effort level: Lite

[jkotas]

What is the failure fixed by the last commit? The lists of CI failures before/after last commit look the same.

[AaronRobinsonMSFT]

What is the failure fixed by the last commit? The lists of CI failures before/after last commit look the same.

It is a contract violation in a different part. The profiler area is like playing whack-a-mole. I need to fully debug it tomorrow. I pushed the revert of the contracts in this one because it triggers at a place I'd like to avoid fixing for now. I'll see all the places this uncovers tomorrow and see if it is harder or easier.