Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes around virtual method hierarchy validation #78157

Merged
merged 5 commits into from
Nov 16, 2022
Merged

Fixes around virtual method hierarchy validation #78157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e43d9b9
Fixes around virtual method hierarchy validation
vitek-karas Nov 10, 2022
f032aa4
Fixes and PR feedback
vitek-karas Nov 15, 2022
0449541
More tests
vitek-karas Nov 15, 2022
96c4027
More fixes and tests
vitek-karas Nov 15, 2022
f2c6e5d
PR feedback and test sync from linker
vitek-karas Nov 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GVMDependenciesNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ public override IEnumerable<CombinedDependencyListEntry> SearchDynamicDependenci
openInstantiation[instArg] = context.GetSignatureVariable(instArg, method: true);
MethodDesc implementingMethodInstantiation = slotDecl.MakeInstantiatedMethod(openInstantiation).InstantiateSignature(potentialOverrideType.Instantiation, _method.Instantiation);
dynamicDependencies.Add(new CombinedDependencyListEntry(factory.GVMDependencies(implementingMethodInstantiation.GetCanonMethodTarget(CanonicalFormKind.Specific)), null, "ImplementingMethodInstantiation"));

factory.MetadataManager.NoteOverridingMethod(_method, implementingMethodInstantiation);
}
}
}
Expand Down Expand Up @@ -206,8 +208,12 @@ public override IEnumerable<CombinedDependencyListEntry> SearchDynamicDependenci
MethodDesc instantiatedTargetMethod = potentialOverrideType.FindVirtualFunctionTargetMethodOnObjectType(methodToResolve)
.GetCanonMethodTarget(CanonicalFormKind.Specific);
if (instantiatedTargetMethod != _method)
{
dynamicDependencies.Add(new CombinedDependencyListEntry(
factory.GVMDependencies(instantiatedTargetMethod), null, "DerivedMethodInstantiation"));

factory.MetadataManager.NoteOverridingMethod(_method, instantiatedTargetMethod);
}
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericLookupResult.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1475,6 +1475,8 @@ public override ISymbolNode GetTarget(NodeFactory factory, GenericLookupResultCo
// AOT use of this generic lookup is restricted to finding methods on valuetypes (runtime usage of this slot in universal generics is more flexible)
Debug.Assert(instantiatedConstraintType.IsValueType || (instantiatedConstrainedMethod.OwningType.IsInterface && instantiatedConstrainedMethod.Signature.IsStatic));

factory.MetadataManager.NoteOverridingMethod(_constrainedMethod, implMethod);

if (implMethod.Signature.IsStatic)
{
if (implMethod.GetCanonMethodTarget(CanonicalFormKind.Specific).IsSharedByGenericInstantiations)
Expand Down
4 changes: 1 addition & 3 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,7 @@ public IEnumerable<InterfaceGVMEntryInfo> ScanForInterfaceGenericVirtualMethodEn
{
foreach (var method in iface.GetVirtualMethods())
{
Debug.Assert(!method.Signature.IsStatic);

if (!method.HasInstantiation)
if (method.Signature.IsStatic || !method.HasInstantiation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand -- do static interface methods not have slots? And can't they enter the exact diamond problem that's presented below?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches what we do for non-default methods here:

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/TypeGVMEntriesNode.cs

Lines 106 to 107 in 7db5a57

if (!method.HasInstantiation || method.Signature.IsStatic)
continue;

This code is used to generate a data structure that can be used to resolve generic virtual methods at runtime. We'll stop filtering them here eventually to fix #77070 but a lot more needs to happen to fix that.

continue;

DefaultInterfaceMethodResolution resolution = DefaultInterfaceMethodResolution.None;
Expand Down
7 changes: 5 additions & 2 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,6 +840,9 @@ public bool GeneratesAttributeMetadata(TypeDesc attributeType)

public override void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc overridingMethod)
{
baseMethod = baseMethod.GetTypicalMethodDefinition();
overridingMethod = overridingMethod.GetTypicalMethodDefinition();

bool baseMethodTypeIsInterface = baseMethod.OwningType.IsInterface;
foreach (var requiresAttribute in _requiresAttributeMismatchNameAndId)
{
Expand All @@ -855,8 +858,8 @@ public override void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc over
}
}

bool baseMethodRequiresDataflow = FlowAnnotations.RequiresDataflowAnalysis(baseMethod);
bool overridingMethodRequiresDataflow = FlowAnnotations.RequiresDataflowAnalysis(overridingMethod);
bool baseMethodRequiresDataflow = FlowAnnotations.RequiresVirtualMethodDataflowAnalysis(baseMethod);
bool overridingMethodRequiresDataflow = FlowAnnotations.RequiresVirtualMethodDataflowAnalysis(overridingMethod);
if (baseMethodRequiresDataflow || overridingMethodRequiresDataflow)
{
FlowAnnotations.ValidateMethodAnnotationsAreSame(overridingMethod, baseMethod);
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,8 @@ private void ImportCall(ILOpcode opcode, int token)
resolvedConstraint = true;

exactType = directMethod.OwningType;

_factory.MetadataManager.NoteOverridingMethod(method, directMethod);
}
else if (method.Signature.IsStatic)
{
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1220,6 +1220,8 @@ private void getCallInfo(ref CORINFO_RESOLVED_TOKEN pResolvedToken, CORINFO_RESO
pResult->thisTransform = CORINFO_THIS_TRANSFORM.CORINFO_NO_THIS_TRANSFORM;

exactType = directMethod.OwningType;

_compilation.NodeFactory.MetadataManager.NoteOverridingMethod(method, directMethod);
}
else if (method.Signature.IsStatic)
{
Expand Down
66 changes: 66 additions & 0 deletions src/coreclr/tools/aot/Mono.Linker.Tests.Cases/DataFlow/Dependencies/Library.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// 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.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Mono.Linker.Tests.Cases.DataFlow.Dependencies
{
public class Library
{
public interface IAnnotatedMethods
{
static abstract void GenericWithMethodsStatic<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T> ();

static abstract void ParamWithMethodsStatic ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type t);

[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
static abstract Type ReturnWithMethodsStatic ();

void GenericWithMethods<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T> ();

void ParamWithMethods ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type t);

[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
Type ReturnWithMethods ();
}

public interface IUnannotatedMethods
{
static abstract void GenericStatic<T> ();

static abstract void ParamStatic (Type t);

static abstract Type ReturnStatic ();

void Generic<T> ();

void Param (Type t);

Type Return ();
}

public abstract class AnnotatedMethods
{
public abstract void GenericWithMethods<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T> ();

public abstract void ParamWithMethods ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type t);

[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
public abstract Type ReturnWithMethods ();
}

public abstract class UnannotatedMethods
{
public abstract void Generic<T> ();

public abstract void Param (Type t);

public abstract Type Return ();
}
}
}
Loading