JIT: Do not skip reference prolog zero inits in async methods - #132326
Conversation
optRemoveRedundantZeroInits marks a local as having an explicit init when its first reference is a full store that is not preceded by a GC safe point, which lets codegen drop the zero initialization from the prolog. In an async method the resumption path branches straight to a resumption block, skipping that store. If the local is GC reportable and is not restored from the continuation, the GC then scans stack garbage. This is reachable today in async versions of synchronous Task-returning methods, which do not save and restore contexts and so have no GC safe point ahead of their user code. Skip the optimization for async methods. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fe20bc01-0eae-4c21-8c3d-8dbc26cc2f4a
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS I checked diffs locally on a commit before HEAD and there were just a handful of diffs, all in libraries_tests_no_tiered_compilation -- 14 improvements 2 regressions. Improvements primarily because keeping the prolog zeroing allows the async transformation to reason that those locals are null and do not need to be saved/restored in the continuation. |
jakobbotsch
changed the title
jkotas
added
area-CodeGen-coreclr
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness issue in the CoreCLR JIT where optRemoveRedundantZeroInits could suppress prolog zero-initialization for GC-reportable locals in runtime-async methods, allowing resumption paths to observe uninitialized stack slots (leading to GC scanning invalid references). It also adds a regression test to reproduce the failure under GC stress.
Changes:
- Disable the “explicit init” marking optimization in
optRemoveRedundantZeroInitsfor runtime async methods (compIsAsync()). - Add a new async regression test (
Runtime_126750) that exercises the resumption-path hazard. - Add a dedicated test project file to build/run the new regression test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/optimizer.cpp | Prevents the zero-init suppression optimization from applying to runtime-async methods. |
| src/tests/async/regression/126750.cs | Adds a regression test that triggers GC scanning of an uninitialized stack slot on async resumption. |
| src/tests/async/regression/126750.csproj | Adds the minimal csproj needed to compile the new regression test. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
I wonder if there are other places where we make similar assumptions. Eg VN or VN dead stores?
I don't see other assignments to The part of this transformation that tries to reason about GC safe points seems to happen a bit too early. It would probably be more reasonable for this to happen much later (perhaps done by LSRA itself). |
|
/ba-g Known failures |
optRemoveRedundantZeroInits marks a reference local as having an explicit init when its first reference is a full store that is not preceded by a GC safe point, which lets codegen drop the zero initialization from the prolog.
In an async method the resumption path branches straight to a resumption block, skipping that store. If the local is GC reportable and is not restored from the continuation, the GC then scans stack garbage. This is reachable today in async versions of synchronous Task-returning methods, which do not save and restore contexts and so have no GC safe point ahead of their user code.
Skip the optimization for async methods. Add a regression test that crashes the runtime under TC=0 and GCStress=0xC (which is exercised by the GC stress pipeline).
Fix #126750