Remove contract system support for RETURN/RETVAL/POSTCONDITION#126610
Remove contract system support for RETURN/RETVAL/POSTCONDITION#126610jkoritzinsky wants to merge 8 commits intodotnet:mainfrom
Conversation
- Convert all POSTCONDITION(expr) checks to _ASSERTE(expr) before return sites where the assertion is safe and meaningful - Remove POSTCONDITION/POSTCONDITION_MSG lines from all contract blocks - Convert CONTRACT(type) and CONTRACT_VOID to CONTRACTL - Convert CONTRACT_END to CONTRACTL_END - Convert RETURN macro to return keyword (all variants: RETURN expr;, RETURN;, RETURN(expr);, RETURN_VOID;, CONTRACT_RETURN, SS_RETURN) - Remove DISABLED(POSTCONDITION(...)) from contract blocks - Fix SHOULD_INJECT_FAULT(RETURN ...) patterns - Remove CheckPointer assertions that don't compile with DAC pointer types Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
Removes CoreCLR’s contract-system support for RETURN/RETVAL/POSTCONDITION and migrates affected code paths to use standard C++ return statements plus explicit _ASSERTE checks where postconditions previously existed.
Changes:
- Replaced
CONTRACT(...)/CONTRACT_VOID+CONTRACT_ENDpatterns withCONTRACTL+CONTRACTL_ENDacross VM/utilcode/debug components. - Replaced
RETURN(...)/RETURN;usages withreturn ...;/return;, and replaced meaningfulPOSTCONDITION(...)usages with explicit_ASSERTE(...)near return sites. - Updated contract header wiring (e.g.,
eecontract.h, DAC contract stubs) to remove the legacy macro hooks.
Reviewed changes
Copilot reviewed 118 out of 118 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/zapsig.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/virtualcallstub.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/typestring.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/threadsuspend.cpp | Replace POSTCONDITION with _ASSERTE near returns. |
| src/coreclr/vm/syncblk.h | Replace RETVAL postconditions with explicit asserts. |
| src/coreclr/vm/syncblk.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/stublink.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/stubhelpers.cpp | Normalize contract terminator to CONTRACTL_END. |
| src/coreclr/vm/stubcache.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/stackingallocator.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/stackingallocator.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/siginfo.cpp | Replace RETVAL postconditions with explicit asserts + return. |
| src/coreclr/vm/runtimecallablewrapper.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/riscv64/stubs.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/reflectclasswriter.cpp | Replace RETVAL postconditions with explicit asserts + return. |
| src/coreclr/vm/readytoruninfo.cpp | Replace RETVAL postconditions with explicit asserts + return. |
| src/coreclr/vm/proftoeeinterfaceimpl.cpp | Replace contract postconditions with explicit runtime assertions. |
| src/coreclr/vm/prestub.cpp | Replace POSTCONDITION with _ASSERTE near returns. |
| src/coreclr/vm/peimagelayout.inl | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/peimagelayout.cpp | Remove constructor-check contract usage. |
| src/coreclr/vm/peimage.inl | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/peimage.cpp | Replace postconditions with asserts; convert RETURN to return. |
| src/coreclr/vm/peassembly.inl | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/peassembly.cpp | Replace postconditions with asserts; convert RETURN to return. |
| src/coreclr/vm/olevariant.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/olecontexthelpers.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/nativelibrary.cpp | Replace RETVAL postcondition with explicit assert + return. |
| src/coreclr/vm/nativeimage.cpp | Remove constructor-check contract usage. |
| src/coreclr/vm/mlinfo.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/methodtablebuilder.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/methodtable.inl | Remove CheckPointer assert tied to contract usage. |
| src/coreclr/vm/methodtable.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/method.cpp | Replace RETVAL postconditions with explicit asserts + return. |
| src/coreclr/vm/memberload.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/managedmdimport.cpp | Replace RETVAL postconditions with explicit asserts + return. |
| src/coreclr/vm/loongarch64/stubs.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/loaderallocator.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/jitinterface.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/invokeutil.cpp | Replace RETVAL postconditions with explicit asserts + return. |
| src/coreclr/vm/interoputil.inl | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/interoputil.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/interoplibinterface_shared.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/interoplibinterface_objc.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/interopconverter.cpp | Replace RETVAL postconditions with explicit asserts + return. |
| src/coreclr/vm/ilstubresolver.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/ilstubcache.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/ilmarshalers.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/i386/stublinkerx86.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/i386/cgenx86.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/generics.cpp | Replace RETVAL postconditions with explicit asserts + return. |
| src/coreclr/vm/genericdict.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/fieldmarshaler.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/fieldmarshaler.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/field.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/eedbginterfaceimpl.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/eecontract.h | Remove legacy CONTRACT/CONTRACT_VOID EE overrides; keep CONTRACTL. |
| src/coreclr/vm/eeconfig.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/dynamicmethod.cpp | Replace postconditions with explicit asserts + return. |
| src/coreclr/vm/dynamicinterfacecastable.cpp | Replace postconditions with explicit asserts + return. |
| src/coreclr/vm/domainassembly.cpp | Remove constructor-check contract usage. |
| src/coreclr/vm/dllimportcallback.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/dllimportcallback.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/dispatchinfo.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/comtoclrcall.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/comtoclrcall.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/commtmemberinfomap.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/commtmemberinfomap.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/comdelegate.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/comconnectionpoints.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/comconnectionpoints.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/comcache.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/codeman.cpp | Replace postconditions with explicit asserts + return. |
| src/coreclr/vm/clrex.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/classhash.cpp | Replace postconditions with explicit asserts + return. |
| src/coreclr/vm/class.cpp | Replace postconditions with explicit asserts + return. |
| src/coreclr/vm/cachelinealloc.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/assemblyspec.hpp | Remove constructor-check contract usage. |
| src/coreclr/vm/assemblynative.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/arraynative.inl | Remove CheckPointer asserts tied to contract usage. |
| src/coreclr/vm/arraynative.cpp | Remove CheckPointer asserts tied to contract usage. |
| src/coreclr/vm/arm64/stubs.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/arm/stubs.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/vm/amd64/cgenamd64.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/utilcode/webcildecoder.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/utilcode/sstring_com.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/utilcode/sbuffer.cpp | Replace postconditions with explicit asserts + return. |
| src/coreclr/utilcode/loaderheap.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/utilcode/interleavedloaderheap.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/utilcode/explicitcontrolloaderheap.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/utilcode/clrconfig.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/inc/cordecoderhelpers.h | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/gc/env/gcenv.base.h | Remove legacy contract macro stubs no longer supported. |
| src/coreclr/debug/inc/dacdbistructures.inl | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/debug/ee/rcthread.cpp | Replace postconditions with explicit asserts + return. |
| src/coreclr/debug/ee/functioninfo.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| src/coreclr/debug/ee/debugger.cpp | Replace postconditions with explicit asserts + return. |
| src/coreclr/debug/ee/controller.cpp | Replace postconditions with explicit asserts + return. |
| src/coreclr/debug/daccess/daccess.cpp | Convert contract/RETURN usage to CONTRACTL/return. |
| _ASSERTE(pImage->m_pLayouts[IMAGE_FLAT] != NULL); | ||
| RETURN dac_cast<PTR_PEImage>(pImage.Extract()); | ||
| _ASSERTE(dac_cast<PTR_PEImage>(pImage.Extract())->HasLoadedLayout()); | ||
| return dac_cast<PTR_PEImage>(pImage.Extract()); | ||
| } |
There was a problem hiding this comment.
PEImageHolder::Extract() transfers ownership; calling it twice here means the second Extract() will likely return null / invalid, and the first call may already have detached the object. Capture the extracted PTR_PEImage into a local once, assert on that, then return it.
| #ifdef PROFILING_SUPPORTED | ||
| PRECONDITION(InterlockedIncrement(&s_generationTableWriterCount) == 1); | ||
| POSTCONDITION(InterlockedDecrement(&s_generationTableWriterCount) == 0); | ||
| #endif // PROFILING_SUPPORTED | ||
| } CONTRACT_END; | ||
| } CONTRACTL_END; |
There was a problem hiding this comment.
UpdateGenerationBounds uses PRECONDITION(InterlockedIncrement(&s_generationTableWriterCount) == 1) when contracts are enabled, but the corresponding decrement is now only done under #ifdef _DEBUG. In non-_DEBUG builds that still enable contracts (e.g., Checked), the counter will never be decremented and the single-writer assertion will fail on subsequent calls. Ensure the decrement runs whenever ENABLE_CONTRACTS is defined (and on all exit paths).
src/coreclr/debug/ee/debugger.cpp
Outdated
| if (pModules == NULL) | ||
| { | ||
| RETURN (E_OUTOFMEMORY); | ||
| _ASSERTE(m_pModules != NULL); |
There was a problem hiding this comment.
Debugger::CheckInitModuleTable: in the OOM path (pModules == NULL) the code now asserts m_pModules != NULL before returning E_OUTOFMEMORY. That assertion is not expected to hold on failure and will fire during OOM handling; it should be removed or made conditional on success.
| _ASSERTE(m_pModules != NULL); |
| if (s_currentGenerationTable == nullptr) | ||
| { | ||
| RETURN; | ||
| #ifdef _DEBUG | ||
| LONG result = InterlockedDecrement(&s_generationTableWriterCount); | ||
| _ASSERTE(result == 0); |
There was a problem hiding this comment.
The s_currentGenerationTable == nullptr early-return path decrements s_generationTableWriterCount only under #ifdef _DEBUG, but the increment (via contracts) can still occur in non-_DEBUG builds with ENABLE_CONTRACTS. Mirror the decrement under #ifdef ENABLE_CONTRACTS here too so the writer count is restored even on early return.
Note
This PR description was created with GitHub Copilot assistance.
The
POSTCONDITION(RETVAL)support in the CoreCLR VM required us to use extremely obtuse compiler tricks to enableRETURN xyz;to work likereturn xyz;in the presence of contracts.Very few of these post-conditions were actually interesting (most were checking for non-null pointers, may checking for always true conditions).
This PR removes POSTCONDITION support and
RETURN/RETVALsupport. For cases where the postconditions were actually interesting, the asserts are preserved and inserted before the return statements.