-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add runtime async support to reflection dataflow analysis #121628
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
Conversation
This is a port of dotnet#120683, with an extra. The `MethodIL` that we deal with in the compiler does async variant method signature munging. The dataflow analysis really doesn't like that. So munge the method signatures back to how they look in metadata and in Cecil on dataflow analysis boundaries. I don't like having to do this, but this is a shared codebase and fewer differences is better.
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
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 runtime async support to reflection dataflow analysis in NativeAOT. The main challenge is that the compiler's MethodIL for async methods uses async variant signatures (where Task becomes T), but the dataflow analysis infrastructure expects metadata signatures. The solution introduces AsyncMaskingILProvider to translate between these representations at dataflow boundaries.
Key Changes
- Introduced
AsyncMaskingILProviderto wrapILProviderand mask async variant signatures, reporting them as they appear in metadata - Updated dataflow infrastructure components to use the masking provider where method IL is accessed
- Modified return statement handling in
MethodBodyScannerto skip stack validation for async methods since IL and metadata signatures don't match - Enhanced test compiler to support
/features:and/nowarn:compiler arguments needed for async tests - Enabled the
RuntimeAsyncMethodstest case for NativeAOT
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| AsyncMaskingILProvider.cs | New IL provider wrapper that masks async variant signatures to report them as metadata signatures |
| ReflectionMethodBodyScanner.cs | Wraps method IL with async masking before scanning |
| MethodBodyScanner.cs | Uses masking provider in lattice initialization; skips return validation for async methods |
| FlowAnnotations.cs | Wraps IL provider with async masking in type annotations hashtable |
| CompilerGeneratedState.cs | Wraps IL provider with async masking in type cache |
| ILCompiler.Compiler.csproj | Adds new AsyncMaskingILProvider.cs file to project |
| TestCaseCompiler.cs | Adds support for /features: and /nowarn: compiler arguments to enable async feature tests |
| RuntimeAsyncMethods.cs | Removes IgnoreTestCase attribute to enable test for NativeAOT |
Comments suppressed due to low confidence (1)
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs:813
- [nitpick] The changes skip stack validation for async methods and then unconditionally process return values when
currentStack.Count == 1. This could mask invalid IL in async methods. For example, if anasync Task Method()(which should have void return in IL) incorrectly has an item on the stack, the validation would be skipped (line 796-803) and the code would still attempt to process it as a return value (line 804-813).
Consider whether this behavior is intended. If async method IL validation should be skipped entirely (including return value processing), the currentStack.Count == 1 check at line 804 should also be conditioned on !methodIL.OwningMethod.IsAsync. Alternatively, if some validation is still needed for async methods, consider implementing async-specific validation logic.
if (!methodIL.OwningMethod.IsAsync)
{
bool ilHasReturnValue = !methodIL.OwningMethod.Signature.ReturnType.IsVoid;
if (currentStack.Count != (ilHasReturnValue ? 1 : 0))
{
WarnAboutInvalidILInMethod(methodIL, offset);
}
}
if (currentStack.Count == 1)
{
StackSlot retStackSlot = PopUnknown(currentStack, 1, methodIL, offset);
// If the return value is a reference, treat it as the value itself for now
// We can handle ref return values better later
MultiValue retValue = DereferenceValue(methodIL, offset, retStackSlot.Value, locals, ref interproceduralState);
var methodReturnValue = GetReturnValue(methodIL);
HandleReturnValue(methodIL, offset, methodReturnValue, retValue);
ValidateNoReferenceToReference(locals, methodIL, offset);
}
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/AsyncMaskingILProvider.cs
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCasesRunner/TestCaseCompiler.cs
Show resolved
Hide resolved
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.
This looks good to me.
Alternatively, though, we could use EcmaMethod with IsAsnyc==true for the ASYNC_CALL_CONV variants and special case the Signature in the jit interface. I forget what the other warts were with doing it that way, though.
Regardless, I think this makes sense to do now.
I think what we're doing within the compiler now (making sure the IL actually matches the signature) is better. With the other representation, dataflow analysis needs to compensate because we don't have But it's probably harder to do the same thing in Cecil so we'll have to live with this "convert to a worse representation" step as long as we have to share code with a Cecil codebase. |
|
/ba-g deadletter in a crypto test; unrelated |
This is a port of #120683, with an extra.
The
MethodILthat we deal with in the compiler does async variant method signature munging. The dataflow analysis really doesn't like that. So munge the method signatures back to how they look in metadata and in Cecil on dataflow analysis boundaries. I don't like having to do this, but this is a shared codebase and fewer differences is better.Cc @dotnet/ilc-contrib