Skip to content

Commit 41ec86b

Browse files
AndyAyersMSAndyAyersMS
authored
Fix logic in compareTypesForCast (#31681)
We weren't careful enough with `__Canon` in some cases, which lead to unsafely returning `MustNot` when the cast outcome was not determined at jit time. Add an extra check, update comments, and add some test cases. Addresses the failures seen in #1195 (which was reverted).
1 parent 8b17725 commit 41ec86b

File tree

4 files changed

+178
-18
lines changed

4 files changed

+178
-18
lines changed

‎src/coreclr/src/tools/Common/JitInterface/CorInfoImpl.cs

+22-9
Original file line numberDiff line numberDiff line change
@@ -1758,16 +1758,29 @@ private TypeCompareState compareTypesForCast(CORINFO_CLASS_STRUCT_* fromClass, C
17581758
{
17591759
result = TypeCompareState.Must;
17601760
}
1761-
// For negative results, the unknown type parameter in
1762-
// fromClass might match some instantiated interface,
1763-
// either directly or via variance.
1761+
// We have __Canon parameter(s) in fromClass, somewhere.
17641762
//
1765-
// However, CanCastTo will report failure in such cases since
1766-
// __Canon won't match the instantiated type on the
1767-
// interface (which can't be __Canon since we screened out
1768-
// canonical subtypes for toClass above). So only report
1769-
// failure if the interface is not instantiated.
1770-
else if (!toType.HasInstantiation)
1763+
// In CanCastTo, these __Canon(s) won't match the interface or
1764+
// instantiated types on the interface, so CanCastTo may
1765+
// return false negatives.
1766+
//
1767+
// Only report MustNot if the fromClass is not __Canon
1768+
// and the interface is not instantiated; then there is
1769+
// no way for the fromClass __Canon(s) to confuse things.
1770+
//
1771+
// __Canon -> IBar May
1772+
// IFoo<__Canon> -> IFoo<string> May
1773+
// IFoo<__Canon> -> IBar MustNot
1774+
//
1775+
else if (fromType.IsCanonicalDefinitionType(CanonicalFormKind.Any))
1776+
{
1777+
result = TypeCompareState.May;
1778+
}
1779+
else if (toType.HasInstantiation)
1780+
{
1781+
result = TypeCompareState.May;
1782+
}
1783+
else
17711784
{
17721785
result = TypeCompareState.MustNot;
17731786
}

‎src/coreclr/src/vm/jitinterface.cpp

+22-9
Original file line numberDiff line numberDiff line change
@@ -4495,16 +4495,29 @@ TypeCompareState CEEInfo::compareTypesForCast(
44954495
{
44964496
result = TypeCompareState::Must;
44974497
}
4498-
// For negative results, the unknown type parameter in
4499-
// fromClass might match some instantiated interface,
4500-
// either directly or via variance.
4498+
// We have __Canon parameter(s) in fromClass, somewhere.
45014499
//
4502-
// However, CanCastTo will report failure in such cases since
4503-
// __Canon won't match the instantiated type on the
4504-
// interface (which can't be __Canon since we screened out
4505-
// canonical subtypes for toClass above). So only report
4506-
// failure if the interface is not instantiated.
4507-
else if (!toHnd.HasInstantiation())
4500+
// In CanCastTo, these __Canon(s) won't match the interface or
4501+
// instantiated types on the interface, so CanCastTo may
4502+
// return false negatives.
4503+
//
4504+
// Only report MustNot if the fromClass is not __Canon
4505+
// and the interface is not instantiated; then there is
4506+
// no way for the fromClass __Canon(s) to confuse things.
4507+
//
4508+
// __Canon -> IBar May
4509+
// IFoo<__Canon> -> IFoo<string> May
4510+
// IFoo<__Canon> -> IBar MustNot
4511+
//
4512+
else if (fromHnd == TypeHandle(g_pCanonMethodTableClass))
4513+
{
4514+
result = TypeCompareState::May;
4515+
}
4516+
else if (toHnd.HasInstantiation())
4517+
{
4518+
result = TypeCompareState::May;
4519+
}
4520+
else
45084521
{
45094522
result = TypeCompareState::MustNot;
45104523
}

‎src/coreclr/tests/src/JIT/opt/Casts/shared2.cs

+122
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
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.Runtime.CompilerServices;
7+
8+
interface IBar {}
9+
interface IFoo<T> {}
10+
class C : IBar {}
11+
class C<T> : IFoo<T> {}
12+
struct S {}
13+
struct SBar : IBar {}
14+
15+
// More tests for shared types passing through compareTypesForCast
16+
17+
class X
18+
{
19+
static int _errors;
20+
21+
static void IsTrue(bool expression, [CallerLineNumber] int line = 0, [CallerFilePath] string file = "")
22+
{
23+
if (!expression)
24+
{
25+
Console.WriteLine($"{file}:L{line} test failed (expected: true).");
26+
_errors++;
27+
}
28+
}
29+
30+
static void IsFalse(bool expression, [CallerLineNumber] int line = 0, [CallerFilePath] string file = "")
31+
{
32+
if (expression)
33+
{
34+
Console.WriteLine($"{file}:L{line} test failed (expected: false).");
35+
_errors++;
36+
}
37+
}
38+
39+
[MethodImpl(MethodImplOptions.NoInlining)]
40+
static bool A1<T>()
41+
{
42+
return typeof(IFoo<string>).IsAssignableFrom(typeof(T));
43+
}
44+
45+
[MethodImpl(MethodImplOptions.NoInlining)]
46+
static bool A2<T>()
47+
{
48+
return typeof(IBar).IsAssignableFrom(typeof(T));
49+
}
50+
51+
[MethodImpl(MethodImplOptions.NoInlining)]
52+
static bool I1<T>(T t)
53+
{
54+
return t is IFoo<string>;
55+
}
56+
57+
[MethodImpl(MethodImplOptions.NoInlining)]
58+
static bool I2<T>(T t)
59+
{
60+
return t is IBar;
61+
}
62+
63+
[MethodImpl(MethodImplOptions.NoInlining)]
64+
static bool C1<T>(T t)
65+
{
66+
return (t as IFoo<string>) != null;
67+
}
68+
69+
[MethodImpl(MethodImplOptions.NoInlining)]
70+
static bool C2<T>(T t)
71+
{
72+
return (t as IBar) != null;
73+
}
74+
75+
public static int Main()
76+
{
77+
var c = new C();
78+
var ci = new C<int>();
79+
var cs = new C<string>();
80+
var s = new S();
81+
var sb = new SBar();
82+
83+
IsTrue(A1<IFoo<string>>());
84+
IsFalse(A1<IFoo<int>>());
85+
IsFalse(A1<IBar>());
86+
IsFalse(A1<S>());
87+
IsFalse(A1<SBar>());
88+
89+
IsTrue(I1(cs));
90+
IsFalse(I1(ci));
91+
IsFalse(I1(c));
92+
IsFalse(I1(s));
93+
IsFalse(I1(sb));
94+
95+
IsTrue(C1(cs));
96+
IsFalse(C1(ci));
97+
IsFalse(C1(c));
98+
IsFalse(C1(s));
99+
IsFalse(C1(sb));
100+
101+
IsFalse(A2<IFoo<string>>());
102+
IsFalse(A2<IFoo<int>>());
103+
IsTrue(A2<IBar>());
104+
IsFalse(A2<S>());
105+
IsTrue(A2<SBar>());
106+
107+
IsFalse(I2(cs));
108+
IsFalse(I2(ci));
109+
IsTrue(I2(c));
110+
IsFalse(I2(s));
111+
IsTrue(I2(sb));
112+
113+
IsFalse(C2(cs));
114+
IsFalse(C2(ci));
115+
IsTrue(C2(c));
116+
IsFalse(C2(s));
117+
IsTrue(C2(sb));
118+
119+
if (_errors == 0) Console.WriteLine("Passed");
120+
return _errors > 0 ? -1 : 100;
121+
}
122+
}

‎src/coreclr/tests/src/JIT/opt/Casts/shared2.csproj

+12
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
<PropertyGroup>
3+
<OutputType>Exe</OutputType>
4+
</PropertyGroup>
5+
<PropertyGroup>
6+
<DebugType>PdbOnly</DebugType>
7+
<Optimize>True</Optimize>
8+
</PropertyGroup>
9+
<ItemGroup>
10+
<Compile Include="shared2.cs" />
11+
</ItemGroup>
12+
</Project>

0 commit comments

Comments
 (0)