Fix for 17398. (#17501)

When enumerating live gc registers, if we are not on the active stack frame,
we need to report callee-save gc registers that are live before the call.
The reason is that the liveness of gc registers may change across a call
to a method that does not return. In this case the instruction after the call
may be a jump target and a register that didn't have a live gc pointer before
the call may have a live gc pointer after the jump. To make sure we report the
registers that have live gc pointers before the call we subtract 1 from curOffs.

3 files changed, 156 insertions, 11 deletions

diff --git a/src/vm/eetwain.cpp b/src/vm/eetwain.cpp
index 1fb332ecd4..511a635509 100644
--- a/src/vm/eetwain.cpp
+++ b/src/vm/eetwain.cpp
@@ -2404,14 +2404,16 @@ FINISHED:
    Returns size of things pushed on the stack for ESP frames
 
    Arguments:
-      table   - The pointer table
-      curOffs - The current code offset
-      info    - Incoming arg used to determine if there's a frame, and to save results
+      table       - The pointer table
+      curOffsRegs - The current code offset that should be used for reporting registers
+      curOffsArgs - The current code offset that should be used for reporting args
+      info        - Incoming arg used to determine if there's a frame, and to save results
  */
 
 static
-unsigned scanArgRegTableI(PTR_CBYTE     table,
-                          unsigned     curOffs,
+unsigned scanArgRegTableI(PTR_CBYTE    table,
+                          unsigned     curOffsRegs,
+                          unsigned     curOffsArgs,
                           hdrInfo   *  info)
 {
     CONTRACTL {
@@ -2433,6 +2435,10 @@ unsigned scanArgRegTableI(PTR_CBYTE     table,
     bool      isThis     = false;
     bool      iptr       = false;
 
+    // The comment before the call to scanArgRegTableI in EnumGCRefs
+    // describes why curOffsRegs can be smaller than curOffsArgs.
+    _ASSERTE(curOffsRegs <= curOffsArgs);
+
 #if VERIFY_GC_TABLES
     _ASSERTE(*castto(table, unsigned short *)++ == 0xBABE);
 #endif
@@ -2506,7 +2512,7 @@ unsigned scanArgRegTableI(PTR_CBYTE     table,
 
     /* Have we reached the instruction we're looking for? */
 
-    while (ptrOffs <= curOffs)
+    while (ptrOffs <= curOffsArgs)
     {
         unsigned    val;
 
@@ -2543,10 +2549,14 @@ unsigned scanArgRegTableI(PTR_CBYTE     table,
             regNum       reg;
 
             ptrOffs += (val     ) & 0x7;
-            if (ptrOffs > curOffs) {
+            if (ptrOffs > curOffsArgs) {
                 iptr = isThis = false;
                 goto REPORT_REFS;
             }
+            else if (ptrOffs > curOffsRegs) {
+                iptr = isThis = false;
+                continue;
+            }
 
             reg     = (regNum)((val >> 3) & 0x7);
             regMask = 1 << reg;         // EAX,ECX,EDX,EBX,---,EBP,ESI,EDI
@@ -2606,7 +2616,7 @@ unsigned scanArgRegTableI(PTR_CBYTE     table,
             /* A small argument encoding */
 
             ptrOffs += (val & 0x07);
-            if (ptrOffs > curOffs) {
+            if (ptrOffs > curOffsArgs) {
                 iptr = isThis = false;
                 goto REPORT_REFS;
             }
@@ -2736,7 +2746,7 @@ unsigned scanArgRegTableI(PTR_CBYTE     table,
                 /* non-ptr arg push */
                 _ASSERTE(!hasPartialArgInfo);
                 ptrOffs += (val & 0x07);
-                if (ptrOffs > curOffs) {
+                if (ptrOffs > curOffsArgs) {
                     iptr = isThis = false;
                     goto REPORT_REFS;
                 }
@@ -2859,6 +2869,7 @@ unsigned GetPushedArgSize(hdrInfo * info, PTR_CBYTE table, DWORD curOffs)
     {
         sz = scanArgRegTableI(skipToArgReg(*info, table),
                               curOffs,
+                              curOffs,
                               info);
     }
     else
@@ -4436,7 +4447,15 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY     pContext,
 
     if  (info.interruptible)
     {
-        pushedSize = scanArgRegTableI(skipToArgReg(info, table), curOffs, &info);
+        // If we are not on the active stack frame, we need to report gc registers
+        // that are live before the call. The reason is that the liveness of gc registers
+        // may change across a call to a method that does not return. In this case the instruction
+        // after the call may be a jump target and a register that didn't have a live gc pointer
+        // before the call may have a live gc pointer after the jump. To make sure we report the
+        // registers that have live gc pointers before the call we subtract 1 from curOffs.
+        unsigned curOffsRegs = (flags & ActiveStackFrame) != 0 ? curOffs : curOffs - 1;
+
+        pushedSize = scanArgRegTableI(skipToArgReg(info, table), curOffsRegs, curOffs, &info);
 
         RegMask   regs  = info.regMaskResult;
         RegMask  iregs  = info.iregMaskResult;
@@ -5342,7 +5361,7 @@ OBJECTREF EECodeManager::GetInstance( PREGDISPLAY    pContext,
 
     if  (info.interruptible)
     {
-        stackDepth = scanArgRegTableI(skipToArgReg(info, table), (unsigned)relOffset, &info);
+        stackDepth = scanArgRegTableI(skipToArgReg(info, table), relOffset, relOffset, &info);
     }
     else
     {
@@ -6047,6 +6066,7 @@ TADDR EECodeManager::GetAmbientSP(PREGDISPLAY     pContext,
     {
         baseSP += scanArgRegTableI(skipToArgReg(stateBuf->hdrInfoBody, table),
                                    dwRelOffset,
+                                   dwRelOffset,
                                    &stateBuf->hdrInfoBody);
     }
     else
diff --git a/tests/src/Regressions/coreclr/GitHub_17398/test17398.cs b/tests/src/Regressions/coreclr/GitHub_17398/test17398.cs
new file mode 100644
index 0000000000..a9cc4b9980
--- /dev/null
+++ b/tests/src/Regressions/coreclr/GitHub_17398/test17398.cs
@@ -0,0 +1,84 @@
+// 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;
+
+// Repro case for CoreCLR 17398
+
+class X
+{
+    static int v;
+
+    string s;
+
+    public override string ToString() => s;
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    X(int x)
+    {
+        s = "String" + x;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static void F() { }
+
+    public static void T0(object o, int x)
+    {
+        GC.Collect(2);
+        throw new Exception(o.ToString());
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public static void Test()
+    {
+        object x1 = new X(1);
+        object x2 = new X(2);
+        
+        if (v == 1)
+        {
+            // Generate enough pressure here to 
+            // kill ESI so in linear flow it is dead
+            // at the call to T0
+            int w = v;
+            int x = v;
+            int y = v;
+            int z = v;
+
+            // Unbounded loop here forces fully interruptible GC
+            for (int i = 0; i < v; i++)
+            {
+                w++;
+            }
+
+            T0(x2, w + x + y + z);
+        }
+
+        // Encourage x1 to be in callee save (ESI)
+        F();
+
+        if (v == 2)
+        {
+            T0(x1, 0);
+        }
+    }
+
+    public static int Main()
+    {
+        v = 1;
+        int r = 0;
+
+        try
+        {
+            Test();
+        }
+        catch (Exception e)
+        {
+            Console.WriteLine(e.Message);
+            r = 100;
+        }
+
+        return r;
+    }
+}
diff --git a/tests/src/Regressions/coreclr/GitHub_17398/test17398.csproj b/tests/src/Regressions/coreclr/GitHub_17398/test17398.csproj
new file mode 100644
index 0000000000..53d0e2ed21
--- /dev/null
+++ b/tests/src/Regressions/coreclr/GitHub_17398/test17398.csproj
@@ -0,0 +1,41 @@
+<?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)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{E55A6F8B-B9E3-45CE-88F4-22AE70F606CB}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <CLRTestKind>BuildAndRun</CLRTestKind>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
+  </PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
+  </PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <PropertyGroup>
+    <DebugType></DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="test17398.cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <ItemGroup>
+    <ProjectReference Include="../../../Common/CoreCLRTestLibrary/CoreCLRTestLibrary.csproj" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
+  </PropertyGroup>
+</Project>
\ No newline at end of file