JIT: Fix CEA cloning when enumerator local is reassigned from an untracked source#127079
JIT: Fix CEA cloning when enumerator local is reassigned from an untracked source#127079EgorBo wants to merge 3 commits intodotnet:mainfrom
Conversation
…acked source In the conditional escape analysis (CEA) pipeline for GDV-guarded enumerator allocations, CheckForGuardedAllocationOrCopy only records stores whose data is GT_ALLOCOBJ, GT_LCL_VAR, or GT_BOX. Stores whose data is something else (e.g. a virtual call that did not devirtualize) were silently ignored, so the allocation of the second enumerator was never recorded as a second def of the shared enumerator local. CheckCanClone therefore saw only a single def and proceeded to clone, reusing the same stack-allocated enumerator slot for both loops. The second loop's fast-path guard still passed (method table slot retains the first constructor's write), so the loop iterated using the first enumerator's _endIndex and _array, producing wrong results. Record such stores as appearances so CheckCanClone detects multiple defs and bails out of unsafe cloning. Fixes dotnet#127075 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Fixes a JIT conditional escape analysis (CEA) miscompile by ensuring “unrecognized” stores to tracked GDV-guarded enumerator locals are recorded as appearances, so cloning is rejected when the enumerator local is multiply-defined.
Changes:
- Extend
ObjectAllocator::CheckForGuardedAllocationOrCopyto record appearances for stores from previously-untracked sources into tracked enumerator locals (preventing unsafe cloning). - Add a JIT regression test that exercises
[.. head, .. tail]collection expression spread under Tiered Compilation + PGO.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/jit/objectalloc.cpp | Records unrecognized enumerator-local defs as appearances so CheckCanClone detects multiple defs and bails out. |
| src/tests/JIT/Regression/JitBlue/Runtime_127075/Runtime_127075.csproj | Configures isolated test execution with Tiered Compilation + Tiered PGO (+ QuickJitForLoops). |
| src/tests/JIT/Regression/JitBlue/Runtime_127075/Runtime_127075.cs | Adds regression coverage for [.. head, .. tail] spread scenario that previously produced wrong results. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // | ||
| // See https://github.com/dotnet/runtime/issues/127075. | ||
| // | ||
| // RecordAppearance(lclNum, block, stmt, use); |
There was a problem hiding this comment.
The new else-branch intends to record this store as an appearance so multiple defs are detected, but the only RecordAppearance(...) call is commented out. As written, unrecognized stores are still ignored and the described miscompile scenario would remain possible. Please invoke RecordAppearance(lclNum, block, stmt, use); (and adjust/remove the comment accordingly).
| // RecordAppearance(lclNum, block, stmt, use); | |
| RecordAppearance(lclNum, block, stmt, use); |
| public interface I; | ||
| public sealed class A : I; | ||
| public sealed class B : I; |
There was a problem hiding this comment.
These type declarations are not valid C#: interfaces/classes must have a body. public interface I; / public sealed class A : I; / public sealed class B : I; will not compile. Please change them to proper empty type declarations (e.g., public interface I { }, public sealed class A : I { }, etc.).
| public interface I; | |
| public sealed class A : I; | |
| public sealed class B : I; | |
| public interface I { } | |
| public sealed class A : I { } | |
| public sealed class B : I { } |
Fixes #127075.
In the conditional escape analysis pipeline for GDV-guarded enumerator allocations,
CheckForGuardedAllocationOrCopyonly records stores whose data isGT_ALLOCOBJ,GT_LCL_VAR, orGT_BOX. Stores from other sources (e.g. a virtual call that did not devirtualize) were silently ignored, so a second def of a shared enumerator local was never recorded.CheckCanClonetherefore saw only a single def and proceeded to clone, reusing the same stack-allocated enumerator slot for both loops. The second loop's fast-path guard still passed (method table slot retains the first constructor's write), so it iterated using the first enumerator's_endIndexand_array, producing wrong results for[.. head, .. Tail]-style collection expressions under Tier1+PGO.The fix records such stores as appearances so
CheckCanClonedetects multiple defs and bails out of unsafe cloning.