Skip to content

Commit

Permalink
ILLink: Sweep .override for interface method if the .interfaceImpl is…
Browse files Browse the repository at this point in the history
… removed (dotnet#102857)

When static interface methods are kept but an implementation of the method is only accessed via a direct call on the implementing type, the implementation method and the .override pointing to the interface method are both kept, but not the .interfaceImpl. This causes a TypeLoadException when the .override points to a method on an interface that the type doesn't implement. To fix this, we needed to update the condition for sweeping overrides to include the case where both the interface and the interface method are kept, but the .interfaceImpl is not kept.

Tests are for static interface methods, but concrete and generic to test type resolution in SweepStep, which can be finicky. Instance methods shouldn't hit this issue since public interface methods don't have a .override and private methods can't be marked without instantiating the type or reflecting over the type, both of which mark the type's .interfaceImpls, and making the .override valid.

For instance methods, we will always mark any methods referenced in a .override as well as their corresponding interface implementation, so we won't end up in a scenario where an instance method would have dangling references in a .override. I did fix up the interface implementation marking (MarkRuntimeInterfaceImplementation) to make sure it can find a recursive interface, and marks the .interfaceImpl with the exact same TypeReference rather than just the same TypeDefinition (for example, IGeneric<int> should be marked, not just the first IGeneric<> interfaceImpl). This was one of the examples where the trimmer would end up doing the right thing (all IGeneric<> implementations would end up marked anyway), but it would do it for a different reason than we expect.
  • Loading branch information
jtschuster committed Jun 12, 2024
1 parent 6b7900d commit 24a436d
Show file tree
Hide file tree
Showing 16 changed files with 1,145 additions and 102 deletions.
60 changes: 25 additions & 35 deletions src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Reflection.Runtime.TypeParsing;
using System.Runtime.CompilerServices;
using System.Text.RegularExpressions;
using ILLink.Shared;
using ILLink.Shared.TrimAnalysis;
Expand Down Expand Up @@ -779,33 +778,25 @@ void MarkMethodIfNeededByBaseMethod (MethodDefinition method)
/// or if any marked interface implementations on <paramref name="type"/> are interfaces that implement <paramref name="interfaceType"/> and that interface implementation is marked
/// </summary>
bool IsInterfaceImplementationMarkedRecursively (TypeDefinition type, TypeDefinition interfaceType)
=> IsInterfaceImplementationMarkedRecursively (type, interfaceType, Context);

/// <summary>
/// Returns true if <paramref name="type"/> implements <paramref name="interfaceType"/> and the interface implementation is marked,
/// or if any marked interface implementations on <paramref name="type"/> are interfaces that implement <paramref name="interfaceType"/> and that interface implementation is marked
/// </summary>
internal static bool IsInterfaceImplementationMarkedRecursively (TypeDefinition type, TypeDefinition interfaceType, LinkContext context)
{
if (type.HasInterfaces) {
foreach (var intf in type.Interfaces) {
TypeDefinition? resolvedInterface = Context.Resolve (intf.InterfaceType);
TypeDefinition? resolvedInterface = context.Resolve (intf.InterfaceType);
if (resolvedInterface == null)
continue;

if (Annotations.IsMarked (intf) && RequiresInterfaceRecursively (resolvedInterface, interfaceType))
return true;
}
}

return false;
}

bool RequiresInterfaceRecursively (TypeDefinition typeToExamine, TypeDefinition interfaceType)
{
if (typeToExamine == interfaceType)
return true;

if (typeToExamine.HasInterfaces) {
foreach (var iface in typeToExamine.Interfaces) {
var resolved = Context.TryResolve (iface.InterfaceType);
if (resolved == null)
if (!context.Annotations.IsMarked (intf))
continue;

if (RequiresInterfaceRecursively (resolved, interfaceType))
if (resolvedInterface == interfaceType)
return true;
if (IsInterfaceImplementationMarkedRecursively (resolvedInterface, interfaceType, context))
return true;
}
}
Expand Down Expand Up @@ -3239,8 +3230,8 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
Context.Resolve (@base) is MethodDefinition baseDefinition
&& new OverrideInformation.OverridePair (baseDefinition, method).IsStaticInterfaceMethodPair ())
continue;
MarkMethod (@base, new DependencyInfo (DependencyKind.MethodImplOverride, method), ScopeStack.CurrentScope.Origin);
MarkExplicitInterfaceImplementation (method, @base);
MarkMethod (@base, new DependencyInfo (DependencyKind.MethodImplOverride, method), methodOrigin);
MarkRuntimeInterfaceImplementation (method, @base);
}
}

Expand Down Expand Up @@ -3355,22 +3346,21 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type
DoAdditionalInstantiatedTypeProcessing (type);
}

void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference ov)
void MarkRuntimeInterfaceImplementation (MethodDefinition method, MethodReference ov)
{
if (Context.Resolve (ov) is not MethodDefinition resolvedOverride)
return;
if (!resolvedOverride.DeclaringType.IsInterface)
return;
var interfaceToBeImplemented = ov.DeclaringType;

if (resolvedOverride.DeclaringType.IsInterface) {
foreach (var ifaceImpl in method.DeclaringType.Interfaces) {
var resolvedInterfaceType = Context.Resolve (ifaceImpl.InterfaceType);
if (resolvedInterfaceType == null) {
continue;
}

if (resolvedInterfaceType == resolvedOverride.DeclaringType) {
MarkInterfaceImplementation (ifaceImpl, new MessageOrigin (method.DeclaringType));
return;
}
var ifaces = Annotations.GetRecursiveInterfaces (method.DeclaringType);
if (ifaces is null)
return;
foreach (var iface in ifaces) {
if (TypeReferenceEqualityComparer.AreEqual (iface.InterfaceType, interfaceToBeImplemented, Context)) {
MarkInterfaceImplementationList (iface.ImplementationChain, new MessageOrigin (method.DeclaringType));
return;
}
}
}
Expand Down
11 changes: 8 additions & 3 deletions src/tools/illink/src/linker/Linker.Steps/SweepStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -456,14 +456,19 @@ void SweepOverrides (MethodDefinition method)
// This can happen for a couple of reasons, but it indicates the method isn't in the final assembly.
// Resolve also may return a removed value if method.Overrides[i] is a MethodDefinition. In this case, Resolve short circuits and returns `this`.
// OR
// ov.DeclaringType is null
// ov.DeclaringType is null
// ov.DeclaringType may be null if Resolve short circuited and returned a removed method. In this case, we want to remove the override.
// OR
// ov is in a `link` scope and is unmarked
// 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.
// OR
// ov is an interface method and the interface is not implemented by the type
#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)))
if (method.Overrides[i].Resolve () is not MethodDefinition ov
|| ov.DeclaringType is null
|| (IsLinkScope (ov.DeclaringType.Scope) && ShouldRemove (ov))
|| (ov.DeclaringType.IsInterface && !MarkStep.IsInterfaceImplementationMarkedRecursively (method.DeclaringType, ov.DeclaringType, Context)))
method.Overrides.RemoveAt (i);
else
i++;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ public Task BaseProvidesInterfaceMethod ()
return RunTest (allowMissingWarnings: false);
}

[Fact]
public Task RemovedInterfaceImplementationRemovedOverride()
{
return RunTest (allowMissingWarnings: false);
}

[Fact]
public Task StaticAbstractInterfaceMethods ()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@ namespace Mono.Linker.Tests.Cases.Expectations.Assertions
[AttributeUsage (AttributeTargets.Method, AllowMultiple = true, Inherited = false)]
public class KeptOverrideAttribute : KeptAttribute
{
public Type TypeWithOverriddenMethodDeclaration;

public KeptOverrideAttribute (Type typeWithOverriddenMethod)
{
ArgumentNullException.ThrowIfNull (typeWithOverriddenMethod);
TypeWithOverriddenMethodDeclaration = typeWithOverriddenMethod;
}
public KeptOverrideAttribute (string typeWithOverriddenMethod)
{
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// 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.All, AllowMultiple = true)]
public class KeptOverrideOnMethodInAssemblyAttribute : BaseInAssemblyAttribute
{
public KeptOverrideOnMethodInAssemblyAttribute (string assemblyName, string typeName, string methodName, string overriddenMethodName)
{
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,14 @@ namespace Mono.Linker.Tests.Cases.Expectations.Assertions
[AttributeUsage (AttributeTargets.Method, AllowMultiple = true, Inherited = false)]
public class RemovedOverrideAttribute : BaseInAssemblyAttribute
{
public Type TypeWithOverriddenMethodDeclaration;
public RemovedOverrideAttribute (Type typeWithOverriddenMethod)
{
if (typeWithOverriddenMethod == null)
throw new ArgumentException ("Value cannot be null or empty.", nameof (typeWithOverriddenMethod));
TypeWithOverriddenMethodDeclaration = typeWithOverriddenMethod;
}

public RemovedOverrideAttribute (string nameOfOverriddenMethod)
{
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// 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.All, AllowMultiple = true, Inherited = false)]
public class RemovedOverrideOnMethodInAssemblyAttribute : BaseInAssemblyAttribute
{
public RemovedOverrideOnMethodInAssemblyAttribute (string library, string typeName, string methodName, string overriddenMethodFullName)
{
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
// 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.

.assembly extern mscorlib { }

.assembly 'library' { }

.class public auto ansi beforefieldinit Program
extends [mscorlib]System.Object
{
// Nested Types
.class interface nested public auto ansi abstract beforefieldinit IBaseUnused
{
// Methods
.method public hidebysig abstract virtual static
void M () cil managed
{
} // end of method IBaseUnused::M

} // end of class IBaseUnused

.class interface nested public auto ansi abstract beforefieldinit IBaseUsed
{
// Methods
.method public hidebysig abstract virtual static
void M () cil managed
{
} // end of method IBaseUsed::M

} // end of class IBaseUsed

.class interface nested private auto ansi abstract beforefieldinit IMiddleUnused
implements Program/IBaseUnused,
Program/IBaseUsed
{
// Methods
.method public hidebysig abstract virtual static
void O () cil managed
{
} // end of method IMiddleUnused::O

} // end of class IMiddleUnused

.class interface nested private auto ansi abstract beforefieldinit IDerived
implements Program/IMiddleUnused
{
// Methods
.method public hidebysig abstract virtual static
void N () cil managed
{
} // end of method IDerived::N

} // end of class IDerived

.class nested private auto ansi beforefieldinit A
extends [mscorlib]System.Object
implements Program/IDerived
{
// Methods
.method public hidebysig static
void M () cil managed
{
.override method void Program/IBaseUnused::M()
.override method void Program/IBaseUsed::M()
// Method begins at RVA 0x2083
// Code size 1 (0x1)
.maxstack 8

IL_0000: ret
} // end of method A::M

.method public hidebysig static
void N () cil managed
{
.override method void Program/IDerived::N()
// Method begins at RVA 0x2083
// Code size 1 (0x1)
.maxstack 8

IL_0000: ret
} // end of method A::N

.method public hidebysig static
void O () cil managed
{
.override method void Program/IMiddleUnused::O()
// Method begins at RVA 0x2083
// Code size 1 (0x1)
.maxstack 8

IL_0000: ret
} // end of method A::O

.method public hidebysig specialname rtspecialname
instance void .ctor () cil managed
{
// Method begins at RVA 0x207b
// Code size 7 (0x7)
.maxstack 8

IL_0000: ldarg.0
IL_0001: call instance void [mscorlib]System.Object::.ctor()
IL_0006: ret
} // end of method A::.ctor

} // end of class A


// Methods
.method public hidebysig static
void MyTest () cil managed
{
// Method begins at RVA 0x2050
// Code size 16 (0x10)
.maxstack 8

IL_0000: call void Program::UseNThroughIDerived<class Program/A>()
IL_0005: call void Program::UseMThroughIDerived<class Program/A>()
IL_000a: call void Program/A::M()
IL_000f: call void Program/A::O()
IL_0014: ret
} // end of method Program::MyTest

.method private hidebysig static
void UseNThroughIDerived<(Program/IDerived) T> () cil managed
{
.param constraint T, Program/IDerived
.custom instance void [mscorlib]System.Runtime.CompilerServices.NullableAttribute::.ctor(uint8) = (
01 00 01 00 00
)
// Method begins at RVA 0x2061
// Code size 12 (0xc)
.maxstack 8

IL_0000: constrained. !!T
IL_0006: call void Program/IDerived::N()
IL_000b: ret
} // end of method Program::UseNThroughIDerived

.method private hidebysig static
void UseMThroughIDerived<(Program/IBaseUsed) T> () cil managed
{
.param constraint T, Program/IBaseUnused
.custom instance void [mscorlib]System.Runtime.CompilerServices.NullableAttribute::.ctor(uint8) = (
01 00 01 00 00
)
// Method begins at RVA 0x206e
// Code size 12 (0xc)
.maxstack 8

IL_0000: constrained. !!T
IL_0006: call void Program/IBaseUsed::M()
IL_000b: ret
} // end of method Program::UseMThroughIDerived

.method public hidebysig specialname rtspecialname
instance void .ctor () cil managed
{
// Method begins at RVA 0x207b
// Code size 7 (0x7)
.maxstack 8

IL_0000: ldarg.0
IL_0001: call instance void [mscorlib]System.Object::.ctor()
IL_0006: ret
} // end of method Program::.ctor

} // end of class Program
Loading

0 comments on commit 24a436d

Please sign in to comment.