Add debug assert to detect CRST_DEFAULT locks taken under ANYMODE locks#128912
Add debug assert to detect CRST_DEFAULT locks taken under ANYMODE locks#128912davidwrighton wants to merge 2 commits into
Conversation
Add a thread_local counter that tracks how many CRST_UNSAFE_ANYMODE (and SimpleRWLock COOPERATIVE_OR_PREEMPTIVE) locks are held on the current thread. When acquiring a CRST_DEFAULT lock or a PREEMPTIVE SimpleRWLock, assert that the counter is zero. Taking a GC_TRIGGERS lock while holding an ANYMODE lock is illegal because it may cause a deadlock: if another thread attempts to acquire the ANYMODE lock from cooperative mode while the DEFAULT lock is transitioning between GC modes, and a third thread triggers a GC, all three threads can deadlock. 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
This PR adds new debug-only invariants in CoreCLR’s VM locking infrastructure to detect a dangerous lock-ordering pattern: acquiring a GC_TRIGGERS-style lock (CRST default / PREEMPTIVE SimpleRWLock) while an ANYMODE-style lock is already held on the same thread.
Changes:
- Introduces a debug-only
thread_localper-thread counter to track “anymode/no-trigger” locks held. - Adds debug asserts in
CrstBase::Enter()andSimpleRWLock::EnterRead/EnterWrite()to fail fast when taking GC_TRIGGERS-style locks under those locks. - Updates
CrstBaseandSimpleRWLockenter/leave paths to increment/decrement the counter.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/coreclr/vm/simplerwlock.hpp | Declares the debug-only t_unsafeAnyModeHeldCount for SimpleRWLock codepaths. |
| src/coreclr/vm/simplerwlock.cpp | Adds PREEMPTIVE acquisition asserts and updates the counter for COOPERATIVE_OR_PREEMPTIVE lock enter/leave. |
| src/coreclr/vm/crst.h | Declares the debug-only t_unsafeAnyModeHeldCount for CRST codepaths. |
| src/coreclr/vm/crst.cpp | Defines the counter and adds the CRST default-lock assert plus counter maintenance for CRST_UNSAFE_ANYMODE locks. |
| if (m_dwFlags & CRST_UNSAFE_ANYMODE) | ||
| { | ||
| t_unsafeAnyModeHeldCount--; | ||
| } |
| if (m_gcMode == COOPERATIVE_OR_PREEMPTIVE) | ||
| { | ||
| t_unsafeAnyModeHeldCount--; | ||
| } |
| // Per-thread count of CRST_UNSAFE_ANYMODE (and equivalent) locks currently held. | ||
| // Used to detect illegal acquisition of GC_TRIGGERS locks under ANYMODE locks. | ||
| extern thread_local int t_unsafeAnyModeHeldCount; |
| _ASSERTE_MSG(t_unsafeAnyModeHeldCount == 0, | ||
| "Taking a PREEMPTIVE SimpleRWLock while a COOPERATIVE_OR_PREEMPTIVE lock or " | ||
| "CRST_UNSAFE_ANYMODE lock is held is illegal. " | ||
| "The PREEMPTIVE lock may trigger a GC, which is not allowed under ANYMODE locks."); |
| _ASSERTE_MSG(t_unsafeAnyModeHeldCount == 0, | ||
| "Taking a PREEMPTIVE SimpleRWLock while a COOPERATIVE_OR_PREEMPTIVE lock or " | ||
| "CRST_UNSAFE_ANYMODE lock is held is illegal. " | ||
| "The PREEMPTIVE lock may trigger a GC, which is not allowed under ANYMODE locks."); |
VSadov
left a comment
There was a problem hiding this comment.
LGTM.
I assume we do not need to worry about misbalanced use of these locks as that would be a bigger problem.
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "8bda26822fcd9df717820c969b412a786d7d2517",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "fd2cfc7f490abf5879639f488b335ab0263daa64",
"last_reviewed_commit": "8bda26822fcd9df717820c969b412a786d7d2517",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "fd2cfc7f490abf5879639f488b335ab0263daa64",
"last_recorded_worker_run_id": "29676149766",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "8bda26822fcd9df717820c969b412a786d7d2517",
"review_id": 4730529851
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: The PR adds a debug-only diagnostic to catch a real, hard-to-debug deadlock class: acquiring a CRST_DEFAULT/PREEMPTIVE (GC_TRIGGERS) lock while a CRST_UNSAFE_ANYMODE/COOPERATIVE_OR_PREEMPTIVE lock is held. The three-way deadlock scenario described in the PR is plausible and worth asserting against. The motivation is sound.
Approach: A thread_local int t_unsafeAnyModeHeldCount tracks how many ANYMODE-style locks the current thread holds. CrstBase::Enter asserts the count is zero before taking a GC-triggering lock (when none of CRST_UNSAFE_ANYMODE | CRST_UNSAFE_COOPGC | CRST_GC_NOTRIGGER_WHEN_TAKEN is set) and increments on CRST_UNSAFE_ANYMODE acquisition; Leave decrements. SimpleRWLock::PostEnter/PreLeave maintain the counter for COOPERATIVE_OR_PREEMPTIVE locks, and EnterRead/EnterWrite assert for PREEMPTIVE locks. This mirrors the existing m_countNoTriggerGC bookkeeping pattern. All logic is under #ifdef _DEBUG, so release builds are unaffected.
Summary: The change is small, correct, and low-risk. I verified balance and build-configuration safety:
- DAC safety:
t_unsafeAnyModeHeldCountis defined under#ifndef DACCESS_COMPILEincrst.cpp. Its references in the fullCrstBase::Enter/Leavelive in the#else // !DACCESS_COMPILEbranch (the DACEnterat the top of the file is an unrelated stub), andsimplerwlock.cppis compiled only into the WKS target (notVM_SOURCES_DAC). So the newthread_localis never referenced from a DAC translation unit, avoiding an undefined-symbol risk. - Counter balance: CRST increments in
Enterare paired with decrements inLeave;SimpleRWLockincrements inPostEnter(called only on the successful acquire pathsTryEnterRead/TryEnterWrite) are paired withPreLeaveonLeaveRead/LeaveWrite. ReentrantCRST_REENTRANCY | CRST_UNSAFE_ANYMODElocks increment/decrement per Enter/Leave, keeping the count correct while still (correctly) forbidding a default lock under such a lock. - False-positive scope: The exclusion mask correctly limits the assert to genuinely GC-triggering locks.
Verdict: LGTM. No actionable findings. This is a useful debug-only invariant with no release-build impact.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 161.9 AIC · ⌖ 10.6 AIC · ⊞ 10K
Note
This PR description was generated with the assistance of GitHub Copilot.
Summary
Adds a debug-only check that detects when a
CRST_DEFAULTlock (orPREEMPTIVESimpleRWLock) is acquired while aCRST_UNSAFE_ANYMODElock (orCOOPERATIVE_OR_PREEMPTIVESimpleRWLock) is already held on the same thread.Motivation
Taking a GC_TRIGGERS lock while holding an ANYMODE lock can cause a three-way deadlock:
All three threads can deadlock waiting on each other.
Changes
thread_local int t_unsafeAnyModeHeldCountcounter (debug-only) that tracks how many ANYMODE-style locks are held on the current thread.CrstBase::Enter()increments the counter when acquiring aCRST_UNSAFE_ANYMODElock and asserts it is zero when acquiring a default lock.CrstBase::Leave()decrements the counter when releasing aCRST_UNSAFE_ANYMODElock.SimpleRWLock::PostEnter()increments the counter forCOOPERATIVE_OR_PREEMPTIVElocks.SimpleRWLock::PreLeave()decrements the counter forCOOPERATIVE_OR_PREEMPTIVElocks.SimpleRWLock::EnterRead()andEnterWrite()assert the counter is zero forPREEMPTIVElocks.All changes are guarded by
#ifdef _DEBUGand have no effect on release builds.