From b63403bd8116ba4713aa72341c8d727d479ad54d Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 13 Dec 2021 14:52:43 +0100 Subject: [PATCH 1/2] Add explicit null-check for tailcalls to VSD There is already a comment that this is necessary, but it is only being done for x86 tailcalls via jit helper. Do it for normal tailcalls to VSD as well. Fix #61486 --- src/coreclr/jit/assertionprop.cpp | 2 +- src/coreclr/jit/compiler.cpp | 2 +- src/coreclr/jit/fginline.cpp | 2 +- src/coreclr/jit/gentree.cpp | 2 +- src/coreclr/jit/importer.cpp | 4 +- src/coreclr/jit/morph.cpp | 18 ++++---- .../JitBlue/Runtime_61486/Runtime_61486.cs | 45 +++++++++++++++++++ .../Runtime_61486/Runtime_61486.csproj | 9 ++++ 8 files changed, 69 insertions(+), 15 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_61486/Runtime_61486.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_61486/Runtime_61486.csproj diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 52246f8fdc76db..79de29d8b5cac3 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3978,7 +3978,7 @@ AssertionIndex Compiler::optAssertionIsNonNullInternal(GenTree* op, */ GenTree* Compiler::optNonNullAssertionProp_Call(ASSERT_VALARG_TP assertions, GenTreeCall* call) { - if ((call->gtFlags & GTF_CALL_NULLCHECK) == 0) + if (!call->NeedsNullCheck()) { return nullptr; } diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 2c35262a5df0db..f2f66fa2818f31 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -9313,7 +9313,7 @@ void cTreeFlags(Compiler* comp, GenTree* tree) { chars += printf("[CALL_VIRT_STUB]"); } - if (tree->gtFlags & GTF_CALL_NULLCHECK) + if (tree->AsCall()->NeedsNullCheck()) { chars += printf("[CALL_NULLCHECK]"); } diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index 29aabbaf40a902..ac4c428dc5c67b 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -1497,7 +1497,7 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo) // GenTree* nullcheck = nullptr; - if (call->gtFlags & GTF_CALL_NULLCHECK && !inlineInfo->thisDereferencedFirst) + if (call->NeedsNullCheck() && !inlineInfo->thisDereferencedFirst) { // Call impInlineFetchArg to "reserve" a temp for the "this" pointer. GenTree* thisOp = impInlineFetchArg(0, inlArgInfo, lclVarInfo); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index d06b00a9d4d4e0..cc8fd84e346e4d 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -10059,7 +10059,7 @@ void Compiler::gtDispNodeName(GenTree* tree) assert(!"Unknown gtCallType"); } - if (tree->gtFlags & GTF_CALL_NULLCHECK) + if (tree->AsCall()->NeedsNullCheck()) { gtfType = " nullcheck"; } diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 04e074cf449120..3e3e9b0c233677 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -8701,7 +8701,7 @@ var_types Compiler::impImportCall(OPCODE opcode, assert(mflags & CORINFO_FLG_FINAL); /* It should have the GTF_CALL_NULLCHECK flag set. Reset it */ - assert(call->gtFlags & GTF_CALL_NULLCHECK); + assert(call->AsCall()->NeedsNullCheck()); call->gtFlags &= ~GTF_CALL_NULLCHECK; } } @@ -9375,7 +9375,7 @@ var_types Compiler::impImportCall(OPCODE opcode, { GenTree* callObj = call->AsCall()->gtCallThisArg->GetNode(); - if ((call->AsCall()->IsVirtual() || (call->gtFlags & GTF_CALL_NULLCHECK)) && + if ((call->AsCall()->IsVirtual() || call->AsCall()->NeedsNullCheck()) && impInlineIsGuaranteedThisDerefBeforeAnySideEffects(nullptr, call->AsCall()->gtCallArgs, callObj, impInlineInfo->inlArgInfo)) { diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index f6bc9a4bbe0598..ba5f450b616c9f 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -7896,6 +7896,15 @@ GenTree* Compiler::fgMorphPotentialTailCall(GenTreeCall* call) // Avoid potential extra work for the return (for example, vzeroupper) call->gtType = TYP_VOID; + // The runtime requires that we perform a null check on the `this` argument before + // tail calling to a virtual dispatch stub. This requirement is a consequence of limitations + // in the runtime's ability to map an AV to a NullReferenceException if + // the AV occurs in a dispatch stub that has unmanaged caller. + if (call->IsVirtualStub()) + { + call->gtFlags |= GTF_CALL_NULLCHECK; + } + // Do some target-specific transformations (before we process the args, // etc.) for the JIT helper case. if (tailCallViaJitHelper) @@ -8622,15 +8631,6 @@ void Compiler::fgMorphTailCallViaJitHelper(GenTreeCall* call) JITDUMP("fgMorphTailCallViaJitHelper (before):\n"); DISPTREE(call); - // The runtime requires that we perform a null check on the `this` argument before - // tail calling to a virtual dispatch stub. This requirement is a consequence of limitations - // in the runtime's ability to map an AV to a NullReferenceException if - // the AV occurs in a dispatch stub that has unmanaged caller. - if (call->IsVirtualStub()) - { - call->gtFlags |= GTF_CALL_NULLCHECK; - } - // For the helper-assisted tail calls, we need to push all the arguments // into a single list, and then add a few extra at the beginning or end. // diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_61486/Runtime_61486.cs b/src/tests/JIT/Regression/JitBlue/Runtime_61486/Runtime_61486.cs new file mode 100644 index 00000000000000..3285ee9cc556a9 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_61486/Runtime_61486.cs @@ -0,0 +1,45 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Reflection; +using System.Runtime.CompilerServices; + +public class Runtime_61486 +{ + public static int Main() + { + var my = new My(new My(null)); + var m = my.GetType().GetMethod("M"); + try + { + m.Invoke(my, null); + return -1; + } + catch (TargetInvocationException ex) when (ex.InnerException is NullReferenceException) + { + return 100; + } + } + + public interface IFace + { + void M(); + } + + public class My : IFace + { + private IFace _face; + + public My(IFace face) + { + _face = face; + } + + // We cannot handle a null ref inside a VSD if the caller is not + // managed frame. This test is verifying that JIT null checks ahead of + // time in this case. + [MethodImpl(MethodImplOptions.AggressiveOptimization)] + public void M() => _face.M(); + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_61486/Runtime_61486.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_61486/Runtime_61486.csproj new file mode 100644 index 00000000000000..6946bed81bfd5b --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_61486/Runtime_61486.csproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + From e1c71b07f0f930c1f088c7f7a978d3c771d570e0 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 13 Dec 2021 15:31:42 +0100 Subject: [PATCH 2/2] Revert cleanup to make potential backport easier --- src/coreclr/jit/assertionprop.cpp | 2 +- src/coreclr/jit/compiler.cpp | 2 +- src/coreclr/jit/fginline.cpp | 2 +- src/coreclr/jit/gentree.cpp | 2 +- src/coreclr/jit/importer.cpp | 4 ++-- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 79de29d8b5cac3..52246f8fdc76db 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3978,7 +3978,7 @@ AssertionIndex Compiler::optAssertionIsNonNullInternal(GenTree* op, */ GenTree* Compiler::optNonNullAssertionProp_Call(ASSERT_VALARG_TP assertions, GenTreeCall* call) { - if (!call->NeedsNullCheck()) + if ((call->gtFlags & GTF_CALL_NULLCHECK) == 0) { return nullptr; } diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index f2f66fa2818f31..2c35262a5df0db 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -9313,7 +9313,7 @@ void cTreeFlags(Compiler* comp, GenTree* tree) { chars += printf("[CALL_VIRT_STUB]"); } - if (tree->AsCall()->NeedsNullCheck()) + if (tree->gtFlags & GTF_CALL_NULLCHECK) { chars += printf("[CALL_NULLCHECK]"); } diff --git a/src/coreclr/jit/fginline.cpp b/src/coreclr/jit/fginline.cpp index ac4c428dc5c67b..29aabbaf40a902 100644 --- a/src/coreclr/jit/fginline.cpp +++ b/src/coreclr/jit/fginline.cpp @@ -1497,7 +1497,7 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo) // GenTree* nullcheck = nullptr; - if (call->NeedsNullCheck() && !inlineInfo->thisDereferencedFirst) + if (call->gtFlags & GTF_CALL_NULLCHECK && !inlineInfo->thisDereferencedFirst) { // Call impInlineFetchArg to "reserve" a temp for the "this" pointer. GenTree* thisOp = impInlineFetchArg(0, inlArgInfo, lclVarInfo); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index cc8fd84e346e4d..d06b00a9d4d4e0 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -10059,7 +10059,7 @@ void Compiler::gtDispNodeName(GenTree* tree) assert(!"Unknown gtCallType"); } - if (tree->AsCall()->NeedsNullCheck()) + if (tree->gtFlags & GTF_CALL_NULLCHECK) { gtfType = " nullcheck"; } diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 3e3e9b0c233677..04e074cf449120 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -8701,7 +8701,7 @@ var_types Compiler::impImportCall(OPCODE opcode, assert(mflags & CORINFO_FLG_FINAL); /* It should have the GTF_CALL_NULLCHECK flag set. Reset it */ - assert(call->AsCall()->NeedsNullCheck()); + assert(call->gtFlags & GTF_CALL_NULLCHECK); call->gtFlags &= ~GTF_CALL_NULLCHECK; } } @@ -9375,7 +9375,7 @@ var_types Compiler::impImportCall(OPCODE opcode, { GenTree* callObj = call->AsCall()->gtCallThisArg->GetNode(); - if ((call->AsCall()->IsVirtual() || call->AsCall()->NeedsNullCheck()) && + if ((call->AsCall()->IsVirtual() || (call->gtFlags & GTF_CALL_NULLCHECK)) && impInlineIsGuaranteedThisDerefBeforeAnySideEffects(nullptr, call->AsCall()->gtCallArgs, callObj, impInlineInfo->inlArgInfo)) {