Route async resumption stub fixups through precode fixup handling

[Copilot]

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

  • Route ResumptionStubEntryPoint fixups through AddPrecodeFixup.
  • Preserve the existing resumption stub signature generation while reusing the standard deferred fixup flow.

• Behavioral impact

  • Avoids adding duplicate resumption stub fixups.
  • Prevents fixups discovered during failed compilation paths from being committed directly to the method node.

private void AddResumptionStubFixup(MethodWithGCInfo compiledStubNode)
{
    AddPrecodeFixup(_compilation.SymbolNodeFactory.ResumptionStubEntryPoint(compiledStubNode));
}

• Tests
  • Added RuntimeAsyncResumptionStubFixupDedup to ILCompiler.ReadyToRun.Tests with a new AsyncMultipleSuspensionPoints source containing async methods that have multiple await 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 one ResumptionStubEntryPoint fixup.
  • Added a HasFixupKindCountOnMethod helper to R2RAssert for asserting an exact fixup count per method (scoped to methods that actually carry that fixup kind, to avoid false-flagging co-named sync thunks).
  • Added an AdditionalArgs plumbing on CrossgenCompilation so tests can pass value-bearing crossgen2 flags (e.g. --determinism-stress=N) not modeled by the existing Crossgen2Option enum.
  • Updated R2RAssert helpers (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.
  • Fixed RuntimeAsyncDevirtualize test to use OpenImpl.GetValueAsync( and SealedImpl.GetValueAsync( as distinct search tokens, since both classes define an async GetValueAsync and the stricter helper would otherwise report an ambiguous match.
  • Note: the new test acts as a property guard for "exactly one resumption stub fixup per async method". It does not fail when the fix is reverted because MethodWithGCInfo.EncodeFixupCells already deduplicates the emitted fixup blob by (TableIndex, ImportOffset), so any in-memory duplicates are squashed at encoding time and are not observable from ReadyToRunReader.

[jtschuster]

/azp run runtime-coreclr crossgen2

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jtschuster]

Failures in crossgen outerloops appear to preexisting.

[jtschuster]

@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.

[Copilot]

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 ResumptionStubEntryPoint fixups via AddPrecodeFixup instead of directly mutating _methodCodeNode.Fixups.
• Ensure resumption stub fixups participate in the existing deduplication logic and are only committed during successful publish.

[Copilot]

@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 tes...

Note

AI-generated content by Copilot coding agent — may contain mistakes.

Added in 05cdd60: a new RuntimeAsyncResumptionStubFixupDedup test plus a HasFixupKindCountOnMethod helper. The test compiles a method with multiple await Task.Yield() suspension points and asserts there is exactly one ResumptionStubEntryPoint fixup per [ASYNC] variant.

However, I want to flag that I couldn't get the test to fail when reverting the fix on top of this, even with --determinism-stress=2 (which I plumbed in via a new AdditionalArgs on CrossgenCompilation to force 3 compile attempts per method, including the [ASYNC] variant — confirmed via --verbose "Processing N recompiles" output).

The reason: MethodWithGCInfo.EncodeFixupCells (lines 192–213) sorts and deduplicates the emitted fixup blob by (TableIndex, ImportOffset). Because every getAsyncResumptionStub call returns the same cached Import (same table/offset), any duplicates that the unfixed code adds directly to _methodCodeNode.Fixups across retries get collapsed at encoding time and are not observable from ReadyToRunReader. So the new test doesn't act as a direct catch for the original bug — it's a regression guard for the "exactly one per method" property.

Happy to take a different approach if you'd prefer something that does fail without the fix — possible options I considered:

1. Drop the new test entirely if a sanity/property test isn't useful.
2. Add a debug-only counter on MethodWithGCInfo (e.g., a RawFixupAddCount exposed under DEBUG) and assert on that — invasive but would directly catch the bug.
3. Keep the property test as-is for future regression coverage on the dedup behavior.

Let me know which you'd like.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

[Copilot]

The test comment claims compilation retries will make duplicate ResumptionStubEntryPoint fixups observable in the emitted R2R image, but MethodWithGCInfo.GetFixupBlob deduplicates fixup cells by (TableIndex, ImportOffset), and ResumptionStubEntryPoint appears to reuse the same Import cell across retries. This means the assertion may pass even if duplicates were added during retries (and the PR description notes it doesn’t fail when the fix is reverted). Consider adjusting the test to validate something that changes without this fix, or update the comment/intent so it doesn't suggest this is a regression guard when it isn't.

[Copilot]

Passing "MultipleAwaits" here will also match "MultipleAwaitsWithRefs" (substring match), so this assertion is not scoped to a single method and can be misleading if additional similarly-named methods are added. Consider using a more specific match token (e.g., including "::MultipleAwaits(" or similar) or enhancing the helper to match method names more precisely to avoid accidental cross-matches.

Suggested change

	R2RAssert.HasAsyncVariant(reader, "MultipleAwaits");
	R2RAssert.HasAsyncVariant(reader, "MultipleAwaitsWithRefs");
	R2RAssert.HasResumptionStubFixup(reader, "MultipleAwaits");
	R2RAssert.HasResumptionStubFixup(reader, "MultipleAwaitsWithRefs");
	R2RAssert.HasFixupKindCountOnMethod(reader, ReadyToRunFixupKind.ResumptionStubEntryPoint, "MultipleAwaits", 1);
	R2RAssert.HasFixupKindCountOnMethod(reader, ReadyToRunFixupKind.ResumptionStubEntryPoint, "MultipleAwaitsWithRefs", 1);
	R2RAssert.HasAsyncVariant(reader, "::MultipleAwaits(");
	R2RAssert.HasAsyncVariant(reader, "::MultipleAwaitsWithRefs(");
	R2RAssert.HasResumptionStubFixup(reader, "::MultipleAwaits(");
	R2RAssert.HasResumptionStubFixup(reader, "::MultipleAwaitsWithRefs(");
	R2RAssert.HasFixupKindCountOnMethod(reader, ReadyToRunFixupKind.ResumptionStubEntryPoint, "::MultipleAwaits(", 1);
	R2RAssert.HasFixupKindCountOnMethod(reader, ReadyToRunFixupKind.ResumptionStubEntryPoint, "::MultipleAwaitsWithRefs(", 1);

[jtschuster]

@copilot this is a good point, make the assertion fail if more than one method matches the search name, and fix these calls to match only one method.

[Copilot]

Done in cb97167. Updated HasAsyncVariant, HasFixupKindOnMethod, and HasFixupKindCountOnMethod to fail if more than one method matches the search name. Fixed the calls to use ::MultipleAwaits( and ::MultipleAwaitsWithRefs( as precise tokens. Also updated RuntimeAsyncDevirtualize to use OpenImpl::GetValueAsync( and SealedImpl::GetValueAsync( since that file defines GetValueAsync on both classes and the strict check would otherwise fail.

[jtschuster]

@copilot The method search strings should not use :: as the separator. See this error:

Expected [ASYNC] variant for 'OpenImpl::GetValueAsync(' not found. Async methods: [[ASYNC] System.Threading.Tasks.Task`1<int> OpenImpl.GetValueAsync(), [ASYNC] System.Threading.Tasks.Task`1<int> SealedImpl.GetValueAsync(), [ASYNC] System.Threading.Tasks.Task`1<int> AsyncDevirtualize.CallGenericConstrained<__Canon>(__Canon), [ASYNC] System.Threading.Tasks.Task`1<int> AsyncDevirtualize.CallOnNewOpen(), [ASYNC] System.Threading.Tasks.Task`1<int> AsyncDevirtualize.CallOnSealed(SealedImpl)]

Fix the search strings to use . and the delimiter, not ::

[Copilot]

Fixed in 68cb13e. Changed all search tokens to use . as the separator (OpenImpl.GetValueAsync(, SealedImpl.GetValueAsync(, .MultipleAwaits(, .MultipleAwaitsWithRefs() and updated the doc comment examples in the helpers accordingly.