Route async resumption stub fixups through precode fixup handling#127523
Route async resumption stub fixups through precode fixup handling#127523
Conversation
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/ff3b55f4-0aee-4788-a560-906f45812f1d Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
|
/azp run runtime-coreclr crossgen2 |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Failures in crossgen outerloops appear to preexisting. |
|
@copilot can you add tests to the ILCompiler.ReadyToRun.Tests project to validate that a method with multiple suspension points only has one ResumptionStub fixup? And make sure the test fails when crossgen doesn't have this change. |
There was a problem hiding this comment.
Pull request overview
This PR fixes how async resumption stub fixups are recorded during ReadyToRun compilation by routing them through the existing precode fixup pipeline, rather than adding them directly to the compiled method’s fixup list. This aligns resumption stub fixups with the established deferred + deduplicated fixup commit flow (only committed when compilation successfully publishes code).
Changes:
- Route
ResumptionStubEntryPointfixups viaAddPrecodeFixupinstead of directly mutating_methodCodeNode.Fixups. - Ensure resumption stub fixups participate in the existing deduplication logic and are only committed during successful publish.
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/28262f27-27b1-45af-b60b-c45d3bf5d90c Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
…f the kind Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/28262f27-27b1-45af-b60b-c45d3bf5d90c Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
Note AI-generated content by Copilot coding agent — may contain mistakes. Added in 05cdd60: a new However, I want to flag that I couldn't get the test to fail when reverting the fix on top of this, even with The reason: Happy to take a different approach if you'd prefer something that does fail without the fix — possible options I considered:
Let me know which you'd like. |
… call sites Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6ff5c01d-ee5a-429c-8f70-1d3c05424c3b Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/50e62b65-3704-4410-8f4a-6658ce495f75 Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
| /// <summary> | ||
| /// Validates that ResumptionStubEntryPoint fixups are deduplicated for a method: | ||
| /// even with multiple suspension points and forced compilation retries (via | ||
| /// --determinism-stress), each compiled method should have exactly one | ||
| /// ResumptionStubEntryPoint fixup. | ||
| /// | ||
| /// Without routing the fixup through the precode-fixup path, retried | ||
| /// compilations would each add a duplicate fixup directly to the method node, | ||
| /// producing more than one ResumptionStubEntryPoint fixup per method. | ||
| /// </summary> | ||
| [Fact] | ||
| public void RuntimeAsyncResumptionStubFixupDedup() | ||
| { | ||
| var asm = new CompiledAssembly | ||
| { | ||
| AssemblyName = nameof(RuntimeAsyncResumptionStubFixupDedup), | ||
| SourceResourceNames = | ||
| [ | ||
| "RuntimeAsync/AsyncMultipleSuspensionPoints.cs", | ||
| "RuntimeAsync/RuntimeAsyncMethodGenerationAttribute.cs", | ||
| ], | ||
| Features = { RuntimeAsyncFeature }, | ||
| }; | ||
|
|
||
| new R2RTestRunner(_output).Run(new R2RTestCase( | ||
| nameof(RuntimeAsyncResumptionStubFixupDedup), | ||
| [ | ||
| new(nameof(RuntimeAsyncResumptionStubFixupDedup), [new CrossgenAssembly(asm)]) | ||
| { | ||
| // Force each method to be compiled multiple times so that | ||
| // any non-deduplicated fixup additions become observable. | ||
| AdditionalArgs = { "--determinism-stress=2" }, |
There was a problem hiding this comment.
The new test’s doc comment / rationale implies that duplicate ResumptionStubEntryPoint fixups would become observable in the emitted R2R image (and that this test would fail without routing through the precode-fixup path). However, MethodWithGCInfo.GetFixupBlob deduplicates fixup cells by (TableIndex, ImportOffset) before encoding, so in-memory duplicates on the method node won’t show up via ReadyToRunReader fixups. As written, this test is unlikely to regress the specific bug being fixed. Consider rewording the comment (and the inline note about “observable”) to reflect that the test validates the emitted image invariant only, or adjust the test strategy to observe the pre-encoding method-node fixup list / retry behavior more directly (e.g., via an in-proc compilation harness or additional crossgen2 diagnostics).
| // Used to validate that ResumptionStubEntryPoint fixups are deduplicated | ||
| // across compilation retries (only one fixup per compiled method). |
There was a problem hiding this comment.
The header comment says this source is used to validate ResumptionStubEntryPoint fixup deduplication across compilation retries. Since the emitted fixup blob is already deduplicated at encoding time (by import cell coordinates), this file isn’t actually able to validate the in-memory duplication issue described in the PR. Consider rewording this comment to avoid implying the emitted R2R fixup list will reflect retry-time duplicates, or add a more direct validation mechanism if the intent is to cover the underlying bug.
| // Used to validate that ResumptionStubEntryPoint fixups are deduplicated | |
| // across compilation retries (only one fixup per compiled method). | |
| // Exercises async state machines that may require ResumptionStubEntryPoint | |
| // fixups, without asserting how fixups are deduplicated across retries. |
| /// <paramref name="expectedCount"/> fixups of that kind. | ||
| /// Fails if no match is found, if more than one method matches the search token, or if | ||
| /// the fixup count differs from <paramref name="expectedCount"/>. | ||
| /// Use a precise token (e.g. <c>".MethodName("</c>) to avoid unintended substring matches. | ||
| /// Useful for ensuring fixups are properly deduplicated. |
There was a problem hiding this comment.
The doc comment suggests this helper is “useful for ensuring fixups are properly deduplicated,” but it’s only validating the emitted fixup-cell list from ReadyToRunReader. Since the fixup blob encoding deduplicates cells before emission (by import cell coordinates), this helper can’t detect duplicate additions to the in-memory method-node fixup list. Consider clarifying the wording so readers don’t assume it validates compilation-time fixup deduplication, or note that it asserts the post-encoding invariant in the final R2R image.
| /// <paramref name="expectedCount"/> fixups of that kind. | |
| /// Fails if no match is found, if more than one method matches the search token, or if | |
| /// the fixup count differs from <paramref name="expectedCount"/>. | |
| /// Use a precise token (e.g. <c>".MethodName("</c>) to avoid unintended substring matches. | |
| /// Useful for ensuring fixups are properly deduplicated. | |
| /// <paramref name="expectedCount"/> emitted fixups of that kind. | |
| /// Fails if no match is found, if more than one method matches the search token, or if | |
| /// the emitted fixup count differs from <paramref name="expectedCount"/>. | |
| /// Use a precise token (e.g. <c>".MethodName("</c>) to avoid unintended substring matches. | |
| /// Useful for asserting the post-encoding fixup-cell count in the final R2R image; this | |
| /// validates emitted fixups, not compilation-time deduplication of in-memory fixup lists. |
Test commentHello
|
Async resumption stub fixups were added directly to the compiled method node, bypassing the existing precode fixup path that deduplicates fixups and only commits them after a successful compile.
Fixup handling
ResumptionStubEntryPointfixups throughAddPrecodeFixup.Behavioral impact
RuntimeAsyncResumptionStubFixupDeduptoILCompiler.ReadyToRun.Testswith a newAsyncMultipleSuspensionPointssource containing async methods that have multipleawait Task.Yield()suspension points. The test runs crossgen2 with--determinism-stress=2(forcing multiple compile attempts per method) and asserts each[ASYNC]variant has exactly oneResumptionStubEntryPointfixup.HasFixupKindCountOnMethodhelper toR2RAssertfor asserting an exact fixup count per method (scoped to methods that actually carry that fixup kind, to avoid false-flagging co-named sync thunks).AdditionalArgsplumbing onCrossgenCompilationso tests can pass value-bearing crossgen2 flags (e.g.--determinism-stress=N) not modeled by the existingCrossgen2Optionenum.R2RAsserthelpers (HasAsyncVariant,HasFixupKindOnMethod,HasFixupKindCountOnMethod) to enforce an exactly-one-match invariant: each helper now fails if more than one method matches the search token, preventing ambiguous substring matches from silently passing. Call sites updated to use precise tokens using.as the type member separator to match R2R signature format (e.g..MultipleAwaits(,.MultipleAwaitsWithRefs() that uniquely identify a single method.RuntimeAsyncDevirtualizetest to useOpenImpl.GetValueAsync(andSealedImpl.GetValueAsync(as distinct search tokens, since both classes define an asyncGetValueAsyncand the stricter helper would otherwise report an ambiguous match.MethodWithGCInfo.EncodeFixupCellsalready deduplicates the emitted fixup blob by(TableIndex, ImportOffset), so any in-memory duplicates are squashed at encoding time and are not observable fromReadyToRunReader.