From c5f2d4f1227f724edcb0b4ffbc8a0b04064579dd Mon Sep 17 00:00:00 2001 From: Sven Boemer Date: Tue, 2 Jul 2024 12:19:17 -0700 Subject: [PATCH] Revert ILLink's usage of dependency analysis framework (#104267) See https://github.com/dotnet/runtime/issues/103987#issuecomment-2197798838 for context. Custom steps rely on getting a chance to see a type before we build the type info (and in particular the interface method mapping), but we make no such guarantee. https://github.com/dotnet/runtime/issues/104266 tracks this problem. Our use of the dependency analysis framework exacerbated this because `MarkType` was split into two pieces, with the part that calls into the custom step being delayed through a dependency analysis framework node, making it more likely to be run too late to influence the type info. This reverts ILLink's usage of the dependency analysis framework to bring us back to a state where the ordering still isn't guaranteed, but works for the testcase that got regressed. We should bring this back as soon as possible with a proper fix that actually guarantees the ordering required by custom steps. This doesn't look completely straightforward, but should be possible to do with the dependency analysis framework. Fixes https://github.com/dotnet/runtime/issues/103987 (but we need a better long-term fix for https://github.com/dotnet/runtime/issues/104266). --- eng/liveILLink.targets | 4 +- src/coreclr/Directory.Build.props | 2 +- ...ompiler.DependencyAnalysisFramework.csproj | 11 +-- .../MarkStep.MethodDefinitionNode.cs | 43 ---------- .../Linker.Steps/MarkStep.NodeFactory.cs | 71 ---------------- .../MarkStep.ProcessCallbackNode.cs | 42 ---------- .../MarkStep.PropertyDefinitionNode.cs | 46 ----------- .../MarkStep.TypeDefinitionNode.cs | 40 --------- ...Step.TypeIsRelevantToVariantCastingNode.cs | 39 --------- .../src/linker/Linker.Steps/MarkStep.cs | 81 +++++++++---------- .../src/linker/Linker/DictionaryExtensions.cs | 11 --- .../illink/src/linker/Mono.Linker.csproj | 6 -- .../CreatedMemberInAssemblyAttribute.cs | 28 +++++++ .../CustomStepCanFixAbstractMethods.cs | 69 ++++++++++++++++ .../Dependencies/FixAbstractMethods.cs | 43 ++++++++++ .../Dependencies/InterfaceImplementation.cs | 9 +++ .../Dependencies/InterfaceType.cs | 19 +++++ .../Mono.Linker.Tests.Cases.csproj | 2 + .../TestCasesRunner/ResultChecker.cs | 44 ++++++++++ 19 files changed, 257 insertions(+), 353 deletions(-) delete mode 100644 src/tools/illink/src/linker/Linker.Steps/MarkStep.MethodDefinitionNode.cs delete mode 100644 src/tools/illink/src/linker/Linker.Steps/MarkStep.NodeFactory.cs delete mode 100644 src/tools/illink/src/linker/Linker.Steps/MarkStep.ProcessCallbackNode.cs delete mode 100644 src/tools/illink/src/linker/Linker.Steps/MarkStep.PropertyDefinitionNode.cs delete mode 100644 src/tools/illink/src/linker/Linker.Steps/MarkStep.TypeDefinitionNode.cs delete mode 100644 src/tools/illink/src/linker/Linker.Steps/MarkStep.TypeIsRelevantToVariantCastingNode.cs create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/CreatedMemberInAssemblyAttribute.cs create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/CustomStepCanFixAbstractMethods.cs create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/Dependencies/FixAbstractMethods.cs create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/Dependencies/InterfaceImplementation.cs create mode 100644 src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/Dependencies/InterfaceType.cs diff --git a/eng/liveILLink.targets b/eng/liveILLink.targets index 6b45e00a49d5ed..f18b89622982d9 100644 --- a/eng/liveILLink.targets +++ b/eng/liveILLink.targets @@ -30,13 +30,11 @@ - + SetConfiguration="Configuration=$(ToolsConfiguration)"> TargetFramework=$(NetCoreAppToolCurrent) TargetFramework=$(NetFrameworkToolCurrent) diff --git a/src/coreclr/Directory.Build.props b/src/coreclr/Directory.Build.props index 2f86002cccf44c..233bfbeacebf0c 100644 --- a/src/coreclr/Directory.Build.props +++ b/src/coreclr/Directory.Build.props @@ -1,6 +1,6 @@ - true + true $(__BuildType) diff --git a/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/ILCompiler.DependencyAnalysisFramework.csproj b/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/ILCompiler.DependencyAnalysisFramework.csproj index 763d6bfbc27bf8..28aa33b5afe733 100644 --- a/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/ILCompiler.DependencyAnalysisFramework.csproj +++ b/src/coreclr/tools/aot/ILCompiler.DependencyAnalysisFramework/ILCompiler.DependencyAnalysisFramework.csproj @@ -1,8 +1,4 @@ - - - AnyCPU - - + Library ILCompiler.DependencyAnalysisFramework @@ -12,10 +8,6 @@ x64;x86 AnyCPU false - - <_RequiresLiveILLink>false - - true - - - all diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/CreatedMemberInAssemblyAttribute.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/CreatedMemberInAssemblyAttribute.cs new file mode 100644 index 00000000000000..f0c50173a08097 --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/CreatedMemberInAssemblyAttribute.cs @@ -0,0 +1,28 @@ +// 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; + +namespace Mono.Linker.Tests.Cases.Expectations.Assertions +{ + [AttributeUsage (AttributeTargets.Class | AttributeTargets.Delegate | AttributeTargets.Struct | AttributeTargets.Enum, AllowMultiple = true, Inherited = false)] + public sealed class CreatedMemberInAssemblyAttribute : BaseInAssemblyAttribute + { + + public CreatedMemberInAssemblyAttribute (string assemblyFileName, Type type, params string[] memberNames) + { + if (string.IsNullOrEmpty (assemblyFileName)) + throw new ArgumentNullException (nameof (assemblyFileName)); + ArgumentNullException.ThrowIfNull (type); + ArgumentNullException.ThrowIfNull (memberNames); + } + + public CreatedMemberInAssemblyAttribute (string assemblyFileName, string typeName, params string[] memberNames) + { + if (string.IsNullOrEmpty (assemblyFileName)) + throw new ArgumentNullException (nameof (assemblyFileName)); + ArgumentNullException.ThrowIfNull (typeName); + ArgumentNullException.ThrowIfNull (memberNames); + } + } +} 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 new file mode 100644 index 00000000000000..230f621cbd9d5b --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/CustomStepCanFixAbstractMethods.cs @@ -0,0 +1,69 @@ +using Mono.Linker.Tests.Cases.Extensibility.Dependencies; +using Mono.Linker.Tests.Cases.Expectations.Assertions; +using Mono.Linker.Tests.Cases.Expectations.Metadata; + +namespace Mono.Linker.Tests.Cases.Extensibility +{ + [ExpectedNoWarnings] + [SetupCompileBefore ("FixAbstractMethods.dll", new[] { "Dependencies/FixAbstractMethods.cs" }, new[] { "illink.dll", "Mono.Cecil.dll", "netstandard.dll" })] + [SetupCompileBefore ("InterfaceType.dll", new[] { "Dependencies/InterfaceType.cs" })] + [SetupCompileAfter ("InterfaceType.dll", new[] { "Dependencies/InterfaceType.cs" }, defines: new[] { "INCLUDE_ABSTRACT_METHOD"})] + [SetupCompileBefore ("InterfaceImplementation.dll", new[] { "Dependencies/InterfaceImplementation.cs" }, references: new[] { "InterfaceType.dll" })] + [CreatedMemberInAssembly ("InterfaceImplementation.dll", typeof (InterfaceImplementationInOtherAssembly), "AbstractMethod()")] + [SetupLinkerArgument ("--custom-step", "FixAbstractMethods,FixAbstractMethods.dll")] + + public class CustomStepCanFixAbstractMethods + { + public static void Main () + { + TestReflectionAccessToOtherAssembly (); + TestReflectionAccess (); + TestDirectAccess (); + } + + [Kept] + static void TestReflectionAccessToOtherAssembly () + { + // Regression test for https://github.com/dotnet/runtime/issues/103987 + // To simulate the issue, the type needs to live in a different assembly than the testcase, and it needs + // 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); + } + + [Kept] + static void TestReflectionAccess () + { + var type = typeof (InterfaceImplementationAccessedViaReflection); + InterfaceType instance = (InterfaceType) System.Activator.CreateInstance (type); + InterfaceType.UseInstance (instance); + } + + [Kept] + [KeptMember (".ctor()")] + [KeptInterface (typeof (InterfaceType))] + // [CreatedMember ("AbstractMethod()")] // https://github.com/dotnet/runtime/issues/104266 + class InterfaceImplementationAccessedViaReflection : InterfaceType + { + } + + [Kept] + static void TestDirectAccess () + { + InterfaceType instance = new InterfaceImplementation (); + InterfaceType.UseInstance (instance); + } + + [Kept] + [KeptMember (".ctor()")] + [KeptInterface (typeof (InterfaceType))] + // [CreatedMember ("AbstractMethod()")] // https://github.com/dotnet/runtime/issues/104266 + class InterfaceImplementation : InterfaceType + { + } + } +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/Dependencies/FixAbstractMethods.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/Dependencies/FixAbstractMethods.cs new file mode 100644 index 00000000000000..99b936b64117a4 --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/Dependencies/FixAbstractMethods.cs @@ -0,0 +1,43 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using Mono.Cecil; +using Mono.Linker; +using Mono.Linker.Steps; + +public class FixAbstractMethods : IMarkHandler +{ + LinkContext _context; + + public void Initialize (LinkContext context, MarkContext markContext) + { + _context = context; + markContext.RegisterMarkTypeAction (type => ProcessType (type)); + } + + void ProcessType (TypeDefinition type) + { + if (!type.Name.Contains ("InterfaceImplementation")) + return; + + Assert (!type.IsAbstract && type.HasInterfaces); + var iface = type.Interfaces[0]; + Assert (iface.InterfaceType.Name == "InterfaceType"); + var interfaceType = iface.InterfaceType.Resolve (); + var method = interfaceType.Methods[0]; + Assert (method.Name == "AbstractMethod"); + + var newMethod = new MethodDefinition (method.Name, (method.Attributes | MethodAttributes.Final) & ~MethodAttributes.Abstract, method.ReturnType); + Assert (!method.HasParameters); + var ilProcessor = newMethod.Body.GetILProcessor (); + ilProcessor.Append (ilProcessor.Create (Mono.Cecil.Cil.OpCodes.Ret)); + + type.Methods.Add (newMethod); + } + + static void Assert (bool b) + { + if (!b) + throw new Exception (); + } +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/Dependencies/InterfaceImplementation.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/Dependencies/InterfaceImplementation.cs new file mode 100644 index 00000000000000..2c5216bc02b765 --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/Dependencies/InterfaceImplementation.cs @@ -0,0 +1,9 @@ +// 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. + +namespace Mono.Linker.Tests.Cases.Extensibility.Dependencies +{ + public class InterfaceImplementationInOtherAssembly : InterfaceType + { + } +} diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/Dependencies/InterfaceType.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/Dependencies/InterfaceType.cs new file mode 100644 index 00000000000000..26b1fa2a7df546 --- /dev/null +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/Extensibility/Dependencies/InterfaceType.cs @@ -0,0 +1,19 @@ +// 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. + +namespace Mono.Linker.Tests.Cases.Extensibility.Dependencies +{ + public interface InterfaceType + { +#if INCLUDE_ABSTRACT_METHOD + public abstract void AbstractMethod (); +#endif + + public static void UseInstance (InterfaceType instance) + { +#if INCLUDE_ABSTRACT_METHOD + instance.AbstractMethod (); +#endif + } + } +} 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 f54ae5639f9b75..94ce936df32b09 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 @@ -25,6 +25,8 @@ + + diff --git a/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs b/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs index f6480e554c3528..f1d7eefae12d5c 100644 --- a/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs +++ b/src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs @@ -370,6 +370,12 @@ void VerifyLinkingOfOtherAssemblies (AssemblyDefinition original) VerifyKeptMemberInAssembly (checkAttrInAssembly, linkedType); break; + case nameof (CreatedMemberInAssemblyAttribute): + if (linkedType == null) + Assert.Fail ($"Type `{expectedTypeName}` should have been kept in assembly {assemblyName}"); + + VerifyCreatedMemberInAssembly (checkAttrInAssembly, linkedType); + break; case nameof (RemovedForwarderAttribute): if (linkedAssembly.MainModule.ExportedTypes.Any (l => l.Name == expectedTypeName)) Assert.Fail ($"Forwarder `{expectedTypeName}' should have been removed from assembly {assemblyName}"); @@ -642,6 +648,26 @@ void VerifyKeptMemberInAssembly (CustomAttribute inAssemblyAttribute, TypeDefini } } + void VerifyCreatedMemberInAssembly (CustomAttribute inAssemblyAttribute, TypeDefinition linkedType) + { + var memberNames = (CustomAttributeArgument[]) inAssemblyAttribute.ConstructorArguments[2].Value; + Assert.IsTrue (memberNames.Length > 0, "Invalid CreatedMemberInAssemblyAttribute. Expected member names."); + foreach (var memberNameAttr in memberNames) { + string memberName = (string) memberNameAttr.Value; + + if (TryVerifyCreatedMemberInAssemblyAsField (memberName, linkedType)) + continue; + + if (TryVerifyCreatedMemberInAssemblyAsProperty (memberName, linkedType)) + continue; + + if (TryVerifyCreatedMemberInAssemblyAsMethod (memberName, linkedType)) + continue; + + Assert.Fail ($"Member `{memberName}` on Type `{linkedType}` should have been created"); + } + } + void VerifyRemovedOverrideOnMethodInAssembly (CustomAttribute attr, TypeDefinition type) { var methodname = (string) attr.ConstructorArguments[2].Value; @@ -710,6 +736,24 @@ protected virtual bool TryVerifyKeptMemberInAssemblyAsMethod (string memberName, return false; } + protected virtual bool TryVerifyCreatedMemberInAssemblyAsField (string memberName, TypeDefinition linkedType) + { + var linkedField = linkedType.Fields.FirstOrDefault (m => m.Name == memberName); + return linkedField != null; + } + + protected virtual bool TryVerifyCreatedMemberInAssemblyAsProperty (string memberName, TypeDefinition linkedType) + { + var linkedProperty = linkedType.Properties.FirstOrDefault (m => m.Name == memberName); + return linkedProperty != null; + } + + protected virtual bool TryVerifyCreatedMemberInAssemblyAsMethod (string memberName, TypeDefinition linkedType) + { + var linkedMethod = linkedType.Methods.FirstOrDefault (m => m.GetSignature () == memberName); + return linkedMethod != null; + } + void VerifyKeptReferencesInAssembly (CustomAttribute inAssemblyAttribute) { var assembly = ResolveLinkedAssembly (inAssemblyAttribute.ConstructorArguments[0].Value.ToString ());