diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 00ae0b97d0d561..ae73c8e2f954ac 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/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; } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_45557/Runtime_45557.cs b/src/tests/JIT/Regression/JitBlue/Runtime_45557/Runtime_45557.cs new file mode 100644 index 00000000000000..b2cbb336fdac7c --- /dev/null +++ b/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 _typeToIndex; + private readonly ImmutableArray _types; + private readonly ImmutableArray> _typeReaders; + + [MethodImpl(MethodImplOptions.AggressiveInlining)] // this needs to get inlined to cause the failure + public ObjectBinderSnapshot( + Dictionary typeToIndex, + List types, + List> typeReaders) + { + _typeToIndex = new Dictionary(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 s_typeToIndex = new(); + private static readonly List s_types = new(); + private static readonly List> 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 + } + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_45557/Runtime_45557.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_45557/Runtime_45557.csproj new file mode 100644 index 00000000000000..f24e83e9abc167 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_45557/Runtime_45557.csproj @@ -0,0 +1,13 @@ + + + Exe + 0 + + + None + True + + + + +