From 840c20ee3c179cae9bcd612e029b21c810eddd4a Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Fri, 19 May 2023 13:41:26 -0700 Subject: [PATCH] Fix marking of saved assemblies (#86474) This ensures the fix from https://github.com/dotnet/runtime/pull/82197 works also for assemblies which get the `save` action (which isn't supported from the command-line, but may happen in custom steps). It also ensures that `save` assemblies get fully marked regardless of `DisableMarkingOfCopyAssemblies`. --------- Co-authored-by: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> --- .../illink/src/linker/Linker.Steps/MarkStep.cs | 13 +++-- ...nce.Interfaces.StaticInterfaceMethodsTests.g.cs | 8 ++- .../Dependencies/CustomStepSaveAssembly.cs | 13 +++++ .../OverrideInCopyAssembly.cs | 2 +- .../OverrideInSaveAssembly.cs | 59 ++++++++++++++++++++++ .../Mono.Linker.Tests.Cases.csproj | 3 +- 6 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/Dependencies/CustomStepSaveAssembly.cs create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/OverrideInSaveAssembly.cs diff --git a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs index 0db018a..98b4566 100644 --- a/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs +++ b/src/tools/illink/src/linker/Linker.Steps/MarkStep.cs @@ -1410,10 +1410,13 @@ namespace Mono.Linker.Steps MarkExportedTypesTarget.ProcessAssembly (assembly, Context); - if (ProcessReferencesStep.IsFullyPreservedAction (Annotations.GetAction (assembly))) { - if (!Context.TryGetCustomData ("DisableMarkingOfCopyAssemblies", out string? disableMarkingOfCopyAssembliesValue) || - disableMarkingOfCopyAssembliesValue != "true") + var action = Annotations.GetAction (assembly); + if (ProcessReferencesStep.IsFullyPreservedAction (action)) { + if (action != AssemblyAction.Copy || + !Context.TryGetCustomData ("DisableMarkingOfCopyAssemblies", out string? disableMarkingOfCopyAssembliesValue) || + disableMarkingOfCopyAssembliesValue != "true") { MarkEntireAssembly (assembly); + } return; } @@ -3213,8 +3216,8 @@ namespace Mono.Linker.Steps if (method.HasOverrides) { var assembly = Context.Resolve (method.DeclaringType.Scope); - // If this method is in a Copy or CopyUsed assembly, .overrides won't get swept and we need to keep all of them - bool markAllOverrides = assembly != null && Annotations.GetAction (assembly) is AssemblyAction.Copy or AssemblyAction.CopyUsed; + // If this method is in a Copy, CopyUsed, or Save assembly, .overrides won't get swept and we need to keep all of them + bool markAllOverrides = assembly != null && Annotations.GetAction (assembly) is AssemblyAction.Copy or AssemblyAction.CopyUsed or AssemblyAction.Save; foreach (MethodReference @base in method.Overrides) { // Method implementing a static interface method will have an override to it - note instance methods usually don't unless they're explicit. // Calling the implementation method directly has no impact on the interface, and as such it should not mark the interface or its method. diff --git a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/Inheritance.Interfaces.StaticInterfaceMethodsTests.g.cs b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/Inheritance.Interfaces.StaticInterfaceMethodsTests.g.cs index e8999de..f9a4f71 100644 --- a/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/Inheritance.Interfaces.StaticInterfaceMethodsTests.g.cs +++ b/src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/Inheritance.Interfaces.StaticInterfaceMethodsTests.g.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Threading.Tasks; using Xunit; @@ -14,6 +14,12 @@ namespace ILLink.RoslynAnalyzer.Tests.Inheritance.Interfaces } [Fact] + public Task OverrideInSaveAssembly () + { + return RunTest (allowMissingWarnings: true); + } + + [Fact] public Task VarianceBasic () { return RunTest (allowMissingWarnings: true); diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/Dependencies/CustomStepSaveAssembly.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/Dependencies/CustomStepSaveAssembly.cs new file mode 100644 index 0000000..031e4d6 --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/Dependencies/CustomStepSaveAssembly.cs @@ -0,0 +1,13 @@ +using System; +using Mono.Cecil; +using Mono.Linker; +using Mono.Linker.Steps; + +public class CustomStepSaveAssembly : BaseStep +{ + protected override void ProcessAssembly (AssemblyDefinition assembly) + { + if (assembly.Name.Name == "test") + Context.Annotations.SetAction (assembly, AssemblyAction.Save); + } +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/OverrideInCopyAssembly.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/OverrideInCopyAssembly.cs index baa6785..d42da65 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/OverrideInCopyAssembly.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/OverrideInCopyAssembly.cs @@ -18,7 +18,7 @@ namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces.StaticInterfaceMethods [SetupLinkerAction ("copy", "test")] /// /// Regression test for issue: https://github.com/dotnet/runtime/issues/81746 - /// OverridesStaticInterfaceMethods.Method() (and Property.set/get) has an entry in .overrides pointing to ISataticAbstractMethods.Method. + /// OverridesStaticInterfaceMethods.Method() (and Property.set/get) has an entry in .overrides pointing to IStaticAbstractMethods.Method. /// IStaticAbstractMethods.Method() isn't referenced anywhere else and isn't otherwise needed. /// Usually the interface method could be removed, and the pointer to it in the .overrides metadata would be removed /// However, since OverridesStaticInterfaceMethods is in a 'copy' assembly, the .overrides metadata isn't swept. If we remove the method from the interface, diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/OverrideInSaveAssembly.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/OverrideInSaveAssembly.cs new file mode 100644 index 0000000..3810dca --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/OverrideInSaveAssembly.cs @@ -0,0 +1,59 @@ +// 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.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; +using Mono.Linker.Tests.Cases.Expectations.Assertions; +using Mono.Linker.Tests.Cases.Expectations.Metadata; +using Mono.Linker.Tests.Cases.Inheritance.Interfaces.StaticInterfaceMethods.Dependencies; + +namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces.StaticInterfaceMethods +{ + [KeptMemberInAssembly ("library", typeof (IStaticAbstractMethods), "Method()", "get_Property()", "Property", "set_Property(System.Int32)")] + // Add a custom step which sets the assembly action of the test to "save" + [SetupCompileBefore ("CustomStepSaveAssembly.dll", new[] { "Dependencies/CustomStepSaveAssembly.cs" }, new[] { "illink.dll", "Mono.Cecil.dll", "netstandard.dll" })] + [SetupLinkerArgument ("--custom-step", "-MarkStep:CustomStepSaveAssembly,CustomStepSaveAssembly.dll")] + [SetupCompileBefore ("library.dll", new[] { "Dependencies/Library.cs" })] + [SetupLinkerAction ("link", "library")] + /// + /// Regression test for issue: https://github.com/dotnet/runtime/issues/86242 + /// OverridesStaticInterfaceMethods.Method() (and Property.set/get) has an entry in .overrides pointing to IStaticAbstractMethods.Method. + /// IStaticAbstractMethods.Method() isn't referenced anywhere else and isn't otherwise needed. + /// Usually the interface method could be removed, and the pointer to it in the .overrides metadata would be removed + /// However, since OverridesStaticInterfaceMethods is in a 'save' assembly, the .overrides metadata isn't swept. If we remove the method from the interface, + /// we have a "dangling reference" which makes the metadata invalid. + /// + static class OverrideInSaveAssembly + { + [Kept] + public static void Main () + { + OverridesStaticInterfaceMethods.Property = OverridesStaticInterfaceMethods.Method (); + var x = OverridesStaticInterfaceMethods.Property; + } + [Kept] + [KeptMember (".ctor()")] + [KeptInterface (typeof (IStaticAbstractMethods))] + class OverridesStaticInterfaceMethods : IStaticAbstractMethods + { + [Kept] + public static int Property { + [Kept] + [KeptOverride (typeof (IStaticAbstractMethods))] + get => throw new NotImplementedException (); + [Kept] + [KeptOverride (typeof (IStaticAbstractMethods))] + set => throw new NotImplementedException (); + } + [Kept] + [KeptOverride (typeof (IStaticAbstractMethods))] + + public static int Method () => throw new NotImplementedException (); + [Kept] + public int InstanceMethod () => throw new NotImplementedException (); + } + } +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Mono.Linker.Tests.Cases.csproj b/src/tools/illink/test/Mono.Linker.Tests.Cases/Mono.Linker.Tests.Cases.csproj index ad7814b..0894335 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/Mono.Linker.Tests.Cases.csproj +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Mono.Linker.Tests.Cases.csproj @@ -1,4 +1,4 @@ - + $(DefineConstants);INCLUDE_EXPECTATIONS;SUPPORTS_DEFAULT_INTERFACE_METHODS @@ -12,6 +12,7 @@ + -- 2.7.4