Repository navigation
Remove BypassReadyToRun attributes from AsyncHelpers - #128626
Draft
jakobbotsch with Copilot wants to merge 8 commits into
Draft
jakobbotsch with Copilot wants to merge 8 commits into
jakobbotsch with Copilot wants to merge 8 commits into
Conversation
Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
Copilot
AI
changed the title
Remove BypassReadyToRun attributes from AsyncHelpers files
Remove BypassReadyToRun attributes from AsyncHelpers
May 27, 2026
Copilot created this pull request from a session on behalf of
jakobbotsch
May 27, 2026 06:57
View session
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices |
Member
|
@jtschuster It seems crossgen2 cannot handle these kind of infrastructure async methods: Is it expected? |
Member
|
This isn't expected, I'll investigate. |
Member
|
Looks like we the non-task-returning async methods are seen as CompilerGeneratedIL and that's causing issues. @copilot apply the following patch: diff --git a/src/coreclr/tools/Common/Compiler/AsyncMethodVariant.cs b/src/coreclr/tools/Common/Compiler/AsyncMethodVariant.cs
index 78b0c9c76b1..ad0c94de662 100644
--- a/src/coreclr/tools/Common/Compiler/AsyncMethodVariant.cs
+++ b/src/coreclr/tools/Common/Compiler/AsyncMethodVariant.cs
@@ -152,9 +152,20 @@ public static bool IsReturnDroppingAsyncThunk(this MethodDesc method)
public static bool IsAsyncThunk(this MethodDesc method)
{
+ if (method.IsNonTaskReturningAsyncMethod())
+ return false;
+
return (method.IsAsyncVariant() ^ method.IsAsync) || method.IsReturnDroppingAsyncThunk();
}
+ public static bool IsNonTaskReturningAsyncMethod(this MethodDesc method)
+ {
+ bool isNonTaskReturningAsyncMethod = method.GetTypicalMethodDefinition() is EcmaMethod && method.IsAsync && !method.Signature.ReturnsTaskOrValueTask();
+ // non-task-returning async methods should only be in corelib
+ Debug.Assert(!isNonTaskReturningAsyncMethod || (method.GetTypicalMethodDefinition() is EcmaMethod methodDef && methodDef.Context.SystemModule == methodDef.Module));
+ return isNonTaskReturningAsyncMethod;
+ }
+
public static bool IsCompilerGeneratedILBodyForAsync(this MethodDesc method)
{
return method.IsAsyncThunk() || method is AsyncResumptionStub;
@@ -163,6 +174,7 @@ public static bool IsCompilerGeneratedILBodyForAsync(this MethodDesc method)
public static MethodDesc GetAsyncVariant(this MethodDesc method)
{
Debug.Assert(!method.IsAsyncVariant());
+ Debug.Assert(!method.IsNonTaskReturningAsyncMethod());
return ((CompilerTypeSystemContext)method.Context).GetAsyncVariantMethod(method);
}
diff --git a/src/tests/Interop/UnmanagedCallersOnly/InvalidCSharp.ilproj b/src/tests/Interop/UnmanagedCallersOnly/InvalidCSharp.ilproj
index b7b8b4d7665..8b9ce4121b1 100644
--- a/src/tests/Interop/UnmanagedCallersOnly/InvalidCSharp.ilproj
+++ b/src/tests/Interop/UnmanagedCallersOnly/InvalidCSharp.ilproj
@@ -1,8 +1,10 @@
<Project Sdk="Microsoft.NET.Sdk.IL">
<PropertyGroup>
<OutputType>library</OutputType>
+ <!-- crossgen2 cannot handle non-task-returning runtime-async methods that are not in CoreLib -->
+ <CrossGenTest>false</CrossGenTest>
</PropertyGroup>
<ItemGroup>
<Compile Include="InvalidCallbacks.il" />
</ItemGroup> |
Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
Contributor
Author
Applied the
Applied the |
Removing [BypassReadyToRun] crashed crossgen2 on browser-wasm: REG_ASYNC_CONTINUATION_RET == REG_NA there, so zeroing the continuation register on a synchronous return tripped genIsValidIntOrFakeReg (instr.cpp). Keep the attributes on platforms that don't support runtime-async, and remove them where it is supported. Mirror the RuntimeAsyncSupported predicate (from src/libraries/Directory.Build.targets, which the corelib build does not import) into both corelib csprojs and define RUNTIME_ASYNC_SUPPORTED when it is true. Wrap each [BypassReadyToRun] in #if !RUNTIME_ASYNC_SUPPORTED. Verified by building clr.corelib for browser/wasm (attributes retained) and linux/x64 (attributes removed) and inspecting the emitted metadata. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates runtime-async helper handling so supported R2R/AOT targets can compile the MethodImpl.Async helpers while unsupported targets keep [BypassReadyToRun]. It also adjusts crossgen2 async-method classification to avoid treating non-Task-returning runtime-async helpers as compiler-generated async thunks.
Changes:
- Conditionally applies
[BypassReadyToRun]based onRUNTIME_ASYNC_SUPPORTED. - Adds the
RUNTIME_ASYNC_SUPPORTEDdefine to CoreCLR and NativeAOT CoreLib projects. - Excludes non-Task/ValueTask-returning async methods from async-thunk classification.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.cs |
Conditions shared async helper [BypassReadyToRun] attributes on runtime-async support. |
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs |
Applies the same conditional attribute behavior to CoreCLR-specific helpers. |
src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj |
Defines RUNTIME_ASYNC_SUPPORTED for supported CoreCLR CoreLib builds. |
src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj |
Defines RUNTIME_ASYNC_SUPPORTED for supported NativeAOT CoreLib builds. |
src/coreclr/tools/Common/Compiler/AsyncMethodVariant.cs |
Prevents non-Task-returning runtime-async methods from being treated as async thunks. |
Comment on lines
+17
to
+19
| // "BypassReadyToRun" keeps these MethodImpl.Async helpers out of AOT/R2R images on platforms | ||
| // that don't support runtime-async (RUNTIME_ASYNC_SUPPORTED is not defined there). On those | ||
| // platforms the JIT cannot emit code for runtime-async returns, so the helpers must be JITted. |
Comment on lines
+155
to
+156
| if (method.IsNonTaskReturningAsyncMethod()) | ||
| return false; |
Comment on lines
+28
to
+30
| #if !RUNTIME_ASYNC_SUPPORTED | ||
| [BypassReadyToRun] | ||
| #endif |
jkotas
reviewed
May 29, 2026
| </PropertyGroup> | ||
|
|
||
| <PropertyGroup> | ||
| <DefineConstants Condition="'$(RuntimeAsyncSupported)' == 'true'">$(DefineConstants);RUNTIME_ASYNC_SUPPORTED</DefineConstants> |
Member
There was a problem hiding this comment.
This is misleading. Runtime async should be unconditionally enabled for CoreCLR and NAOT Corelib, including wasm.
If crossgen is crashing on runtime async under wasm, I think it is best to add wasm specific temporary workaround in crossgen or JIT so that it just skips the method without crashing.
Member
There was a problem hiding this comment.
We can probably wait for WASM runtime async support since it is close to be able to crossgen SPC I believe. Is that right @AndyAyersMS?
Member
There was a problem hiding this comment.
Runtime async support in wasm is underway; maybe 1 week away.
Nothing yet depends on codegen handling all of SPC and we have an interim R2R failover for things codegen can't handle, so no reason to wait on Wasm.
Member
There was a problem hiding this comment.
@copilot Remove the RUNTIME_ASYNC_SUPPORTED checks and the msbuild changes that were added
Contributor
Author
There was a problem hiding this comment.
Addressed in 0d320a2: removed the RUNTIME_ASYNC_SUPPORTED checks and the added CoreLib/NativeAOT CoreLib msbuild property changes, while keeping the crossgen2 fix and disabling CrossGen for InvalidCSharp.ilproj.
…readytorun-attributes # Conflicts: # src/coreclr/tools/Common/Compiler/AsyncMethodVariant.cs Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
Contributor
Author
Resolved the merge conflicts with |
Member
|
@copilot resolve the merge conflicts in this pull request |
…readytorun-attributes # Conflicts: # src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs # src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.cs Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com>
Contributor
Author
Resolved the merge conflicts with the latest |
This was referenced Jul 27, 2026
Member
|
/azp run runtime-coreclr crossgen2 outerloop |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
This was referenced Sep 5, 2026
Member
|
I tested this with #134676 and the wasm side was green |
lewing
added a commit
that referenced
this pull request
Sep 29, 2026
…terpreter thunks (#134676) ## Problem For shared-generic runtime-async methods, crossgen2's Wasm thunks passed the hidden generic context and the async continuation in each other's slots. `WasmLowering.RaiseSignature` has no way to mark a parameter as the hidden generic context, so it returns it as the first entry of the `MethodSignature` parameter list, with `this` and the return buffer implied by the signature flags and return type. The thunks built their `ArgIterator` from that signature without `methodRequiresInstArg`, so `ArgIterator` laid the frame out as `[this][continuation][ctx][args]`. The interpreter, the VM `ArgIterator` and the callee's GC ref map (`GCRefMapBuilder.GetCallRefMap`) all expect `[this][ctx][continuation][args]`. Andy diagnosed this in #133627 and proposed modeling the context as the hidden instantiation argument; this PR follows that suggestion. Symptoms: - #133953: `SanityCheck()` in interpreted `AsyncHelpers.Await<__Canon>(ValueTask<T>)` (R2R→interpreter). - #134660: `numGenericArgs > 0` in `ProcessDynamicDictionaryLookup` for a generic virtual async method. - #133627: memory access out of bounds in `WasmR2RToInterpreterThunk(iiaS8p)` (`AwaitAwaiter<TAwaiter>`). - #128626 (removing `[BypassReadyToRun]` from `AsyncHelpers`) hits the interpreter→R2R direction of the same bug on both the R2R and non-R2R browser legs. ## Fix - When a generic context precedes the async continuation, drop it from the layout signature and build the `ArgIterator` with `methodRequiresInstArg: true`. The context is then stored and loaded through `GetParamTypeArgOffset()`, and the continuation through `GetAsyncContinuationArgOffset()`. Without an async continuation the context occupies the same slot as a leading pointer argument, so it keeps the pointer encoding (`i`/`l`). `InitHelpers.CallClassConstructor` relies on that when it calls a shared generic class constructor as `delegate*<void*, void>`. - Share one argument layout across the three thunks that spill arguments: `WasmR2RToInterpreterThunkNode`, `WasmInterpreterToR2RThunkNode` and `WasmImportThunk`. Each used to hand-code the hidden-argument sequence, which is how the slots got swapped. `WasmThunkArgLayout` walks the Wasm signature string in Wasm parameter order, `[this] [retbuf] [generic context] [continuation] [args]` per [clr-abi.md](https://github.com/dotnet/runtime/blob/main/docs/design/coreclr/botr/clr-abi.md#passing-continuation-argument), takes each offset from that `ArgIterator`, and asserts that the two agree. Each thunk keeps its own emission and loops over the entries. - For `WasmImportThunk`, the swapped spill disagreed with the delay-load GC ref map, which `GCRefMapNode` builds from the callee's `MethodDesc` via `GCRefMapBuilder.GetCallRefMap`. A GC during the fixup would have reported the generic context slot as an object reference and missed the continuation. The thunk now spills both to the offsets the GC ref map describes, and `WasmThunkArgLayoutMatchesCallRefMapLayout` asserts the two layouts are identical. - Move the three copies of `HasGenericContextBeforeAsync` into `WasmLowering`, and use it from `RaiseSignature` so the encoding is parsed in one place. ## Testing - Reproduced the #133953 and #134660 failures locally from the CI Helix payload of `run_test_p0_coreclr_R2R_CG2_browser_wasm_checked` (the `async` work item), with an osx-arm64 crossgen2 and wasm JIT. - With this change, the full `async` work item passes 132/132 in both R2R (`RunCrossGen2=1`, no tiered compilation) and non-R2R modes. Without it, both failures reproduce. - #128626's diff on top of this change also passes 132/132 in both modes. - New `WasmArgumentLayoutTests` cases assert the kind, offset and Wasm parameter index of every thunk argument for `iiaip`, `iTiaip`, `S16iaip` and `S16Tiaip`, the no-context cases, and multi-slot and by-reference arguments. `GenericContextEncodesAsLeadingPointerArgument` pins the `CallClassConstructor` invariant: without an async continuation, a context and a leading pointer argument lower to the same key. `WasmArgumentLayoutTests` passes 98/98 locally with a browser-wasm target. - `WasmThunkArgLayoutMatchesCallRefMapLayout` compares every slot of the thunk layout with the `ArgIterator` that `GetCallRefMap` uses, for shared generic, async and shared generic async CoreLib methods. - The shared layout produces byte-identical crossgen2 output to the previous head of this PR (4e66ef0) for System.Private.CoreLib and the 78 assemblies in the `async` work item (`--parallelism:1`), and the `async` work item passes 132/132 in R2R mode. - Re-enabled `AsyncProfilerTests.RuntimeAsync_WhenAny_TracksAllBranches` (#133627). It runs only in the full browser CoreCLR R2R library lane, which PRs can't reach today: `runtime-extra-platforms` adds Wasm jobs only for scheduled builds, and `runtime-wasm-libtests` is disabled in AzDO. It hasn't run in CI for this PR. The first scheduled `runtime-extra-platforms` build after merge will run it; #134613 restored that lane. ## Notes - #134643 adds ActiveIssues for #133953 and #134660; those shouldn't be needed with this change. - #134211 touches the members right above the removed `HasGenericContextBeforeAsync` properties, so whichever lands second will have a small textual conflict. - The two interpreter transition thunks are shared across images by signature string, first registration wins (`WasmImportThunk` is not; each image references its own). An image compiled by an older crossgen2 would still carry the old layout under the same key. That only matters for mixing crossgen2 versions, which Wasm R2R doesn't ship yet. - A dedicated `g` token for the context (#134716) was folded in and then reverted. The VM computed `Ivgp` for a shared generic class constructor while R2R code registered only `Ivip` from `CallClassConstructor`, so the portable entry point got no interpreter thunk and every app in the browser R2R smoke lane failed at startup. Emitting `g` only before `a` would carry no more information than the pointer char in that position. cc @AndyAyersMS @davidwrighton Resolves #133953 Resolves #134660 Resolves #133627 > [!NOTE] > This PR description was drafted with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes
[BypassReadyToRun]from all runtime async helper methods inAsyncHelpers.csandAsyncHelpers.CoreCLR.cs, now that the AOT/R2R typesystem supportsMethodImpl.Async.src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.cs: dropped the attribute from allAwaitAwaiter/UnsafeAwaitAwaiter/Await(Task[/<T>])/Await(ValueTask[/<T>])/Await(ConfiguredTaskAwaitable...)overloads.src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs: dropped the attribute from the CoreCLR-specific awaiter helpers.// "BypassReadyToRun" is until AOT/R2R typesystem has support for MethodImpl.Asynccomment.src/coreclr/tools/Common/Compiler/AsyncMethodVariant.cs: fixed a crossgen2 crash where non-task-returning async methods (e.g. theAsyncHelpersinfrastructure methods) were incorrectly classified as compiler-generated IL bodies. AddedIsNonTaskReturningAsyncMethod()helper and updatedIsAsyncThunk/GetAsyncVariantaccordingly.