From a2ef32cb6b186cf85f65d2ccc310e9e1c0aede9d Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Tue, 16 Jul 2024 14:21:11 -0700 Subject: [PATCH 1/3] Fix feature switch IL when feature attributes are removed --- .../src/linker/Linker/MemberActionStore.cs | 15 ++++++- .../Dependencies/FeatureProperties.cs | 19 ++++++++ .../FeatureAttributeRemovalInCopyAssembly.cs | 43 +++++++++++++++++++ .../TestRemoveFeatureAttributes.xml | 10 +++++ 4 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/FeatureProperties.cs create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/FeatureAttributeRemovalInCopyAssembly.cs create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/TestRemoveFeatureAttributes.xml diff --git a/src/tools/illink/src/linker/Linker/MemberActionStore.cs b/src/tools/illink/src/linker/Linker/MemberActionStore.cs index 51d792fc45b40d..a284f1dd5c4aef 100644 --- a/src/tools/illink/src/linker/Linker/MemberActionStore.cs +++ b/src/tools/illink/src/linker/Linker/MemberActionStore.cs @@ -13,12 +13,14 @@ public class MemberActionStore { public SubstitutionInfo PrimarySubstitutionInfo { get; } private readonly Dictionary _embeddedXmlInfos; + private readonly Dictionary _featureCheckValues; readonly LinkContext _context; public MemberActionStore (LinkContext context) { PrimarySubstitutionInfo = new SubstitutionInfo (); _embeddedXmlInfos = new Dictionary (); + _featureCheckValues = new Dictionary (); _context = context; } @@ -68,6 +70,9 @@ public bool TryGetMethodStubValue (MethodDefinition method, out object? value) internal bool TryGetFeatureCheckValue (MethodDefinition method, out bool value) { + if (_featureCheckValues.TryGetValue (method, out value)) + return true; + value = false; if (!method.IsStatic) @@ -88,7 +93,10 @@ internal bool TryGetFeatureCheckValue (MethodDefinition method, out bool value) // If there's a FeatureSwitchDefinition, don't continue looking for FeatureGuard. // We don't want to infer feature switch settings from FeatureGuard. - return _context.FeatureSettings.TryGetValue (switchName, out value); + if (_context.FeatureSettings.TryGetValue (switchName, out value)) { + _featureCheckValues[method] = value; + return true; + } } if (!_context.IsOptimizationEnabled (CodeOptimizations.SubstituteFeatureGuards, method)) @@ -101,13 +109,16 @@ internal bool TryGetFeatureCheckValue (MethodDefinition method, out bool value) if (featureType.Namespace == "System.Diagnostics.CodeAnalysis") { switch (featureType.Name) { case "RequiresUnreferencedCodeAttribute": + _featureCheckValues[method] = value; return true; case "RequiresDynamicCodeAttribute": if (_context.FeatureSettings.TryGetValue ( "System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported", out bool isDynamicCodeSupported) - && !isDynamicCodeSupported) + && !isDynamicCodeSupported) { + _featureCheckValues[method] = value; return true; + } break; } } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/FeatureProperties.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/FeatureProperties.cs new file mode 100644 index 00000000000000..954e220c93141a --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/FeatureProperties.cs @@ -0,0 +1,19 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System; +using System.Diagnostics.CodeAnalysis; + +namespace Mono.Linker.Tests.Cases.LinkAttributes.Dependencies +{ + public class FeatureProperties + { + [FeatureSwitchDefinition ("FeatureSwitch")] + public static bool FeatureSwitchDefinition => Removed (); + + [FeatureGuard (typeof (RequiresUnreferencedCodeAttribute))] + public static bool FeatureGuard => Removed (); + + static bool Removed () => true; + } +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/FeatureAttributeRemovalInCopyAssembly.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/FeatureAttributeRemovalInCopyAssembly.cs new file mode 100644 index 00000000000000..fd216fcab20f35 --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/FeatureAttributeRemovalInCopyAssembly.cs @@ -0,0 +1,43 @@ +// Copyright (c) .NET Foundation and contributors. All rights reserved. +// Licensed under the MIT license. See LICENSE file in the project root for full license information. + +using System.Diagnostics.CodeAnalysis; +using Mono.Linker.Tests.Cases.Expectations.Assertions; +using Mono.Linker.Tests.Cases.Expectations.Metadata; +using Mono.Linker.Tests.Cases.LinkAttributes.Dependencies; + +namespace Mono.Linker.Tests.Cases.LinkAttributes +{ + [KeptMember (".ctor()")] + [SetupLinkAttributesFile ("TestRemoveFeatureAttributes.xml")] + [RemovedAttributeInAssembly ("FeatureProperties.dll", typeof (FeatureSwitchDefinitionAttribute), typeof (FeatureProperties), nameof (FeatureProperties.FeatureSwitchDefinition))] + [RemovedAttributeInAssembly ("FeatureProperties.dll", typeof (FeatureGuardAttribute), typeof (FeatureProperties), nameof (FeatureProperties.FeatureGuard))] + [RemovedMemberInAssembly ("FeatureProperties.dll", typeof (FeatureProperties), "Removed()")] + [SetupCompileBefore ("FeatureProperties.dll", new[] { "Dependencies/FeatureProperties.cs" })] + [SetupLinkerArgument ("--feature", "FeatureSwitch", "false")] + [SetupLinkerAction ("copy", "test")] // prevent trimming calls to feature switch properties + [IgnoreLinkAttributes (false)] + class FeatureAttributeRemovalInCopyAssembly + { + public static void Main () + { + TestFeatureSwitch (); + TestFeatureGuard (); + } + + [Kept] + static void TestFeatureSwitch () { + if (FeatureProperties.FeatureSwitchDefinition) + Unused (); + } + + [Kept] + static void TestFeatureGuard () { + if (FeatureProperties.FeatureGuard) + Unused (); + } + + [Kept] + static void Unused () { } + } +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/TestRemoveFeatureAttributes.xml b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/TestRemoveFeatureAttributes.xml new file mode 100644 index 00000000000000..bcd48e31399d6c --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/TestRemoveFeatureAttributes.xml @@ -0,0 +1,10 @@ + + + + + + + + + + From e650ff35b3e06bd42af488cd83f061c094ff6880 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Tue, 16 Jul 2024 22:26:52 +0000 Subject: [PATCH 2/3] Add missing return statement --- src/tools/illink/src/linker/Linker/MemberActionStore.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tools/illink/src/linker/Linker/MemberActionStore.cs b/src/tools/illink/src/linker/Linker/MemberActionStore.cs index a284f1dd5c4aef..f8eaeccadd264f 100644 --- a/src/tools/illink/src/linker/Linker/MemberActionStore.cs +++ b/src/tools/illink/src/linker/Linker/MemberActionStore.cs @@ -97,6 +97,7 @@ internal bool TryGetFeatureCheckValue (MethodDefinition method, out bool value) _featureCheckValues[method] = value; return true; } + return false; } if (!_context.IsOptimizationEnabled (CodeOptimizations.SubstituteFeatureGuards, method)) From abcaa9edb24ce8719cf93aa19cc62ac11e590a3c Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Wed, 17 Jul 2024 21:54:30 +0000 Subject: [PATCH 3/3] Ensure we precompute stub value whenever it may be needed --- .../illink/src/linker/Linker.Steps/MarkStep.cs | 11 ++++++++++- .../Dependencies/FeatureProperties.cs | 3 +++ .../FeatureAttributeRemovalInCopyAssembly.cs | 15 +++++++++++++++ .../LinkAttributes/StubFeatureSwitch.xml | 7 +++++++ 4 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/StubFeatureSwitch.xml diff --git a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs index 54547a581b4737..f93542350efbc1 100644 --- a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs +++ b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs @@ -2951,7 +2951,16 @@ void MarkMethodCollection (IList methods, in DependencyInfo re if (method == null) return null; - if (Annotations.GetAction (method) == MethodAction.Nothing) + var methodAction = Annotations.GetAction (method); + if (methodAction is MethodAction.ConvertToStub) { + // CodeRewriterStep runs after sweeping, and may request the stubbed value for any preserved method + // with the action ConvertToStub. Ensure we have precomputed any stub value that may be needed by + // CodeRewriterStep. This ensures sweeping doesn't change the stub value (which can be determined by + // FeatureGuardAttribute or FeatureSwitchDefinitionAttribute that might have been removed). + Annotations.TryGetMethodStubValue (method, out _); + } + + if (methodAction == MethodAction.Nothing) Annotations.SetAction (method, MethodAction.Parse); diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/FeatureProperties.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/FeatureProperties.cs index 954e220c93141a..0f60956f5279a0 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/FeatureProperties.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/Dependencies/FeatureProperties.cs @@ -14,6 +14,9 @@ public class FeatureProperties [FeatureGuard (typeof (RequiresUnreferencedCodeAttribute))] public static bool FeatureGuard => Removed (); + [FeatureSwitchDefinition ("StubbedFeatureSwitch")] + public static bool StubbedFeatureSwitch => Removed (); + static bool Removed () => true; } } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/FeatureAttributeRemovalInCopyAssembly.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/FeatureAttributeRemovalInCopyAssembly.cs index fd216fcab20f35..f4e012bd0906e3 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/FeatureAttributeRemovalInCopyAssembly.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/FeatureAttributeRemovalInCopyAssembly.cs @@ -9,20 +9,29 @@ namespace Mono.Linker.Tests.Cases.LinkAttributes { [KeptMember (".ctor()")] + [ExpectedInstructionSequenceOnMemberInAssembly ("FeatureProperties.dll", typeof (FeatureProperties), "get_StubbedFeatureSwitch()", new[] { + "ldc.i4.1", + "ret", + })] [SetupLinkAttributesFile ("TestRemoveFeatureAttributes.xml")] + [SetupLinkerSubstitutionFile ("StubFeatureSwitch.xml")] [RemovedAttributeInAssembly ("FeatureProperties.dll", typeof (FeatureSwitchDefinitionAttribute), typeof (FeatureProperties), nameof (FeatureProperties.FeatureSwitchDefinition))] [RemovedAttributeInAssembly ("FeatureProperties.dll", typeof (FeatureGuardAttribute), typeof (FeatureProperties), nameof (FeatureProperties.FeatureGuard))] + [RemovedAttributeInAssembly ("FeatureProperties.dll", typeof (FeatureSwitchDefinitionAttribute), typeof (FeatureProperties), nameof (FeatureProperties.StubbedFeatureSwitch))] [RemovedMemberInAssembly ("FeatureProperties.dll", typeof (FeatureProperties), "Removed()")] [SetupCompileBefore ("FeatureProperties.dll", new[] { "Dependencies/FeatureProperties.cs" })] [SetupLinkerArgument ("--feature", "FeatureSwitch", "false")] + [SetupLinkerArgument ("--feature", "StubbedFeatureSwitch", "true")] [SetupLinkerAction ("copy", "test")] // prevent trimming calls to feature switch properties [IgnoreLinkAttributes (false)] + [IgnoreSubstitutions (false)] class FeatureAttributeRemovalInCopyAssembly { public static void Main () { TestFeatureSwitch (); TestFeatureGuard (); + TestStubbedFeatureSwitch (); } [Kept] @@ -37,6 +46,12 @@ static void TestFeatureGuard () { Unused (); } + [Kept] + static void TestStubbedFeatureSwitch () { + if (FeatureProperties.StubbedFeatureSwitch) + Unused (); + } + [Kept] static void Unused () { } } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/StubFeatureSwitch.xml b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/StubFeatureSwitch.xml new file mode 100644 index 00000000000000..a4b57da82a67ca --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/LinkAttributes/StubFeatureSwitch.xml @@ -0,0 +1,7 @@ + + + + + + +