[release/10.0] Use volatile access for CoreCLR dictionary layout publication#131099
Conversation
Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
Co-authored-by: davidwrighton <10779849+davidwrighton@users.noreply.github.com>
|
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. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "823784df18c8fda82645e4dcf753b1e56407b0ef",
"last_dispatched_base_ref": "release/10.0",
"last_dispatched_base_sha": "b09bb32a9529f922aaef13e2629ab1cbac68ac10",
"last_reviewed_commit": "823784df18c8fda82645e4dcf753b1e56407b0ef",
"last_reviewed_base_ref": "release/10.0",
"last_reviewed_base_sha": "b09bb32a9529f922aaef13e2629ab1cbac68ac10",
"last_recorded_worker_run_id": "29770120765",
"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": "823784df18c8fda82645e4dcf753b1e56407b0ef",
"review_id": 4738149685
}
]
} |
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Holistic Review
Motivation: This is a backport of #129763 to release/10.0. It addresses a rare CoreCLR crash in generic dictionary/method resolution caused by racing publication of dictionary layout pointers (m_pDictLayout). Making publication and observation of those pointers use volatile semantics ensures layout expansion becomes visible to concurrent readers with proper release/acquire ordering.
Approach: The change is intentionally narrow. It converts the m_pDictLayout accessors to volatile access: EEClass::GetDictionaryLayout/SetDictionaryLayout in class.h, and InstantiatedMethodDesc::GetDictLayoutRaw/IMD_GetDictionaryLayout/IMD_SetDictionaryLayout in method.hpp — using VolatileLoad/VolatileStore. No other behavior is altered.
Summary: This is a faithful, minimal cherry-pick of the merged fix. The diff (+7/-7 across two headers) matches the description of the original PR, and all published read/write paths for m_pDictLayout now participate in the same volatile publication protocol. I verified that the only remaining non-volatile write is genmeth.cpp:1494 (m_pDictLayout = pDL;), which occurs during InstantiatedMethodDesc construction before the object is published, so it does not need volatile semantics — consistent with the upstream change. This is a low-risk, well-scoped servicing backport with no actionable findings.
Detailed Findings
None. The change is a correct and complete backport.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 46.9 AIC · ⌖ 15.9 AIC · ⊞ 10K
Backport of #129763 to release/10.0
/cc @VSadov @copilot
There were multiple changes in net10 in threadstatic area that introduced subtle memory ordering issues.
The issue has been fixed in net11, but can still happen on net10.
Customer Impact
#130551
Customer reports intermittent crashes after switching to net10 on linux-arm64 (NVIDIA DGX Spark / GB10, 20-core).
The failure behavior match the fixed issues and according to the report a private build that includes the backported fixes does not experience failures.
Regression
Testing
Regular tests.
Customer reports trying manually backporting the fixes and successfully using a private build with fixes.
Risk
Low - this is a strict backport of a change that strengthens a few memory accesses.