Support for RequiresAttributeMismatch testing #72496
Conversation
…aredCompilationModuleGroup, otherwise the logic for checking for mismatch in NativeAOT will not work as currently implemented. Property names dont need parentesis, escape before they are added. Small fix when ExplicitInterface names are used, cannot rely on name starting with get_/set_. Additional RUC warning check when a method is marked via DAM Adds RequiresAttributeMismatch test file from linker (includes adding NativeAOT specific ProducedBy attributes)
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MultiFileCompilationModuleGroup.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
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/Mono.Linker.Tests/Extensions/CecilExtensions.cs
Outdated
Show resolved
Hide resolved
| _logger.ShouldSuppressAnalysisWarningsForRequires(origin.MemberDefinition, DiagnosticUtilities.RequiresDynamicCodeAttribute), | ||
| _logger.ShouldSuppressAnalysisWarningsForRequires(origin.MemberDefinition, DiagnosticUtilities.RequiresAssemblyFilesAttribute), | ||
| _logger); | ||
| ReflectionMethodBodyScanner.CheckAndReportRequires(diagnosticContext, entity, DiagnosticUtilities.RequiresUnreferencedCodeAttribute); |
There was a problem hiding this comment.
Why only RUC - why not also RAF and RDC?
There was a problem hiding this comment.
This is dataflow related so marking here only happens when using DAM. The only attribute that interacts with DAM is RUC. That's why we don't produce RDC and RAF warnings here
There was a problem hiding this comment.
But the problem applies to all right.
Calling Reflection.Emit directly will produce an RDC warning - great.
But calling it indirectly through reflection may go through this code, in which case it would not warn? If so, that's a hole.
There was a problem hiding this comment.
I guess what I'm trying to say is that DAM is basically another way to gain reflection access to a method - but it's still access to a method. For RUC, RAF, RDC methods - ANY access should produce a warning.
There was a problem hiding this comment.
I couldn't find the comment, but this was discussed before for the analyzer when marking KnownTypes, and the agreement we got there was that gaining access using DAM was only interesting for trimming (RequiresUnreferencedCode). The analyzer will not produce any RAF/RDC warnings when access via DAM, in part because the analyzer is restricted to run only if PublishTrimmed/PublishAOT is enabled.
https://github.com/dotnet/linker/blob/main/src/ILLink.RoslynAnalyzer/TrimAnalysis/ReflectionAccessAnalyzer.cs#L81-L98
There was a problem hiding this comment.
RUC is a trimming-related attribute - when we're analyzing for that, we have a complete view of reflection use within the app and that all reflection in it is statically analyzable (we know there won't be any holes).
RAF and RDC don't technically need a complete view of what's reflected on. We could warn if we see provable reflection on something that is annotated, but what to do if there's unknown reflection happening? Would we warn because we don't understand the reflection pattern and the reflection might point to something RAF/RDC-annotated? We probably wouldn't want that - single file doesn't imply trimming and we shouldn't bother the user with that. RDC currently implies trimming. But maybe it doesn't always have to?
In this sense I look at RAF and RDC same as PlatformSpecific attributes - those analyzers also scope out reflection access.
There was a problem hiding this comment.
My view was that currently NativeAOT implies all three - trimming, single-file and AOT. Or in other words, if we're analyzing for trimming, we should use that to warn about single-file/AOT in all cases we can know about.
There was a discussion that the RUC/RDC/RAF analyzer should be merged with the trimming analyzer (for data flow reasons, not for trimming necessarily), we might as well make use of it and report better warnings if trimming is also enabled.
That said, if others think it's better to have more "consistent" behavior (the analysis behaves the same regardless if trimming is on or not) I'm fine with it.
There was a problem hiding this comment.
I'm in favor on trying to do full reachability analysis for all Requires and even Platform specific attributes independent of the trimming restriction.
I can reach methods and properties using System.Reflection (don't even need to use DAM) that could be annotated with RDC/RAF/SupportedOS, have an app that will break in runtime and the current implementation of Analyzer and NativeAOT wouldn't account for that.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs
Outdated
Show resolved
Hide resolved
…snt have anything to do with the marking process. For warnings generated via TypeHierarchy marking we already have a more descriptive warning being generated
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/TestInfraMultiFileCompilationModuleGroup.cs
Outdated
Show resolved
Hide resolved
ghost
locked as resolved and limited conversation to collaborators
Default answer of ShouldProduceFullVTable to false in the MultiFileSharedCompilationModuleGroup, otherwise the logic for checking for mismatch in NativeAOT will not work (as currently implemented).
Property names don't need parenthesis, escape before they are added.
Small fix when ExplicitInterface names are used, cannot rely on name starting with get_/set_. The explicit name might start with something like
Mono.Linker...Additional RUC warning check when a method is marked via DAM
Adds RequiresAttributeMismatch test file from linker (includes adding NativeAOT specific ProducedBy attributes)