[clr-interp] Tweak GC reporting (#122620)

The interpreter calling convention can result in double reporting of
arguments passed to callee's. This works today as all of the arguments
will be conservatively reported, but it also adds the problem of excess
conservative reporting. This change fixes that most of that problem by
only dropping reporting of conservative pointers when they point into a
callee's stack space.

Note that this does not fix is the scenario where an object reference is
held on the IL stack across a call. In those cases the value shall still
be reported conservatively. This PR also tweaks the Collect0 test to
disable it under the interpreter as the C# compiler generates that
particular problematic pattern for this test case.

Author: davidwrighton

src/coreclr/vm/gcinfodecoder.cpp

@@ -6,6 +6,9 @@
 #endif
 
 #include "gcinfodecoder.h"
+#ifdef FEATURE_INTERPRETER
+#include "interpexec.h"
+#endif // FEATURE_INTERPRETER
 
 #ifdef USE_GC_INFO_DECODER
 
@@ -2113,6 +2116,7 @@ template <typename GcInfoEncoding> OBJECTREF* TGcInfoDecoder<GcInfoEncoding>::Ge
 #endif // Unknown platform
 
 #ifdef FEATURE_INTERPRETER
+
 template <> OBJECTREF* TGcInfoDecoder<InterpreterGcInfoEncoding>::GetStackSlot(
                         INT32           spOffset,
                         GcStackSlotBase spBase,
@@ -2138,7 +2142,26 @@ template <> OBJECTREF* TGcInfoDecoder<InterpreterGcInfoEncoding>::GetStackSlot(
         _ASSERTE(fp);
         pObjRef = (OBJECTREF*)(fp + spOffset);
     }
+    InterpMethodContextFrame* pFrame = (InterpMethodContextFrame*)GetSP(pRD->pCurrentContext);
+    _ASSERTE(pFrame->pStack == (int8_t *)GetFP(pRD->pCurrentContext));
+    InterpMethodContextFrame* pFrameCallee = pFrame->pNext;
 
+    // If the stack slot is in a callee's frame, then we do not actually need to report it. This should ONLY happen if the
+    // stack slot is in the argument area of the caller. As a double check, we validate in the caller of this function that
+    // the stack slot in question is a interior pinned slot (which when FEATURE_INTERPRETER is defined indicates a conservatively reported stack slot).
+    if (pFrameCallee != NULL)
+    {
+        if (pFrameCallee->ip != 0)
+        {
+            _ASSERTE(pFrameCallee->pStack > pFrame->pStack); // Since only the last funclet is GC reported, we shouldn't have any cases where the stack doesn't grow
+            if (pFrameCallee->pStack <= (int8_t*)pObjRef)
+            {
+                // The stack slot is in the callee's frame, not the caller's frame.
+                // Return as a sentinel to indicate nothing is reported here.
+                pObjRef = NULL;
+            }
+        }
+    }
     return pObjRef;
 }
 #endif
@@ -2220,7 +2243,16 @@ template <typename GcInfoEncoding> void TGcInfoDecoder<GcInfoEncoding>::ReportSt
 
     OBJECTREF* pObjRef = GetStackSlot(spOffset, spBase, pRD);
     _ASSERTE(IS_ALIGNED(pObjRef, sizeof(OBJECTREF*)));
-
+#ifdef FEATURE_INTERPRETER
+    // This value is returned when the interpreter stack slot is not actually meaningful.
+    // This should only happen for stack slots which are conservatively reported, and for better perf
+    // we completely skip reporting them.
+    if (pObjRef == (OBJECTREF*)NULL)
+    {
+        _ASSERTE((gcFlags & (GC_CALL_PINNED | GC_CALL_INTERIOR)) == (GC_CALL_PINNED | GC_CALL_INTERIOR));
+        return;
+    }
+#endif // FEATURE_INTERPRETER
 #ifdef _DEBUG
     LOG((LF_GCROOTS, LL_INFO1000, /* Part One */
              "Reporting %s" FMT_STK,

src/tests/GC/API/GC/Collect0.cs

@@ -7,6 +7,7 @@
 
 public class Test_Collect0 {
 	[Fact]
+	[ActiveIssue("https://github.com/dotnet/runtime/issues/118965", typeof(TestLibrary.Utilities), nameof(TestLibrary.Utilities.IsCoreClrInterpreter))]
 	public static int TestEntryPoint() {
 
 		int[] array = new int[25];

src/tests/GC/API/GC/Collect0.csproj

@@ -9,5 +9,6 @@
   </PropertyGroup>
   <ItemGroup>
     <Compile Include="Collect0.cs" />
+    <ProjectReference Include="$(TestLibraryProjectPath)" />
   </ItemGroup>
 </Project>