From 0147e213f63ded2509eb6c7366e9b73eafbdc5e7 Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Tue, 8 Dec 2020 22:36:07 -0800 Subject: [PATCH 1/3] 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 --- src/coreclr/jit/emitxarch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 00ae0b97d0d561..5dd55ca57581ed 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -3116,7 +3116,7 @@ void emitter::emitInsStoreInd(instruction ins, emitAttr attr, GenTreeStoreInd* m } // Updating variable liveness after instruction was emitted - codeGen->genUpdateLife(varNode); + codeGen->genUpdateLife(mem); return; } From 985f43018df3ec06d0b4f17e0d0c121d6eebe5ef Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Thu, 10 Dec 2020 16:59:15 -0800 Subject: [PATCH 2/3] Add unit test --- .../JitBlue/Runtime_45557/Runtime_45557.cs | 90 +++++++++++++++++++ .../Runtime_45557/Runtime_45557.csproj | 13 +++ 2 files changed, 103 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_45557/Runtime_45557.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_45557/Runtime_45557.csproj 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 + + + + + From 8a62ffcaf6ac33158ba8105a7e47c9da7029f711 Mon Sep 17 00:00:00 2001 From: Bruce Forstall Date: Thu, 10 Dec 2020 17:06:18 -0800 Subject: [PATCH 3/3] Add comment to `emitInsLoadInd()` --- src/coreclr/jit/emitxarch.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 5dd55ca57581ed..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; }