Fix GC hole with STOREIND of LCL_VAR_ADDR/LCL_FLD_ADDR (#45818)

* Fix GC hole with STOREIND of LCL_VAR_ADDR/LCL_FLD_ADDR

We were updating the GC liveness of the ADDR node, but
`genUpdateLife()` expects to get the parent node, so no
liveness was ever updated.

There were 4 SPMI GC info diffs in the libraries, all
related to uses of `System.Collections.Immutable.ImmutableArray`
where we struct promote fields who are themselves single-element
gc ref structs that are kept on the stack and not in registers.
In all cases, the liveness of the stack local was not reflected
in codegen's GC sets, but it was reflected in the emitter's GC
sets, so it was marked as a GC lifetime. However, that lifetime
would get cut short if we hit a call site before the last use,
as calls (sometimes) carry the full set of live variables across
the call. So, variables not in this set (including the
"accidental" emitter-created GC lifetimes here) would get killed,
leaving a hole between the intermediate call and actual stack
local last use.

Fixes #45557

* Add unit test

* Add comment to `emitInsLoadInd()`

Author: BruceForstall

src/coreclr/jit/emitxarch.cpp

@@ -3054,7 +3054,12 @@ void emitter::emitInsLoadInd(instruction ins, emitAttr attr, regNumber dstReg, G
         unsigned             offset  = varNode->GetLclOffs();
         emitIns_R_S(ins, attr, dstReg, varNode->GetLclNum(), offset);
 
-        // Updating variable liveness after instruction was emitted
+        // Updating variable liveness after instruction was emitted.
+        // TODO-Review: it appears that this call to genUpdateLife does nothing because it
+        // returns quickly when passed GT_LCL_VAR_ADDR or GT_LCL_FLD_ADDR. Below, emitInsStoreInd
+        // had similar code that replaced `varNode` with `mem` (to fix a GC hole). It might be
+        // appropriate to do that here as well, but doing so showed no asm diffs, so it's not
+        // clear when this scenario gets hit, at least for GC refs.
         codeGen->genUpdateLife(varNode);
         return;
     }
@@ -3116,7 +3121,7 @@ void emitter::emitInsStoreInd(instruction ins, emitAttr attr, GenTreeStoreInd* m
         }
 
         // Updating variable liveness after instruction was emitted
-        codeGen->genUpdateLife(varNode);
+        codeGen->genUpdateLife(mem);
         return;
     }
 

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

@@ -0,0 +1,90 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+// Regression test for https://github.com/dotnet/runtime/issues/45557,
+// derived from Roslyn failure case.
+//
+// Bug was a GC hole with STOREIND of LCL_VAR_ADDR/LCL_FLD_ADDR.
+// We were updating the GC liveness of the ADDR node, but
+// genUpdateLife() expects to get the parent node, so no
+// liveness was ever updated.
+//
+// The bad code cases in the libraries were related to uses of
+// System.Collections.Immutable.ImmutableArray
+// where we struct promote fields who are themselves single-element
+// gc ref structs that are kept on the stack and not in registers.
+// In all cases, the liveness of the stack local was not reflected
+// in codegen's GC sets, but it was reflected in the emitter's GC
+// sets, so it was marked as a GC lifetime. However, that lifetime
+// would get cut short if we hit a call site before the last use,
+// as calls (sometimes) carry the full set of live variables across
+// the call. So, variables not in this set (including the
+// "accidental" emitter-created GC lifetimes here) would get killed,
+// leaving a hole between the intermediate call and actual stack
+// local last use.
+
+using System;
+using System.Collections.Generic;
+using System.Collections.Immutable;
+using System.Runtime.CompilerServices;
+
+namespace Runtime_45557
+{
+    internal readonly struct ObjectBinderSnapshot
+    {
+        private readonly Dictionary<Type, int> _typeToIndex;
+        private readonly ImmutableArray<Type> _types;
+        private readonly ImmutableArray<Func<Object, Object>> _typeReaders;
+ 
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]          // this needs to get inlined to cause the failure
+        public ObjectBinderSnapshot(
+            Dictionary<Type, int> typeToIndex,
+            List<Type> types,
+            List<Func<Object, Object>> typeReaders)
+        {
+            _typeToIndex = new Dictionary<Type, int>(typeToIndex);
+            _types = types.ToImmutableArray();                      // stack variable here would go live
+            _typeReaders = typeReaders.ToImmutableArray();          // it would get erroneously killed in GC info here
+            GC.Collect();                                           // try to cause a crash by collecting the variable
+            Console.WriteLine($"{_types.Length}");                  // use the collected variable; should crash (most of the time, depending on GC behavior)
+        }
+
+        public string SomeValue => _types.ToString();
+    }
+
+    internal static class ObjectBinder
+    {
+        private static readonly object s_gate = new();
+ 
+        private static ObjectBinderSnapshot? s_lastSnapshot = null;
+ 
+        private static readonly Dictionary<Type, int> s_typeToIndex = new();
+        private static readonly List<Type> s_types = new();
+        private static readonly List<Func<Object, Object>> s_typeReaders = new();
+ 
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        public static ObjectBinderSnapshot GetSnapshot()
+        {
+            lock (s_gate)
+            {
+                if (s_lastSnapshot == null)
+                {
+                    s_lastSnapshot = new ObjectBinderSnapshot(s_typeToIndex, s_types, s_typeReaders);
+                }
+ 
+                return s_lastSnapshot.Value;
+            }
+        }
+    }
+
+    class Program
+    {
+        static int Main(string[] args)
+        {
+            ObjectBinderSnapshot o = ObjectBinder.GetSnapshot();
+            Console.WriteLine($"Test output: {o.SomeValue}");
+
+            return 100; // success if we got here without crashing
+        }
+    }
+}

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

@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <CLRTestPriority>0</CLRTestPriority>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="Runtime_45557.cs" />
+  </ItemGroup>
+</Project>