From f20d18d9a00fbebb5f33cb5f36d9378a1dd891c7 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Thu, 22 Feb 2018 16:11:16 -0800 Subject: [PATCH 1/4] Tolerate assembly resolution errors Previously, we swallowed AssemblyResolutionException in a few places in various steps. We need an option to be generally more tolerant of AssemblyResolutionExceptions, because there can be cases during portable publish in which referenced assemblies are not present during publish because they're part of the shared framework. Instead of handling AssemblyResolutionException in every place in the linker that tries to resolve methods/types/etc, this change adds an option that enables the resolver to return null when an assembly is not found. The null result is also cached to avoid multiple lookups. Only a few places in the linker need to be modified to handle null results, and the existing places where we used to catch AssemblyResolutionException now only need to deal with a null result instead. The downside of this approach is that these places cannot distinguish between types not found in an existing assembly, and assemblies that were not found. The new behavior is grouped under the option --skip-unresolved, which already handles unresolved types and methods in some cases. When this option is enabled, a warning is logged, and no exception is thrown. --- .../ILLink.Tasks/ILLink.Tasks.targets | 2 +- linker/Linker.Steps/MarkStep.cs | 8 ++--- .../Linker.Steps/ResolveFromAssemblyStep.cs | 7 +---- linker/Linker.Steps/ResolveFromXmlStep.cs | 7 +---- linker/Linker.Steps/SweepStep.cs | 8 ++--- linker/Linker/AssemblyResolver.cs | 29 ++++++++++++++++--- linker/Linker/Driver.cs | 2 +- linker/Linker/LinkContext.cs | 15 +++++----- 8 files changed, 41 insertions(+), 37 deletions(-) diff --git a/corebuild/integration/ILLink.Tasks/ILLink.Tasks.targets b/corebuild/integration/ILLink.Tasks/ILLink.Tasks.targets index 3fd4a7880e1f..ce76e7136831 100644 --- a/corebuild/integration/ILLink.Tasks/ILLink.Tasks.targets +++ b/corebuild/integration/ILLink.Tasks/ILLink.Tasks.targets @@ -222,7 +222,7 @@ the future we will want to generate these depending on the scenario in which the linker is invoked. --> - -t -l none -b true + -t -l none -b true --skip-unresolved true _assemblies; + readonly HashSet _unresolvedAssemblies; + bool _ignoreUnresolved = false; public IDictionary AssemblyCache { get { return _assemblies; } @@ -53,14 +55,32 @@ public AssemblyResolver () public AssemblyResolver (Dictionary assembly_cache) { _assemblies = assembly_cache; + _unresolvedAssemblies = new HashSet(); + } + + public bool IgnoreUnresolved + { + get { return _ignoreUnresolved; } + set { _ignoreUnresolved = value; } } public override AssemblyDefinition Resolve (AssemblyNameReference name, ReaderParameters parameters) { - AssemblyDefinition asm; - if (!_assemblies.TryGetValue (name.Name, out asm)) { - asm = base.Resolve (name, parameters); - _assemblies [asm.Name.Name] = asm; + AssemblyDefinition asm = null; + if (!_assemblies.TryGetValue (name.Name, out asm) && !_unresolvedAssemblies.Contains (name.Name)) { + try + { + asm = base.Resolve (name, parameters); + _assemblies [name.Name] = asm; + } + catch (AssemblyResolutionException) + { + if (!_ignoreUnresolved) { + throw; + } + Console.WriteLine($"warning: unresolved assembly {name.Name}"); + _unresolvedAssemblies.Add (name.Name); + } } return asm; @@ -80,6 +100,7 @@ protected override void Dispose (bool disposing) } _assemblies.Clear (); + _unresolvedAssemblies.Clear (); } } } diff --git a/linker/Linker/Driver.cs b/linker/Linker/Driver.cs index 26ebe038e938..7d56f164f36c 100644 --- a/linker/Linker/Driver.cs +++ b/linker/Linker/Driver.cs @@ -343,7 +343,7 @@ static void Usage (string msg) Console.WriteLine (" --about About the {0}", _linker); Console.WriteLine (" --version Print the version number of the {0}", _linker); - Console.WriteLine (" --skip-unresolved Ignore unresolved types and methods (true or false)"); + Console.WriteLine (" --skip-unresolved Ignore unresolved types, methods, and assemblies (true or false)"); Console.WriteLine (" --dependencies-file Specify the dependencies file path, if unset the default path is used: /linker-dependencies.xml.gz"); Console.WriteLine (" --dump-dependencies Dump dependencies for the linker analyzer tool"); Console.WriteLine (" --reduced-tracing Reduces dependency output related to assemblies that will not be modified"); diff --git a/linker/Linker/LinkContext.cs b/linker/Linker/LinkContext.cs index a87d7dbc3eae..2db058897556 100644 --- a/linker/Linker/LinkContext.cs +++ b/linker/Linker/LinkContext.cs @@ -104,7 +104,10 @@ public bool KeepMembersForDebuggerAttributes public bool IgnoreUnresolved { get { return _ignoreUnresolved; } - set { _ignoreUnresolved = value; } + set { + _ignoreUnresolved = value; + _resolver.IgnoreUnresolved = value; + } } public bool EnableReducedTracing { get; set; } @@ -208,7 +211,7 @@ public AssemblyDefinition Resolve (IMetadataScope scope) try { AssemblyDefinition assembly = _resolver.Resolve (reference, _readerParameters); - if (SeenFirstTime (assembly)) { + if (assembly != null && SeenFirstTime (assembly)) { SafeReadSymbols (assembly); SetAction (assembly); } @@ -248,11 +251,9 @@ public virtual ICollection ResolveReferences (AssemblyDefini { List references = new List (); foreach (AssemblyNameReference reference in assembly.MainModule.AssemblyReferences) { - try { - references.Add (Resolve (reference)); - } - catch (AssemblyResolutionException) { - } + AssemblyDefinition definition = Resolve (reference); + if (definition != null) + references.Add (definition); } return references; } From d47d9d7eeefc7fb854c316d181782df3af82d826 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Thu, 22 Feb 2018 19:31:01 -0800 Subject: [PATCH 2/4] Attempt to fix test failure Introduce a SkipUnresolved attribute --- .../Metadata/SkipUnresolvedAttribute.cs | 12 ++++++++++++ .../Mono.Linker.Tests.Cases.Expectations.csproj | 1 + .../TypeForwarding/MissingTargetReference.cs | 1 + .../Tests/TestCasesRunner/LinkerArgumentBuilder.cs | 10 ++++++++++ .../Tests/TestCasesRunner/TestCaseLinkerOptions.cs | 1 + .../Tests/TestCasesRunner/TestCaseMetadaProvider.cs | 3 ++- 6 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 linker/Tests/Mono.Linker.Tests.Cases.Expectations/Metadata/SkipUnresolvedAttribute.cs diff --git a/linker/Tests/Mono.Linker.Tests.Cases.Expectations/Metadata/SkipUnresolvedAttribute.cs b/linker/Tests/Mono.Linker.Tests.Cases.Expectations/Metadata/SkipUnresolvedAttribute.cs new file mode 100644 index 000000000000..bee9a7f4fc86 --- /dev/null +++ b/linker/Tests/Mono.Linker.Tests.Cases.Expectations/Metadata/SkipUnresolvedAttribute.cs @@ -0,0 +1,12 @@ +using System; + +namespace Mono.Linker.Tests.Cases.Expectations.Metadata { + public sealed class SkipUnresolvedAttribute : BaseMetadataAttribute { + public readonly bool Value; + + public SkipUnresolvedAttribute (bool value) + { + Value = value; + } + } +} diff --git a/linker/Tests/Mono.Linker.Tests.Cases.Expectations/Mono.Linker.Tests.Cases.Expectations.csproj b/linker/Tests/Mono.Linker.Tests.Cases.Expectations/Mono.Linker.Tests.Cases.Expectations.csproj index 843401413ca4..0c0212fa8398 100644 --- a/linker/Tests/Mono.Linker.Tests.Cases.Expectations/Mono.Linker.Tests.Cases.Expectations.csproj +++ b/linker/Tests/Mono.Linker.Tests.Cases.Expectations/Mono.Linker.Tests.Cases.Expectations.csproj @@ -71,6 +71,7 @@ + diff --git a/linker/Tests/Mono.Linker.Tests.Cases/TypeForwarding/MissingTargetReference.cs b/linker/Tests/Mono.Linker.Tests.Cases/TypeForwarding/MissingTargetReference.cs index 07f43ef3210a..5e8b115717cd 100644 --- a/linker/Tests/Mono.Linker.Tests.Cases/TypeForwarding/MissingTargetReference.cs +++ b/linker/Tests/Mono.Linker.Tests.Cases/TypeForwarding/MissingTargetReference.cs @@ -4,6 +4,7 @@ namespace Mono.Linker.Tests.Cases.TypeForwarding { + [SkipUnresolved (true)] [Define ("IL_ASSEMBLY_AVAILABLE")] [SetupCompileBefore ("TypeForwarderMissingReference.dll", new [] { "Dependencies/TypeForwarderMissingReference.il" })] [SetupLinkerAction ("link", "TypeForwarderMissingReference.dll")] diff --git a/linker/Tests/TestCasesRunner/LinkerArgumentBuilder.cs b/linker/Tests/TestCasesRunner/LinkerArgumentBuilder.cs index a5bf7f658d37..b776923cc6d3 100644 --- a/linker/Tests/TestCasesRunner/LinkerArgumentBuilder.cs +++ b/linker/Tests/TestCasesRunner/LinkerArgumentBuilder.cs @@ -66,6 +66,14 @@ public virtual void AddAssemblyAction (string action, string assembly) Append (assembly); } + public virtual void AddSkipUnresolved (bool skipUnresolved) + { + if (skipUnresolved) { + Append ("--skip-unresolved"); + Append ("true"); + } + } + public string [] ToArgs () { return _arguments.ToArray (); @@ -108,6 +116,8 @@ public virtual void ProcessOptions (TestCaseLinkerOptions options) if (!string.IsNullOrEmpty (options.LinkSymbols)) AddLinkSymbols (options.LinkSymbols); + AddSkipUnresolved (options.SkipUnresolved); + // Unity uses different argument format and needs to be able to translate to their format. In order to make that easier // we keep the information in flag + values format for as long as we can so that this information doesn't have to be parsed out of a single string foreach (var additionalArgument in options.AdditionalArguments) diff --git a/linker/Tests/TestCasesRunner/TestCaseLinkerOptions.cs b/linker/Tests/TestCasesRunner/TestCaseLinkerOptions.cs index 675a0641edce..916bc38d71bb 100644 --- a/linker/Tests/TestCasesRunner/TestCaseLinkerOptions.cs +++ b/linker/Tests/TestCasesRunner/TestCaseLinkerOptions.cs @@ -11,6 +11,7 @@ public class TestCaseLinkerOptions public bool IncludeBlacklistStep; public string KeepTypeForwarderOnlyAssemblies; public string LinkSymbols; + public bool SkipUnresolved; public List> AdditionalArguments = new List> (); } diff --git a/linker/Tests/TestCasesRunner/TestCaseMetadaProvider.cs b/linker/Tests/TestCasesRunner/TestCaseMetadaProvider.cs index cc87f0bf81de..3bd1d531b38a 100644 --- a/linker/Tests/TestCasesRunner/TestCaseMetadaProvider.cs +++ b/linker/Tests/TestCasesRunner/TestCaseMetadaProvider.cs @@ -31,7 +31,8 @@ public virtual TestCaseLinkerOptions GetLinkerOptions () IncludeBlacklistStep = GetOptionAttributeValue (nameof (IncludeBlacklistStepAttribute), false), KeepTypeForwarderOnlyAssemblies = GetOptionAttributeValue (nameof (KeepTypeForwarderOnlyAssembliesAttribute), string.Empty), LinkSymbols = GetOptionAttributeValue (nameof (SetupLinkerLinkSymbolsAttribute), string.Empty), - CoreAssembliesAction = GetOptionAttributeValue (nameof (SetupLinkerCoreActionAttribute), null) + CoreAssembliesAction = GetOptionAttributeValue (nameof (SetupLinkerCoreActionAttribute), null), + SkipUnresolved = GetOptionAttributeValue (nameof (SkipUnresolvedAttribute), false) }; foreach (var assemblyAction in _testCaseTypeDefinition.CustomAttributes.Where (attr => attr.AttributeType.Name == nameof (SetupLinkerActionAttribute))) From 68875a46fce9efc560ae9a7c6dac7b5f2da66092 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Fri, 23 Feb 2018 10:09:35 -0800 Subject: [PATCH 3/4] Address PR feedback - follow coding conventions - use LinkContext::LogMessage instead of Console.WriteLine - delay initialization of unresolved assembly cache to catch block - keep AssemblyResolver.IgnoreUnresolved independent of LinkContext.IgnoreUnresolved - remove redundant initialization --- linker/Linker/AssemblyResolver.cs | 31 +++++++++++++++++-------------- linker/Linker/Driver.cs | 4 +++- linker/Linker/LinkContext.cs | 6 ++---- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/linker/Linker/AssemblyResolver.cs b/linker/Linker/AssemblyResolver.cs index 5414e667ca7e..42af3d452533 100644 --- a/linker/Linker/AssemblyResolver.cs +++ b/linker/Linker/AssemblyResolver.cs @@ -40,8 +40,9 @@ public class AssemblyResolver : BaseAssemblyResolver { #endif readonly Dictionary _assemblies; - readonly HashSet _unresolvedAssemblies; - bool _ignoreUnresolved = false; + HashSet _unresolvedAssemblies; + bool _ignoreUnresolved; + LinkContext _context; public IDictionary AssemblyCache { get { return _assemblies; } @@ -55,30 +56,32 @@ public AssemblyResolver () public AssemblyResolver (Dictionary assembly_cache) { _assemblies = assembly_cache; - _unresolvedAssemblies = new HashSet(); } - public bool IgnoreUnresolved - { + public bool IgnoreUnresolved { get { return _ignoreUnresolved; } set { _ignoreUnresolved = value; } } + public LinkContext Context { + get { return _context; } + set { _context = value; } + } + public override AssemblyDefinition Resolve (AssemblyNameReference name, ReaderParameters parameters) { AssemblyDefinition asm = null; - if (!_assemblies.TryGetValue (name.Name, out asm) && !_unresolvedAssemblies.Contains (name.Name)) { - try - { + if (!_assemblies.TryGetValue (name.Name, out asm) && (_unresolvedAssemblies == null || !_unresolvedAssemblies.Contains (name.Name))) { + try { asm = base.Resolve (name, parameters); _assemblies [name.Name] = asm; - } - catch (AssemblyResolutionException) - { - if (!_ignoreUnresolved) { + } catch (AssemblyResolutionException) { + if (!_ignoreUnresolved) throw; - } - Console.WriteLine($"warning: unresolved assembly {name.Name}"); + + _context.LogMessage ($"warning: unresolved assembly {name.Name}"); + if (_unresolvedAssemblies == null) + _unresolvedAssemblies = new HashSet (); _unresolvedAssemblies.Add (name.Name); } } diff --git a/linker/Linker/Driver.cs b/linker/Linker/Driver.cs index 7d56f164f36c..35f03edf63ba 100644 --- a/linker/Linker/Driver.cs +++ b/linker/Linker/Driver.cs @@ -106,7 +106,9 @@ public void Run (ILogger customLogger = null) Usage ("Option is too short"); if (token == "--skip-unresolved") { - context.IgnoreUnresolved = bool.Parse (GetParam ()); + bool ignoreUnresolved = bool.Parse (GetParam ()); + context.IgnoreUnresolved = ignoreUnresolved; + context.Resolver.IgnoreUnresolved = ignoreUnresolved; continue; } diff --git a/linker/Linker/LinkContext.cs b/linker/Linker/LinkContext.cs index 2db058897556..9339df33e617 100644 --- a/linker/Linker/LinkContext.cs +++ b/linker/Linker/LinkContext.cs @@ -104,10 +104,7 @@ public bool KeepMembersForDebuggerAttributes public bool IgnoreUnresolved { get { return _ignoreUnresolved; } - set { - _ignoreUnresolved = value; - _resolver.IgnoreUnresolved = value; - } + set { _ignoreUnresolved = value; } } public bool EnableReducedTracing { get; set; } @@ -161,6 +158,7 @@ public LinkContext (Pipeline pipeline, AssemblyResolver resolver, ReaderParamete { _pipeline = pipeline; _resolver = resolver; + _resolver.Context = this; _actions = new Dictionary (); _parameters = new Dictionary (); _readerParameters = readerParameters; From 8d9de2ccfa8e4714c0c18b4c3655e50d6c497c98 Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Fri, 23 Feb 2018 11:31:34 -0800 Subject: [PATCH 4/4] Handle null _unresolvedAssemblies in Dispose method --- linker/Linker/AssemblyResolver.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/linker/Linker/AssemblyResolver.cs b/linker/Linker/AssemblyResolver.cs index 42af3d452533..72ac6e2923c6 100644 --- a/linker/Linker/AssemblyResolver.cs +++ b/linker/Linker/AssemblyResolver.cs @@ -103,7 +103,8 @@ protected override void Dispose (bool disposing) } _assemblies.Clear (); - _unresolvedAssemblies.Clear (); + if (_unresolvedAssemblies != null) + _unresolvedAssemblies.Clear (); } } }