From 9144ba62c08e66f55bca9e988b9793d0e08f0344 Mon Sep 17 00:00:00 2001 From: Thomas Farr Date: Mon, 26 Feb 2024 12:42:18 +1300 Subject: [PATCH] Preserve all InternalsVisibleTo attributes in ILLink Trimmer Removes previous special handling to only keep IVT attributes for assemblies that were resolvable and marked at link time. This behaviour could cause a noticeable difference in behaviour at runtime without emitting any trim analysis warnings while providing no mechanism to retain the attributes. The removal of these attributes provides only a neglible size reduction. Fixes #92582 --- .../src/linker/Linker.Steps/MarkStep.cs | 49 ++----------------- .../TestCaseUtils.cs | 2 +- .../AttributesTests.g.cs | 6 +++ .../Attributes/Dependencies/IVTUnused_Lib.cs | 8 +++ .../Attributes/IVTUnused.cs | 20 ++++---- ...UnusedKeptWhenKeepingUsedAttributesOnly.cs | 20 ++++++++ 6 files changed, 49 insertions(+), 56 deletions(-) create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/Attributes/Dependencies/IVTUnused_Lib.cs create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/Attributes/IVTUnusedKeptWhenKeepingUsedAttributesOnly.cs diff --git a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs index 02fd1412caf845..7de5697faba3f5 100644 --- a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs +++ b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs @@ -62,7 +62,6 @@ protected LinkContext Context { protected Queue<(MethodDefinition, DependencyInfo, MessageOrigin)> _methods; protected HashSet<(MethodDefinition, MarkScopeStack.Scope)> _virtual_methods; protected Queue _assemblyLevelAttributes; - readonly List _ivt_attributes; protected Queue<(AttributeProviderPair, DependencyInfo, MarkScopeStack.Scope)> _lateMarkedAttributes; protected List<(TypeDefinition, MarkScopeStack.Scope)> _typesWithInterfaces; protected HashSet _dynamicInterfaceCastableImplementationTypesDiscovered; @@ -222,7 +221,6 @@ public MarkStep () _methods = new Queue<(MethodDefinition, DependencyInfo, MessageOrigin)> (); _virtual_methods = new HashSet<(MethodDefinition, MarkScopeStack.Scope)> (); _assemblyLevelAttributes = new Queue (); - _ivt_attributes = new List (); _lateMarkedAttributes = new Queue<(AttributeProviderPair, DependencyInfo, MarkScopeStack.Scope)> (); _typesWithInterfaces = new List<(TypeDefinition, MarkScopeStack.Scope)> (); _dynamicInterfaceCastableImplementationTypesDiscovered = new HashSet (); @@ -288,42 +286,6 @@ protected virtual void Complete () } } - bool ProcessInternalsVisibleAttributes () - { - bool marked_any = false; - foreach (var attr in _ivt_attributes) { - - var provider = attr.Provider; - Debug.Assert (attr.Provider is ModuleDefinition or AssemblyDefinition); - var assembly = (provider is ModuleDefinition module) ? module.Assembly : provider as AssemblyDefinition; - - using var assemblyScope = ScopeStack.PushLocalScope (new MessageOrigin (assembly)); - - if (!Annotations.IsMarked (attr.Attribute) && IsInternalsVisibleAttributeAssemblyMarked (attr.Attribute)) { - MarkCustomAttribute (attr.Attribute, new DependencyInfo (DependencyKind.AssemblyOrModuleAttribute, attr.Provider)); - marked_any = true; - } - } - - return marked_any; - - bool IsInternalsVisibleAttributeAssemblyMarked (CustomAttribute ca) - { - System.Reflection.AssemblyName an; - try { - an = new System.Reflection.AssemblyName ((string) ca.ConstructorArguments[0].Value); - } catch { - return false; - } - - var assembly = Context.GetLoadedAssembly (an.Name!); - if (assembly == null) - return false; - - return Annotations.IsMarked (assembly.MainModule); - } - } - static bool TypeIsDynamicInterfaceCastableImplementation (TypeDefinition type) { if (!type.IsInterface || !type.HasInterfaces || !type.HasCustomAttributes) @@ -416,8 +378,7 @@ void Process () ProcessMarkedPending () || ProcessLazyAttributes () || ProcessLateMarkedAttributes () || - MarkFullyPreservedAssemblies () || - ProcessInternalsVisibleAttributes ()) ; + MarkFullyPreservedAssemblies ()) ; ProcessPendingTypeChecks (); } @@ -1170,6 +1131,9 @@ protected virtual bool ShouldMarkCustomAttribute (CustomAttribute ca, ICustomAtt case "System.Runtime.InteropServices.InterfaceTypeAttribute": case "System.Runtime.InteropServices.GuidAttribute": return true; + // May be implicitly used by the runtime + case "System.Runtime.CompilerServices.InternalsVisibleToAttribute": + return true; } TypeDefinition? type = Context.Resolve (attr_type); @@ -1547,10 +1511,7 @@ bool ProcessLazyAttributes () if (IsAttributeRemoved (customAttribute, resolved.DeclaringType) && Annotations.GetAction (CustomAttributeSource.GetAssemblyFromCustomAttributeProvider (assemblyLevelAttribute.Provider)) == AssemblyAction.Link) continue; - if (customAttribute.AttributeType.IsTypeOf ("System.Runtime.CompilerServices", "InternalsVisibleToAttribute") && !Annotations.IsMarked (customAttribute)) { - _ivt_attributes.Add (assemblyLevelAttribute); - continue; - } else if (!ShouldMarkTopLevelCustomAttribute (assemblyLevelAttribute, resolved)) { + if (!ShouldMarkTopLevelCustomAttribute (assemblyLevelAttribute, resolved)) { skippedItems.Add (assemblyLevelAttribute); continue; } diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/TestCaseUtils.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/TestCaseUtils.cs index d05326239a39e5..4bf104b67a7c75 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/TestCaseUtils.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/TestCaseUtils.cs @@ -90,7 +90,7 @@ private static IEnumerable GetTestDependencies (string testCaseDir, Synt switch (attributeName) { case "SetupCompileBefore": { var arrayExpression = args["#1"]; - if (arrayExpression is not (ArrayCreationExpressionSyntax or ImplicitArrayCreationExpressionSyntax)) + if (arrayExpression is not (ArrayCreationExpressionSyntax or ImplicitArrayCreationExpressionSyntax or CollectionExpressionSyntax)) throw new InvalidOperationException (); foreach (var sourceFile in args["#1"].DescendantNodes ().OfType ()) yield return Path.Combine (testCaseDir, LinkerTestBase.GetStringFromExpression (sourceFile)); diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/AttributesTests.g.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/AttributesTests.g.cs index f563a23f9e3daf..a912649e9cedc5 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/AttributesTests.g.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/AttributesTests.g.cs @@ -123,6 +123,12 @@ public Task IVTUnused () return RunTest (allowMissingWarnings: true); } + [Fact] + public Task IVTUnusedKeptWhenKeepingUsedAttributesOnly () + { + return RunTest (allowMissingWarnings: true); + } + [Fact] public Task IVTUsed () { diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Attributes/Dependencies/IVTUnused_Lib.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Attributes/Dependencies/IVTUnused_Lib.cs new file mode 100644 index 00000000000000..93a680a2858962 --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Attributes/Dependencies/IVTUnused_Lib.cs @@ -0,0 +1,8 @@ +#if IVT +[assembly: System.Runtime.CompilerServices.InternalsVisibleTo ("missing")] +[assembly: System.Runtime.CompilerServices.InternalsVisibleTo ("test-with-key, PublicKey=00240000")] +#endif + +namespace Mono.Linker.Tests.Cases.Attributes.Dependencies; + +public class IVTUnusedLib; diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Attributes/IVTUnused.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Attributes/IVTUnused.cs index 82a1007d1b456d..fed2aa80f8a951 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Attributes/IVTUnused.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Attributes/IVTUnused.cs @@ -1,20 +1,18 @@ using System.Runtime.CompilerServices; +using Mono.Linker.Tests.Cases.Attributes.Dependencies; using Mono.Linker.Tests.Cases.Expectations.Assertions; using Mono.Linker.Tests.Cases.Expectations.Metadata; -#if IVT -[assembly: InternalsVisibleTo ("missing")] -[assembly: InternalsVisibleTo ("test-with-key, PublicKey=00240000")] +namespace Mono.Linker.Tests.Cases.Attributes; -#endif - -namespace Mono.Linker.Tests.Cases.Attributes +[SetupCompileBefore ("lib.dll", ["Dependencies/IVTUnused_Lib.cs"], defines: ["IVT"])] +[KeptAssembly ("lib.dll")] +[KeptTypeInAssembly ("lib.dll", typeof(IVTUnusedLib))] +[KeptAttributeInAssembly ("lib.dll", typeof (InternalsVisibleToAttribute))] +class IVTUnused { - [Define ("IVT")] - class IVTUnused + static void Main () { - static void Main () - { - } + _ = new IVTUnusedLib (); } } diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Attributes/IVTUnusedKeptWhenKeepingUsedAttributesOnly.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Attributes/IVTUnusedKeptWhenKeepingUsedAttributesOnly.cs new file mode 100644 index 00000000000000..1713c8b2600da9 --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Attributes/IVTUnusedKeptWhenKeepingUsedAttributesOnly.cs @@ -0,0 +1,20 @@ +using System.Runtime.CompilerServices; +using Mono.Linker.Tests.Cases.Attributes.Dependencies; +using Mono.Linker.Tests.Cases.Expectations.Assertions; +using Mono.Linker.Tests.Cases.Expectations.Metadata; + +namespace Mono.Linker.Tests.Cases.Attributes; + +[SetupCompileBefore ("lib.dll", ["Dependencies/IVTUnused_Lib.cs"], defines: ["IVT"])] +[KeptAssembly ("lib.dll")] +[KeptTypeInAssembly ("lib.dll", typeof(IVTUnusedLib))] +[KeptAttributeInAssembly ("lib.dll", typeof (InternalsVisibleToAttribute))] + +[SetupLinkerArgument("--used-attrs-only", "true")] +class IVTUnusedKeptWhenKeepingUsedAttributesOnly +{ + static void Main () + { + _ = new IVTUnusedLib (); + } +}