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.

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

Managed type system has trouble resolving virtual methods #52444

Closed
MichalStrehovsky opened this issue May 7, 2021 · 2 comments · Fixed by #53400
Closed

Managed type system has trouble resolving virtual methods #52444

MichalStrehovsky opened this issue May 7, 2021 · 2 comments · Fixed by #53400
Assignees
Milestone

Comments

@MichalStrehovsky
Copy link
Member

I assume this is a regression from #51764.

In the \src\tests\JIT\Generics\Instantiation\Interfaces\Class05.cs test, we have a situation where:

public interface IGen<T>
{
    void _Init(T fld1);
    bool InstVerify(System.Type t1);
}

public interface IGenSub2DStringArray : IGen<string[,]> { }

public class Gen2DStringArray : IGenSub2DStringArray
{
    public void _Init(string[,] fld1)
    {
    }
}

When the managed type system tries to find implementation of IGen<string[]>._Init on Gen2DStringArray, it will try to match up the signature of _Init between those two types. The signatures don't match because the signature of _Init on Gen2DStringArray has EmbeddedSignatureData of type ArrayShape with contents 1.2.2.1||0,0, while the signature on the interface doesn't have an ArrayShape. The Equals check on the signature fails at callstack:

>	ILCompiler.TypeSystem.dll!Internal.TypeSystem.MethodSignature.Equals(Internal.TypeSystem.MethodSignature otherSignature, bool allowCovariantReturn) Line 220	C#
 	ILCompiler.TypeSystem.dll!Internal.TypeSystem.MethodSignature.Equals(Internal.TypeSystem.MethodSignature otherSignature) Line 185	C#
 	ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataVirtualMethodAlgorithm.FindMatchingVirtualMethodOnTypeByNameAndSig(Internal.TypeSystem.MethodDesc targetMethod, Internal.TypeSystem.DefType currentType, bool reverseMethodSearch, System.Func<Internal.TypeSystem.MethodDesc, Internal.TypeSystem.MethodDesc, bool> nameSigMatchMethodIsValidCandidate) Line 340	C#
 	ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataVirtualMethodAlgorithm.ResolveInterfaceMethodToVirtualMethodOnType(Internal.TypeSystem.MethodDesc interfaceMethod, Internal.TypeSystem.MetadataType currentType) Line 601	C#
 	ILCompiler.TypeSystem.dll!Internal.TypeSystem.MetadataVirtualMethodAlgorithm.ResolveInterfaceMethodToVirtualMethodOnType(Internal.TypeSystem.MethodDesc interfaceMethod, Internal.TypeSystem.TypeDesc currentType) Line 565	C#
 	ILCompiler.TypeSystem.dll!Internal.TypeSystem.TypeSystemHelpers.ResolveInterfaceMethodToVirtualMethodOnType(Internal.TypeSystem.TypeDesc type, Internal.TypeSystem.MethodDesc interfaceMethod) Line 275	C#

There are more tests where this is failing, but I assume in crossgen2 this only means a failure to devirtualize in these tests. The failure mode is more brutal in NativeAOT.

But this can result in incorrect devirtualization with crossgen2 if there's a matching signature in one of the base types - I assume we don't have tests for that.

Cc @dotnet/crossgen-contrib

@MichalStrehovsky MichalStrehovsky added this to the 6.0.0 milestone May 7, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label May 7, 2021
@mangod9 mangod9 removed the untriaged New issue has not been triaged by the area owner label May 7, 2021
@MichalStrehovsky
Copy link
Member Author

Might have been fixed with #52796 but needs verification.

@davidwrighton davidwrighton self-assigned this May 25, 2021
@davidwrighton
Copy link
Member

Sigh. #53796 didn't fix this one. Looking into this more deeply. The fix here is likely to be confusing.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 28, 2021
MichalStrehovsky pushed a commit that referenced this issue May 31, 2021
- Change up the definition of an MDArray which doesn't need a special descriptor in the embedded signature data to match the type of mdarray thay everyone always uses. This fixes the virtual function handling behavior for all reasonable cases. (All cases where the MDArray is used for a base type in the expected fashion.)
- Add a devirtualization test for this specific scenario

Fixes #52444
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 31, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants