Remove contract system support for RETURN/RETVAL/POSTCONDITION#126625
Remove contract system support for RETURN/RETVAL/POSTCONDITION#126625jkoritzinsky wants to merge 18 commits intomainfrom
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>
…ements, fix indentation and trailing newlines
|
Tagging subscribers to this area: @agocke |
🤖 Copilot Code Review — PR #126625Note This review was generated by GitHub Copilot. Holistic AssessmentMotivation: Justified and overdue cleanup. The Approach: The approach is sound — mechanical bulk conversion followed by targeted manual adjustments to preserve meaningful assertions as Summary: Detailed Findings❌ Correctness — Missing braces in
|
src/coreclr/debug/ee/debugger.cpp
Outdated
| if (pModules == NULL) | ||
| { | ||
| RETURN (E_OUTOFMEMORY); | ||
| return (E_OUTOFMEMORY); |
There was a problem hiding this comment.
@copilot Delete unnecessary brackets here and other similar places changed in this PR
There was a problem hiding this comment.
Fixed in 8b0c137. Removed unnecessary brackets from all return (X) statements (where X is not a C-style cast or operator-precedence expression) throughout all files changed in this PR.
src/coreclr/debug/ee/rcthread.cpp
Outdated
| ThrowLastError(); | ||
|
|
||
| RETURN h; | ||
| _ASSERTE(h != NULL); |
There was a problem hiding this comment.
| _ASSERTE(h != NULL); |
This is trivially guaranteed by the if check on previous line. @copilot Check for similar patterns throughout the code changed in this PR
There was a problem hiding this comment.
Fixed in 8b0c137. Removed _ASSERTE(h != NULL) here, and also removed a similar trivially-guaranteed _ASSERTE(hmod != NULL) in nativelibrary.cpp (where if (hmod == NULL) block always throws before reaching it).
| } | ||
| m_nEntries = 0; | ||
| RETURN; | ||
| return; |
There was a problem hiding this comment.
| return; |
Unnecessary return . @copilot Check for similar pattern throughout the code changed in this PR
There was a problem hiding this comment.
Fixed in 8b0c137. Removed return; at end of void functions throughout all changed files (sbuffer.inl, sstring.inl, shash.inl, and many .cpp files).
|
@copilot Fix build breaks [462/1183] Building CXX object utilcode/CMakeFiles/utilcodestaticnohost.dir/clrhost_nodependencies.cpp.o |
…d asserts, and unnecessary return; at end of void functions Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/bab73c63-7f58-4362-b57c-19a42f3b4860 Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
|
@copilot Build breaks |
…ointer) which require contracts to be enabled CheckInvariant is only available when CHECK_INVARIANTS is defined (requires _DEBUG && !DISABLE_CONTRACTS). CheckPointer is only available when CHECK_INVARIANTS or _DEBUG_IMPL is defined (not in DACCESS_COMPILE builds). These were originally in POSTCONDITION macros that are no-ops when contracts are disabled, so simply removing them is correct. Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/c57940e4-c2a9-4f04-8027-58f3bdf8a540 Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1675a825-640c-47a2-b0b4-47ccd34079ea Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Fixed in 398f4d6. Converted the two remaining |
|
@copilot still seeing the following for the CoreMangLib test suite (amongst others): Child process took too long. Timed out... Exiting... |
…d syncblk.cpp, remove incorrect assert in threadsuspend.cpp Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/1e6beb65-563c-4c69-8031-825e1e864c43 Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
Fixed in 360b3da. The root cause was in Also restored the two missing |
@copilot Are you sure that the ThreadStore lock is explicitly released before that point? What is the codepath that ends up releasing it? |
|
@copilot Merge from main and resolve conflicts |
…se in SysSweepThreadsForDebug; fix misleading comment Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/f3d09590-9cc6-40aa-afae-a8ee3e19268c Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
You're right to question this — my previous reasoning was incorrect. The misleading comment "We release the threadstore lock and return false" was incorrect — the ThreadStore lock is never released in this code path. The original Restored the assertion and fixed the comment in 3d1150c. |
|
@copilot Merge from main and resolve conflicts |
…return # Conflicts: # src/coreclr/vm/assemblyspec.hpp Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Removes the
POSTCONDITION/RETURN/RETVALmacro infrastructure from CoreCLR. ThePseudoTemplate/Returner/Box__USE_PARENS_WITH_THIS_EXPRESSIONtemplate machinery required to makeRETURN xyzwork insideCONTRACT(T)blocks added significant complexity for little value — most postconditions were tautological or duplicated preconditions.Description
POSTCONDITION/RETURN/RETVALinfrastructure fromcontract.handeecontract.h; converted allCONTRACT/CONTRACT_VOIDblocks toCONTRACTLRETURN expr→return expracross ~100 VM/debug/utilcode files; non-trivial postconditions preserved as_ASSERTE(...)before the correspondingreturn_ASSERTE(CheckInvariant(*this))—CheckInvariantrequiresCHECK_INVARIANTS(_DEBUG && !DISABLE_CONTRACTS)_ASSERTE(CheckPointer(...))— not available inDACCESS_COMPILEbuildsStorePEAssembly: splittingRETURN TRUEinto_ASSERTE(...); return TRUE;without braces caused unconditional return, breaking assembly binding cache and causing test timeouts (exit code 110)ceeload.cpp,comconnectionpoints.h,olevariant.cpp,peimage.cpp(Holder::Extract()+SuppressRelease()→LifetimeHolder::Detach())return (expr)parens, trailingreturn;in void functions, trivially-guaranteed null asserts, and a duplicate_ASSERTE(Check())/CONSISTENCY_CHECK(Check())pair insbuffer.inl