From 6e01ad7bdb8419c84a734d6f714f2ebf089ed541 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 18 Oct 2022 14:08:49 -0700 Subject: [PATCH 1/5] Cache ExportedType.Resolve() --- src/linker/Linker/LinkContext.cs | 25 +++++++++++++++++++ src/linker/Linker/MarkingHelpers.cs | 4 +-- .../Linker/ModuleDefinitionExtensions.cs | 7 +++--- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/linker/Linker/LinkContext.cs b/src/linker/Linker/LinkContext.cs index 5a66c4566bf0..686863501f90 100644 --- a/src/linker/Linker/LinkContext.cs +++ b/src/linker/Linker/LinkContext.cs @@ -753,6 +753,7 @@ public int GetTargetRuntimeVersion () readonly Dictionary methodresolveCache = new (); readonly Dictionary fieldresolveCache = new (); readonly Dictionary typeresolveCache = new (); + readonly Dictionary exportedTypeResolveCache = new (); public MethodDefinition? Resolve (MethodReference methodReference) { @@ -881,6 +882,25 @@ public int GetTargetRuntimeVersion () return td; } + public TypeDefinition? Resolve (ExportedType et) + { + if (TryResolve (et) is not TypeDefinition td) { + ReportUnresolved (et); + return null; + } + return td; + } + + public TypeDefinition? TryResolve (ExportedType et) + { + if (exportedTypeResolveCache.TryGetValue (et, out var td)) { + return td; + } + td = et.Resolve (); + exportedTypeResolveCache.Add (et, td); + return td; + } + public TypeDefinition? TryResolve (AssemblyDefinition assembly, string typeNameString) { // It could be cached if it shows up on fast path @@ -908,6 +928,11 @@ protected virtual void ReportUnresolved (TypeReference typeReference) if (unresolved_reported.Add (typeReference)) LogError (string.Format (SharedStrings.FailedToResolveTypeElementMessage, typeReference.GetDisplayName ()), (int) DiagnosticId.FailedToResolveMetadataElement); } + + protected virtual void ReportUnresolved (ExportedType typeReference) + { + LogError (string.Format (SharedStrings.FailedToResolveTypeElementMessage, typeReference.Name), (int) DiagnosticId.FailedToResolveMetadataElement); + } } public class CodeOptimizationsSettings diff --git a/src/linker/Linker/MarkingHelpers.cs b/src/linker/Linker/MarkingHelpers.cs index b99351c377ad..746722625ecd 100644 --- a/src/linker/Linker/MarkingHelpers.cs +++ b/src/linker/Linker/MarkingHelpers.cs @@ -19,7 +19,7 @@ public void MarkMatchingExportedType (TypeDefinition typeToMatch, AssemblyDefini if (typeToMatch == null || assembly == null) return; - if (assembly.MainModule.GetMatchingExportedType (typeToMatch, out var exportedType)) + if (assembly.MainModule.GetMatchingExportedType (typeToMatch, _context, out var exportedType)) MarkExportedType (exportedType, assembly.MainModule, reason, origin); } @@ -40,7 +40,7 @@ public void MarkForwardedScope (TypeReference typeReference, in MessageOrigin or var assembly = _context.Resolve (typeReference.Scope); if (assembly != null && _context.TryResolve (typeReference) is TypeDefinition typeDefinition && - assembly.MainModule.GetMatchingExportedType (typeDefinition, out var exportedType)) + assembly.MainModule.GetMatchingExportedType (typeDefinition, _context, out var exportedType)) MarkExportedType (exportedType, assembly.MainModule, new DependencyInfo (DependencyKind.ExportedType, typeReference), origin); } } diff --git a/src/linker/Linker/ModuleDefinitionExtensions.cs b/src/linker/Linker/ModuleDefinitionExtensions.cs index 2fab23e9e5e6..64ad1df836f6 100644 --- a/src/linker/Linker/ModuleDefinitionExtensions.cs +++ b/src/linker/Linker/ModuleDefinitionExtensions.cs @@ -15,17 +15,18 @@ public static bool IsCrossgened (this ModuleDefinition module) (module.Attributes & ModuleAttributes.ILLibrary) != 0; } - public static bool GetMatchingExportedType (this ModuleDefinition module, TypeDefinition typeDefinition, [NotNullWhen (true)] out ExportedType? exportedType) + public static bool GetMatchingExportedType (this ModuleDefinition module, TypeDefinition typeDefinition, LinkContext context, [NotNullWhen (true)] out ExportedType? exportedType) { exportedType = null; if (!module.HasExportedTypes) return false; - foreach (var et in module.ExportedTypes) - if (et.Resolve () == typeDefinition) { + foreach (var et in module.ExportedTypes) { + if (context.TryResolve (et) == typeDefinition) { exportedType = et; return true; } + } return false; } From 3e45b14d9a9f3e59b91cc0446ac18c03cd5a1dfa Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 18 Oct 2022 14:32:19 -0700 Subject: [PATCH 2/5] Ban Cecil's Resolve and replace calls to the cached calls --- src/linker/BannedSymbols.txt | 2 ++ src/linker/Linker.Steps/CodeRewriterStep.cs | 2 +- .../MarkExportedTypesTargetStep.cs | 2 +- .../Linker.Steps/ProcessLinkerXmlBase.cs | 2 +- src/linker/Linker.Steps/SweepStep.cs | 21 +++++++----- src/linker/Linker/LinkContext.cs | 34 +++++++++++++++++-- .../Linker/MethodReferenceExtensions.cs | 1 + src/linker/Linker/TypeReferenceExtensions.cs | 10 +++--- 8 files changed, 55 insertions(+), 19 deletions(-) diff --git a/src/linker/BannedSymbols.txt b/src/linker/BannedSymbols.txt index 6ef58b3b0f17..d7c70af181c2 100644 --- a/src/linker/BannedSymbols.txt +++ b/src/linker/BannedSymbols.txt @@ -1,4 +1,6 @@ T:Mono.Cecil.Cil.ILProcessor;Use LinkerILProcessor instead M:Mono.Cecil.TypeReference.Resolve();Use LinkContext.Resolve and LinkContext.TryResolve helpers instead +M:Mono.Cecil.MethodReference.Resolve();Use LinkContext.Resolve and LinkContext.TryResolve helpers instead +M:Mono.Cecil.ExportedType.Resolve();Use LinkContext.Resolve and LinkContext.TryResolve helpers instead P:Mono.Collections.Generic.Collection`1{Mono.Cecil.ParameterDefinition}.Item(System.Int32); use x P:Mono.Cecil.ParameterDefinitionCollection.Item(System.Int32); use x diff --git a/src/linker/Linker.Steps/CodeRewriterStep.cs b/src/linker/Linker.Steps/CodeRewriterStep.cs index 716c1fd1ea8f..c71624451ff4 100644 --- a/src/linker/Linker.Steps/CodeRewriterStep.cs +++ b/src/linker/Linker.Steps/CodeRewriterStep.cs @@ -166,7 +166,7 @@ MethodBody CreateStubBody (MethodDefinition method) if (baseType is null) return body; - MethodReference base_ctor = baseType.GetDefaultInstanceConstructor (); + MethodReference base_ctor = baseType.GetDefaultInstanceConstructor (Context); if (base_ctor == null) throw new NotSupportedException ($"Cannot replace constructor for '{method.DeclaringType}' when no base default constructor exists"); diff --git a/src/linker/Linker.Steps/MarkExportedTypesTargetStep.cs b/src/linker/Linker.Steps/MarkExportedTypesTargetStep.cs index 0d14f634fa65..1c319a20b455 100644 --- a/src/linker/Linker.Steps/MarkExportedTypesTargetStep.cs +++ b/src/linker/Linker.Steps/MarkExportedTypesTargetStep.cs @@ -25,7 +25,7 @@ static void InitializeExportedType (ExportedType exportedType, LinkContext conte if (!context.Annotations.TryGetPreservedMembers (exportedType, out TypePreserveMembers members)) return; - TypeDefinition type = exportedType.Resolve (); + TypeDefinition? type = context.TryResolve (exportedType); if (type == null) { if (!context.IgnoreUnresolved) context.LogError (null, DiagnosticId.ExportedTypeCannotBeResolved, exportedType.Name); diff --git a/src/linker/Linker.Steps/ProcessLinkerXmlBase.cs b/src/linker/Linker.Steps/ProcessLinkerXmlBase.cs index bde82c464125..6184a30330ea 100644 --- a/src/linker/Linker.Steps/ProcessLinkerXmlBase.cs +++ b/src/linker/Linker.Steps/ProcessLinkerXmlBase.cs @@ -191,7 +191,7 @@ protected virtual void ProcessTypes (AssemblyDefinition assembly, XPathNavigator } } - protected virtual TypeDefinition? ProcessExportedType (ExportedType exported, AssemblyDefinition assembly, XPathNavigator nav) => exported.Resolve (); + protected virtual TypeDefinition? ProcessExportedType (ExportedType exported, AssemblyDefinition assembly, XPathNavigator nav) => _context.TryResolve (exported); void MatchType (TypeDefinition type, Regex regex, XPathNavigator nav) { diff --git a/src/linker/Linker.Steps/SweepStep.cs b/src/linker/Linker.Steps/SweepStep.cs index 5da72ea659d7..b75d99eaca8f 100644 --- a/src/linker/Linker.Steps/SweepStep.cs +++ b/src/linker/Linker.Steps/SweepStep.cs @@ -69,7 +69,7 @@ protected override void Process () case AssemblyAction.CopyUsed: case AssemblyAction.Link: case AssemblyAction.Save: - bool changed = AssemblyReferencesCorrector.SweepAssemblyReferences (assembly); + bool changed = AssemblyReferencesCorrector.SweepAssemblyReferences (assembly, Context); if (changed && action == AssemblyAction.CopyUsed) Annotations.SetAction (assembly, AssemblyAction.Save); break; @@ -174,7 +174,7 @@ protected void ProcessAssemblyAction (AssemblyDefinition assembly) AssemblyAction assemblyAction = AssemblyAction.Copy; if (SweepTypeForwarders (assembly)) { // Need to sweep references, in case sweeping type forwarders removed any - AssemblyReferencesCorrector.SweepAssemblyReferences (assembly); + AssemblyReferencesCorrector.SweepAssemblyReferences (assembly, Context); assemblyAction = AssemblyAction.Save; } @@ -194,7 +194,7 @@ protected void ProcessAssemblyAction (AssemblyDefinition assembly) case AssemblyAction.Save: if (SweepTypeForwarders (assembly)) { // Need to sweep references, in case sweeping type forwarders removed any - AssemblyReferencesCorrector.SweepAssemblyReferences (assembly); + AssemblyReferencesCorrector.SweepAssemblyReferences (assembly, Context); } break; } @@ -242,7 +242,7 @@ protected virtual void SweepAssembly (AssemblyDefinition assembly) } if (SweepTypeForwarders (assembly) || updateScopes) - AssemblyReferencesCorrector.SweepAssemblyReferences (assembly); + AssemblyReferencesCorrector.SweepAssemblyReferences (assembly, Context); } bool IsMarkedAssembly (AssemblyDefinition assembly) @@ -460,7 +460,7 @@ void SweepOverrides (MethodDefinition method) // ov is in a `link` scope and is unmarked // ShouldRemove returns true if the method is unmarked, but we also We need to make sure the override is in a link scope. // Only things in a link scope are marked, so ShouldRemove is only valid for items in a `link` scope. - if (method.Overrides[i].Resolve () is not MethodDefinition ov || ov.DeclaringType is null || (IsLinkScope (ov.DeclaringType.Scope) && ShouldRemove (ov))) + if (Context.TryResolve (method.Overrides[i]) is not MethodDefinition ov || ov.DeclaringType is null || (IsLinkScope (ov.DeclaringType.Scope) && ShouldRemove (ov))) method.Overrides.RemoveAt (i); else i++; @@ -570,13 +570,16 @@ sealed class AssemblyReferencesCorrector : TypeReferenceWalker bool changedAnyScopes; - AssemblyReferencesCorrector (AssemblyDefinition assembly) : base (assembly) + readonly LinkContext _context; + + AssemblyReferencesCorrector (AssemblyDefinition assembly, LinkContext context) : base (assembly) { this.importer = new DefaultMetadataImporter (assembly.MainModule); + this._context = context; changedAnyScopes = false; } - public static bool SweepAssemblyReferences (AssemblyDefinition assembly) + public static bool SweepAssemblyReferences (AssemblyDefinition assembly, LinkContext context) { // // We used to run over list returned by GetTypeReferences but @@ -586,7 +589,7 @@ public static bool SweepAssemblyReferences (AssemblyDefinition assembly) // assembly.MainModule.AssemblyReferences.Clear (); - var arc = new AssemblyReferencesCorrector (assembly); + var arc = new AssemblyReferencesCorrector (assembly, context); arc.Process (); return arc.changedAnyScopes; @@ -627,7 +630,7 @@ protected override void ProcessTypeReference (TypeReference type) protected override void ProcessExportedType (ExportedType exportedType) { - TypeDefinition td = exportedType.Resolve (); + TypeDefinition? td = _context.TryResolve (exportedType); if (td == null) { // Forwarded type cannot be resolved but it was marked // linker is running in --skip-unresolved true mode diff --git a/src/linker/Linker/LinkContext.cs b/src/linker/Linker/LinkContext.cs index 686863501f90..d3b17dd50b05 100644 --- a/src/linker/Linker/LinkContext.cs +++ b/src/linker/Linker/LinkContext.cs @@ -755,6 +755,9 @@ public int GetTargetRuntimeVersion () readonly Dictionary typeresolveCache = new (); readonly Dictionary exportedTypeResolveCache = new (); + /// + /// Tries to resolve the MethodReference to a MethodDefinition and logs a warning if it can't + /// public MethodDefinition? Resolve (MethodReference methodReference) { if (methodReference is MethodDefinition methodDefinition) @@ -766,7 +769,9 @@ public int GetTargetRuntimeVersion () if (methodresolveCache.TryGetValue (methodReference, out MethodDefinition? md)) return md; +#pragma warning disable RS0030 // Cecil's resolve is banned -- this provides the wrapper md = methodReference.Resolve (); +#pragma warning restore RS0030 if (md == null && !IgnoreUnresolved) ReportUnresolved (methodReference); @@ -774,6 +779,9 @@ public int GetTargetRuntimeVersion () return md; } + /// + /// Tries to resolve the MethodReference to a MethodDefinition and returns null if it can't + /// public MethodDefinition? TryResolve (MethodReference methodReference) { if (methodReference is MethodDefinition methodDefinition) @@ -785,11 +793,16 @@ public int GetTargetRuntimeVersion () if (methodresolveCache.TryGetValue (methodReference, out MethodDefinition? md)) return md; +#pragma warning disable RS0030 // Cecil's resolve is banned -- this method provides the wrapper md = methodReference.Resolve (); +#pragma warning restore RS0030 methodresolveCache.Add (methodReference, md); return md; } + /// + /// Tries to resolve the FieldReference to a FieldDefinition and logs a warning if it can't + /// public FieldDefinition? Resolve (FieldReference fieldReference) { if (fieldReference is FieldDefinition fieldDefinition) @@ -809,6 +822,9 @@ public int GetTargetRuntimeVersion () return fd; } + /// + /// Tries to resolve the FieldReference to a FieldDefinition and returns null if it can't + /// public FieldDefinition? TryResolve (FieldReference fieldReference) { if (fieldReference is FieldDefinition fieldDefinition) @@ -825,6 +841,9 @@ public int GetTargetRuntimeVersion () return fd; } + /// + /// Tries to resolve the TypeReference to a TypeDefinition and logs a warning if it can't + /// public TypeDefinition? Resolve (TypeReference typeReference) { if (typeReference is TypeDefinition typeDefinition) @@ -852,6 +871,9 @@ public int GetTargetRuntimeVersion () return td; } + /// + /// Tries to resolve the TypeReference to a TypeDefinition and returns null if it can't + /// public TypeDefinition? TryResolve (TypeReference typeReference) { if (typeReference is TypeDefinition typeDefinition) @@ -882,6 +904,9 @@ public int GetTargetRuntimeVersion () return td; } + /// + /// Tries to resolve the ExportedType to a TypeDefinition and logs a warning if it can't + /// public TypeDefinition? Resolve (ExportedType et) { if (TryResolve (et) is not TypeDefinition td) { @@ -891,12 +916,17 @@ public int GetTargetRuntimeVersion () return td; } + /// + /// Tries to resolve the ExportedType to a TypeDefinition and returns null if it can't + /// public TypeDefinition? TryResolve (ExportedType et) { if (exportedTypeResolveCache.TryGetValue (et, out var td)) { return td; } +#pragma warning disable RS0030 // Cecil's Resolve is banned -- this method provides the wrapper td = et.Resolve (); +#pragma warning restore RS0030 exportedTypeResolveCache.Add (et, td); return td; } @@ -929,9 +959,9 @@ protected virtual void ReportUnresolved (TypeReference typeReference) LogError (string.Format (SharedStrings.FailedToResolveTypeElementMessage, typeReference.GetDisplayName ()), (int) DiagnosticId.FailedToResolveMetadataElement); } - protected virtual void ReportUnresolved (ExportedType typeReference) + protected virtual void ReportUnresolved (ExportedType et) { - LogError (string.Format (SharedStrings.FailedToResolveTypeElementMessage, typeReference.Name), (int) DiagnosticId.FailedToResolveMetadataElement); + LogError (string.Format (SharedStrings.FailedToResolveTypeElementMessage, et.Name), (int) DiagnosticId.FailedToResolveMetadataElement); } } diff --git a/src/linker/Linker/MethodReferenceExtensions.cs b/src/linker/Linker/MethodReferenceExtensions.cs index 3bbe6816ac10..9d1806f41e90 100644 --- a/src/linker/Linker/MethodReferenceExtensions.cs +++ b/src/linker/Linker/MethodReferenceExtensions.cs @@ -10,6 +10,7 @@ namespace Mono.Linker { public static class MethodReferenceExtensions { + [System.Diagnostics.CodeAnalysis.SuppressMessage ("ApiDesign", "RS0030:Do not used banned APIs", Justification = "MethodReference.Resolve is banned for performance reasons. GetDisplayName should be a really cold path and shouldn't require a context to call.")] public static string GetDisplayName (this MethodReference method) { var sb = new System.Text.StringBuilder (); diff --git a/src/linker/Linker/TypeReferenceExtensions.cs b/src/linker/Linker/TypeReferenceExtensions.cs index 04dece0f92e9..17fcc766b516 100644 --- a/src/linker/Linker/TypeReferenceExtensions.cs +++ b/src/linker/Linker/TypeReferenceExtensions.cs @@ -305,13 +305,13 @@ public static string ToCecilName (this string fullTypeName) return fullTypeName.Replace ('+', '/'); } - public static bool HasDefaultConstructor (this TypeDefinition type) + public static bool HasDefaultConstructor (this TypeDefinition type, LinkContext context) { foreach (var m in type.Methods) { if (m.HasParameters) continue; - var definition = m.Resolve (); + var definition = context.Resolve (m); if (definition?.IsDefaultConstructor () == true) return true; } @@ -319,14 +319,14 @@ public static bool HasDefaultConstructor (this TypeDefinition type) return false; } - public static MethodReference GetDefaultInstanceConstructor (this TypeDefinition type) + public static MethodReference GetDefaultInstanceConstructor (this TypeDefinition type, LinkContext context) { foreach (var m in type.Methods) { if (m.HasParameters) continue; - var definition = m.Resolve (); - if (!definition.IsDefaultConstructor ()) + var definition = context.Resolve (m); + if (definition?.IsDefaultConstructor () != true) continue; return m; From 4b17eb118210023cc95a2a01b91528a6930da137 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 18 Oct 2022 15:27:11 -0700 Subject: [PATCH 3/5] Don't use cached resolves in SweepStep --- src/linker/Linker.Steps/SweepStep.cs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/linker/Linker.Steps/SweepStep.cs b/src/linker/Linker.Steps/SweepStep.cs index b75d99eaca8f..1b0edf068e91 100644 --- a/src/linker/Linker.Steps/SweepStep.cs +++ b/src/linker/Linker.Steps/SweepStep.cs @@ -40,6 +40,7 @@ namespace Mono.Linker.Steps { + [System.Diagnostics.CodeAnalysis.SuppressMessage ("ApiDesign", "RS0030:Do not used banned APIs", Justification = "When sweeping, we may not want to resolve to a removed member that was cached previously.")] public class SweepStep : BaseStep { readonly bool sweepSymbols; @@ -69,7 +70,7 @@ protected override void Process () case AssemblyAction.CopyUsed: case AssemblyAction.Link: case AssemblyAction.Save: - bool changed = AssemblyReferencesCorrector.SweepAssemblyReferences (assembly, Context); + bool changed = AssemblyReferencesCorrector.SweepAssemblyReferences (assembly); if (changed && action == AssemblyAction.CopyUsed) Annotations.SetAction (assembly, AssemblyAction.Save); break; @@ -174,7 +175,7 @@ protected void ProcessAssemblyAction (AssemblyDefinition assembly) AssemblyAction assemblyAction = AssemblyAction.Copy; if (SweepTypeForwarders (assembly)) { // Need to sweep references, in case sweeping type forwarders removed any - AssemblyReferencesCorrector.SweepAssemblyReferences (assembly, Context); + AssemblyReferencesCorrector.SweepAssemblyReferences (assembly); assemblyAction = AssemblyAction.Save; } @@ -194,7 +195,7 @@ protected void ProcessAssemblyAction (AssemblyDefinition assembly) case AssemblyAction.Save: if (SweepTypeForwarders (assembly)) { // Need to sweep references, in case sweeping type forwarders removed any - AssemblyReferencesCorrector.SweepAssemblyReferences (assembly, Context); + AssemblyReferencesCorrector.SweepAssemblyReferences (assembly); } break; } @@ -242,7 +243,7 @@ protected virtual void SweepAssembly (AssemblyDefinition assembly) } if (SweepTypeForwarders (assembly) || updateScopes) - AssemblyReferencesCorrector.SweepAssemblyReferences (assembly, Context); + AssemblyReferencesCorrector.SweepAssemblyReferences (assembly); } bool IsMarkedAssembly (AssemblyDefinition assembly) @@ -460,7 +461,7 @@ void SweepOverrides (MethodDefinition method) // ov is in a `link` scope and is unmarked // ShouldRemove returns true if the method is unmarked, but we also We need to make sure the override is in a link scope. // Only things in a link scope are marked, so ShouldRemove is only valid for items in a `link` scope. - if (Context.TryResolve (method.Overrides[i]) is not MethodDefinition ov || ov.DeclaringType is null || (IsLinkScope (ov.DeclaringType.Scope) && ShouldRemove (ov))) + if (method.Overrides[i].Resolve () is not MethodDefinition ov || ov.DeclaringType is null || (IsLinkScope (ov.DeclaringType.Scope) && ShouldRemove (ov))) method.Overrides.RemoveAt (i); else i++; @@ -570,16 +571,14 @@ sealed class AssemblyReferencesCorrector : TypeReferenceWalker bool changedAnyScopes; - readonly LinkContext _context; - AssemblyReferencesCorrector (AssemblyDefinition assembly, LinkContext context) : base (assembly) + AssemblyReferencesCorrector (AssemblyDefinition assembly) : base (assembly) { this.importer = new DefaultMetadataImporter (assembly.MainModule); - this._context = context; changedAnyScopes = false; } - public static bool SweepAssemblyReferences (AssemblyDefinition assembly, LinkContext context) + public static bool SweepAssemblyReferences (AssemblyDefinition assembly) { // // We used to run over list returned by GetTypeReferences but @@ -589,7 +588,7 @@ public static bool SweepAssemblyReferences (AssemblyDefinition assembly, LinkCon // assembly.MainModule.AssemblyReferences.Clear (); - var arc = new AssemblyReferencesCorrector (assembly, context); + var arc = new AssemblyReferencesCorrector (assembly); arc.Process (); return arc.changedAnyScopes; @@ -606,9 +605,7 @@ protected override void ProcessTypeReference (TypeReference type) // But the cache doesn't know that, it would still "resolve" the type-ref to now defunct type-def. // For this reason we can't use the context resolution here, and must force Cecil to perform // real type resolution again (since it can fail, and that's OK). -#pragma warning disable RS0030 // Do not used banned APIs TypeDefinition td = type.Resolve (); -#pragma warning restore RS0030 // Do not used banned APIs if (td == null) { // // This can happen when not all assembly refences were provided and we @@ -630,7 +627,7 @@ protected override void ProcessTypeReference (TypeReference type) protected override void ProcessExportedType (ExportedType exportedType) { - TypeDefinition? td = _context.TryResolve (exportedType); + TypeDefinition? td = exportedType.Resolve (); if (td == null) { // Forwarded type cannot be resolved but it was marked // linker is running in --skip-unresolved true mode From e708fe9455ac45c50ad7088bdf4517932165e1b0 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Wed, 19 Oct 2022 10:32:37 -0700 Subject: [PATCH 4/5] PR feedback: Use pragma instead of attribute, warn unresolved exported type once --- src/linker/Linker.Steps/SweepStep.cs | 8 ++++++-- src/linker/Linker/LinkContext.cs | 5 ++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/linker/Linker.Steps/SweepStep.cs b/src/linker/Linker.Steps/SweepStep.cs index 1b0edf068e91..de9a4f0bdf61 100644 --- a/src/linker/Linker.Steps/SweepStep.cs +++ b/src/linker/Linker.Steps/SweepStep.cs @@ -40,7 +40,6 @@ namespace Mono.Linker.Steps { - [System.Diagnostics.CodeAnalysis.SuppressMessage ("ApiDesign", "RS0030:Do not used banned APIs", Justification = "When sweeping, we may not want to resolve to a removed member that was cached previously.")] public class SweepStep : BaseStep { readonly bool sweepSymbols; @@ -461,10 +460,12 @@ void SweepOverrides (MethodDefinition method) // ov is in a `link` scope and is unmarked // ShouldRemove returns true if the method is unmarked, but we also We need to make sure the override is in a link scope. // Only things in a link scope are marked, so ShouldRemove is only valid for items in a `link` scope. +#pragma warning disable RS0030 // Cecil's Resolve is banned - it's necessary when the metadata graph isn't stable if (method.Overrides[i].Resolve () is not MethodDefinition ov || ov.DeclaringType is null || (IsLinkScope (ov.DeclaringType.Scope) && ShouldRemove (ov))) method.Overrides.RemoveAt (i); else i++; +#pragma warning restore RS0030 } } @@ -571,7 +572,6 @@ sealed class AssemblyReferencesCorrector : TypeReferenceWalker bool changedAnyScopes; - AssemblyReferencesCorrector (AssemblyDefinition assembly) : base (assembly) { this.importer = new DefaultMetadataImporter (assembly.MainModule); @@ -605,7 +605,9 @@ protected override void ProcessTypeReference (TypeReference type) // But the cache doesn't know that, it would still "resolve" the type-ref to now defunct type-def. // For this reason we can't use the context resolution here, and must force Cecil to perform // real type resolution again (since it can fail, and that's OK). +#pragma warning disable RS0030 // Cecil's Resolve is banned -- it's necessary when the metadata graph isn't stable TypeDefinition td = type.Resolve (); +#pragma warning restore RS0030 if (td == null) { // // This can happen when not all assembly refences were provided and we @@ -627,7 +629,9 @@ protected override void ProcessTypeReference (TypeReference type) protected override void ProcessExportedType (ExportedType exportedType) { +#pragma warning disable RS0030 // Cecil's Resolve is banned -- it's necessary when the metadata graph is unstable TypeDefinition? td = exportedType.Resolve (); +#pragma warning restore RS0030 if (td == null) { // Forwarded type cannot be resolved but it was marked // linker is running in --skip-unresolved true mode diff --git a/src/linker/Linker/LinkContext.cs b/src/linker/Linker/LinkContext.cs index d3b17dd50b05..baa348ff1f0d 100644 --- a/src/linker/Linker/LinkContext.cs +++ b/src/linker/Linker/LinkContext.cs @@ -941,6 +941,8 @@ public int GetTargetRuntimeVersion () readonly HashSet unresolved_reported = new (); + readonly HashSet unresolved_exported_types_reported = new (); + protected virtual void ReportUnresolved (FieldReference fieldReference) { if (unresolved_reported.Add (fieldReference)) @@ -961,7 +963,8 @@ protected virtual void ReportUnresolved (TypeReference typeReference) protected virtual void ReportUnresolved (ExportedType et) { - LogError (string.Format (SharedStrings.FailedToResolveTypeElementMessage, et.Name), (int) DiagnosticId.FailedToResolveMetadataElement); + if (unresolved_exported_types_reported.Add (et)) + LogError (string.Format (SharedStrings.FailedToResolveTypeElementMessage, et.Name), (int) DiagnosticId.FailedToResolveMetadataElement); } } From b3ab8cff049032367c87469caebfa9a0c53e3f49 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Wed, 19 Oct 2022 10:41:46 -0700 Subject: [PATCH 5/5] Remove suppression attribute from GetDisplayName --- src/linker/Linker/MethodReferenceExtensions.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/linker/Linker/MethodReferenceExtensions.cs b/src/linker/Linker/MethodReferenceExtensions.cs index 9d1806f41e90..0911a84cd3ce 100644 --- a/src/linker/Linker/MethodReferenceExtensions.cs +++ b/src/linker/Linker/MethodReferenceExtensions.cs @@ -10,13 +10,14 @@ namespace Mono.Linker { public static class MethodReferenceExtensions { - [System.Diagnostics.CodeAnalysis.SuppressMessage ("ApiDesign", "RS0030:Do not used banned APIs", Justification = "MethodReference.Resolve is banned for performance reasons. GetDisplayName should be a really cold path and shouldn't require a context to call.")] public static string GetDisplayName (this MethodReference method) { var sb = new System.Text.StringBuilder (); // Match C# syntaxis name if setter or getter +#pragma warning disable RS0030 // Cecil's Resolve is banned -- this should be a very cold path and makes calling this method much simpler var methodDefinition = method.Resolve (); +#pragma warning restore RS0030 if (methodDefinition != null && (methodDefinition.IsSetter || methodDefinition.IsGetter)) { // Append property name string name = methodDefinition.IsSetter ? string.Concat (methodDefinition.Name.AsSpan (4), ".set") : string.Concat (methodDefinition.Name.AsSpan (4), ".get");