Add star into link attributes (#81407)
authorTlakaelel Axayakatl Ceja <tlakaelel.ceja@microsoft.com>
Fri, 10 Feb 2023 19:12:25 +0000 (11:12 -0800)
committerGitHub <noreply@github.com>
Fri, 10 Feb 2023 19:12:25 +0000 (11:12 -0800)
One of the functionalities of the LinkAttributes files is to occurrences of custom attributes on the current assembly, on top of this functionality there is a special functionality that is only permitted if it's embedded in an XML inside the System.Private.CorLib module that allows the deletion of custom attributes from all the assemblies.
Given that NativeAOT is multithreaded it cannot process all the assemblies to remove attributes as ILLink does, it also does not have a shared storage in which it can store the information about the removed attributes from System.Private.CoreLib.
This PR adds logic to process the special functionality in the embedded System.Private.CoreLib XML every time an assembly is being processed.
In order to make this more performant, an extra argument is passed to the XML reader to only look at the assembly="*" to get the set of attributes that need to be deleted from all assemblies.

* Handle the star argument in LinkAttributes files for NativeAOT
* Minor fixes to print more warnings
* Add a way to only process star/nonstar assemblies in ProcessLinkerXmlBase
* Add smoke test

src/coreclr/tools/Common/Compiler/ProcessLinkerXmlBase.cs
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs
src/coreclr/tools/aot/Mono.Linker.Tests/TestCasesRunner/ILCompilerDriver.cs
src/tests/nativeaot/SmokeTests/TrimmingBehaviors/ILLink.LinkAttributes.xml [new file with mode: 0644]
src/tests/nativeaot/SmokeTests/TrimmingBehaviors/ILLinkLinkAttributes.cs [new file with mode: 0644]
src/tests/nativeaot/SmokeTests/TrimmingBehaviors/TrimmingBehaviors.csproj

index a46d844..cc39535 100644 (file)
@@ -55,6 +55,7 @@ namespace ILCompiler
         private readonly XPathNavigator _document;
         private readonly Logger _logger;
         protected readonly ModuleDesc? _owningModule;
+        protected readonly bool _globalAttributeRemoval;
         private readonly IReadOnlyDictionary<string, bool> _featureSwitchValues;
         protected readonly TypeSystemContext _context;
 
@@ -70,10 +71,11 @@ namespace ILCompiler
             _featureSwitchValues = featureSwitchValues;
         }
 
-        protected ProcessLinkerXmlBase(Logger logger, TypeSystemContext context, Stream documentStream, ManifestResource resource, ModuleDesc resourceAssembly, string xmlDocumentLocation, IReadOnlyDictionary<string, bool> featureSwitchValues)
+        protected ProcessLinkerXmlBase(Logger logger, TypeSystemContext context, Stream documentStream, ManifestResource resource, ModuleDesc resourceAssembly, string xmlDocumentLocation, IReadOnlyDictionary<string, bool> featureSwitchValues, bool globalAttributeRemoval = false)
             : this(logger, context, documentStream, xmlDocumentLocation, featureSwitchValues)
         {
             _owningModule = resourceAssembly;
+            _globalAttributeRemoval = globalAttributeRemoval;
         }
 
         protected virtual bool ShouldProcessElement(XPathNavigator nav) => FeatureSettings.ShouldProcessElement(nav, _featureSwitchValues);
@@ -133,44 +135,46 @@ namespace ILCompiler
         {
             foreach (XPathNavigator assemblyNav in nav.SelectChildren("assembly", ""))
             {
+                if (!ShouldProcessElement(assemblyNav))
+                    continue;
+
                 // Errors for invalid assembly names should show up even if this element will be
                 // skipped due to feature conditions.
                 bool processAllAssemblies = ShouldProcessAllAssemblies(assemblyNav, out AssemblyName? name);
-                if (processAllAssemblies && AllowedAssemblySelector != AllowedAssemblies.AllAssemblies)
+                if (processAllAssemblies && !_globalAttributeRemoval)
                 {
-                    // NativeAOT doesn't have a way to eliminate all the occurrences of an attribute yet
-                    // https://github.com/dotnet/runtime/issues/77753
-                    //LogWarning(assemblyNav, DiagnosticId.XmlUnsupportedWildcard);
+#if !READYTORUN
+                    if (AllowedAssemblySelector != AllowedAssemblies.AllAssemblies)
+                        LogWarning(assemblyNav, DiagnosticId.XmlUnsuportedWildcard);
+#endif
                     continue;
                 }
-
-                ModuleDesc? assemblyToProcess = null;
-                if (!AllowedAssemblySelector.HasFlag(AllowedAssemblies.AnyAssembly))
+                else if (!processAllAssemblies && _globalAttributeRemoval)
+                {
+                    continue;
+                }
+                else if (processAllAssemblies && _globalAttributeRemoval)
                 {
-                    Debug.Assert(!processAllAssemblies);
                     Debug.Assert(_owningModule != null);
-                    if (_owningModule.Assembly.GetName().Name != name!.Name)
+                    ProcessAssembly(_owningModule, assemblyNav, warnOnUnresolvedTypes: false);
+                }
+                else
+                {
+                    ModuleDesc? assemblyToProcess = null;
+                    if (!AllowedAssemblySelector.HasFlag(AllowedAssemblies.AnyAssembly))
                     {
+                        Debug.Assert(!processAllAssemblies);
+                        Debug.Assert(_owningModule != null);
+                        if (_owningModule.Assembly.GetName().Name != name!.Name)
+                        {
 #if !READYTORUN
-                        LogWarning(assemblyNav, DiagnosticId.AssemblyWithEmbeddedXmlApplyToAnotherAssembly, _owningModule.Assembly.GetName().Name ?? "", name.ToString());
+                            LogWarning(assemblyNav, DiagnosticId.AssemblyWithEmbeddedXmlApplyToAnotherAssembly, _owningModule.Assembly.GetName().Name ?? "", name.ToString());
 #endif
-                        continue;
+                            continue;
+                        }
+                        assemblyToProcess = _owningModule;
                     }
-                    assemblyToProcess = _owningModule;
-                }
-
-                if (!ShouldProcessElement(assemblyNav))
-                    continue;
 
-                if (processAllAssemblies)
-                {
-                    throw new NotImplementedException();
-                    // We could avoid loading all references in this case: https://github.com/dotnet/linker/issues/1708
-                    //foreach (ModuleDesc assembly in GetReferencedAssemblies())
-                    //    ProcessAssembly(assembly, assemblyNav, warnOnUnresolvedTypes: false);
-                }
-                else
-                {
                     Debug.Assert(!processAllAssemblies);
                     ModuleDesc? assembly = assemblyToProcess ?? _context.ResolveAssembly(name!, false);
 
@@ -469,8 +473,8 @@ namespace ILCompiler
                 bool foundMatch = false;
                 foreach (PropertyPseudoDesc property in type.GetPropertiesOnTypeHierarchy(p => p.Name == name))
                 {
-                   foundMatch = true;
-                   ProcessProperty(type, property, nav, customData, false);
+                    foundMatch = true;
+                    ProcessProperty(type, property, nav, customData, false);
                 }
 
                 if (!foundMatch)
index 1209299..0fa74eb 100644 (file)
@@ -39,7 +39,7 @@ namespace ILCompiler
 
         internal readonly UsageBasedMetadataGenerationOptions _generationOptions;
 
-        private readonly FeatureSwitchHashtable _featureSwitchHashtable;
+        private readonly LinkAttributesHashTable _linkAttributesHashTable;
 
         private static (string AttributeName, DiagnosticId Id)[] _requiresAttributeMismatchNameAndId = new[]
             {
@@ -91,7 +91,7 @@ namespace ILCompiler
             FlowAnnotations = flowAnnotations;
             Logger = logger;
 
-            _featureSwitchHashtable = new FeatureSwitchHashtable(Logger, new Dictionary<string, bool>(featureSwitchValues));
+            _linkAttributesHashTable = new LinkAttributesHashTable(Logger, new Dictionary<string, bool>(featureSwitchValues));
             FeatureSwitches = new Dictionary<string, bool>(featureSwitchValues);
 
             _rootEntireAssembliesModules = new HashSet<string>(rootEntireAssembliesModules);
@@ -821,7 +821,7 @@ namespace ILCompiler
             var ecmaType = attributeType.GetTypeDefinition() as EcmaType;
             if (ecmaType != null)
             {
-                var moduleInfo = _featureSwitchHashtable.GetOrCreateValue(ecmaType.EcmaModule);
+                var moduleInfo = _linkAttributesHashTable.GetOrCreateValue(ecmaType.EcmaModule);
                 return !moduleInfo.RemovedAttributes.Contains(ecmaType);
             }
 
@@ -1070,12 +1070,12 @@ namespace ILCompiler
             }
         }
 
-        private sealed class FeatureSwitchHashtable : LockFreeReaderHashtable<EcmaModule, AssemblyFeatureInfo>
+        private sealed class LinkAttributesHashTable : LockFreeReaderHashtable<EcmaModule, AssemblyFeatureInfo>
         {
             private readonly Dictionary<string, bool> _switchValues;
             private readonly Logger _logger;
 
-            public FeatureSwitchHashtable(Logger logger, Dictionary<string, bool> switchValues)
+            public LinkAttributesHashTable(Logger logger, Dictionary<string, bool> switchValues)
             {
                 _logger = logger;
                 _switchValues = switchValues;
@@ -1103,11 +1103,22 @@ namespace ILCompiler
                 Module = module;
                 RemovedAttributes = new HashSet<TypeDesc>();
 
-                PEMemoryBlock resourceDirectory = module.PEReader.GetSectionData(module.PEReader.PEHeaders.CorHeader.ResourcesDirectory.RelativeVirtualAddress);
+                // System.Private.CorLib has a special functionality that could delete an attribute in all modules.
+                // In order to get the set of attributes that need to be removed the modules need collect both the
+                // set of attributes in it's embedded XML file and the set inside System.Private.CorLib embedded
+                // XML file
+                ParseLinkAttributesXml(module, logger, featureSwitchValues, globalAttributeRemoval: false);
+                ParseLinkAttributesXml(module, logger, featureSwitchValues, globalAttributeRemoval: true);
+            }
+
+            public void ParseLinkAttributesXml(EcmaModule module, Logger logger, IReadOnlyDictionary<string, bool> featureSwitchValues, bool globalAttributeRemoval)
+            {
+                EcmaModule xmlModule = globalAttributeRemoval ? (EcmaModule)module.Context.SystemModule : module;
+                PEMemoryBlock resourceDirectory = xmlModule.PEReader.GetSectionData(xmlModule.PEReader.PEHeaders.CorHeader.ResourcesDirectory.RelativeVirtualAddress);
 
-                foreach (var resourceHandle in module.MetadataReader.ManifestResources)
+                foreach (var resourceHandle in xmlModule.MetadataReader.ManifestResources)
                 {
-                    ManifestResource resource = module.MetadataReader.GetManifestResource(resourceHandle);
+                    ManifestResource resource = xmlModule.MetadataReader.GetManifestResource(resourceHandle);
 
                     // Don't try to process linked resources or resources in other assemblies
                     if (!resource.Implementation.IsNil)
@@ -1115,7 +1126,7 @@ namespace ILCompiler
                         continue;
                     }
 
-                    string resourceName = module.MetadataReader.GetString(resource.Name);
+                    string resourceName = xmlModule.MetadataReader.GetString(resource.Name);
                     if (resourceName == "ILLink.LinkAttributes.xml")
                     {
                         BlobReader reader = resourceDirectory.GetReader((int)resource.Offset, resourceDirectory.Length - (int)resource.Offset);
@@ -1127,37 +1138,61 @@ namespace ILCompiler
                             ms = new UnmanagedMemoryStream(reader.CurrentPointer, length);
                         }
 
-                        RemovedAttributes = LinkAttributesReader.GetRemovedAttributes(logger, module.Context, ms, resource, module, "resource " + resourceName + " in " + module.ToString(), featureSwitchValues);
+                        RemovedAttributes.UnionWith(LinkAttributesReader.GetRemovedAttributes(logger, xmlModule.Context, ms, resource, module, "resource " + resourceName + " in " + module.ToString(), featureSwitchValues, globalAttributeRemoval));
                     }
                 }
             }
         }
 
-        private sealed class LinkAttributesReader : ProcessLinkerXmlBase
+        internal sealed class LinkAttributesReader : ProcessLinkerXmlBase
         {
             private readonly HashSet<TypeDesc> _removedAttributes = new();
 
-            public LinkAttributesReader(Logger logger, TypeSystemContext context, Stream documentStream, ManifestResource resource, ModuleDesc resourceAssembly, string xmlDocumentLocation, IReadOnlyDictionary<string, bool> featureSwitchValues)
-                : base(logger, context, documentStream, resource, resourceAssembly, xmlDocumentLocation, featureSwitchValues)
+            public LinkAttributesReader(Logger logger, TypeSystemContext context, Stream documentStream, ManifestResource resource, ModuleDesc resourceAssembly, string xmlDocumentLocation, IReadOnlyDictionary<string, bool> featureSwitchValues, bool globalAttributeRemoval)
+                : base(logger, context, documentStream, resource, resourceAssembly, xmlDocumentLocation, featureSwitchValues, globalAttributeRemoval)
             {
             }
 
+            private static bool IsRemoveAttributeInstances(string attributeName) => attributeName == "RemoveAttributeInstances" || attributeName == "RemoveAttributeInstancesAttribute";
+
             private void ProcessAttribute(TypeDesc type, XPathNavigator nav)
             {
                 string internalValue = GetAttribute(nav, "internal");
-                if (internalValue == "RemoveAttributeInstances" && nav.IsEmptyElement)
+                if (!string.IsNullOrEmpty(internalValue))
                 {
-                    _removedAttributes.Add(type);
+                    if (!IsRemoveAttributeInstances(internalValue) || !nav.IsEmptyElement)
+                    {
+                        LogWarning(nav, DiagnosticId.UnrecognizedInternalAttribute, internalValue);
+                    }
+                    if (IsRemoveAttributeInstances(internalValue) && nav.IsEmptyElement)
+                    {
+                        _removedAttributes.Add(type);
+                    }
                 }
             }
 
-            public static HashSet<TypeDesc> GetRemovedAttributes(Logger logger, TypeSystemContext context, Stream documentStream, ManifestResource resource, ModuleDesc resourceAssembly, string xmlDocumentLocation, IReadOnlyDictionary<string, bool> featureSwitchValues)
+            public static HashSet<TypeDesc> GetRemovedAttributes(Logger logger, TypeSystemContext context, Stream documentStream, ManifestResource resource, ModuleDesc resourceAssembly, string xmlDocumentLocation, IReadOnlyDictionary<string, bool> featureSwitchValues, bool globalAttributeRemoval)
             {
-                var rdr = new LinkAttributesReader(logger, context, documentStream, resource, resourceAssembly, xmlDocumentLocation, featureSwitchValues);
+                var rdr = new LinkAttributesReader(logger, context, documentStream, resource, resourceAssembly, xmlDocumentLocation, featureSwitchValues, globalAttributeRemoval);
                 rdr.ProcessXml(false);
                 return rdr._removedAttributes;
             }
 
+            protected override AllowedAssemblies AllowedAssemblySelector
+            {
+                get
+                {
+                    if (_owningModule?.Assembly == null)
+                        return AllowedAssemblies.AllAssemblies;
+
+                    // Corelib XML may contain assembly wildcard to support compiler-injected attribute types
+                    if (_owningModule?.Assembly == _context.SystemModule)
+                        return AllowedAssemblies.AllAssemblies;
+
+                    return AllowedAssemblies.ContainingAssembly;
+                }
+            }
+
             protected override void ProcessAssembly(ModuleDesc assembly, XPathNavigator nav, bool warnOnUnresolvedTypes)
             {
                 ProcessTypes(assembly, nav, warnOnUnresolvedTypes);
index f5100d6..5181d1b 100644 (file)
@@ -66,12 +66,6 @@ namespace Mono.Linker.Tests.TestCasesRunner
 
                        ILProvider ilProvider = new NativeAotILProvider ();
 
-                       foreach (var descriptor in options.Descriptors) {
-                               if (!File.Exists (descriptor))
-                                       throw new FileNotFoundException ($"'{descriptor}' doesn't exist");
-                               compilationRoots.Add (new ILCompiler.DependencyAnalysis.TrimmingDescriptorNode (descriptor));
-                       }
-
                        Logger logger = new Logger (
                                logWriter,
                                ilProvider,
@@ -82,6 +76,12 @@ namespace Mono.Linker.Tests.TestCasesRunner
                                singleWarnDisabledModules: Enumerable.Empty<string> (),
                                suppressedCategories: Enumerable.Empty<string> ());
 
+                       foreach (var descriptor in options.Descriptors) {
+                               if (!File.Exists (descriptor))
+                                       throw new FileNotFoundException ($"'{descriptor}' doesn't exist");
+                               compilationRoots.Add (new ILCompiler.DependencyAnalysis.TrimmingDescriptorNode (descriptor));
+                       }
+                       
                        ilProvider = new FeatureSwitchManager (ilProvider, logger, options.FeatureSwitches);
 
                        CompilerGeneratedState compilerGeneratedState = new CompilerGeneratedState (ilProvider, logger);
diff --git a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/ILLink.LinkAttributes.xml b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/ILLink.LinkAttributes.xml
new file mode 100644 (file)
index 0000000..83e579f
--- /dev/null
@@ -0,0 +1,16 @@
+<linker>
+  <!-- IL2049 -->
+  <type fullname="ILLinkLinkAttributes/TestRemoveAttribute">
+    <attribute internal="RemoveAttributeInstances"/>
+    <attribute internal="InvalidInternal"/>
+  </type>
+  <!-- IL2048 -->
+  <type fullname="ILLinkLinkAttributes">
+    <method signature="_fieldWithCustomAttribute">
+      <attribute internal="RemoveAttributeInstances"/>
+    </method>
+  </type>
+  <type fullname="ILLinkLinkAttributes/TestMarkAllRemoveAttribute">
+    <attribute internal="RemoveAttributeInstances"/>
+  </type>
+</linker>
diff --git a/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/ILLinkLinkAttributes.cs b/src/tests/nativeaot/SmokeTests/TrimmingBehaviors/ILLinkLinkAttributes.cs
new file mode 100644 (file)
index 0000000..5cfaff7
--- /dev/null
@@ -0,0 +1,98 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Diagnostics.CodeAnalysis;
+
+using BindingFlags = System.Reflection.BindingFlags;
+
+class ILLinkLinkAttributes
+{
+    public static int Run()
+    {
+        ThrowIfTypeNotPresent(typeof(ILLinkLinkAttributes), nameof(TestDontRemoveAttribute));
+        ThrowIfTypePresent(typeof(ILLinkLinkAttributes), nameof(TestRemoveAttribute));
+        ThrowIfTypePresent(typeof(ILLinkLinkAttributes), nameof(TestMarkAllRemoveAttribute));
+        ThrowIfAttributePresent(typeof(ILLinkLinkAttributes), nameof(_fieldWithCustomAttribute), nameof(TestRemoveAttribute));
+        ThrowIfAttributeNotPresent(typeof(ILLinkLinkAttributes), nameof(_fieldWithCustomAttribute), nameof(TestDontRemoveAttribute));
+        ThrowIfAttributePresent(typeof(ILLinkLinkAttributes), nameof(_fieldWithCustomAttribute), nameof(AllowNullAttribute));
+        return 100;
+    }
+
+    [TestDontRemoveAttribute]
+    [TestRemoveAttribute]
+    [AllowNullAttribute]
+    private string _fieldWithCustomAttribute = "Hello world";
+
+    class TestDontRemoveAttribute : Attribute
+    {
+        public TestDontRemoveAttribute()
+        {
+        }
+    }
+
+    class TestRemoveAttribute : Attribute
+    {
+        public TestRemoveAttribute()
+        {
+        }
+    }
+
+    class TestMarkAllRemoveAttribute : Attribute
+    {
+    }
+
+    [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2070:UnrecognizedReflectionPattern",
+        Justification = "That's the point")]
+    private static bool IsTypePresent(Type testType, string typeName) => testType.GetNestedType(typeName, BindingFlags.NonPublic | BindingFlags.Public) != null;
+
+    private static void ThrowIfTypeNotPresent(Type testType, string typeName)
+    {
+        if (!IsTypePresent(testType, typeName))
+        {
+            throw new Exception(typeName);
+        }
+    }
+
+    private static void ThrowIfTypePresent(Type testType, string typeName)
+    {
+        if (IsTypePresent(testType, typeName))
+        {
+            throw new Exception(typeName);
+        }
+    }
+
+    private static bool IsAttributePresent(Type testType, string memberName, string attributeName)
+    {
+        foreach (var member in testType.GetMembers())
+        {
+            if (member.Name == memberName)
+            {
+                foreach (var attribute in member.GetCustomAttributes(false))
+                {
+                    if (attribute.GetType().Name == attributeName)
+                    {
+                        return true;
+                    }
+                }
+            }
+        }
+        return false;
+    }
+
+    private static void ThrowIfAttributeNotPresent(Type testType, string memberName, string attributeName)
+    {
+        if (!IsAttributePresent(testType, memberName, attributeName))
+        {
+            throw new Exception(attributeName);
+        }
+    }
+
+    private static void ThrowIfAttributePresent(Type testType, string memberName, string attributeName)
+    {
+        if (IsAttributePresent(testType, memberName, attributeName))
+        {
+            throw new Exception(attributeName);
+        }
+    }
+}
index d324f0a..799e3a9 100644 (file)
@@ -4,6 +4,7 @@
     <CLRTestKind>BuildAndRun</CLRTestKind>
     <CLRTestPriority>0</CLRTestPriority>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <NullabilityInfoContextSupport>false</NullabilityInfoContextSupport>
 
     <!-- We don't run the scanner in optimized builds -->
     <CLRTestTargetUnsupported Condition="'$(IlcMultiModule)' == 'true'">true</CLRTestTargetUnsupported>
     <Compile Include="Dataflow.cs" />
     <Compile Include="DeadCodeElimination.cs" />
     <Compile Include="ILLinkDescriptor.cs" />
+    <Compile Include="ILLinkLinkAttributes.cs" />
     <Compile Include="FeatureSwitches.cs" />
     <Compile Include="Main.cs" />
   </ItemGroup>
 
   <ItemGroup>
+    <EmbeddedResource Include="ILLink.LinkAttributes.xml">
+      <LogicalName>ILLink.LinkAttributes.xml</LogicalName>
+    </EmbeddedResource>
     <EmbeddedResource Include="ILLink.Substitutions.xml">
       <LogicalName>ILLink.Substitutions.xml</LogicalName>
     </EmbeddedResource>