Skip to content

Commit

Permalink
Add star into link attributes (#81407)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
tlakollo authored Feb 10, 2023
1 parent f6b9578 commit 98514de
Show file tree
Hide file tree
Showing 6 changed files with 209 additions and 51 deletions.
60 changes: 32 additions & 28 deletions src/coreclr/tools/Common/Compiler/ProcessLinkerXmlBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public abstract class ProcessLinkerXmlBase
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;

Expand All @@ -70,10 +71,11 @@ protected ProcessLinkerXmlBase(Logger logger, TypeSystemContext context, Stream
_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);
Expand Down Expand Up @@ -133,44 +135,46 @@ protected virtual void ProcessAssemblies(XPathNavigator nav)
{
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);

Expand Down Expand Up @@ -469,8 +473,8 @@ protected virtual void ProcessProperty(TypeDesc type, XPathNavigator nav, object
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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public sealed class UsageBasedMetadataManager : GeneratingMetadataManager

internal readonly UsageBasedMetadataGenerationOptions _generationOptions;

private readonly FeatureSwitchHashtable _featureSwitchHashtable;
private readonly LinkAttributesHashTable _linkAttributesHashTable;

private static (string AttributeName, DiagnosticId Id)[] _requiresAttributeMismatchNameAndId = new[]
{
Expand Down Expand Up @@ -91,7 +91,7 @@ public UsageBasedMetadataManager(
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);
Expand Down Expand Up @@ -821,7 +821,7 @@ public bool GeneratesAttributeMetadata(TypeDesc attributeType)
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);
}

Expand Down Expand Up @@ -1070,12 +1070,12 @@ public bool IsBlocked(MethodDesc methodDef)
}
}

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;
Expand Down Expand Up @@ -1103,19 +1103,30 @@ public AssemblyFeatureInfo(EcmaModule module, Logger logger, IReadOnlyDictionary
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)
{
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);
Expand All @@ -1127,37 +1138,61 @@ public AssemblyFeatureInfo(EcmaModule module, Logger logger, IReadOnlyDictionary
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,6 @@ public ILScanResults Trim (ILCompilerOptions options, ILogWriter logWriter)

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,
Expand All @@ -82,6 +76,12 @@ public ILScanResults Trim (ILCompilerOptions options, ILogWriter logWriter)
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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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>
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
Loading

0 comments on commit 98514de

Please sign in to comment.