From d87c29af036a543f5853781ec884c4cf7fe46171 Mon Sep 17 00:00:00 2001 From: Eugene Rozenfeld Date: Thu, 14 Feb 2019 14:43:26 -0800 Subject: [PATCH] Fix check for memory containment safety. (dotnet/coreclr#22563) This change ensures that if an operand can produce an exception and any instructions executed after the operand evaluation but before the operand's parent can also produce an exception, the operand shouldn't be contained. The reason is that in this case operand containment may reorder exceptions. With `strict` set to true the containment is blocked here: https://github.com/dotnet/coreclr/blob/dotnet/coreclr@d27fff3f65193dd71c6197e9876101f496bbd28b/src/jit/sideeffects.cpp#L485-L488 Also, make the check for ordering side-effect interference less conservative. Fixes dotnet/coreclr#22556. Commit migrated from https://github.com/dotnet/coreclr/commit/6c9ad257e78977c9fd5d2429490b5d2f72c1b612 --- src/coreclr/src/jit/lower.cpp | 3 +- src/coreclr/src/jit/sideeffects.cpp | 12 +++-- .../JitBlue/GitHub_22556/GitHub_22556.cs | 46 +++++++++++++++++++ .../JitBlue/GitHub_22556/GitHub_22556.csproj | 34 ++++++++++++++ 4 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.cs create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.csproj diff --git a/src/coreclr/src/jit/lower.cpp b/src/coreclr/src/jit/lower.cpp index 2c4c071a3561d..fc507c4c50207 100644 --- a/src/coreclr/src/jit/lower.cpp +++ b/src/coreclr/src/jit/lower.cpp @@ -87,7 +87,8 @@ bool Lowering::IsSafeToContainMem(GenTree* parentNode, GenTree* childNode) for (GenTree* node = childNode->gtNext; node != parentNode; node = node->gtNext) { - if (m_scratchSideEffects.InterferesWith(comp, node, false)) + const bool strict = true; + if (m_scratchSideEffects.InterferesWith(comp, node, strict)) { return false; } diff --git a/src/coreclr/src/jit/sideeffects.cpp b/src/coreclr/src/jit/sideeffects.cpp index 75b7cc7b7f6a8..10fa5d5c49a57 100644 --- a/src/coreclr/src/jit/sideeffects.cpp +++ b/src/coreclr/src/jit/sideeffects.cpp @@ -450,7 +450,7 @@ void SideEffectSet::AddNode(Compiler* compiler, GenTree* node) // Two side effect sets interfere under any of the following // conditions: // - If the analysis is strict, and: -// - Either set contains a compiler barrier, or +// - One set contains a compiler barrier and the other set contains a global reference, or // - Both sets produce an exception // - Whether or not the analysis is strict: // - One set produces an exception and the other set contains a @@ -475,8 +475,14 @@ bool SideEffectSet::InterferesWith(unsigned otherSideEffectFlags, if (strict) { - // If either set contains a compiler barrier, the sets interfere. - if (((m_sideEffectFlags | otherSideEffectFlags) & GTF_ORDER_SIDEEFF) != 0) + // If either set contains a compiler barrier, and the other set contains a global reference, + // the sets interfere. + if (((m_sideEffectFlags & GTF_ORDER_SIDEEFF) != 0) && ((otherSideEffectFlags & GTF_GLOB_REF) != 0)) + { + return true; + } + + if (((otherSideEffectFlags & GTF_ORDER_SIDEEFF) != 0) && ((m_sideEffectFlags & GTF_GLOB_REF) != 0)) { return true; } diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.cs new file mode 100644 index 0000000000000..e66288b24474d --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.cs @@ -0,0 +1,46 @@ +// 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; + +public class Test +{ + int f; + + Test(int f) + { + this.f = f; + } + + public static int Main() + { + try + { + Add(null, 0); + } + catch (Exception e) + { + if (e is NullReferenceException) + { + Console.WriteLine("PASS"); + return 100; + } + } + Console.WriteLine("FAIL"); + return -1; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int Add(Test t, int i) + { + // When t is null and i is 0, this should throw + // NullReferenceException since the operands of + // addition have to be evaluated left to right. + int x = t.f + 1 / i; + return x; + } +} + diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.csproj new file mode 100644 index 0000000000000..c46f1c3549dde --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_22556/GitHub_22556.csproj @@ -0,0 +1,34 @@ + + + + + Debug + AnyCPU + 2.0 + {2649FAFE-07BF-4F93-8120-BA9A69285ABB} + Exe + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + ..\..\ + + + + + + latest + None + True + + + + False + + + + + + + + + + + \ No newline at end of file