-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix dataflow analysis of lambdas in async methods #122450
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
When we enter ILLinker part of the ILC codebase, we need to switch to the mode where async variants don't exist and async methods have mismatched bodies. There was a spot where we didn't do the switch here: https://github.com/dotnet/runtime/blob/d31e5990b896447bfc3dbe98cfe6ec3b169a4896/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs#L939-L949 The fix is to do the switch early in `FlowAnnotations` so that the `ILProvider` `FlowAnnotations` use (and other parts take advantage of) is already ILLinkerified. Fixes the situation shown in the regression test (we were analyzing the IL of the async variant of `RuntimeAsyncWithLambda` and missed analyzing the lambda completely).
|
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 fixes a dataflow analysis bug where lambdas inside async methods were not being properly analyzed. The issue occurred because the async IL masking transformation wasn't applied consistently across all dataflow analysis components.
- Centralized AsyncMaskingILProvider wrapping in FlowAnnotations constructor
- Removed duplicate wrapping from TypeAnnotationsHashtable and MethodBodyScanner
- Added regression test for lambda analysis in async methods
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs | Wraps the ILProvider with AsyncMaskingILProvider at construction time, ensuring all consumers use the masked version; removes duplicate wrapping from TypeAnnotationsHashtable |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs | Removes duplicate AsyncMaskingILProvider wrapping since FlowAnnotations now provides a pre-wrapped ILProvider |
| src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/RuntimeAsyncMethods.cs | Adds ExpectedNoWarnings attribute and new test case RuntimeAsyncWithLambda to verify lambdas in async methods are properly analyzed for dataflow warnings |
To the same tune as dotnet#122450. This is a bug farm, as expected, but likely still better than trying to teach the code that is shared with ILLink and Roslyn analyzer about async variants.
To the same tune as #122450. This is a bug farm, as expected, but likely still better than trying to teach the code that is shared with ILLink and Roslyn analyzer about async variants. Cc @dotnet/ilc-contrib
When we enter ILLinker part of the ILC codebase, we need to switch to the mode where async variants don't exist and async methods have mismatched bodies. There was a spot where we didn't do the switch here:
runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs
Lines 939 to 949 in d31e599
The fix is to do the switch early in
FlowAnnotationsso that theILProviderFlowAnnotationsuse (and other parts take advantage of) is already ILLinkerified.Fixes the situation shown in the regression test (we were analyzing the IL of the async variant of
RuntimeAsyncWithLambdaand missed analyzing the lambda completely).Cc @dotnet/ilc-contrib