Fix generic parameter data flow validation in NativeAOT #80956
Conversation
Previously generic data flow was done from generic dictionary nodes. Problem with that approach is that there's no origin information at that point. The warnings can't point to the place where the problematic instantiation is in the code - we only know that it exists. Aside from it being unfriendly for the users, it means any RUC or suppressions don't work on these warnings the same way they do in linker/analyzer. This change modifies the logic to tag the method as "needs data flow" whenever we spot an instantiation of an annotated generic in it somewhere. Then the actualy validation/marking is done from data flow using the trim analysis patterns. The only exception to this is generic data flow for base types and interface implementations, that one is done on the EEType nodes. Note that AOT implements a much more precise version of the generic data flow validation as compared to linker/analyzer. See the big comment at the bening of GenericParameterWarningLocation.cs for how that works.
Changes the expected warning validation to use tokens to compare message origins (same reason as with Kept validation - consistently converting things to string is hard)
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsThis reworks how generic parameter data flow validation is done in the NativeAOT compiler. Previously generic data flow was done from generic dictionary nodes. Problem with that approach is that there's no origin information at that point. The warnings can't point to the place where the problematic instantiation is in the code - we only know that it exists. This change modifies the logic to tag the method as "needs data flow" whenever we spot an instantiation of an annotated generic in it somewhere. The only exception to this is generic data flow for base types and interface implementations, that one is done on the EEType nodes. Note that AOT implements a much more precise version of the generic data flow validation as compared to linker/analyzer. See the big comment at the beginning of Test changes: Change the test infra to use token to compare message origins for expected warnings. Consistently converting generic types/methods into strings across two type systems is just very difficult - the tokens are simple and reliable. Changes the tests to avoid expecting specific generic types/methods formatting in the messages - again, it's too hard to make this consistent without lot of effort. And the tests don't really need it.
|
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/GenericArgumentDataFlow.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs
Show resolved
Hide resolved
The biggest change is to rework how the type inheritance checks are triggered. The new solution adds a new dependency node with the generic type definition to the graph and then does analysis on that node. This is to avoid potential noise warnings which could happen due to multiple instantiations calling the checking code multiple times. With this the check is done only once on the type definition. Tweaked some tests to try to cover the multiple instances scenario.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/Mono.Linker.Tests.Cases/DataFlow/GenericParameterDataFlow.cs
Outdated
Show resolved
Hide resolved
…OT (dotnet#80956)" (dotnet#81259)" This reverts commit faa19f2.
[This is a revert of a revert of #80956 with additional fixes for #81358) This reworks how generic parameter data flow validation is done in the NativeAOT compiler. Previously generic data flow was done from generic dictionary nodes. Problem with that approach is that there's no origin information at that point. The warnings can't point to the place where the problematic instantiation is in the code - we only know that it exists. Aside from it being unfriendly for the users, it means any RUC or suppressions don't work on these warnings the same way they do in linker/analyzer. This change modifies the logic to tag the method as "needs data flow" whenever we spot an instantiation of an annotated generic in it somewhere. Then the actual validation/marking is done from data flow using the trim analysis patterns. The only exception to this is generic data flow for base types and interface implementations, that one is done on the EEType nodes. Note that AOT implements a much more precise version of the generic data flow validation as compared to linker/analyzer. See the big comment at the beginning of `GenericParameterWarningLocation.cs` for how that works. Due to an issue with DependencyInjection, this change also implements a behavior where if a method or field is reflection accessible, the compiler will perform generic argument data flow on all types in the signature of the method/field (which it normally wouldn't do). See #81358 for details about the issue and discussions on the fix approach. Test changes: Adds the two tests from linker which cover this functionality. Change the test infra to use token to compare message origins for expected warnings. Consistently converting generic types/methods into strings across two type systems is just very difficult - the tokens are simple and reliable. Changes the tests to avoid expecting specific generic types/methods formatting in the messages - again, it's too hard to make this consistent without lot of effort. And the tests don't really need it. Adds a smoke test which has a simplified version of the DI problem from #81358.
ghost
locked as resolved and limited conversation to collaborators
This reworks how generic parameter data flow validation is done in the NativeAOT compiler.
Previously generic data flow was done from generic dictionary nodes. Problem with that approach is that there's no origin information at that point. The warnings can't point to the place where the problematic instantiation is in the code - we only know that it exists.
Aside from it being unfriendly for the users, it means any RUC or suppressions don't work on these warnings the same way they do in linker/analyzer.
This change modifies the logic to tag the method as "needs data flow" whenever we spot an instantiation of an annotated generic in it somewhere.
Then the actual validation/marking is done from data flow using the trim analysis patterns.
The only exception to this is generic data flow for base types and interface implementations, that one is done on the EEType nodes.
Note that AOT implements a much more precise version of the generic data flow validation as compared to linker/analyzer. See the big comment at the beginning of
GenericParameterWarningLocation.csfor how that works.Test changes:
Adds the two tests from linker which cover this functionality.
Change the test infra to use token to compare message origins for expected warnings. Consistently converting generic types/methods into strings across two type systems is just very difficult - the tokens are simple and reliable.
Changes the tests to avoid expecting specific generic types/methods formatting in the messages - again, it's too hard to make this consistent without lot of effort. And the tests don't really need it.
Fixes #77455
Fixes #75898