[wasm] Report GC locals held across a safepoint as pinned

src/coreclr/jit/compiler.h

@@ -537,6 +537,12 @@ class LclVarDsc
 #endif
     unsigned char lvPinned : 1; // is this a pinned variable?
 
+#ifdef TARGET_WASM
+    unsigned char lvStackPinned : 1; // the GC must not move this local's referent, because a copy of it is held on
+                                     // the wasm operand stack across a safepoint. Unlike lvPinned this says nothing
+                                     // about the local itself, so it is safe to set after liveness.
+#endif
+
     unsigned char lvMustInit : 1; // must be initialized
 
 private:

src/coreclr/jit/fgwasm.cpp

@@ -1850,6 +1850,24 @@ PhaseStatus Compiler::fgWasmControlFlow()
     return PhaseStatus::MODIFIED_EVERYTHING;
 }
 
+// Determine whether `operand` is a direct operand of `user`.
+static bool fgWasmIsOperandOf(GenTree* user, GenTree* operand)
+{
+    bool isOperand = false;
+
+    user->VisitOperands([operand, &isOperand](GenTree* op) {
+        if (op != operand)
+        {
+            return GenTree::VisitResult::Continue;
+        }
+
+        isOperand = true;
+        return GenTree::VisitResult::Abort;
+    });
+
+    return isOperand;
+}
+
 PhaseStatus Compiler::fgWasmSpillRefs()
 {
     bool anyChanges = false;
@@ -1879,6 +1897,41 @@ PhaseStatus Compiler::fgWasmSpillRefs()
         {
             if (tree->IsCall())
             {
+                // A ref sourced from a non-address-exposed local already has a linear-stack home, so
+                //  rather than spilling it to a second slot we report that home as pinned. The GC then
+                //  can't move the referent, and the copy on the operand stack stays valid across the
+                //  call. This is only safe because the local can't be stored to between its def and its
+                //  use, so the home still holds the object the operand stack refers to.
+                //
+                // A value this call consumes directly needs neither: there is no safepoint between
+                //  loading it and the call, and the local's home keeps the referent reachable meanwhile.
+                for (size_t i = defs.size(); i > 0; i--)
+                {
+                    GenTree* const def = defs[i - 1];
+
+                    if (!def->OperIs(GT_LCL_VAR))
+                    {
+                        continue;
+                    }
+
+                    LclVarDsc* const dsc = lvaGetDesc(def->AsLclVarCommon());
+                    if (dsc->IsAddressExposed())
+                    {
+                        continue;
+                    }
+
+                    if (!fgWasmIsOperandOf(tree, def))
+                    {
+                        JITDUMP("Pinning V%02u, held across call\n", def->AsLclVarCommon()->GetLclNum());
+                        dsc->lvStackPinned = true;
+                    }
+
+                    // Swapping with the last element is safe because we walk backwards, so whatever
+                    //  lands here has already been examined.
+                    defs[i - 1] = defs[defs.size() - 1];
+                    defs.pop_back();
+                }
+
                 // For any ref/byref values live at the point of a call, spill them into pinned slots
                 //  on the stack where the GC can see them so it won't move them.
                 if (!defs.empty())
@@ -1986,21 +2039,7 @@ PhaseStatus Compiler::fgWasmSpillRefs()
                 continue;
             }
 
-            // If a value is just a GT_LCL_VAR that isn't address-exposed, by construction we ensure that
-            //  it won't be mutated between its def (here) and its use (the call that would produce a spill)
-            //  and we won't need to spill it.
-            if (tree->OperIs(GT_LCL_VAR))
-            {
-                GenTreeLclVarCommon* lclVar = tree->AsLclVarCommon();
-                LclVarDsc*           dsc    = lvaGetDesc(lclVar);
-                if (!dsc->IsAddressExposed())
-                {
-                    continue;
-                }
-            }
-
-            // We have a ref sourced from something like a call result or an indirection that hasn't been
-            //  spilled yet, so record it for potential spilling at the next call.
+            // We have a ref that may be live across a future call.
             defs.push_back(tree);
         }
 

src/coreclr/jit/gcencode.cpp

@@ -4187,7 +4187,13 @@ void GCInfo::gcMakeRegPtrTable(
                 flags = (GcSlotFlags)(flags | GC_SLOT_INTERIOR);
             }
 
+            // On wasm a local's home is also reported pinned when a copy of it is held on the operand
+            //  stack across a safepoint, since the GC cannot update that copy. See fgWasmSpillRefs.
+#ifdef TARGET_WASM
+            if (varDsc->lvPinned || varDsc->lvStackPinned)
+#else
             if (varDsc->lvPinned)
+#endif
             {
                 // Or in pinned_OFFSET_FLAG for 'pinned' pointer tracking
                 flags = (GcSlotFlags)(flags | GC_SLOT_PINNED);

src/tests/JIT/Regression/JitBlue/Runtime_131373/Runtime_131373.cs

@@ -0,0 +1,55 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+// A GC ref pushed onto the wasm operand stack is a copy the GC can't update, so it goes stale
+// when a call relocates the referent. Only reproduces under crossgen wasm R2R (TargetOS=browser).
+
+using System;
+using System.Runtime.CompilerServices;
+using Xunit;
+
+public class Runtime_131373
+{
+    public sealed class Box
+    {
+        public object Slot;
+        public int Tag;
+    }
+
+    [Fact]
+    public static void TestEntryPoint()
+    {
+        for (int i = 0; i < 400; i++)
+        {
+            for (int j = 0; j < 32; j++)
+            {
+                byte[] garbage = new byte[8192];
+                GC.KeepAlive(garbage);
+            }
+
+            Box box = new Box { Tag = i };
+            StoreThroughByref(ref box.Slot, i);
+            Assert.NotNull(box.Slot);
+        }
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static void StoreThroughByref(ref object slot, int i)
+    {
+        slot = Allocate(i);
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static object Allocate(int i)
+    {
+        for (int j = 0; j < 48; j++)
+        {
+            byte[] garbage = new byte[8192];
+            GC.KeepAlive(garbage);
+        }
+
+        GC.Collect(2, GCCollectionMode.Forced, blocking: true, compacting: true);
+
+        return new string('x', 1 + (i & 7));
+    }
+}

src/tests/JIT/Regression/JitBlue/Runtime_131373/Runtime_131373.csproj

@@ -0,0 +1,10 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <Optimize>True</Optimize>
+    <!-- Force crossgen on the browser leg so this actually runs as wasm R2R. -->
+    <AlwaysUseCrossGen2 Condition="'$(TargetOS)' == 'browser'">true</AlwaysUseCrossGen2>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>