Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit d02fbd2

Browse files
AndyAyersMSAndyAyersMS
committed
Fix several devirtualization issues
When doing interface devirtualization, if the object type is canonical, ensure that the owner type is too. The jit may present a mixed set when inlining a shared method into a non-shared method. Ideally the jit would also be able to present exact object types in such cases but currently it cannot guarantee this. Closes #10311. Adjust contracts to address some contract violations seen in desktop testing. Make the helper non-static and fold in some of the info that was passed from the caller to bring the desktop and CoreCLR implementations closer. Disallow interface devirt if the method is final but the class is not exact or final, since derived classes can still override final methods when implementing interfaces. Don't try and devirtualize interface calls from com objects. Add some related test cases.
1 parent 68012bf commit d02fbd2

File tree

11 files changed

+560
-10
lines changed

11 files changed

+560
-10
lines changed

‎src/jit/importer.cpp‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18521,7 +18521,14 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
1852118521
// the time of jitting, objClass has no subclasses that
1852218522
// override this method), then perhaps we'd be willing to
1852318523
// make a bet...?
18524-
JITDUMP(" Class NOT final or exact, no devirtualization\n");
18524+
JITDUMP(" Class not final or exact, method not final, no devirtualization\n");
18525+
return;
18526+
}
18527+
18528+
// For interface calls we must have an exact type or final class.
18529+
if (isInterface && !isExact && !objClassIsFinal)
18530+
{
18531+
JITDUMP(" Class not final or exact for interface, no devirtualization\n");
1852518532
return;
1852618533
}
1852718534

@@ -18604,4 +18611,4 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
1860418611
baseMethodName, derivedClassName, derivedMethodName, note);
1860518612
}
1860618613
#endif // defined(DEBUG)
18607-
}
18614+
}

‎src/vm/jitinterface.cpp‎

Lines changed: 37 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8714,12 +8714,16 @@ void CEEInfo::getMethodVTableOffset (CORINFO_METHOD_HANDLE methodHnd,
87148714
}
87158715

87168716
/*********************************************************************/
8717-
static CORINFO_METHOD_HANDLE resolveVirtualMethodHelper(MethodDesc* callerMethod,
8718-
CORINFO_METHOD_HANDLE baseMethod,
8719-
CORINFO_CLASS_HANDLE derivedClass,
8720-
CORINFO_CONTEXT_HANDLE ownerType)
8717+
CORINFO_METHOD_HANDLE CEEInfo::resolveVirtualMethodHelper(CORINFO_METHOD_HANDLE baseMethod,
8718+
CORINFO_CLASS_HANDLE derivedClass,
8719+
CORINFO_CONTEXT_HANDLE ownerType)
87218720
{
8722-
STANDARD_VM_CONTRACT;
8721+
CONTRACTL {
8722+
SO_TOLERANT;
8723+
THROWS;
8724+
GC_TRIGGERS;
8725+
MODE_PREEMPTIVE;
8726+
} CONTRACTL_END;
87238727

87248728
MethodDesc* pBaseMD = GetMethod(baseMethod);
87258729
MethodTable* pBaseMT = pBaseMD->GetMethodTable();
@@ -8747,6 +8751,15 @@ static CORINFO_METHOD_HANDLE resolveVirtualMethodHelper(MethodDesc* callerMethod
87478751

87488752
if (pBaseMT->IsInterface())
87498753
{
8754+
8755+
#ifdef FEATURE_COMINTEROP
8756+
// Don't try and devirtualize com interface calls.
8757+
if (pDerivedMT->IsComObjectType())
8758+
{
8759+
return nullptr;
8760+
}
8761+
#endif // FEATURE_COMINTEROP
8762+
87508763
// Interface call devirtualization.
87518764
//
87528765
// We must ensure that pDerivedMT actually implements the
@@ -8760,7 +8773,17 @@ static CORINFO_METHOD_HANDLE resolveVirtualMethodHelper(MethodDesc* callerMethod
87608773
// safely devirtualize.
87618774
if (ownerType != nullptr)
87628775
{
8763-
pDevirtMD = pDerivedMT->GetMethodDescForInterfaceMethod(GetTypeFromContext(ownerType), pBaseMD);
8776+
TypeHandle OwnerClsHnd = GetTypeFromContext(ownerType);
8777+
MethodTable* pOwnerMT = OwnerClsHnd.GetMethodTable();
8778+
8779+
// If the derived class is a shared class, make sure the
8780+
// owner class is too.
8781+
if (pDerivedMT->IsSharedByGenericInstantiations())
8782+
{
8783+
pOwnerMT = pOwnerMT->GetCanonicalMethodTable();
8784+
}
8785+
8786+
pDevirtMD = pDerivedMT->GetMethodDescForInterfaceMethod(TypeHandle(pOwnerMT), pBaseMD);
87648787
}
87658788
else if (!pBaseMD->HasClassOrMethodInstantiation())
87668789
{
@@ -8810,6 +8833,7 @@ static CORINFO_METHOD_HANDLE resolveVirtualMethodHelper(MethodDesc* callerMethod
88108833
// bubble information and if so, disallow it.
88118834
if (IsReadyToRunCompilation())
88128835
{
8836+
MethodDesc* callerMethod = m_pMethodBeingCompiled;
88138837
Assembly* pCallerAssembly = callerMethod->GetModule()->GetAssembly();
88148838
bool allowDevirt =
88158839
IsInSameVersionBubble(pCallerAssembly , pDevirtMD->GetModule()->GetAssembly())
@@ -8829,13 +8853,18 @@ CORINFO_METHOD_HANDLE CEEInfo::resolveVirtualMethod(CORINFO_METHOD_HANDLE method
88298853
CORINFO_CLASS_HANDLE derivedClass,
88308854
CORINFO_CONTEXT_HANDLE ownerType)
88318855
{
8832-
STANDARD_VM_CONTRACT;
8856+
CONTRACTL {
8857+
SO_TOLERANT;
8858+
THROWS;
8859+
GC_TRIGGERS;
8860+
MODE_PREEMPTIVE;
8861+
} CONTRACTL_END;
88338862

88348863
CORINFO_METHOD_HANDLE result = nullptr;
88358864

88368865
JIT_TO_EE_TRANSITION();
88378866

8838-
result = resolveVirtualMethodHelper(m_pMethodBeingCompiled, methodHnd, derivedClass, ownerType);
8867+
result = resolveVirtualMethodHelper(methodHnd, derivedClass, ownerType);
88398868

88408869
EE_TO_JIT_TRANSITION();
88418870

‎src/vm/jitinterface.h‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -735,6 +735,12 @@ class CEEInfo : public ICorJitInfo
735735
CORINFO_CONTEXT_HANDLE ownerType
736736
);
737737

738+
CORINFO_METHOD_HANDLE resolveVirtualMethodHelper(
739+
CORINFO_METHOD_HANDLE virtualMethod,
740+
CORINFO_CLASS_HANDLE implementingClass,
741+
CORINFO_CONTEXT_HANDLE ownerType
742+
);
743+
738744
CorInfoIntrinsics getIntrinsicID(CORINFO_METHOD_HANDLE method,
739745
bool * pMustExpand = NULL);
740746

‎tests/src/JIT/opt/Devirtualization/GitHub_10311.cs‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System;
6+
using System.Collections;
7+
using System.Collections.Generic;
8+
9+
public class MyCollection<T> : ICollection<T>
10+
{
11+
private List<T> _items = new List<T>();
12+
13+
public MyCollection()
14+
{
15+
}
16+
17+
public MyCollection(params T[] values)
18+
{
19+
_items.AddRange(values);
20+
}
21+
22+
public void Add(T item)
23+
{
24+
_items.Add(item);
25+
}
26+
27+
public void Clear()
28+
{
29+
_items.Clear();
30+
}
31+
32+
public bool Contains(T item)
33+
{
34+
return _items.Contains(item);
35+
}
36+
37+
public void CopyTo(T[] array, int arrayIndex)
38+
{
39+
_items.CopyTo(array, arrayIndex);
40+
}
41+
42+
public int Count
43+
{
44+
get { return _items.Count; }
45+
}
46+
47+
public bool IsReadOnly
48+
{
49+
get { return ((ICollection<T>)_items).IsReadOnly; }
50+
}
51+
52+
public bool Remove(T item)
53+
{
54+
return _items.Remove(item);
55+
}
56+
57+
public IEnumerator<T> GetEnumerator()
58+
{
59+
return ((ICollection<T>)_items).GetEnumerator();
60+
}
61+
62+
IEnumerator IEnumerable.GetEnumerator()
63+
{
64+
return ((IEnumerable)_items).GetEnumerator();
65+
}
66+
}
67+
68+
class Bug
69+
{
70+
public static int Main()
71+
{
72+
int v = 0;
73+
MyCollection<string> x = new MyCollection<string>("a1", "a2");
74+
foreach (string item in x)
75+
{
76+
v += item[0];
77+
}
78+
return v - 94;
79+
}
80+
}

‎tests/src/JIT/opt/Devirtualization/GitHub_10311.csproj‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
3+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
4+
<PropertyGroup>
5+
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
6+
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
7+
<AssemblyName>$(MSBuildProjectName)</AssemblyName>
8+
<SchemaVersion>2.0</SchemaVersion>
9+
<ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
10+
<OutputType>Exe</OutputType>
11+
<ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
12+
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
13+
</PropertyGroup>
14+
<!-- Default configurations to help VS understand the configurations -->
15+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
16+
</PropertyGroup>
17+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
18+
</PropertyGroup>
19+
<ItemGroup>
20+
<CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
21+
<Visible>False</Visible>
22+
</CodeAnalysisDependentAssemblyPaths>
23+
</ItemGroup>
24+
<PropertyGroup>
25+
<DebugType>None</DebugType>
26+
<Optimize>True</Optimize>
27+
</PropertyGroup>
28+
<ItemGroup>
29+
<Compile Include="GitHub_10311.cs" />
30+
</ItemGroup>
31+
<ItemGroup>
32+
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
33+
</ItemGroup>
34+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
35+
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
36+
</PropertyGroup>
37+
</Project>

‎tests/src/JIT/opt/Devirtualization/generic.cs‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System;
6+
7+
interface Ix
8+
{
9+
int F();
10+
int G();
11+
}
12+
13+
public class B<T> : Ix
14+
{
15+
int Ix.F()
16+
{
17+
if (typeof(T) == typeof(string))
18+
{
19+
return 3;
20+
}
21+
else
22+
{
23+
return 5;
24+
}
25+
}
26+
27+
public virtual int G()
28+
{
29+
if (typeof(T) == typeof(object))
30+
{
31+
return 7;
32+
}
33+
else
34+
{
35+
return 11;
36+
}
37+
38+
}
39+
}
40+
41+
public class D : B<string>, Ix
42+
{
43+
int Ix.F() { return 13; }
44+
}
45+
46+
class E : D
47+
{
48+
public sealed override int G() { return 17; }
49+
}
50+
51+
// K overrides E.G for interface purposes, even though it is sealed
52+
class K : E, Ix
53+
{
54+
int Ix.G() { return 19; }
55+
}
56+
57+
sealed class J : E, Ix
58+
{
59+
int Ix.F() { return 21; }
60+
}
61+
62+
public class Z
63+
{
64+
static int IxF(Ix x) { return x.F(); }
65+
static int IxG(Ix x) { return x.G(); }
66+
67+
public static int Main(string[] args)
68+
{
69+
E e = new E();
70+
K k = new K();
71+
J j = new J();
72+
E q = k;
73+
74+
int callsBFs = IxF(new B<string>());
75+
int callsBFo = IxF(new B<object>());
76+
int callsBGo = IxG(new B<object>());
77+
int callsBGs = IxG(new B<string>()) + IxG(new D());
78+
int callsDF = IxF(new D()) + IxF(e) + IxF(k) + IxF(q);
79+
int callsEG = IxG(e) + IxG(j);
80+
int callsKG = IxG(k) + IxG(q);
81+
int callsJF = IxF(j);
82+
83+
int expected = 3 + 5 + 7 + 2 * 11 + 4 * 13 + 2 * 17 + 2 * 19 + 21;
84+
int val = callsBFs + callsBFo + callsDF + callsBGs + callsBGo + callsEG + callsKG + callsJF;
85+
86+
return val - expected + 100;
87+
}
88+
}
89+
90+

‎tests/src/JIT/opt/Devirtualization/generic.csproj‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?xml version="1.0" encoding="utf-8"?>
2+
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
3+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
4+
<PropertyGroup>
5+
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
6+
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
7+
<AssemblyName>$(MSBuildProjectName)</AssemblyName>
8+
<SchemaVersion>2.0</SchemaVersion>
9+
<ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
10+
<OutputType>Exe</OutputType>
11+
<ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
12+
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
13+
</PropertyGroup>
14+
<!-- Default configurations to help VS understand the configurations -->
15+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
16+
</PropertyGroup>
17+
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
18+
</PropertyGroup>
19+
<ItemGroup>
20+
<CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
21+
<Visible>False</Visible>
22+
</CodeAnalysisDependentAssemblyPaths>
23+
</ItemGroup>
24+
<PropertyGroup>
25+
<DebugType>PdbOnly</DebugType>
26+
<Optimize>True</Optimize>
27+
</PropertyGroup>
28+
<ItemGroup>
29+
<Compile Include="generic.cs" />
30+
</ItemGroup>
31+
<ItemGroup>
32+
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
33+
</ItemGroup>
34+
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
35+
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
36+
</PropertyGroup>
37+
</Project>

‎tests/src/JIT/opt/Devirtualization/overload.cs‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System;
6+
7+
interface Io<T,U> where T:class where U:class
8+
{
9+
T FromU(U u);
10+
T FromS(string s);
11+
}
12+
13+
public class Z : Io<string, string>
14+
{
15+
string Io<string, string>.FromU(string s) { return "U"; }
16+
string Io<string, string>.FromS(string s) { return "S"; }
17+
18+
public static int Main(string[] args)
19+
{
20+
string fromU = ((Io<string, string>) new Z()).FromU("u");
21+
string fromS = ((Io<string, string>) new Z()).FromS("s");
22+
23+
return fromU[0] + fromS[0] - 68;
24+
}
25+
}
26+
27+

0 commit comments

Comments
 (0)