From ca702d8459e7fe5fe0442209bf19a5b7c72e485c Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Fri, 13 Dec 2019 16:38:47 -0800 Subject: [PATCH 1/2] [release/3.1] Port fix for JIT silent bad code Release/3.1 port of https://github.com/dotnet/runtime/pull/797. Fixes https://github.com/dotnet/runtime/issues/764 The jit might incorrectly order a read from a struct field with an operation that modifies the field, so that the read returns the wrong value. Silent bad code; program behaves incorrectly. Yes, introduced in the 3.0 cycle. Verified the user's test case now passes; no diffs seen in any existing framework or test code. **Low**: the jit is now spilling the eval stack entries to temps in cases where it did not before; this should be conservatively safe. cc: @brucefo ____ If we're appending an assignment whose LHS is is a location within a local struct, we need to spill all references to that struct from the eval stack. Update the existing logic for this to handle the case where the LHS is a field of a local struct, and the field is updated by unusual means (here, `initobj`). Fixes dotnet/runtime#764. --- src/jit/importer.cpp | 21 ++++++-- .../JitBlue/Runtime_764/Runtime_764.cs | 50 +++++++++++++++++++ .../JitBlue/Runtime_764/Runtime_764.csproj | 33 ++++++++++++ 3 files changed, 99 insertions(+), 5 deletions(-) create mode 100644 tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.cs create mode 100644 tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.csproj diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp index bbb0d14e9793..50ddc58d89c7 100644 --- a/src/jit/importer.cpp +++ b/src/jit/importer.cpp @@ -10837,11 +10837,22 @@ void Compiler::impImportBlockCode(BasicBlock* block) } else if (lhs->OperIsBlk()) { - // Check for ADDR(LCL_VAR), or ADD(ADDR(LCL_VAR),CNS_INT)) - // (the latter may appear explicitly in the IL). - // Local field stores will cause the stack to be spilled when - // they are encountered. - lclVar = lhs->AsBlk()->Addr()->IsLocalAddrExpr(); + // Check if LHS address is within some struct local, to catch + // cases where we're updating the struct by something other than a stfld + GenTree* addr = lhs->AsBlk()->Addr(); + + // Catches ADDR(LCL_VAR), or ADD(ADDR(LCL_VAR),CNS_INT)) + lclVar = addr->IsLocalAddrExpr(); + + // Catches ADDR(FIELD(... ADDR(LCL_VAR))) + if (lclVar == nullptr) + { + GenTree* lclTree = nullptr; + if (impIsAddressInLocal(addr, &lclTree)) + { + lclVar = lclTree->AsLclVarCommon(); + } + } } if (lclVar != nullptr) { diff --git a/tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.cs b/tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.cs new file mode 100644 index 000000000000..ee98ab31ce65 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.cs @@ -0,0 +1,50 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Runtime.CompilerServices; + +// Regression test case for importer bug. +// If Release is inlined into Main, the importer may unsafely re-order trees. + +public struct Ptr where T: class +{ + public Ptr(T value) + { + _value = value; + } + + public T Release() + { + T tmp = _value; + _value = null; + return tmp; + } +} + +class Runtime_764 +{ + private static int Main(string[] args) + { + Ptr ptr = new Ptr("Hello, world"); + + bool res = false; + while (res) + { + } + + string summary = ptr.Release(); + + if (summary == null) + { + Console.WriteLine("FAILED"); + return -1; + } + else + { + Console.WriteLine("PASSED"); + return 100; + } + } +} diff --git a/tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.csproj b/tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.csproj new file mode 100644 index 000000000000..95052d9884f1 --- /dev/null +++ b/tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.csproj @@ -0,0 +1,33 @@ + + + + + Debug + AnyCPU + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + + + + + + + False + + + + None + True + + + + + + + + + + From 54f2a819e863c594c30b5d26dadff3e8a627c6c9 Mon Sep 17 00:00:00 2001 From: Andy Ayers Date: Sat, 14 Dec 2019 00:27:14 -0800 Subject: [PATCH 2/2] Fix test --- tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.cs b/tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.cs index ee98ab31ce65..52e9d11c9a7b 100644 --- a/tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.cs +++ b/tests/src/JIT/Regression/JitBlue/Runtime_764/Runtime_764.cs @@ -21,6 +21,8 @@ public T Release() _value = null; return tmp; } + + T _value; } class Runtime_764