Fix GC reporting for slow tail calls with by-ref-like args (#16264)

Fix GC reporting for slow tail calls with by-ref-like args

Fixes https://github.com/dotnet/coreclr/issues/16229:
- Offsets recorded in the GC layout are intended to be offsets from the top of the stack, fixed by calculating the proper offset

diff --git a/src/vm/i386/stublinkerx86.cpp b/src/vm/i386/stublinkerx86.cpp
index 7674464..28a012e 100644 (file)
--- a/src/vm/i386/stublinkerx86.cpp
+++ b/src/vm/i386/stublinkerx86.cpp
@@ -5930,11 +5930,35 @@ static void AppendGCLayout(ULONGARRAY &gcLayout, size_t baseOffset, BOOL fIsType
         {
             FindByRefPointerOffsetsInByRefLikeObject(
                 pMT,
-                baseOffset,
+                0 /* baseOffset */,
                 [&](size_t pointerOffset)
                 {
-                    _ASSERTE(gcLayout.Count() == 0 || pointerOffset > (gcLayout[gcLayout.Count() - 1] & ~(ULONG)1));
-                    *gcLayout.AppendThrowing() = (ULONG)(pointerOffset | 1); // "| 1" to mark it as an interior pointer
+                    // 'gcLayout' requires stack offsets relative to the top of the stack to be recorded, such that subtracting
+                    // the offset from the stack top yields the address of the field, given that subtracting 'baseOffset' from
+                    // the stack top yields the address of the first field in this struct. See TailCallFrame::GcScanRoots() for
+                    // how these offsets are used to calculate stack addresses for fields.
+                    _ASSERTE(pointerOffset < baseOffset);
+                    size_t stackOffsetFromTop = baseOffset - pointerOffset;
+                    _ASSERTE(FitsInU4(stackOffsetFromTop));
+
+                    // Offsets in 'gcLayout' are expected to be in increasing order
+                    int gcLayoutInsertIndex = gcLayout.Count();
+                    _ASSERTE(gcLayoutInsertIndex >= 0);
+                    for (; gcLayoutInsertIndex != 0; --gcLayoutInsertIndex)
+                    {
+                        ULONG prevStackOffsetFromTop = gcLayout[gcLayoutInsertIndex - 1] & ~(ULONG)1;
+                        if (stackOffsetFromTop > prevStackOffsetFromTop)
+                        {
+                            break;
+                        }
+                        if (stackOffsetFromTop == prevStackOffsetFromTop)
+                        {
+                            return;
+                        }
+                    }
+
+                    _ASSERTE(gcLayout.Count() == 0 || stackOffsetFromTop > (gcLayout[gcLayout.Count() - 1] & ~(ULONG)1));
+                    *gcLayout.InsertThrowing(gcLayoutInsertIndex) = (ULONG)(stackOffsetFromTop | 1); // "| 1" to mark it as an interior pointer
                 });
         }
 

diff --git a/tests/src/CoreMangLib/system/span/SlowTailCallArgs.cs b/tests/src/CoreMangLib/system/span/SlowTailCallArgs.cs
index 1d85cf8..da87827 100644 (file)
--- a/tests/src/CoreMangLib/system/span/SlowTailCallArgs.cs
+++ b/tests/src/CoreMangLib/system/span/SlowTailCallArgs.cs
@@ -5,11 +5,33 @@
 using System;
 using System.Reflection;
 using System.Reflection.Emit;
+using System.Runtime.InteropServices;
 
 internal static class Program
 {
     private static int Main()
     {
+        bool allPassed = true;
+        bool passed;
+
+        Console.Write("    SpanTest: ");
+        passed = SpanTest.Run();
+        Console.WriteLine(passed ? "pass" : "fail");
+        allPassed &= passed;
+
+        Console.Write("    ByRefLikeTest: ");
+        passed = ByRefLikeTest.Run();
+        Console.WriteLine(passed ? "pass" : "fail");
+        allPassed &= passed;
+
+        return allPassed ? 100 : 1;
+    }
+}
+
+internal static class SpanTest
+{
+    public static bool Run()
+    {
         DynamicMethod dm = new DynamicMethod("TailCaller", typeof(void), new Type[] { typeof(Span<int>) });
         ILGenerator il = dm.GetILGenerator();
         il.Emit(OpCodes.Ldarg_0);
@@ -18,7 +40,7 @@ internal static class Program
         il.Emit(OpCodes.Ldarg_0);
         il.Emit(OpCodes.Ldarg_0);
         il.Emit(OpCodes.Tailcall);
-        il.Emit(OpCodes.Call, typeof(Program).GetMethod("TailCallee", BindingFlags.Static | BindingFlags.NonPublic));
+        il.Emit(OpCodes.Call, typeof(SpanTest).GetMethod("TailCallee", BindingFlags.Static | BindingFlags.NonPublic));
         il.Emit(OpCodes.Ret);
 
         var tailCaller = (ActionOfSpanOfInt)dm.CreateDelegate(typeof(ActionOfSpanOfInt));
@@ -34,10 +56,9 @@ internal static class Program
         }
         catch (ArgumentException)
         {
-            return 1; // fail
+            return false; // fail
         }
-
-        return 100; // pass
+        return true;
     }
 
     private static void TailCallee(Span<int> a, Span<int> b, Span<int> c, Span<int> d, Span<int> e)
@@ -51,3 +72,81 @@ internal static class Program
 
     private delegate void ActionOfSpanOfInt(Span<int> x);
 }
+
+internal static class ByRefLikeTest
+{
+    public static bool Run()
+    {
+        DynamicMethod dm = new DynamicMethod("TailCaller", typeof(void), new Type[] { typeof(TestByRefLike) });
+        ILGenerator il = dm.GetILGenerator();
+        il.Emit(OpCodes.Ldarg_0);
+        il.Emit(OpCodes.Ldarg_0);
+        il.Emit(OpCodes.Ldarg_0);
+        il.Emit(OpCodes.Ldarg_0);
+        il.Emit(OpCodes.Ldarg_0);
+        il.Emit(OpCodes.Tailcall);
+        il.Emit(OpCodes.Call, typeof(ByRefLikeTest).GetMethod("TailCallee", BindingFlags.Static | BindingFlags.NonPublic));
+        il.Emit(OpCodes.Ret);
+
+        var tailCaller = (ActionOfTestByRefLike)dm.CreateDelegate(typeof(ActionOfTestByRefLike));
+
+        try
+        {
+            for (int i = 0; i < 1000; ++i)
+            {
+                GC.KeepAlive(new object());
+                tailCaller(new TestByRefLike(new int[] { 42 }));
+                GC.KeepAlive(new object());
+            }
+        }
+        catch (ArgumentException)
+        {
+            return false; // fail
+        }
+        return true;
+    }
+
+    private static void TailCallee(TestByRefLike a, TestByRefLike b, TestByRefLike c, TestByRefLike d, TestByRefLike e)
+    {
+        GC.Collect();
+        for (int i = 0; i < 10000; i++)
+            GC.KeepAlive(new object());
+        if (a.span[0] != 42 || b.span[0] != 42 || c.span[0] != 42 || d.span[0] != 42 || e.span[0] != 42 ||
+            a.span2[0] != 42 || b.span2[0] != 42 || c.span2[0] != 42 || d.span2[0] != 42 || e.span2[0] != 42)
+        {
+            throw new ArgumentException();
+        }
+    }
+
+    private delegate void ActionOfTestByRefLike(TestByRefLike x);
+
+    [StructLayout(LayoutKind.Explicit)]
+    private ref struct TestByRefLike
+    {
+        [FieldOffset(8 * 0)]
+        private object obj;
+        [FieldOffset(8 * 0)]
+        private object obj2;
+        [FieldOffset(8 * 1)]
+        public Span<int> span;
+        [FieldOffset(8 * 1)]
+        public Span<int> span2;
+        [FieldOffset(8 * 3)]
+        private object obj3;
+
+        public TestByRefLike(int[] values)
+        {
+            obj = null;
+            if (obj != null)
+                obj = null;
+            obj2 = null;
+            if (obj2 != null)
+                obj2 = null;
+            obj3 = null;
+            if (obj3 != null)
+                obj3 = null;
+            span = new Span<int>(values);
+            span2 = span;
+        }
+    }
+}

diff --git a/tests/src/CoreMangLib/system/span/SlowTailCallArgs.csproj b/tests/src/CoreMangLib/system/span/SlowTailCallArgs.csproj
index 6afa0ee..8acdab7 100644 (file)
--- a/tests/src/CoreMangLib/system/span/SlowTailCallArgs.csproj
+++ b/tests/src/CoreMangLib/system/span/SlowTailCallArgs.csproj
@@ -9,6 +9,7 @@
     <OutputType>Exe</OutputType>
     <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
     <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+    <LangVersion>latest</LangVersion>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
     <CLRTestKind>BuildAndRun</CLRTestKind>
     <CLRTestPriority>1</CLRTestPriority>