From 375d1373b8947224d0c6c945eba12a0e15ead97a Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 12 Mar 2020 02:19:00 +0300 Subject: [PATCH] Fix JIT crash caused by GetType() == typeof(SealedClass) optimization (#33337) Downstream phases can't handle commas under JTRUE, so do more thorough cleanup in `fgFoldConditional` for JTRUE (and SWITCH). Fixes #33333. --- src/coreclr/src/jit/morph.cpp | 37 +++-- .../tests/src/JIT/Intrinsics/TypeEqualitySealed.cs | 167 +++++++++++++++++++++ .../src/JIT/Intrinsics/TypeEqualitySealed_r.csproj | 10 ++ .../JIT/Intrinsics/TypeEqualitySealed_ro.csproj | 10 ++ 4 files changed, 215 insertions(+), 9 deletions(-) create mode 100644 src/coreclr/tests/src/JIT/Intrinsics/TypeEqualitySealed.cs create mode 100644 src/coreclr/tests/src/JIT/Intrinsics/TypeEqualitySealed_r.csproj create mode 100644 src/coreclr/tests/src/JIT/Intrinsics/TypeEqualitySealed_ro.csproj diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 7303259..f3aec3a 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -14985,8 +14985,10 @@ bool Compiler::fgFoldConditional(BasicBlock* block) /* Did we fold the conditional */ noway_assert(lastStmt->GetRootNode()->AsOp()->gtOp1); + GenTree* condTree; + condTree = lastStmt->GetRootNode()->AsOp()->gtOp1; GenTree* cond; - cond = lastStmt->GetRootNode()->AsOp()->gtOp1; + cond = condTree->gtEffectiveVal(true); if (cond->OperKind() & GTK_CONST) { @@ -14996,10 +14998,17 @@ bool Compiler::fgFoldConditional(BasicBlock* block) noway_assert(cond->gtOper == GT_CNS_INT); noway_assert((block->bbNext->countOfInEdges() > 0) && (block->bbJumpDest->countOfInEdges() > 0)); - /* remove the statement from bbTreelist - No need to update - * the reference counts since there are no lcl vars */ - fgRemoveStmt(block, lastStmt); - + if (condTree != cond) + { + // Preserve any side effects + assert(condTree->OperIs(GT_COMMA)); + lastStmt->SetRootNode(condTree); + } + else + { + // no side effects, remove the jump entirely + fgRemoveStmt(block, lastStmt); + } // block is a BBJ_COND that we are folding the conditional for // bTaken is the path that will always be taken from block // bNotTaken is the path that will never be taken from block @@ -15198,8 +15207,10 @@ bool Compiler::fgFoldConditional(BasicBlock* block) /* Did we fold the conditional */ noway_assert(lastStmt->GetRootNode()->AsOp()->gtOp1); + GenTree* condTree; + condTree = lastStmt->GetRootNode()->AsOp()->gtOp1; GenTree* cond; - cond = lastStmt->GetRootNode()->AsOp()->gtOp1; + cond = condTree->gtEffectiveVal(true); if (cond->OperKind() & GTK_CONST) { @@ -15208,9 +15219,17 @@ bool Compiler::fgFoldConditional(BasicBlock* block) noway_assert(cond->gtOper == GT_CNS_INT); - /* remove the statement from bbTreelist - No need to update - * the reference counts since there are no lcl vars */ - fgRemoveStmt(block, lastStmt); + if (condTree != cond) + { + // Preserve any side effects + assert(condTree->OperIs(GT_COMMA)); + lastStmt->SetRootNode(condTree); + } + else + { + // no side effects, remove the switch entirely + fgRemoveStmt(block, lastStmt); + } /* modify the flow graph */ diff --git a/src/coreclr/tests/src/JIT/Intrinsics/TypeEqualitySealed.cs b/src/coreclr/tests/src/JIT/Intrinsics/TypeEqualitySealed.cs new file mode 100644 index 0000000..1686b1d --- /dev/null +++ b/src/coreclr/tests/src/JIT/Intrinsics/TypeEqualitySealed.cs @@ -0,0 +1,167 @@ +// 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; + +// Make sure optimization works correctly under GT_JTRUE and GT_RETURN nodes +// Also, if it respects/preserves side-effects and doesn't optimize away +// possible NREs. + +public sealed class SealedClass1 +{ + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private Type GetTypeInlineable() => GetType(); + + + [MethodImpl(MethodImplOptions.NoInlining)] + public object TestTernary1() => GetTypeInlineable() == typeof(SealedClass1) ? "Ok" : "Fail"; + + [MethodImpl(MethodImplOptions.NoInlining)] + public object TestTernary2() => GetTypeInlineable() == typeof(SealedClass2) ? "Fail" : "Ok"; + + [MethodImpl(MethodImplOptions.NoInlining)] + public object TestTernary3() => GetTypeInlineable() == typeof(NotSealedClass1) ? "Fail" : "Ok"; + + [MethodImpl(MethodImplOptions.NoInlining)] + public object TestTernary4() => GetType() == typeof(SealedClass1) ? "Ok" : "Fail"; + + [MethodImpl(MethodImplOptions.NoInlining)] + public object TestTernary5() => GetType() == typeof(SealedClass2) ? "Fail" : "Ok"; + + [MethodImpl(MethodImplOptions.NoInlining)] + public object TestTernary6() => GetType() == typeof(NotSealedClass1) ? "Fail" : "Ok"; + + + [MethodImpl(MethodImplOptions.NoInlining)] + public static object TestTernary7(SealedClass1 instance) => instance.GetType() == typeof(SealedClass1) ? "Ok" : "Fail"; + + [MethodImpl(MethodImplOptions.NoInlining)] + public static object TestTernary8(SealedClass1 instance) => instance.GetType() == typeof(SealedClass2) ? "Fail" : "Ok"; + + [MethodImpl(MethodImplOptions.NoInlining)] + public static object TestTernary9(SealedClass1 instance) => instance.GetType() == typeof(NotSealedClass1) ? "Fail" : "Ok"; + + [MethodImpl(MethodImplOptions.NoInlining)] + public static object TestTernary10(SealedClass1 instance) => instance.GetTypeInlineable() == typeof(SealedClass1) ? "Ok" : "Fail"; + + [MethodImpl(MethodImplOptions.NoInlining)] + public static object TestTernary11(SealedClass1 instance) => instance.GetTypeInlineable() == typeof(SealedClass2) ? "Fail" : "Ok"; + + [MethodImpl(MethodImplOptions.NoInlining)] + public static object TestTernary12(SealedClass1 instance) => instance.GetTypeInlineable() == typeof(NotSealedClass1) ? "Fail" : "Ok"; + + + [MethodImpl(MethodImplOptions.NoInlining)] + public bool TestReturn1() => GetTypeInlineable() == typeof(SealedClass1); + + [MethodImpl(MethodImplOptions.NoInlining)] + public bool TestReturn2() => GetTypeInlineable() == typeof(SealedClass2); + + [MethodImpl(MethodImplOptions.NoInlining)] + public bool TestReturn3() => GetTypeInlineable() == typeof(NotSealedClass1); + + [MethodImpl(MethodImplOptions.NoInlining)] + public bool TestReturn4() => GetTypeInlineable() != typeof(SealedClass1); + + [MethodImpl(MethodImplOptions.NoInlining)] + public bool TestReturn5() => GetTypeInlineable() != typeof(SealedClass2); + + [MethodImpl(MethodImplOptions.NoInlining)] + public bool TestReturn6() => GetTypeInlineable() != typeof(NotSealedClass1); + + + [MethodImpl(MethodImplOptions.NoInlining)] + public bool TestReturn7() => GetType() == typeof(SealedClass1); + + [MethodImpl(MethodImplOptions.NoInlining)] + public bool TestReturn8() => GetType() == typeof(SealedClass2); + + [MethodImpl(MethodImplOptions.NoInlining)] + public bool TestReturn9() => GetType() == typeof(NotSealedClass1); +} + +public sealed class SealedClass2 { } +public class NotSealedClass1 { } + +public static class Program +{ + private static int returnCode = 100; + + public static int Main(string[] args) + { + var tests = new SealedClass1(); + AssertEquals("Ok", tests.TestTernary1()); + AssertEquals("Ok", tests.TestTernary2()); + AssertEquals("Ok", tests.TestTernary3()); + AssertEquals("Ok", tests.TestTernary4()); + AssertEquals("Ok", tests.TestTernary5()); + AssertEquals("Ok", tests.TestTernary6()); + AssertEquals("Ok", SealedClass1.TestTernary7(new SealedClass1())); + AssertEquals("Ok", SealedClass1.TestTernary8(new SealedClass1())); + AssertEquals("Ok", SealedClass1.TestTernary9(new SealedClass1())); + AssertEquals("Ok", SealedClass1.TestTernary10(new SealedClass1())); + AssertEquals("Ok", SealedClass1.TestTernary11(new SealedClass1())); + AssertEquals("Ok", SealedClass1.TestTernary12(new SealedClass1())); + ThrowsNRE(() => SealedClass1.TestTernary7(null)); + ThrowsNRE(() => SealedClass1.TestTernary8(null)); + ThrowsNRE(() => SealedClass1.TestTernary9(null)); + ThrowsNRE(() => SealedClass1.TestTernary10(null)); + ThrowsNRE(() => SealedClass1.TestTernary11(null)); + ThrowsNRE(() => SealedClass1.TestTernary12(null)); + AssertIsTrue(tests.TestReturn1()); + AssertIsFalse(tests.TestReturn2()); + AssertIsFalse(tests.TestReturn3()); + AssertIsFalse(tests.TestReturn4()); + AssertIsTrue(tests.TestReturn5()); + AssertIsTrue(tests.TestReturn6()); + AssertIsTrue(tests.TestReturn7()); + AssertIsFalse(tests.TestReturn8()); + AssertIsFalse(tests.TestReturn9()); + return returnCode; + } + + private static void ThrowsNRE(Action action) + { + try + { + action(); + } + catch (NullReferenceException) + { + return; + } + + returnCode++; + Console.WriteLine($"Expected: NullReferenceException"); + } + + private static void AssertEquals(object expected, object actual, [CallerLineNumber] int line = 0) + { + if (expected != actual) + { + returnCode++; + Console.WriteLine($"{expected} != {actual}, L:{line}"); + } + } + + private static void AssertIsTrue(bool value, [CallerLineNumber] int line = 0) + { + if (!value) + { + returnCode++; + Console.WriteLine($"Expected: True, L:{line}"); + } + } + + private static void AssertIsFalse(bool value, [CallerLineNumber] int line = 0) + { + if (value) + { + returnCode++; + Console.WriteLine($"Expected: False, L:{line}"); + } + } +} \ No newline at end of file diff --git a/src/coreclr/tests/src/JIT/Intrinsics/TypeEqualitySealed_r.csproj b/src/coreclr/tests/src/JIT/Intrinsics/TypeEqualitySealed_r.csproj new file mode 100644 index 0000000..f616891 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Intrinsics/TypeEqualitySealed_r.csproj @@ -0,0 +1,10 @@ + + + Exe + None + + + + + + diff --git a/src/coreclr/tests/src/JIT/Intrinsics/TypeEqualitySealed_ro.csproj b/src/coreclr/tests/src/JIT/Intrinsics/TypeEqualitySealed_ro.csproj new file mode 100644 index 0000000..1a39126 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Intrinsics/TypeEqualitySealed_ro.csproj @@ -0,0 +1,10 @@ + + + Exe + None + True + + + + + -- 2.7.4