Skip to content
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

Add ILLink/ILCompiler support for feature checks #99267

Merged
merged 16 commits into from
Mar 11, 2024

Conversation

sbomer
Copy link
Member

@sbomer sbomer commented Mar 4, 2024

This will substitute static boolean properties with false when decorated with [FeatureGuard(typeof(SomeFeature))], where SomeFeature is known to be disabled.

The "feature" of unreferenced code, represented by RequiresUnreferencedCodeAttribute, is always considered disabled. For ILC, RequiresDynamicCodeAttribute and RequiresAssemblyFilesAttribute are also disabled. ILLink also respects the feature switch for IsDynamicCodeSupported to disable the RequiresDynamicCodeAttribute feature.

We don't want this kicking in when trimming in library mode, so this adds an ILLink option to disable the optimization.

Additionally, a property is substituted if it has [FeatureSwitchDefinition("SomeFeatureName")] and "SomeFeatureName" is set in the command-line arguments.

XML substitutions take precedence over this behavior.

Includes a few tweaks and cleanup to the analyzer logic added in #94944.

See #94625 for notes on the design.
See #96859 for the API proposal.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Mar 4, 2024
@ghost ghost assigned sbomer Mar 4, 2024
@ghost
Copy link

ghost commented Mar 4, 2024

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

Issue Details

This will substitute static boolean properties with false when decorated with [FeatureCheck(typeof(SomeFeature))], where SomeFeature is known to be disabled.

The "feature" of unreferenced code, represented by RequiresUnreferencedCodeAttribute, is always considered disabled. For ILC, RequiresDynamicCodeAttribute and RequiresAssemblyFilesAttribute are also disabled.

Additionally, a feature is considered disabled if the type has [FeatureSwitchDefinition("SomeFeatureName")] and "SomeFeatureName" is set to false in the command-line arguments.

We don't want this kicking in when trimming in library mode, so this adds an ILLink option to disable the optimization.

XML substitutions take precedence over this behavior.

Includes a few tweaks and cleanup to the analyzer logic added in #94944.

See #94625 for notes on the design.
See #96859 for the API proposal.

Author: sbomer
Assignees: -
Labels:

area-Tools-ILLink

Milestone: -

@sbomer
Copy link
Member Author

sbomer commented Mar 6, 2024

I will update this to match the approved API shape once #99340 is merged.

@sbomer
Copy link
Member Author

sbomer commented Mar 7, 2024

This is ready for review.

Copy link
Member

@jtschuster jtschuster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Copy link
Member

@jtschuster jtschuster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Native AOT side LGTM modulo the questions. Thanks!


foreach (var featureGuardAttribute in property.GetDecodedCustomAttributes("System.Diagnostics.CodeAnalysis", "FeatureGuardAttribute"))
{
if (featureGuardAttribute.FixedArguments is not [CustomAttributeTypedArgument<TypeDesc> { Value: EcmaType featureType }])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just double checking that we don't allow feature types to be generic/arrays/pointers (non-definition type will never be EcmaType).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The feature types aren't restricted in any way, but everything will be a no-op except for RequiresUnreferencedCodeAttribute, RequiresDynamicCodeAttribute, and RequiresAssemblyFilesAttribute. A third-party analyzer could theoretically add support for other types (including generics) so we don't block other types.

Comment on lines +82 to +86
if (_hashtable._switchValues.TryGetValue(
"System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported",
out bool isDynamicCodeSupported)
&& !isDynamicCodeSupported)
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? IsDynamicCodeSupported/Compiled is hardcoded to false in native AOT corelib.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this to match the behavior of some substitutions I noticed for System.Linq, that only kick in when the feature is explicitly passed on the command-line. It's kind of a moot point unless somebody tries to set DynamicCodeSupport to true in an AOT app.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that's an unintended consequence of #89638. I don't think we should allow IsDynamicCodeSupported to be set to true. It would produce unactionable warnings (we don't have user-facing warning messages for these) and a broken app.

Submitted #99590 to ban this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sbomer sbomer merged commit e9e33e1 into dotnet:main Mar 11, 2024
134 of 136 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants