-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Insert a set of known types and methods references necessary for async thunks into the mutable module #121679
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds infrastructure to insert references for known types and methods required by async thunks into the mutable module. This is preparatory work for supporting async methods in ReadyToRun compilation, though async method compilation remains disabled due to unresolved issues with AsyncMethodVariant.GetTypicalMethodDefinition.
Key Changes
- Added
AddingReferencesToR2RKnownTypesAndMethodsflag to MutableModule to allow adding references to known async-related types/methods - Implemented
InitializeKnownMethodsAndTypes()to populate known async-related types and methods from System.Private.CoreLib into the mutable module - Updated assertion logic to handle references to these known types/methods that exist outside the version bubble
- Added early exit for async variant methods in compilation (they remain unsupported for now)
- Refactored AsyncThunks.cs to declare exception type earlier for use in catch region definition
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/TypeSystem/Mutable/MutableModule.cs | Added AddingReferencesToR2RKnownTypesAndMethods flag and updated assertion to allow references during async type/method initialization |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs | Added early exit for async variants and updated token resolution logic to handle known mutable module types/methods |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj | Added project references to async stub files (AsyncThunks.cs, AsyncResumptionStub.cs and related files) |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/IL/ReadyToRunILProvider.cs | Added resolver parameter to InitManifestMutableModule and removed assertion that blocked async variants |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRunCodegenNodeFactory.cs | Updated InitManifestMutableModule call to pass the Resolver parameter |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ModuleTokenResolver.cs | Implemented InitializeKnownMethodsAndTypes() to populate known async types/methods and added helper methods to check if types/methods are in the known set |
| src/coreclr/tools/Common/TypeSystem/IL/Stubs/AsyncThunks.cs | Moved exception type declaration earlier to be accessible for catch region creation |
| if (_compilationModuleGroup.VersionsWithMethodBody((EcmaMethod)method)) | ||
| continue; | ||
|
|
||
| EntityHandle ? handle = _manifestMutableModule.TryGetEntityHandle(method); |
Copilot
AI
Nov 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is inconsistent spacing in the nullable type declaration. Line 515 uses EntityHandle? handle (no space before ?), but this line has EntityHandle ? handle (with a space before ?). The space should be removed for consistency.
| EntityHandle ? handle = _manifestMutableModule.TryGetEntityHandle(method); | |
| EntityHandle? handle = _manifestMutableModule.TryGetEntityHandle(method); |
| throw new NotImplementedException($"Entity handle for known method ({method}) not found in manifest module"); | ||
|
|
||
| var token = new ModuleToken(_manifestMutableModule, handle.Value); | ||
| this.AddModuleTokenForMethod(method, token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that every R2R image is going to have metadata for references to all async helpers?
We should be creating these metadata tokens lazily, only when the async helper is actually used.
| knownMethods.Add(asyncHelpers.GetKnownMethod("ValueTaskFromException"u8, new MethodSignature(MethodSignatureFlags.Static, 0, CompilerContext.SystemModule.GetKnownType("System.Threading.Tasks"u8, "ValueTask"u8), new TypeDesc[] { exception }))); | ||
| knownMethods.Add(asyncHelpers.GetKnownMethod("TaskFromException"u8, new MethodSignature(MethodSignatureFlags.Static, 1, CompilerContext.SystemModule.GetKnownType("System.Threading.Tasks"u8, "Task`1"u8).MakeInstantiatedType(CompilerContext.GetSignatureVariable(0, method: true)), new TypeDesc[] { exception }))); | ||
| knownMethods.Add(asyncHelpers.GetKnownMethod("ValueTaskFromException"u8, new MethodSignature(MethodSignatureFlags.Static, 1, CompilerContext.SystemModule.GetKnownType("System.Threading.Tasks"u8, "ValueTask`1"u8).MakeInstantiatedType(CompilerContext.GetSignatureVariable(0, method: true)), new TypeDesc[] { exception }))); | ||
| knownMethods.Add(asyncHelpers.GetKnownMethod("FinalizeTaskReturningThunk"u8, new MethodSignature(MethodSignatureFlags.Static, 0, CompilerContext.SystemModule.GetKnownType("System.Threading.Tasks"u8, "Task"u8), Array.Empty<TypeDesc>()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is somewhat setting the names of these methods in stone, we should make sure that they have good names, following the same principles we use to name public APIs.
I would expect that a method called FinalizeFoo is going to take Foo as an argument. It is not what this method does. This can use a better name.
| knownMethods.Add(asyncHelpers.GetKnownMethod("CompletedTaskResult"u8, null)); | ||
|
|
||
| // ExecutionAndSyncBlockStore type and methods | ||
| var executionSyncBlockStore = CompilerContext.SystemModule.GetKnownType("System.Runtime.CompilerServices"u8, "ExecutionAndSyncBlockStore"u8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar here - ExecutionAndSyncBlockStore does not look like a great name to me.
| } | ||
| } | ||
|
|
||
| private ImmutableHashSet<MethodDesc> KnownMethods; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Immutable collections are meant to be used for collections updated in lock-free way. They pay extra overhead for the lock-free nature.
If you just a have a hashtable that gets initialized once and then just used for lookups, regular HashSet is going to work just fine and be faster.
| knownTypes.Add(asyncHelpers); | ||
|
|
||
| knownMethods.Add(asyncHelpers.GetKnownMethod("AsyncCallContinuation"u8, null)); | ||
| knownMethods.Add(asyncHelpers.GetKnownMethod("TaskFromException"u8, new MethodSignature(MethodSignatureFlags.Static, 0, CompilerContext.SystemModule.GetKnownType("System.Threading.Tasks"u8, "Task"u8), new TypeDesc[] { exception }))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to have some correct-by-construction scheme that guarantees that this list captures all methods used by the thunk IL emitters.
E.g. have an enum for all these methods and make sure that the thunk IL emitters get the method reference via the ID. It is going to guarantee that we do not forget to add the method here as we are changing the emitters.
|
This is effectively creating an alternative scheme for encoding ReadyToRunHelper ( runtime/src/coreclr/inc/readytorun.h Line 307 in 0b9d2cc
I expect that we will use it for encoding higher-level helpers outside async too. For example, it would be fairly natural to use it to encode |
| knownMethods.Add(asyncHelpers.GetKnownMethod("FinalizeTaskReturningThunk"u8, new MethodSignature(MethodSignatureFlags.Static, 1, CompilerContext.SystemModule.GetKnownType("System.Threading.Tasks"u8, "Task`1"u8).MakeInstantiatedType(CompilerContext.GetSignatureVariable(0, method: true)), Array.Empty<TypeDesc>()))); | ||
| knownMethods.Add(asyncHelpers.GetKnownMethod("FinalizeValueTaskReturningThunk"u8, new MethodSignature(MethodSignatureFlags.Static, 1, CompilerContext.SystemModule.GetKnownType("System.Threading.Tasks"u8, "ValueTask`1"u8).MakeInstantiatedType(CompilerContext.GetSignatureVariable(0, method: true)), Array.Empty<TypeDesc>()))); | ||
| knownMethods.Add(asyncHelpers.GetKnownMethod("TransparentAwait"u8, null)); | ||
| knownMethods.Add(asyncHelpers.GetKnownMethod("CompletedTask"u8, null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CompletedTask makes sense as a name for a property that returns completed Task. It does not make sense as a name for a method that consumes completed task.
Async thunk IL uses types that may not have MethodRef or TypeRef tokens in the assembly being crossgen'ed. There are some assertions to validate that any references in "faux il" version with the assembly, which isn't true for these types and methods. To work around this, we can inject these references into the mutable module, a synthetic module added to the assembly for things like this. During initialization, this adds all the known necessary types and methods. There is still the issue of AsyncMethodVariant.GetTypicalMethodDefinition not resolving to an EcmaMethod, so we aren't able to enable compiling any async methods yet.