[release/10.0] Fix tree type in impStoreNullableFields and impLoadNullableFields for simds - #130867
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@JulieLeeMSFT as an FYI, backport for servicing consideration |
|
@EgorBo will submit an approval request. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "5e384b0064d887fd46879967db84dd02442a74e5",
"last_dispatched_base_ref": "release/10.0",
"last_dispatched_base_sha": "6d9e1b3058c87cb4ba51e2b5f160e098819a5ad2",
"last_reviewed_commit": "5e384b0064d887fd46879967db84dd02442a74e5",
"last_reviewed_base_ref": "release/10.0",
"last_reviewed_base_sha": "6d9e1b3058c87cb4ba51e2b5f160e098819a5ad2",
"last_recorded_worker_run_id": "29687177371",
"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": "5e384b0064d887fd46879967db84dd02442a74e5",
"review_id": 4730767685
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Backport of #124432 to release/10.0. A Nullable<T> whose T is a SIMD type (Vector2?/Vector3?/Vector4?) is miscompiled. In impStoreNullableFields/impLoadNullableFields the value field's var_types was computed with JITtype2varType, which maps SIMD structs to TYP_STRUCT rather than the correct TYP_SIMD*. Under MinOpts this produces invalid IR that surfaces as an InvalidProgramException. The bug has existed since these helpers were introduced in .NET 9 (#104931).
Approach: Replace the two JITtype2varType(getFieldType(...)) calls with TypeHandleToVarType(corFldType, valueStructCls, &layout). That helper resolves the class layout and returns the precise element type (layout->GetType()), yielding TYP_SIMD* for SIMD structs, and simultaneously provides the ClassLayout* the subsequent gtNewStoreLclFldNode/gtNewLclFldNode calls need. This is the same idiom already used elsewhere in the importer (e.g. field/array element handling). The removed manual valueType == TYP_STRUCT ? typGetObjLayout(...) : nullptr computation is now correctly folded into the helper. A Runtime_124425 regression test casts a null SIMD Nullable<T> under DOTNET_JITMinOpts=1 and asserts NullReferenceException instead of InvalidProgramException.
Summary: LGTM. This is a faithful, clean backport of the merged PR #124432 — the touched files and change sizes match the original exactly (importer.cpp +11/-7, plus the new test files). The fix is small, localized, and low-risk, and it reuses an established helper that already handles both the SIMD type mapping and layout retrieval. The regression test is well-targeted and reproduces the Debug-only failure. I have no actionable findings.
Minor (non-blocking, matches the original PR): Runtime_124425.cs ends without a trailing newline. Not worth changing in a backport that mirrors the merged commit.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 55.1 AIC · ⌖ 10.3 AIC · ⊞ 10K
Backport of #124432 to release/10.0
/cc @tannergooding @EgorBo
Customer Impact
Reported in #124425. A
Nullable<T>whoseTis a SIMD type (Vector2?,Vector3?,Vector4?, etc.) is miscompiled. InimpStoreNullableFields/impLoadNullableFieldsthe value field was typed viaJITtype2varType, which maps the SIMD type toTYP_STRUCTinstead of the correctTYP_SIMD*. Under MinOpts (Debug /Optimize=false) this produces invalid IR, surfacing as anInvalidProgramException("Common Language Runtime detected an invalid program") at runtime. The failure is Debug-only; optimized builds happened to avoid it.Regression
Introduced in .NET 9 by #104931, which added
impStoreNullableFields/impLoadNullableFieldsto optimizeBOX+UNBOXforT? <-> T. The bug has been present since those helpers shipped.Testing
New regression test
Runtime_124425exercises casting a nullNullable<T>ofVector2/Vector3/Vector4underDOTNET_JITMinOpts=1, asserting the expectedNullReferenceExceptionrather than anInvalidProgramException. Verified failing before the fix and passing after.Risk
Low. The fix replaces
JITtype2varTypewithTypeHandleToVarType, the same helper already used elsewhere to correctly map aCORINFO_CLASS_HANDLEto a SIMDvar_types(and to obtain theClassLayout). The change is small and localized to the two nullable field helpers in the importer.Note
This PR description was drafted by Copilot.