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.

Sign up for GitHub

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

Jump to bottom

Add star into link attributes #81407

Merged
merged 7 commits into from
Feb 10, 2023
Merged

Add star into link attributes #81407

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
51281dd
Initial commit of handling the star argument in LinkAttributes files …
Jan 25, 2023
01369e2
Remove linker testing since it tests more unsupported scenarios and n…
Jan 26, 2023
65e887a
Add a way to only process star/nonstar assemblies in ProcessLinkerXml…
Jan 31, 2023
7612cf3
Run formatter
Jan 31, 2023
c905faa
Ask early on if the assembly should be processed, no reason to do wor…
tlakollo Feb 1, 2023
22d114d
Merge branch 'main' into AddStarIntoLinkAttributes
tlakollo Feb 8, 2023
0227865
Actually fix the merge conflict
Feb 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions src/coreclr/tools/Common/Compiler/ProcessLinkerXmlBase.cs
View file
Open in desktop
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 @@ -138,9 +140,7 @@ protected virtual void ProcessAssemblies(XPathNavigator nav)
bool processAllAssemblies = ShouldProcessAllAssemblies(assemblyNav, out AssemblyName? name);
if (processAllAssemblies && AllowedAssemblySelector != AllowedAssemblies.AllAssemblies)
{
// 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);
LogWarning(assemblyNav, DiagnosticId.XmlUnsuportedWildcard);
continue;
}

Expand All @@ -162,12 +162,14 @@ protected virtual void ProcessAssemblies(XPathNavigator nav)
if (!ShouldProcessElement(assemblyNav))
continue;

if (processAllAssemblies)
if (processAllAssemblies && _globalAttributeRemoval)
{
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);
Debug.Assert(_owningModule != null);
ProcessAssembly(_owningModule, assemblyNav, warnOnUnresolvedTypes: false);
}
else if (processAllAssemblies ^ _globalAttributeRemoval)
{
continue;
}
else
{
Expand Down
74 changes: 58 additions & 16 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs
View file
Open in desktop
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 _featureSwitchHashtable;
tlakollo marked this conversation as resolved.
Show resolved Hide resolved

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));
_featureSwitchHashtable = new LinkAttributesHashTable(Logger, new Dictionary<string, bool>(featureSwitchValues));
FeatureSwitches = new Dictionary<string, bool>(featureSwitchValues);

_rootEntireAssembliesModules = new HashSet<string>(rootEntireAssembliesModules);
Expand Down Expand Up @@ -1093,12 +1093,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 @@ -1126,19 +1126,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;
Copy link
Member

Choose a reason for hiding this comment

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

I rarely have to comment on formatting but the IL Linker formatting rules appearing where they shouldn't is starting to be a nuisance (this comment is not directly on your PR - I've commented on these in other PRs of people who work in both codebases in the past weeks, cc @vitek-karas).

There's an undeniable overhead in switching between styles. This is starting to be a friction point in PRs. Formatting never was a friction point. Honestly the linker coding style is creating a problem that I haven't seen in my 10 years on the team.

Suggested change
EcmaModule xmlModule = globalAttributeRemoval ? (EcmaModule) module.Context.SystemModule : module;
EcmaModule xmlModule = globalAttributeRemoval ? (EcmaModule)module.Context.SystemModule : module;

Copy link
Member

Choose a reason for hiding this comment

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

We should switch to linker in runtime repo real soon (hopefully this week). Once that is done I think the first thing we do on the linker is to update the formatting to match runtime rules. And then all of this will be history...

Copy link
Member

Choose a reason for hiding this comment

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

the first thing we do on the linker is to update the formatting to match runtime rules

Before doing that, it might be good to catch up parts of NativeAOT that are line-by-line ports of ILLink and delete ReferenceSource. Reformatting will introduce lots of diffs to ReferenceSource. It's not impossible to do the catchup later, but it will be an extra annoyance. Once it's all caught up, we don't need ReferenceSource anymore - we just have to be diligent in PRs to update both files always.

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 @@ -1150,37 +1161,68 @@ 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
public static class PlatformAssemblies
{
public const string CoreLib = "System.Private.CoreLib";
}

internal sealed class LinkAttributesReader : ProcessLinkerXmlBase
{
private readonly HashSet<TypeDesc> _removedAttributes = new();
private readonly ModuleDesc _resource;
tlakollo marked this conversation as resolved.
Show resolved Hide resolved

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)
{
_resource = resourceAssembly;
}

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 (_resource?.Assembly == null)
return AllowedAssemblies.AllAssemblies;

// Corelib XML may contain assembly wildcard to support compiler-injected attribute types
if (_resource?.Assembly.GetName().Name == PlatformAssemblies.CoreLib)
tlakollo marked this conversation as resolved.
Show resolved Hide resolved
return AllowedAssemblies.AllAssemblies;

return AllowedAssemblies.ContainingAssembly;
}
}

protected override void ProcessAssembly(ModuleDesc assembly, XPathNavigator nav, bool warnOnUnresolvedTypes)
{
ProcessTypes(assembly, nav, warnOnUnresolvedTypes);
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/tools/aot/Mono.Linker.Tests/TestCasesRunner/ILCompilerDriver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,14 @@ public ILScanResults Trim (ILCompilerOptions options, ILogWriter logWriter)

ILProvider ilProvider = new NativeAotILProvider ();

Logger logger = new Logger (logWriter, ilProvider, isVerbose: true);

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, isVerbose: true);

ilProvider = new FeatureSwitchManager (ilProvider, logger, options.FeatureSwitches);

CompilerGeneratedState compilerGeneratedState = new CompilerGeneratedState (ilProvider, logger);
Expand Down
16 changes: 16 additions & 0 deletions src/tests/nativeaot/SmokeTests/TrimmingBehaviors/ILLink.LinkAttributes.xml
View file
Open in desktop
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>
98 changes: 98 additions & 0 deletions src/tests/nativeaot/SmokeTests/TrimmingBehaviors/ILLinkLinkAttributes.cs
View file
Open in desktop
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 ()
tlakollo marked this conversation as resolved.
Show resolved Hide resolved
{
}
}

class TestRemoveAttribute : Attribute
{
public TestRemoveAttribute ()
tlakollo marked this conversation as resolved.
Show resolved Hide resolved
{
}
}

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);
}
}
}
5 changes: 5 additions & 0 deletions src/tests/nativeaot/SmokeTests/TrimmingBehaviors/TrimmingBehaviors.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -12,11 +13,15 @@
<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>
Expand Down