[ILLink analyzer] Add branch analysis #94123

sbomer · 2023-10-27T23:07:09Z

This adds an understanding of RuntimeFeature.IsDynamicCodeSupported to the ILLink Roslyn analyzer. This is done by maintaining an additional state, FeatureContext, alongside the tracking done for local variables.

The FeatureContext maintains a set of features known to be enabled at a given point. The lattice meet operator is set intersection, so that we get features enabled along all paths, and the top value is the set of all features. At the beginning of the analysis, each basic block starts out in the "top" state, but the entry point gets initialized to the empty set of features. Feature checks (calls to RuntimeFeature.IsDynamicCodeSupported) modify the state in the outgoing branch to track the fact that a given feature is enabled.

When a branch operation is seen at the end of a basic block, the analysis examines it to see if it is a supported feature check. If so, it returns an abstract representation of the checked condition (which can be a boolean expression involving various features): FeatureCheckValue. Because this may be assumed to be true or false (depending on the outgoing branch), it has to track included and excluded features, unlike FeatureContext.

The analysis then separates the output state for the basic block into two, one for the conditional successor, and one for the fall-through successor. It applies the FeatureCheckValue to each state separately, once assuming that the check is true, and once assuming that it's false, depending on the branch semantics, possibly modifying the FeatureContext in each branch.

To support this, the dataflow state tracking is now done per branch, instead of per basic block. Previously we tracked the state at the end of each block; now we track the state at the input to each edge in the control-flow graph.

The supported feature checks are hard-coded in the analyzer (this change isn't introducing any kind of attribute-based model to replace the feature xml files). For now the only supported check is RuntimeFeature.IsDynamicCodeSupported, but it should be easy to add new feature checks. This change includes testing of feature checks for RequiresUnreferencedCodeAttribute and RequiresAssemblyFilesAttribute, by including code in the analyzer that looks for specific feature guards in the test namespace. Happy to change to another approach if we don't like this.

There are still some pieces of the Requires analyzer logic (generic instantiations and dynamic objects, for example) that need to be moved over to the dataflow analysis, so that the feature checks can act as guards for all of the related patterns. Until that is done, feature checks won't silence those particular warnings. I'll continue working on moving the remaining logic over, but I don't think it needs to block this change.

Fixes dotnet/linker#2715

ghost · 2023-10-27T23:07:26Z

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

This adds an understanding of RuntimeFeature.IsDynamicCodeSupported to the ILLink Roslyn analyzer. This is done by maintaining an additional state, FeatureContext, alongside the tracking done for local variables.

The FeatureContext maintains a set of features known to be enabled at a given point. The lattice meet operator is set intersection, so that we get features enabled along all paths, and the top value is the set of all features. At the beginning of the analysis, each basic block starts out in the "top" state, but the entry point gets initialized to the empty set of features. Feature checks (calls to RuntimeFeature.IsDynamicCodeSupported) modify the state in the outgoing branch to track the fact that a given feature is enabled.

When a branch operation is seen at the end of a basic block, the analysis examines it to see if it is a supported feature check. If so, it returns an abstract representation of the checked condition (which can be a boolean expression involving various features): FeatureCheckValue. Because this may be assumed to be true or false (depending on the outgoing branch), it has to track included and excluded features, unlike FeatureContext.

The analysis then separates the output state for the basic block into two, one for the conditional successor, and one for the fall-through successor. It applies the FeatureCheckValue to each state separately, once assuming that the check is true, and once assuming that it's false, depending on the branch semantics, possibly modifying the FeatureContext in each branch.

To support this, the dataflow state tracking is now done per branch, instead of per basic block. Previously we tracked the state at the end of each block; now we track the state at the input to each edge in the control-flow graph.

For now the only supported check is RuntimeFeature.IsDynamicCodeSupported, but it should be easy to add new feature checks. This change includes testing of feature checks for RequiresUnreferencedCodeAttribute and RequiresAssemblyFilesAttribute, by including code in the analyzer that looks for specific feature guards in the test namespace. Happy to change to another approach if we don't like this.

There are still some pieces of the Requires analyzer logic (generic instantiations and dynamic objects, for example) that need to be moved over to the dataflow analysis, so that the feature checks can act as guards for all of the related patterns. Until that is done, feature checks won't silence those particular warnings. I'll continue working on moving the remaining logic over, but I don't think it needs to block this change.

Fixes dotnet/linker#2715

Author:	sbomer
Assignees:	-
Labels:	`area-Tools-ILLink`
Milestone:	-

sbomer · 2023-10-27T23:11:31Z

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs

@@ -1194,32 +1194,32 @@ protected virtual void HandleStoreMethodReturnValue(MethodIL method, int offset,
                switch (value)
                {
                    case FieldReferenceValue fieldReferenceValue:
-                        dereferencedValue = MultiValue.Meet(
+                        dereferencedValue = MultiValue.Union(


I renamed Meet on ValueSet to Union since I also added an Intersection method. I'm thinking it's most readable if we use set operation names for ValueSet, and the lattice operation names (Meet) when using ValueSetLattice. In the case of this method, I think the fact that DereferenceValue computes a union of dereferenced values for the items in the ValueSet has more to do with the semantics of the dereference operation and ValueSet, than with ValueSetLattice, so I think it makes sense to use Union here.

sbomer · 2023-10-27T23:12:26Z

src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisFieldAccessPattern.cs

 		}

-		// No Merge - there's nothing to merge since this pattern is uniquely identified by both the origin and the entity
-		// and there's only one way to "access" a field.
+		public TrimAnalysisFieldAccessPattern Merge (


Merge is now necessary since this includes FeatureContext as part of the pattern.

src/tools/illink/src/ILLink.Shared/DataFlow/ForwardDataFlowAnalysis.cs

vitek-karas · 2023-11-01T13:34:50Z

src/tools/illink/src/ILLink.Shared/DataFlow/ForwardDataFlowAnalysis.cs

+				// Duplicate the current state so that it's not shared with fall-through state.
+				TValue conditionalCurrentState = lattice.Meet (lattice.Top, state.Current);
+
+				if (conditionValue != null) {


Vast majority of cases will have conditionValue==null (or should, feature checks should be very rare). Why do we need to duplicate the state for conditions which are not about feature checks?

Summarizing our discussion for posterity:

I duplicated the states for all conditions as part of moving the state tracking to be per branch instead of per basic block

We could still optimize this by only tracking a duplicate state for relevant conditions. It would mean defining a contract where you could request a state for a branch, but the implementation would be able to give you back the same copy for either outgoing branch, if the condition wasn't relevant.

We agreed this optimization isn't worth doing at this point. In most cases the duplicated state will just be TopValue which is not expensive to copy. We also already track a dictionary entry of a state per basic block, and the extra overhead from one more state for the conditional branch exits is probably not high.

vitek-karas · 2023-11-01T13:40:39Z

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/FeatureSwitchDataFlow.cs

+				if (RuntimeFeature.IsDynamicCodeSupported && TestFeatures.IsUnreferencedCodeSupported) {
+					RequiresDynamicCode ();
+					RequiresUnreferencedCode ();
+				}


How come NativeAOT produces 3050 here? I would expect the branch to be eliminated. I must be missing something here. Is it because of compound condition?

I didn't debug it, but yes, I think it's because of the compound condition. Roslyn sees this as a CFG where the short-circuiting is a new edge in the CFG and it just works. But the IL looks like this:

IL_0001: call bool C::get_Prop1() IL_0006: brfalse.s IL_000f IL_0008: call bool C::get_Prop2() IL_000d: br.s IL_0010 IL_000f: ldc.i4.0 IL_0010: stloc.0 // sequence point: hidden IL_0011: ldloc.0 IL_0012: brfalse.s IL_0021

For NativeAot to understand this it would have to be able to propagate the constant ldc.i4.0 through the stloc/ldloc to the brfalse, and use that to eliminate the branch.

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowAnalysis.cs

- Clarify that this doesn't model everything supported by feature switches - Add comment about ControlFlowBranch - Avoid using "feature switch" terminology - Avoid mentioning "feature context" in generic code

vitek-karas

Didn't review all of it - but enough comments to submit the review so that I don't lose them :-)

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalContextStateLattice.cs

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/FeatureCheckValue.cs

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/FeatureCheckVisitor.cs

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs

src/tools/illink/src/ILLink.Shared/DataFlow/ValueSet.cs

src/tools/illink/src/ILLink.Shared/DataFlow/IControlFlowGraph.cs

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/ControlFlowGraphProxy.cs

src/tools/illink/src/ILLink.Shared/DataFlow/IControlFlowGraph.cs

- LocalContextState -> LoecalStateAndContext - FeatureCheckValue -> FeatureChecksValue - FeatureCheckVisitor -> FeatureChecksVisitor - Comments about state passed to FeatureChecksVisitor - TryGetConditionValue -> GetConditionValue - Use in parameter for GetFieldValue - Fix HandleMethodCallHelper to set entire state to Top, and fix comments to not mention feature context - Consistently pass in TContext to handlers - Comments about test-only feature checks - Move test-only feature checks to ILLink.RoslynAnalyzer namespace - Move INegate after ITransfer - Use 'Empty' in ValueSet Intersection for readability - Remove unnecessary DeepCopy - Remove unnecessary using - Replace throw with assert and continue - Remove left-over unused Successor type

vitek-karas

Reviewed all of product code, still have tests to do.

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalStateAndContextLattice.cs

src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimDataFlowAnalysis.cs

- Comments about FinallyRegions - LocalContextLattice -> LocalStateAndContextLattice - Comment about initial FeatureContext

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/FeatureCheckDataFlow.cs

vitek-karas

Some comments on the tests, but this looks good. Thanks a lot!

- SupportedFeatureChecks -> TestFeatureChecks - Add some calls to RequiresUnreferencedCode/RequiresAssemblyFiles

Change most of the tests to check IsUnreferencedCodeSupported and RequiresUnreferencedCode, with substitutions for ILLink/ILCompiler to make them treat IsUnreferencedCodeSupported as a constant. There are some differences in the current ILLink/ILC test infra: - ILC only allows embedded substitutions (not separate global substitutions) - ILLink doesn't allow modifying CoreLib from test embedded substitutions Because of this, we don't substitute IsDynamicCodeSupported for ILLink (it is already substituted by default for NativeAot). This matches the product behavior for ILLink, and leads to a small difference (extra warning) in the tests. We also substitute IsAssemblyFilesSupported so that NativeAot treats it as a constant, to keep it close to the product behavior and the analyzer behavior. For simplicity, this is done for both NativeAot and ILLink, even though it should really be treated the same as IsDynamicCodeCompiled for ILLink. This makes no difference in the test behavior because we don't test how ILLink specifically behaves with IsAssemblyFilesSupported.

...tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/FeatureCheckDataFlowTestSubstitutions.xml

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/FeatureCheckDataFlow.cs

Fix indentation Add a testcase showing ILLink branch removal behavior when both branches of an if throw after branch removal. This is to check that ILLink behaves like NativeAot despite showing an extra warning in the existing MeetFeaturesEmptyIntersection testcase, because IsDynamicCodeSupported isn't treated as a constant for ILLink.

Also rename left-over LocalContextLattice

eerhardt · 2023-11-07T02:06:11Z

src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresAssemblyFilesAnalyzer.cs

+			if (runtimeFeaturesType == null)
+				return false;
+
+			var isDynamicCodeSupportedProperty = runtimeFeaturesType.GetMembers ("IsAssemblyFilesSupported").OfType<IPropertySymbol> ().FirstOrDefault ();


Did we add code/tests for IsDynamicCodeCompiled? See dotnet/linker#2715 (comment)

Working on this: #94625. I'd appreciate your input on the design.

eerhardt · 2023-11-07T02:08:09Z

Can we also remove these suppressions now?

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptionsUpdateHandler.cs

Lines 28 to 33 in e61f45a

    
                       if (RuntimeFeature.IsDynamicCodeSupported) 
        
                       { 
        
                           // Flush the dynamic method cache 
        
           #pragma warning disable IL3050 // The analyzer doesn't understand runtime feature conditions: https://github.com/dotnet/linker/issues/2715 
        
                           ReflectionEmitCachingMemberAccessor.Clear(); 
        
           #pragma warning restore IL3050

runtime/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceProvider.cs

Lines 252 to 266 in e61f45a

    
                       if (RuntimeFeature.IsDynamicCodeCompiled && !DisableDynamicEngine) 
        
                       { 
        
                           engine = CreateDynamicEngine(); 
        
                       } 
        
                       else 
        
                       { 
        
                           // Don't try to compile Expressions/IL if they are going to get interpreted 
        
                           engine = RuntimeServiceProviderEngine.Instance; 
        
                       } 
        
           #endif 
        
                       return engine; 
        
                       [UnconditionalSuppressMessage("AotAnalysis", "IL3050:RequiresDynamicCode", 
        
                           Justification = "CreateDynamicEngine won't be called when using NativeAOT.")] // see also https://github.com/dotnet/linker/issues/2715 
        
                       ServiceProviderEngine CreateDynamicEngine() => new DynamicServiceProviderEngine(this);

runtime/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Regex.cs

Lines 86 to 87 in e61f45a

    
                   [UnconditionalSuppressMessage("AotAnalysis", "IL3050:RequiresDynamicCode", 
        
                       Justification = "Compiled Regex is only used when RuntimeFeature.IsDynamicCodeCompiled is true. Workaround https://github.com/dotnet/linker/issues/2715.")]

sbomer · 2023-11-14T17:29:08Z

Yes, I have it on my list to remove these and other suppressions for various issues that have been fixed.

Add branch analysis

532bb45

sbomer requested review from agocke and vitek-karas October 27, 2023 23:07

sbomer requested review from marek-safar and MichalStrehovsky as code owners October 27, 2023 23:07

dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Oct 27, 2023

ghost assigned sbomer Oct 27, 2023

sbomer commented Oct 27, 2023

View reviewed changes

src/tools/illink/src/ILLink.Shared/DataFlow/ForwardDataFlowAnalysis.cs Outdated Show resolved Hide resolved

This was referenced Oct 28, 2023

Timeout in System.Net.Quic.Functional.Tests #86019

Closed

CI error: System.Net.Quic.QuicException: The connection timed out from inactivity #91757

Closed

sbomer added 2 commits October 30, 2023 16:25

Update APICompat suppressions

20cc6d0

Merge remote-tracking branch 'origin/main' into branchAnalysisRebase

9a441ef

vitek-karas reviewed Nov 1, 2023

View reviewed changes

sbomer commented Nov 1, 2023

View reviewed changes

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowAnalysis.cs Outdated Show resolved Hide resolved

sbomer added 3 commits November 1, 2023 22:20

Feedback from discussion

69f11bd

- Clarify that this doesn't model everything supported by feature switches - Add comment about ControlFlowBranch - Avoid using "feature switch" terminology - Avoid mentioning "feature context" in generic code

Merge remote-tracking branch 'origin/main' into branchAnalysisRebase

e1fda9b

Merge remote-tracking branch 'origin/main' into branchAnalysisRebase

0a80a16

build-analysis bot mentioned this pull request Nov 2, 2023

MSBuild crashing in the build #92290

Open

vitek-karas reviewed Nov 2, 2023

View reviewed changes

sbomer mentioned this pull request Nov 2, 2023

Investigate/optimize performance of TrimAnalysisPatterns tracked by ILLink Roslyn analyzer #94312

Open

vitek-karas reviewed Nov 3, 2023

View reviewed changes

src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalStateAndContextLattice.cs Outdated Show resolved Hide resolved

src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimDataFlowAnalysis.cs Show resolved Hide resolved

PR feedback

abbc54f

- Comments about FinallyRegions - LocalContextLattice -> LocalStateAndContextLattice - Comment about initial FeatureContext

build-analysis bot mentioned this pull request Nov 3, 2023

[wasm] Playwright failing #94240

Closed

vitek-karas approved these changes Nov 3, 2023

View reviewed changes

vitek-karas reviewed Nov 3, 2023

View reviewed changes

sbomer added 2 commits November 3, 2023 21:54

PR feedback

73275d2

- SupportedFeatureChecks -> TestFeatureChecks - Add some calls to RequiresUnreferencedCode/RequiresAssemblyFiles

vitek-karas reviewed Nov 6, 2023

View reviewed changes

...tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/FeatureCheckDataFlowTestSubstitutions.xml Show resolved Hide resolved

vitek-karas reviewed Nov 6, 2023

View reviewed changes

src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/FeatureCheckDataFlow.cs Outdated Show resolved Hide resolved

sbomer added 3 commits November 6, 2023 17:33

Merge remote-tracking branch 'origin/main' into branchAnalysisRebase

3672801

Fix merge

2574f85

Also rename left-over LocalContextLattice

sbomer merged commit e61f45a into dotnet:main Nov 7, 2023
132 of 134 checks passed

eerhardt reviewed Nov 7, 2023

View reviewed changes

github-actions bot locked and limited conversation to collaborators Dec 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ILLink analyzer] Add branch analysis #94123

[ILLink analyzer] Add branch analysis #94123

sbomer commented Oct 27, 2023 •

edited

Loading

ghost commented Oct 27, 2023

sbomer Oct 27, 2023

sbomer Oct 27, 2023

vitek-karas Nov 1, 2023

sbomer Nov 1, 2023

vitek-karas Nov 1, 2023

sbomer Nov 1, 2023

vitek-karas left a comment

vitek-karas left a comment

vitek-karas left a comment

eerhardt Nov 7, 2023

sbomer Nov 14, 2023 •

edited

Loading

eerhardt commented Nov 7, 2023 •

edited

Loading

sbomer commented Nov 14, 2023

[ILLink analyzer] Add branch analysis #94123

[ILLink analyzer] Add branch analysis #94123

Conversation

sbomer commented Oct 27, 2023 • edited Loading

ghost commented Oct 27, 2023

sbomer Oct 27, 2023

Choose a reason for hiding this comment

sbomer Oct 27, 2023

Choose a reason for hiding this comment

vitek-karas Nov 1, 2023

Choose a reason for hiding this comment

sbomer Nov 1, 2023

Choose a reason for hiding this comment

vitek-karas Nov 1, 2023

Choose a reason for hiding this comment

sbomer Nov 1, 2023

Choose a reason for hiding this comment

vitek-karas left a comment

Choose a reason for hiding this comment

vitek-karas left a comment

Choose a reason for hiding this comment

vitek-karas left a comment

Choose a reason for hiding this comment

eerhardt Nov 7, 2023

Choose a reason for hiding this comment

sbomer Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

eerhardt commented Nov 7, 2023 • edited Loading

sbomer commented Nov 14, 2023

sbomer commented Oct 27, 2023 •

edited

Loading

sbomer Nov 14, 2023 •

edited

Loading

eerhardt commented Nov 7, 2023 •

edited

Loading