From e64148792193bf7d706428a64632ebf7f8023d3b Mon Sep 17 00:00:00 2001 From: Michael Voorhees Date: Fri, 8 Feb 2019 16:05:58 -0500 Subject: Fix an invalid IL bug with interface sweeping If a type can be on the stack then we need to keep the interface implementation on that type even if the type is never instantiated. I think we can get away with only doing this when the using of the type and the interface are within the same body. Once beyond the body, there won't be invalid IL. Casting is similar, see `ObjectHardCastedToInterface` --- linker/Linker.Steps/MarkStep.cs | 22 +++- linker/Linker.Steps/TypeMapStep.cs | 25 +++++ linker/Linker/Annotations.cs | 13 +++ linker/Linker/MethodBodyScanner.cs | 119 +++++++++++++++++++++ linker/Linker/TypeDefinitionExtensions.cs | 21 ++++ linker/Mono.Linker.csproj | 6 +- .../OverrideThatAlsoFulfilsInterface.cs | 8 +- .../OnReferenceType/NoKeptCtor/GenericType.cs | 22 ++++ ...straintDoesNotCauseOtherTypesToKeepInterface.cs | 41 +++++++ ...owncastDoesNotCuaseOtherTypesToKeepInterface.cs | 47 ++++++++ .../NoKeptCtor/ObjectHardCastedToInterface.cs | 40 +++++++ .../ArrayPassedAsParameter.cs | 25 +++++ .../ArrayWithIndexAssignedToReturnValue.cs | 26 +++++ .../FieldDowncastedToInterface.cs | 32 ++++++ .../NoKeptCtorButInterfaceNeeded/GenericType.cs | 30 ++++++ .../GenericTypeWithConstraint.cs | 24 +++++ .../GenericTypeWithConstraint2.cs | 28 +++++ .../GenericTypeWithConstraint3.cs | 58 ++++++++++ .../InterfaceOnMultipleBases.cs | 42 ++++++++ .../LocalDowncastedToInterface.cs | 27 +++++ .../LocalPassedAsParameter.cs | 25 +++++ .../LocalPassedAsParameterToGeneric.cs | 25 +++++ ...ocalPassedAsParameterToGenericWithConstraint.cs | 25 +++++ ...calPassedAsParameterToGenericWithConstraint2.cs | 32 ++++++ .../ReturnValueDowncastedToInterface.cs | 30 ++++++ .../Mono.Linker.Tests.Cases.csproj | 18 ++++ 26 files changed, 807 insertions(+), 4 deletions(-) create mode 100644 linker/Linker/MethodBodyScanner.cs create mode 100644 linker/Linker/TypeDefinitionExtensions.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtor/GenericType.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtor/GenericWithConstraintDoesNotCauseOtherTypesToKeepInterface.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtor/LocalDowncastDoesNotCuaseOtherTypesToKeepInterface.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtor/ObjectHardCastedToInterface.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/ArrayPassedAsParameter.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/ArrayWithIndexAssignedToReturnValue.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/FieldDowncastedToInterface.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/GenericType.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/GenericTypeWithConstraint.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/GenericTypeWithConstraint2.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/GenericTypeWithConstraint3.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/InterfaceOnMultipleBases.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/LocalDowncastedToInterface.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/LocalPassedAsParameter.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/LocalPassedAsParameterToGeneric.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/LocalPassedAsParameterToGenericWithConstraint.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/LocalPassedAsParameterToGenericWithConstraint2.cs create mode 100644 linker/Tests/Mono.Linker.Tests.Cases/Inheritance.Interfaces/OnReferenceType/NoKeptCtorButInterfaceNeeded/ReturnValueDowncastedToInterface.cs diff --git a/linker/Linker.Steps/MarkStep.cs b/linker/Linker.Steps/MarkStep.cs index fb06bc70c..a42c5facd 100644 --- a/linker/Linker.Steps/MarkStep.cs +++ b/linker/Linker.Steps/MarkStep.cs @@ -266,7 +266,7 @@ namespace Mono.Linker.Steps { // We don't need to mark overrides until it is possible that the type could be instantiated // Note : The base type is interface check should be removed once we have base type sweeping - if (!isInstantiated && @base.DeclaringType.IsInterface) + if (@base.DeclaringType.IsInterface && !isInstantiated && !IsInterfaceImplementationMarked (method.DeclaringType, @base.DeclaringType)) return; if (!isInstantiated && !@base.IsAbstract) @@ -276,6 +276,11 @@ namespace Mono.Linker.Steps { ProcessVirtualMethod (method); } + bool IsInterfaceImplementationMarked (TypeDefinition type, TypeDefinition interfaceType) + { + return type.HasInterface (@interfaceType, out InterfaceImplementation implementation) && Annotations.IsMarked (implementation); + } + void MarkMarshalSpec (IMarshalInfoProvider spec) { if (!spec.HasMarshalInfo) @@ -2004,6 +2009,8 @@ namespace Mono.Linker.Steps { foreach (Instruction instruction in body.Instructions) MarkInstruction (instruction); + MarkInterfacesNeededByBodyStack (body); + MarkThingsUsedViaReflection (body); PostMarkMethodBody (body); @@ -2011,6 +2018,19 @@ namespace Mono.Linker.Steps { partial void PostMarkMethodBody (MethodBody body); + void MarkInterfacesNeededByBodyStack (MethodBody body) + { + // If a type could be on the stack in the body and an interface it implements could be on the stack on the body + // then we need to mark that interface implementation. When this occurs it is not safe to remove the interface implementation from the type + // even if the type is never instantiated + var implementations = MethodBodyScanner.GetReferencedInterfaces (_context.Annotations, body); + if (implementations == null) + return; + + foreach (var implementation in implementations) + MarkInterfaceImplementation (implementation); + } + protected virtual void MarkThingsUsedViaReflection (MethodBody body) { MarkSomethingUsedViaReflection ("GetConstructor", MarkConstructorsUsedViaReflection, body.Instructions); diff --git a/linker/Linker.Steps/TypeMapStep.cs b/linker/Linker.Steps/TypeMapStep.cs index 44e882330..4189e65a8 100644 --- a/linker/Linker.Steps/TypeMapStep.cs +++ b/linker/Linker.Steps/TypeMapStep.cs @@ -44,6 +44,7 @@ namespace Mono.Linker.Steps { { MapVirtualMethods (type); MapInterfaceMethodsInTypeHierarchy (type); + MapBaseTypeHierarchy (type); if (!type.HasNestedTypes) return; @@ -123,6 +124,30 @@ namespace Mono.Linker.Steps { } } + void MapBaseTypeHierarchy (TypeDefinition type) + { + if (!type.IsClass) + return; + + var bases = new List (); + var current = type.BaseType; + + while (current != null) { + var resolved = current.Resolve (); + if (resolved == null) + break; + + // Exclude Object. That's implied and adding it to the list will just lead to lots of extra unnecessary processing + if (resolved.BaseType == null) + break; + + bases.Add (resolved); + current = resolved.BaseType; + } + + Annotations.SetClassHierarchy (type, bases); + } + void AnnotateMethods (MethodDefinition @base, MethodDefinition @override) { Annotations.AddBaseMethod (@override, @base); diff --git a/linker/Linker/Annotations.cs b/linker/Linker/Annotations.cs index 80b90e0b5..488ce9cc4 100644 --- a/linker/Linker/Annotations.cs +++ b/linker/Linker/Annotations.cs @@ -48,6 +48,7 @@ namespace Mono.Linker { protected readonly Dictionary> override_methods = new Dictionary> (); protected readonly Dictionary> base_methods = new Dictionary> (); protected readonly Dictionary symbol_readers = new Dictionary (); + protected readonly Dictionary> class_type_base_hierarchy = new Dictionary> (); protected readonly Dictionary> custom_annotations = new Dictionary> (); protected readonly Dictionary> resources_to_remove = new Dictionary> (); @@ -355,5 +356,17 @@ namespace Mono.Linker { return marked_types_with_cctor.Add (type); } + public void SetClassHierarchy (TypeDefinition type, List bases) + { + class_type_base_hierarchy [type] = bases; + } + + public List GetClassHierarchy (TypeDefinition type) + { + if (class_type_base_hierarchy.TryGetValue (type, out List bases)) + return bases; + + return null; + } } } diff --git a/linker/Linker/MethodBodyScanner.cs b/linker/Linker/MethodBodyScanner.cs new file mode 100644 index 000000000..f875ad162 --- /dev/null +++ b/linker/Linker/MethodBodyScanner.cs @@ -0,0 +1,119 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using Mono.Cecil; +using Mono.Cecil.Cil; + +namespace Mono.Linker { + public static class MethodBodyScanner { + public static IEnumerable GetReferencedInterfaces (AnnotationStore annotations, MethodBody body) + { + var possibleStackTypes = AllPossibleStackTypes (body.Method); + if (possibleStackTypes.Count == 0) + return null; + + var interfaceTypes = possibleStackTypes.Where (t => t.IsInterface).ToArray (); + if (interfaceTypes.Length == 0) + return null; + + var interfaceImplementations = new HashSet (); + + // If a type could be on the stack in the body and an interface it implements could be on the stack on the body + // then we need to mark that interface implementation. When this occurs it is not safe to remove the interface implementation from the type + // even if the type is never instantiated + foreach (var type in possibleStackTypes) { + // We only sweep interfaces on classes so that's why we only care about classes + if (!type.IsClass) + continue; + + AddMatchingInterfaces (interfaceImplementations, type, interfaceTypes); + var bases = annotations.GetClassHierarchy (type); + foreach (var @base in bases) { + AddMatchingInterfaces (interfaceImplementations, @base, interfaceTypes); + } + } + + return interfaceImplementations; + } + + static HashSet AllPossibleStackTypes (MethodDefinition method) + { + if (!method.HasBody) + throw new ArgumentException(); + + var body = method.Body; + var types = new HashSet (); + + foreach (VariableDefinition var in body.Variables) + AddIfResolved (types, var.VariableType); + + foreach (ExceptionHandler eh in body.ExceptionHandlers) { + if (eh.HandlerType == ExceptionHandlerType.Catch) { + AddIfResolved (types, eh.CatchType); + } + } + + foreach (Instruction instruction in body.Instructions) { + if (instruction.Operand is FieldReference fieldReference) { + AddIfResolved (types, fieldReference.Resolve ()?.FieldType); + } else if (instruction.Operand is MethodReference methodReference) { + if (methodReference is GenericInstanceMethod genericInstanceMethod) + AddFromGenericInstance (types, genericInstanceMethod); + + if (methodReference.DeclaringType is GenericInstanceType genericInstanceType) + AddFromGenericInstance (types, genericInstanceType); + + var resolvedMethod = methodReference.Resolve (); + if (resolvedMethod != null) { + if (resolvedMethod.HasParameters) { + foreach (var param in resolvedMethod.Parameters) + AddIfResolved (types, param.ParameterType); + } + + AddFromGenericParameterProvider (types, resolvedMethod); + AddFromGenericParameterProvider (types, resolvedMethod.DeclaringType); + AddIfResolved (types, resolvedMethod.ReturnType); + } + } + } + + return types; + } + + static void AddMatchingInterfaces (HashSet results, TypeDefinition type, TypeDefinition [] interfaceTypes) + { + foreach (var interfaceType in interfaceTypes) { + if (type.HasInterface (interfaceType, out InterfaceImplementation implementation)) + results.Add (implementation); + } + } + + static void AddFromGenericInstance (HashSet set, IGenericInstance instance) + { + if (!instance.HasGenericArguments) + return; + + foreach (var genericArgument in instance.GenericArguments) + AddIfResolved (set, genericArgument); + } + + static void AddFromGenericParameterProvider (HashSet set, IGenericParameterProvider provider) + { + if (!provider.HasGenericParameters) + return; + + foreach (var genericParameter in provider.GenericParameters) { + foreach (var constraint in genericParameter.Constraints) + AddIfResolved (set, constraint); + } + } + + static void AddIfResolved (HashSet set, TypeReference item) + { + var resolved = item.Resolve (); + if (resolved == null) + return; + set.Add (resolved); + } + } +} \ No newline at end of file diff --git a/linker/Linker/TypeDefinitionExtensions.cs b/linker/Linker/TypeDefinitionExtensions.cs new file mode 100644 index 000000000..f94e340c3 --- /dev/null +++ b/linker/Linker/TypeDefinitionExtensions.cs @@ -0,0 +1,21 @@ +using Mono.Cecil; + +namespace Mono.Linker { + public static class TypeDefinitionExtensions { + public static bool HasInterface (this TypeDefinition type, TypeDefinition interfaceType, out InterfaceImplementation implementation) + { + implementation = null; + if (!type.HasInterfaces) + return false; + + foreach (var iface in type.Interfaces) { + if (iface.InterfaceType.Resolve () == interfaceType) { + implementation = iface; + return true; + } + } + + return false; + } + } +} \ No newline at end of file diff --git a/linker/Mono.Linker.csproj b/linker/Mono.Linker.csproj index cd547ca51..2b82f3838 100644 --- a/linker/Mono.Linker.csproj +++ b/linker/Mono.Linker.csproj @@ -1,4 +1,4 @@ - +