From cae5ab86deafa02dae014fe395001bdb92784435 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Mon, 8 Jul 2024 18:03:26 +0000 Subject: [PATCH 1/3] Rebuild override info after custom step runs --- .../src/linker/Linker.Steps/MarkStep.cs | 9 ++++- .../src/linker/Linker/DictionaryExtensions.cs | 17 ++++++--- .../src/linker/Linker/InterfaceImplementor.cs | 17 ++++++++- .../src/linker/Linker/OverrideInformation.cs | 24 +++++++++++- .../illink/src/linker/Linker/TypeMapInfo.cs | 38 +++++++++++-------- .../CustomStepCanFixAbstractMethods.cs | 5 +-- 6 files changed, 82 insertions(+), 28 deletions(-) diff --git a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs index 990065fa69c3d8..6866369e59b527 100644 --- a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs +++ b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs @@ -2012,8 +2012,13 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in var typeOrigin = new MessageOrigin (type); - foreach (Action handleMarkType in MarkContext.MarkTypeActions) - handleMarkType (type); + if (MarkContext.MarkTypeActions.Count > 0) { + foreach (Action handleMarkType in MarkContext.MarkTypeActions) + handleMarkType (type); + + // Rebuild type info for the type in case a mark action added new methods. + Annotations.TypeMapInfo.MapType (type); + } MarkType (type.BaseType, new DependencyInfo (DependencyKind.BaseType, type), typeOrigin); diff --git a/src/tools/illink/src/linker/Linker/DictionaryExtensions.cs b/src/tools/illink/src/linker/Linker/DictionaryExtensions.cs index 9cf8945fe480c5..e92f945ef84585 100644 --- a/src/tools/illink/src/linker/Linker/DictionaryExtensions.cs +++ b/src/tools/illink/src/linker/Linker/DictionaryExtensions.cs @@ -2,19 +2,26 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.Collections.Generic; +using System.Diagnostics; +using System.Linq; namespace Mono.Linker { internal static class DictionaryExtensions { - public static void AddToList (this Dictionary> me, TKey key, TElement value) + public static void AddToSet (this Dictionary> me, TKey key, TElement value) where TKey : notnull { - if (!me.TryGetValue (key, out List? valueList)) { - valueList = new (); - me[key] = valueList; + if (!me.TryGetValue (key, out HashSet? valueSet)) { + valueSet = new (); + me[key] = valueSet; } - valueList.Add (value); + if (valueSet.ToList().Count == 1) { + + var b = valueSet.ToList()[0]?.Equals (value); + Debug.WriteLine(b); + } + valueSet.Add (value); } } } diff --git a/src/tools/illink/src/linker/Linker/InterfaceImplementor.cs b/src/tools/illink/src/linker/Linker/InterfaceImplementor.cs index e981ce872703f7..8d077a5e5a8c82 100644 --- a/src/tools/illink/src/linker/Linker/InterfaceImplementor.cs +++ b/src/tools/illink/src/linker/Linker/InterfaceImplementor.cs @@ -9,7 +9,7 @@ namespace Mono.Linker { - public class InterfaceImplementor + public class InterfaceImplementor : IEquatable { /// /// The type that implements . @@ -55,5 +55,20 @@ public static InterfaceImplementor Create(TypeDefinition implementor, TypeDefini } throw new InvalidOperationException ($"Type '{implementor.FullName}' does not implement interface '{interfaceType.FullName}' directly or through any interfaces"); } + + public bool Equals (InterfaceImplementor? other) + { + if (other is null) + return false; + + if (ReferenceEquals (this, other)) + return true; + + return Implementor == other.Implementor && InterfaceImplementation == other.InterfaceImplementation && InterfaceType == other.InterfaceType; + } + + public override bool Equals (object? obj) => obj is InterfaceImplementor other && Equals (other); + + public override int GetHashCode () => HashCode.Combine (Implementor, InterfaceImplementation, InterfaceType); } } diff --git a/src/tools/illink/src/linker/Linker/OverrideInformation.cs b/src/tools/illink/src/linker/Linker/OverrideInformation.cs index 0727d5d25c19a0..2dee67a9c4aa84 100644 --- a/src/tools/illink/src/linker/Linker/OverrideInformation.cs +++ b/src/tools/illink/src/linker/Linker/OverrideInformation.cs @@ -1,6 +1,7 @@ // 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; using Mono.Cecil; using System.Diagnostics.CodeAnalysis; @@ -8,7 +9,7 @@ namespace Mono.Linker { [DebuggerDisplay ("{Override}")] - public class OverrideInformation + public class OverrideInformation : IEquatable { public MethodDefinition Base { get; } @@ -37,5 +38,26 @@ public TypeDefinition? InterfaceType [MemberNotNullWhen (true, nameof (InterfaceImplementor), nameof (MatchingInterfaceImplementation))] public bool IsOverrideOfInterfaceMember => InterfaceImplementor != null; + + public bool Equals (OverrideInformation? other) + { + if (other is null) + return false; + + if (ReferenceEquals (this, other)) + return true; + + if (Base != other.Base || Override != other.Override) + return false; + + if (InterfaceImplementor == null) + return other.InterfaceImplementor == null; + + return InterfaceImplementor.Equals (other.InterfaceImplementor); + } + + public override bool Equals (object? obj) => obj is OverrideInformation other && Equals (other); + + public override int GetHashCode () => HashCode.Combine (Base, Override, InterfaceImplementor); } } diff --git a/src/tools/illink/src/linker/Linker/TypeMapInfo.cs b/src/tools/illink/src/linker/Linker/TypeMapInfo.cs index 1d7b24e070d148..9303b19dc9945a 100644 --- a/src/tools/illink/src/linker/Linker/TypeMapInfo.cs +++ b/src/tools/illink/src/linker/Linker/TypeMapInfo.cs @@ -1,4 +1,4 @@ -// Copyright (c) .NET Foundation and contributors. All rights reserved. +// 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. // @@ -43,9 +43,10 @@ public class TypeMapInfo { readonly HashSet assemblies = new HashSet (); readonly LinkContext context; - protected readonly Dictionary> base_methods = new Dictionary> (); - protected readonly Dictionary> override_methods = new Dictionary> (); - protected readonly Dictionary> default_interface_implementations = new Dictionary> (); + protected readonly Dictionary> base_methods = new Dictionary> (); + protected readonly Dictionary> override_methods = new Dictionary> (); + protected readonly Dictionary> default_interface_implementations = new Dictionary> (); + readonly Dictionary)>> interfaces = new (); public TypeMapInfo (LinkContext context) { @@ -58,7 +59,7 @@ public void EnsureProcessed (AssemblyDefinition assembly) return; foreach (TypeDefinition type in assembly.MainModule.Types) - MapType (type); + MapTypes (type); } public ICollection MethodsWithOverrideInformation => override_methods.Keys; @@ -69,8 +70,8 @@ public void EnsureProcessed (AssemblyDefinition assembly) public List? GetOverrides (MethodDefinition method) { EnsureProcessed (method.Module.Assembly); - override_methods.TryGetValue (method, out List? overrides); - return overrides; + override_methods.TryGetValue (method, out HashSet? overrides); + return overrides?.ToList (); } /// @@ -83,8 +84,8 @@ public void EnsureProcessed (AssemblyDefinition assembly) public List? GetBaseMethods (MethodDefinition method) { EnsureProcessed (method.Module.Assembly); - base_methods.TryGetValue (method, out List? bases); - return bases; + base_methods.TryGetValue (method, out HashSet? bases); + return bases?.ToList (); } /// @@ -102,32 +103,37 @@ public void EnsureProcessed (AssemblyDefinition assembly) public void AddBaseMethod (MethodDefinition method, MethodDefinition @base, InterfaceImplementor? interfaceImplementor) { - base_methods.AddToList (method, new OverrideInformation (@base, method, interfaceImplementor)); + base_methods.AddToSet (method, new OverrideInformation (@base, method, interfaceImplementor)); } public void AddOverride (MethodDefinition @base, MethodDefinition @override, InterfaceImplementor? interfaceImplementor = null) { - override_methods.AddToList (@base, new OverrideInformation (@base, @override, interfaceImplementor)); + override_methods.AddToSet (@base, new OverrideInformation (@base, @override, interfaceImplementor)); } public void AddDefaultInterfaceImplementation (MethodDefinition @base, InterfaceImplementor interfaceImplementor, MethodDefinition defaultImplementationMethod) { Debug.Assert(@base.DeclaringType.IsInterface); - default_interface_implementations.AddToList (@base, new OverrideInformation (@base, defaultImplementationMethod, interfaceImplementor)); + default_interface_implementations.AddToSet (@base, new OverrideInformation (@base, defaultImplementationMethod, interfaceImplementor)); } - Dictionary)>> interfaces = new (); - protected virtual void MapType (TypeDefinition type) + public void MapType (TypeDefinition type) { MapVirtualMethods (type); MapInterfaceMethodsInTypeHierarchy (type); - interfaces[type] = GetRecursiveInterfaceImplementations (type); + if (!interfaces.ContainsKey (type)) + interfaces[type] = GetRecursiveInterfaceImplementations (type); + } + + protected virtual void MapTypes (TypeDefinition type) + { + MapType (type); if (!type.HasNestedTypes) return; foreach (var nested in type.NestedTypes) - MapType (nested); + MapTypes (nested); } internal List<(TypeReference InterfaceType, List ImplementationChain)>? GetRecursiveInterfaces (TypeDefinition type) diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/CustomStepCanFixAbstractMethods.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/CustomStepCanFixAbstractMethods.cs index 230f621cbd9d5b..6833d0b4afacab 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/CustomStepCanFixAbstractMethods.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/CustomStepCanFixAbstractMethods.cs @@ -29,7 +29,6 @@ static void TestReflectionAccessToOtherAssembly () // to be created through reflection instead of a direct call to the constructor, otherwise we build the // TypeMapInfo cache too early for the custom step. - // var type = typeof (InterfaceImplementation); var type = typeof (InterfaceImplementationInOtherAssembly); InterfaceType instance = (InterfaceType) System.Activator.CreateInstance (type); InterfaceType.UseInstance (instance); @@ -46,7 +45,7 @@ static void TestReflectionAccess () [Kept] [KeptMember (".ctor()")] [KeptInterface (typeof (InterfaceType))] - // [CreatedMember ("AbstractMethod()")] // https://github.com/dotnet/runtime/issues/104266 + [CreatedMember ("AbstractMethod()")] class InterfaceImplementationAccessedViaReflection : InterfaceType { } @@ -61,7 +60,7 @@ static void TestDirectAccess () [Kept] [KeptMember (".ctor()")] [KeptInterface (typeof (InterfaceType))] - // [CreatedMember ("AbstractMethod()")] // https://github.com/dotnet/runtime/issues/104266 + [CreatedMember ("AbstractMethod()")] class InterfaceImplementation : InterfaceType { } From 81c39283b102810841f57f6a87c74a4eb7cafb89 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Mon, 8 Jul 2024 18:42:31 +0000 Subject: [PATCH 2/3] Fix ApiCompat --- src/tools/illink/src/linker/CompatibilitySuppressions.xml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/tools/illink/src/linker/CompatibilitySuppressions.xml b/src/tools/illink/src/linker/CompatibilitySuppressions.xml index af38ca258f95ba..5aa12066312a41 100644 --- a/src/tools/illink/src/linker/CompatibilitySuppressions.xml +++ b/src/tools/illink/src/linker/CompatibilitySuppressions.xml @@ -1533,6 +1533,12 @@ CP0008 T:Mono.Linker.MessageOrigin + + CP0008 + T:Mono.Linker.OverrideInformation + ref/net9.0/illink.dll + lib/net9.0/illink.dll + CP0009 T:Mono.Linker.AnnotationStore From bae67ca1566c0789c400aa62e47bd77f20ea67ad Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Fri, 6 Sep 2024 10:14:59 -0700 Subject: [PATCH 3/3] Only rebuild when there is a custom IMarkHandler --- .../src/linker/Linker.Steps/MarkStep.cs | 6 ++-- src/tools/illink/src/linker/Linker/Driver.cs | 35 ++----------------- .../illink/src/linker/Linker/LinkContext.cs | 2 ++ 3 files changed, 8 insertions(+), 35 deletions(-) diff --git a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs index 6866369e59b527..e51b21c007fb12 100644 --- a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs +++ b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs @@ -2016,8 +2016,10 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in foreach (Action handleMarkType in MarkContext.MarkTypeActions) handleMarkType (type); - // Rebuild type info for the type in case a mark action added new methods. - Annotations.TypeMapInfo.MapType (type); + if (Context.HasCustomMarkHandler) { + // Rebuild type info for the type in case a mark action added new methods. + Annotations.TypeMapInfo.MapType (type); + } } MarkType (type.BaseType, new DependencyInfo (DependencyKind.BaseType, type), typeOrigin); diff --git a/src/tools/illink/src/linker/Linker/Driver.cs b/src/tools/illink/src/linker/Linker/Driver.cs index 5b0c6971a06e9a..e509316d7ce0ef 100644 --- a/src/tools/illink/src/linker/Linker/Driver.cs +++ b/src/tools/illink/src/linker/Linker/Driver.cs @@ -942,19 +942,6 @@ protected virtual void AddDgmlDependencyRecorder (LinkContext context, string? f context.Tracer.AddRecorder (new DgmlDependencyRecorder (context, fileName)); } - protected bool AddMarkHandler (Pipeline pipeline, string arg) - { - if (!TryGetCustomAssembly (ref arg, out Assembly? custom_assembly)) - return false; - - var step = ResolveStep (arg, custom_assembly); - if (step == null) - return false; - - pipeline.AppendMarkHandler (step); - return true; - } - bool TryGetCustomAssembly (ref string arg, [NotNullWhen (true)] out Assembly? assembly) { assembly = null; @@ -1028,6 +1015,7 @@ protected bool AddCustomStep (Pipeline pipeline, string arg) var customStep = (IMarkHandler?) Activator.CreateInstance (stepType) ?? throw new InvalidOperationException (); if (targetName == null) { pipeline.AppendMarkHandler (customStep); + Context.HasCustomMarkHandler = true; return true; } @@ -1042,6 +1030,7 @@ protected bool AddCustomStep (Pipeline pipeline, string arg) else pipeline.AddMarkHandlerAfter (target, customStep); + Context.HasCustomMarkHandler = true; return true; } @@ -1086,26 +1075,6 @@ protected bool AddCustomStep (Pipeline pipeline, string arg) return step; } - TStep? ResolveStep (string type, Assembly assembly) where TStep : class - { - // Ignore warning, since we're just enabling analyzer for dogfooding -#pragma warning disable IL2026 - Type? step = assembly != null ? assembly.GetType (type) : Type.GetType (type, false); -#pragma warning restore IL2026 - - if (step == null) { - Context.LogError (null, DiagnosticId.CustomStepTypeCouldNotBeFound, type); - return null; - } - - if (!typeof (TStep).IsAssignableFrom (step)) { - Context.LogError (null, DiagnosticId.CustomStepTypeIsIncompatibleWithLinkerVersion, type); - return null; - } - - return (TStep?) Activator.CreateInstance (step); - } - static string[] GetFiles (string param) { if (param.Length < 1 || param[0] != '@') diff --git a/src/tools/illink/src/linker/Linker/LinkContext.cs b/src/tools/illink/src/linker/Linker/LinkContext.cs index 31733b0c6ad565..64c6586eb56aee 100644 --- a/src/tools/illink/src/linker/Linker/LinkContext.cs +++ b/src/tools/illink/src/linker/Linker/LinkContext.cs @@ -184,6 +184,8 @@ internal TypeNameResolver TypeNameResolver { public string? AssemblyListFile { get; set; } + internal bool HasCustomMarkHandler { get; set; } + public List MarkHandlers { get; } public Dictionary SingleWarn { get; set; }