JIT: fix overly aggressive type propagation from returns (#21316)

For quite a while now the jit has been propagating return types from
callees to the return spill temp. However this is only safe when the
callee has a single return site (or all return sites return the same
type).

Because return spill temps often end up getting assigned to still more
temps we haven't seen this overly aggressive type propgagation lead to
bugs, but now that we're tracking single def temps and doing more type
propagation during the late devirtualization callback, the fact that
these types are wrong has been exposed and can lead to incorrect
devirtualization.

The fix is to only consider the return spill temp as single def if the
callee has a single return site, and to check that the return spill temp
is single def before trying to propagate the type.

Fixes #21295.

diff --git a/src/jit/compiler.cpp b/src/jit/compiler.cpp
index fe42bb0..569f491 100644 (file)
--- a/src/jit/compiler.cpp
+++ b/src/jit/compiler.cpp
@@ -4560,14 +4560,21 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags
 
             fgRemoveEmptyBlocks();
 
-            // Update type of return spill temp if we have gathered better info
-            // when importing the inlinee.
+            // Update type of return spill temp if we have gathered
+            // better info when importing the inlinee, and the return
+            // spill temp is single def.
             if (fgNeedReturnSpillTemp())
             {
                 CORINFO_CLASS_HANDLE retExprClassHnd = impInlineInfo->retExprClassHnd;
                 if (retExprClassHnd != nullptr)
                 {
-                    lvaUpdateClass(lvaInlineeReturnSpillTemp, retExprClassHnd, impInlineInfo->retExprClassHndIsExact);
+                    LclVarDsc* returnSpillVarDsc = lvaGetDesc(lvaInlineeReturnSpillTemp);
+
+                    if (returnSpillVarDsc->lvSingleDef)
+                    {
+                        lvaUpdateClass(lvaInlineeReturnSpillTemp, retExprClassHnd,
+                                       impInlineInfo->retExprClassHndIsExact);
+                    }
                 }
             }
         }

diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp
index 41a88d9..dd18453 100644 (file)
--- a/src/jit/flowgraph.cpp
+++ b/src/jit/flowgraph.cpp
@@ -5904,8 +5904,12 @@ void Compiler::fgFindBasicBlocks()
                 // out we can prove the method returns a more specific type.
                 if (info.compRetType == TYP_REF)
                 {
-                    lvaTable[lvaInlineeReturnSpillTemp].lvSingleDef = 1;
-                    JITDUMP("Marked V%02u as a single def temp\n", lvaInlineeReturnSpillTemp);
+                    // The return spill temp is single def only if the method has a single return block.
+                    if (retBlocks == 1)
+                    {
+                        lvaTable[lvaInlineeReturnSpillTemp].lvSingleDef = 1;
+                        JITDUMP("Marked return spill temp V%02u as a single def temp\n", lvaInlineeReturnSpillTemp);
+                    }
 
                     CORINFO_CLASS_HANDLE retClassHnd = impInlineInfo->inlineCandidateInfo->methInfo.args.retTypeClass;
                     if (retClassHnd != nullptr)

diff --git a/src/jit/lclvars.cpp b/src/jit/lclvars.cpp
index 6bd1501..8ce2b59 100644 (file)
--- a/src/jit/lclvars.cpp
+++ b/src/jit/lclvars.cpp
@@ -2732,10 +2732,15 @@ void Compiler::lvaUpdateClass(unsigned varNum, CORINFO_CLASS_HANDLE clsHnd, bool
         shouldUpdate = true;
     }
 
-    JITDUMP("\nlvaUpdateClass:%s Updating class for V%02u", shouldUpdate ? "" : " NOT", varNum);
-    JITDUMP(" from(%p) %s%s", dspPtr(varDsc->lvClassHnd), info.compCompHnd->getClassName(varDsc->lvClassHnd),
-            varDsc->lvClassIsExact ? " [exact]" : "");
-    JITDUMP(" to(%p) %s%s\n", dspPtr(clsHnd), info.compCompHnd->getClassName(clsHnd), isExact ? " [exact]" : "");
+#if DEBUG
+    if (isNewClass || (isExact != varDsc->lvClassIsExact))
+    {
+        JITDUMP("\nlvaUpdateClass:%s Updating class for V%02u", shouldUpdate ? "" : " NOT", varNum);
+        JITDUMP(" from(%p) %s%s", dspPtr(varDsc->lvClassHnd), info.compCompHnd->getClassName(varDsc->lvClassHnd),
+                varDsc->lvClassIsExact ? " [exact]" : "");
+        JITDUMP(" to(%p) %s%s\n", dspPtr(clsHnd), info.compCompHnd->getClassName(clsHnd), isExact ? " [exact]" : "");
+    }
+#endif // DEBUG
 
     if (shouldUpdate)
     {

diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_21295/GitHub_21295.cs b/tests/src/JIT/Regression/JitBlue/GitHub_21295/GitHub_21295.cs
new file mode 100644 (file)
index 0000000..7bd3d75
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_21295/GitHub_21295.cs
@@ -0,0 +1,39 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Runtime.CompilerServices;
+
+class B
+{
+    public virtual int F() => 33;
+}
+
+sealed class D : B
+{
+    public override int F() => 44;
+}
+
+class X
+{
+    volatile static bool p;
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static B GB() => new B();
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static D GD() => new D();
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    static B G() => p ? GD() : GB();
+
+    public static int Main()
+    {
+        p = false;
+        // After inlining G(), the jit must not update
+        // the type of the return spill temp for G(), or it 
+        // may incorrectly devirtualize the call to F()
+        return G().F() + 67;
+    }
+}

diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_21295/GitHub_21295.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_21295/GitHub_21295.csproj
new file mode 100644 (file)
index 0000000..d86ed9f
--- /dev/null
+++ b/tests/src/JIT/Regression/JitBlue/GitHub_21295/GitHub_21295.csproj
@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Release</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <OutputType>Exe</OutputType>
+    <DebugType></DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>
\ No newline at end of file