From 7075393b295b2cccff6ae80c9fa50083ea46ece8 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jan 2022 09:06:17 +0100 Subject: [PATCH 01/30] Implement control-flow guard support for x64 and arm64 --- .../templates/runtimes/run-test-job.yml | 10 +- eng/pipelines/coreclr/jit-cfg.yml | 52 ++++ src/coreclr/inc/corinfo.h | 3 + src/coreclr/inc/corjitflags.h | 2 +- src/coreclr/inc/jithelpers.h | 8 + src/coreclr/jit/codegenarmarch.cpp | 31 ++- src/coreclr/jit/codegencommon.cpp | 3 + src/coreclr/jit/codegenxarch.cpp | 36 ++- src/coreclr/jit/compiler.h | 47 +++- src/coreclr/jit/flowgraph.cpp | 2 +- src/coreclr/jit/gentree.h | 43 +++ src/coreclr/jit/jitconfigvalues.h | 8 + src/coreclr/jit/jitee.h | 3 +- src/coreclr/jit/lower.cpp | 255 +++++++++++++++--- src/coreclr/jit/lower.h | 4 +- src/coreclr/jit/lsrabuild.cpp | 40 ++- src/coreclr/jit/morph.cpp | 43 ++- src/coreclr/jit/targetamd64.h | 4 + src/coreclr/jit/targetarm.h | 3 + src/coreclr/jit/targetarm64.h | 4 + src/coreclr/jit/targetx86.h | 3 + .../Common/JitInterface/CorInfoHelpFunc.cs | 8 + .../tools/Common/JitInterface/CorInfoTypes.cs | 2 +- src/coreclr/vm/amd64/JitHelpers_Fast.asm | 13 + src/coreclr/vm/amd64/jithelpers_fast.S | 10 + src/coreclr/vm/arm64/asmhelpers.S | 8 + src/coreclr/vm/arm64/asmhelpers.asm | 8 + src/coreclr/vm/jithelpers.cpp | 4 + src/tests/Common/testenvironment.proj | 5 + 29 files changed, 562 insertions(+), 100 deletions(-) create mode 100644 eng/pipelines/coreclr/jit-cfg.yml diff --git a/eng/pipelines/common/templates/runtimes/run-test-job.yml b/eng/pipelines/common/templates/runtimes/run-test-job.yml index 214b56547b7ac..d145070fe79a6 100644 --- a/eng/pipelines/common/templates/runtimes/run-test-job.yml +++ b/eng/pipelines/common/templates/runtimes/run-test-job.yml @@ -197,6 +197,9 @@ jobs: - ${{ if in(parameters.testGroup, 'pgo') }}: - name: timeoutPerTestCollectionInMinutes value: 120 + - ${{ if in(parameters.testGroup, 'jit-cfg') }}: + - name: timeoutPerTestCollectionInMinutes + value: 120 - ${{ if eq(parameters.compositeBuildMode, true) }}: - name: crossgenArg @@ -210,7 +213,7 @@ jobs: # TODO: update these numbers as they were determined long ago ${{ if eq(parameters.testGroup, 'innerloop') }}: timeoutInMinutes: 200 - ${{ if in(parameters.testGroup, 'outerloop', 'jit-experimental', 'pgo') }}: + ${{ if in(parameters.testGroup, 'outerloop', 'jit-experimental', 'pgo', 'jit-cfg') }}: timeoutInMinutes: 270 ${{ if in(parameters.testGroup, 'gc-longrunning', 'gc-simulator') }}: timeoutInMinutes: 480 @@ -551,6 +554,11 @@ jobs: - jitpartialcompilation_osr - jitpartialcompilation_osr_pgo - jitobjectstackallocation + ${{ if in(parameters.testGroup, 'jit-cfg') }}: + scenarios: + - jitcfg + - jitcfg_dispatcher_always + - jitcfg_dispatcher_never ${{ if in(parameters.testGroup, 'ilasm') }}: scenarios: - ilasmroundtrip diff --git a/eng/pipelines/coreclr/jit-cfg.yml b/eng/pipelines/coreclr/jit-cfg.yml new file mode 100644 index 0000000000000..6b30445c2363c --- /dev/null +++ b/eng/pipelines/coreclr/jit-cfg.yml @@ -0,0 +1,52 @@ +trigger: none + +schedules: +- cron: "0 22 * * 0,6" + displayName: Sun at 2:00 PM (UTC-8:00) + branches: + include: + - main + always: true + +jobs: + +- template: /eng/pipelines/common/platform-matrix.yml + parameters: + jobTemplate: /eng/pipelines/common/build-coreclr-and-libraries-job.yml + buildConfig: checked + platforms: + - OSX_arm64 + - OSX_x64 + - Linux_arm64 + - Linux_x64 + - windows_arm64 + - windows_x64 + - CoreClrTestBuildHost # Either OSX_x64 or Linux_x64 + jobParameters: + testGroup: jit-cfg + +- template: /eng/pipelines/common/platform-matrix.yml + parameters: + jobTemplate: /eng/pipelines/common/templates/runtimes/build-test-job.yml + buildConfig: checked + platforms: + - CoreClrTestBuildHost # Either OSX_x64 or Linux_x64 + jobParameters: + testGroup: jit-cfg + +- template: /eng/pipelines/common/platform-matrix.yml + parameters: + jobTemplate: /eng/pipelines/common/templates/runtimes/run-test-job.yml + buildConfig: checked + platforms: + - OSX_arm64 + - OSX_x64 + - Linux_arm64 + - Linux_x64 + - windows_arm64 + - windows_x64 + helixQueueGroup: ci + helixQueuesTemplate: /eng/pipelines/coreclr/templates/helix-queues-setup.yml + jobParameters: + testGroup: jit-cfg + liveLibrariesBuildConfig: Release diff --git a/src/coreclr/inc/corinfo.h b/src/coreclr/inc/corinfo.h index 3bad0df909e9a..e8b12248d04bb 100644 --- a/src/coreclr/inc/corinfo.h +++ b/src/coreclr/inc/corinfo.h @@ -614,6 +614,9 @@ enum CorInfoHelpFunc CORINFO_HELP_CLASSPROFILE64, // Update 64-bit class profile for a call site CORINFO_HELP_PARTIAL_COMPILATION_PATCHPOINT, // Notify runtime that code has reached a part of the method that wasn't originally jitted. + CORINFO_HELP_VALIDATE_INDIRECT_CALL, // CFG: Validate function pointer + CORINFO_HELP_DISPATCH_INDIRECT_CALL, // CFG: Validate and dispatch to pointer + CORINFO_HELP_COUNT, }; diff --git a/src/coreclr/inc/corjitflags.h b/src/coreclr/inc/corjitflags.h index c749e876d2e0f..bf4f990c7aa19 100644 --- a/src/coreclr/inc/corjitflags.h +++ b/src/coreclr/inc/corjitflags.h @@ -32,7 +32,7 @@ class CORJIT_FLAGS CORJIT_FLAG_DEBUG_EnC = 3, // We are in Edit-n-Continue mode CORJIT_FLAG_DEBUG_INFO = 4, // generate line and local-var info CORJIT_FLAG_MIN_OPT = 5, // disable all jit optimizations (not necesarily debuggable code) - CORJIT_FLAG_UNUSED1 = 6, + CORJIT_FLAG_ENABLE_CFG = 6, // generate control-flow guard checks CORJIT_FLAG_MCJIT_BACKGROUND = 7, // Calling from multicore JIT background thread, do not call JitComplete #if defined(TARGET_X86) diff --git a/src/coreclr/inc/jithelpers.h b/src/coreclr/inc/jithelpers.h index b9a1e3941da38..82b43a7ab01f5 100644 --- a/src/coreclr/inc/jithelpers.h +++ b/src/coreclr/inc/jithelpers.h @@ -351,6 +351,14 @@ JITHELPER(CORINFO_HELP_CLASSPROFILE64, JIT_ClassProfile64, CORINFO_HELP_SIG_REG_ONLY) JITHELPER(CORINFO_HELP_PARTIAL_COMPILATION_PATCHPOINT, JIT_PartialCompilationPatchpoint, CORINFO_HELP_SIG_REG_ONLY) +#if defined(TARGET_AMD64) || defined(TARGET_ARM64) + JITHELPER(CORINFO_HELP_VALIDATE_INDIRECT_CALL, JIT_ValidateIndirectCall, CORINFO_HELP_SIG_REG_ONLY) + JITHELPER(CORINFO_HELP_DISPATCH_INDIRECT_CALL, JIT_DispatchIndirectCall, CORINFO_HELP_SIG_REG_ONLY) +#else + JITHELPER(CORINFO_HELP_VALIDATE_INDIRECT_CALL, NULL, CORINFO_HELP_SIG_REG_ONLY) + JITHELPER(CORINFO_HELP_DISPATCH_INDIRECT_CALL, NULL, CORINFO_HELP_SIG_REG_ONLY) +#endif + #undef JITHELPER #undef DYNAMICJITHELPER #undef JITHELPER diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index 227de58aedc97..ca15540f2d063 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -3155,9 +3155,6 @@ void CodeGen::genCall(GenTreeCall* call) // into a volatile register that won't be restored by epilog sequence. if (call->IsFastTailCall()) { - // Don't support fast tail calling JIT helpers - assert(call->gtCallType != CT_HELPER); - GenTree* target = getCallTarget(call, nullptr); if (target != nullptr) @@ -3198,22 +3195,28 @@ void CodeGen::genCall(GenTreeCall* call) genCallInstruction(call); - // if it was a pinvoke we may have needed to get the address of a label - if (genPendingCallLabel) + // for pinvoke/intrinsic/tailcalls we may have needed to get the address of + // a label. In case it is indirect with CFG enabled make sure we do not get + // the address after the validation but only after the actual call that + // comes after. + if (genPendingCallLabel && !call->IsHelperCall(compiler, CORINFO_HELP_VALIDATE_INDIRECT_CALL)) { genDefineInlineTempLabel(genPendingCallLabel); genPendingCallLabel = nullptr; } - // Update GC info: - // All Callee arg registers are trashed and no longer contain any GC pointers. - // TODO-Bug?: As a matter of fact shouldn't we be killing all of callee trashed regs here? - // For now we will assert that other than arg regs gc ref/byref set doesn't contain any other - // registers from RBM_CALLEE_TRASH - assert((gcInfo.gcRegGCrefSetCur & (RBM_CALLEE_TRASH & ~RBM_ARG_REGS)) == 0); - assert((gcInfo.gcRegByrefSetCur & (RBM_CALLEE_TRASH & ~RBM_ARG_REGS)) == 0); - gcInfo.gcRegGCrefSetCur &= ~RBM_ARG_REGS; - gcInfo.gcRegByrefSetCur &= ~RBM_ARG_REGS; +#ifdef DEBUG + // Killed registers should no longer contain any GC pointers. + regMaskTP killMask = RBM_CALLEE_TRASH; + if (call->IsHelperCall()) + { + CorInfoHelpFunc helpFunc = compiler->eeGetHelperNum(call->gtCallMethHnd); + killMask = compiler->compHelperCallKillSet(helpFunc); + } + + assert((gcInfo.gcRegGCrefSetCur & killMask) == 0); + assert((gcInfo.gcRegByrefSetCur & killMask) == 0); +#endif var_types returnType = call->TypeGet(); if (returnType != TYP_VOID) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 1c9eda6f63afc..94c9646cfd57e 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -661,6 +661,9 @@ regMaskTP Compiler::compHelperCallKillSet(CorInfoHelpFunc helper) case CORINFO_HELP_INIT_PINVOKE_FRAME: return RBM_INIT_PINVOKE_FRAME_TRASH; + case CORINFO_HELP_VALIDATE_INDIRECT_CALL: + return RBM_VALIDATE_INDIRECT_CALL_TRASH; + default: return RBM_CALLEE_TRASH; } diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 7da0bd7285625..183546d5dc708 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -5217,9 +5217,6 @@ void CodeGen::genCall(GenTreeCall* call) // that won't be restored by epilog sequence. if (call->IsFastTailCall()) { - // Don't support fast tail calling JIT helpers - assert(call->gtCallType != CT_HELPER); - GenTree* target = getCallTarget(call, nullptr); if (target != nullptr) { @@ -5271,22 +5268,28 @@ void CodeGen::genCall(GenTreeCall* call) genCallInstruction(call X86_ARG(stackArgBytes)); - // if it was a pinvoke or intrinsic we may have needed to get the address of a label - if (genPendingCallLabel) + // for pinvoke/intrinsic/tailcalls we may have needed to get the address of + // a label. In case it is indirect with CFG enabled make sure we do not get + // the address after the validation but only after the actual call that + // comes after. + if (genPendingCallLabel && !call->IsHelperCall(compiler, CORINFO_HELP_VALIDATE_INDIRECT_CALL)) { genDefineInlineTempLabel(genPendingCallLabel); genPendingCallLabel = nullptr; } - // Update GC info: - // All Callee arg registers are trashed and no longer contain any GC pointers. - // TODO-XArch-Bug?: As a matter of fact shouldn't we be killing all of callee trashed regs here? - // For now we will assert that other than arg regs gc ref/byref set doesn't contain any other - // registers from RBM_CALLEE_TRASH. - assert((gcInfo.gcRegGCrefSetCur & (RBM_CALLEE_TRASH & ~RBM_ARG_REGS)) == 0); - assert((gcInfo.gcRegByrefSetCur & (RBM_CALLEE_TRASH & ~RBM_ARG_REGS)) == 0); - gcInfo.gcRegGCrefSetCur &= ~RBM_ARG_REGS; - gcInfo.gcRegByrefSetCur &= ~RBM_ARG_REGS; +#ifdef DEBUG + // Killed registers should no longer contain any GC pointers. + regMaskTP killMask = RBM_CALLEE_TRASH; + if (call->IsHelperCall()) + { + CorInfoHelpFunc helpFunc = compiler->eeGetHelperNum(call->gtCallMethHnd); + killMask = compiler->compHelperCallKillSet(helpFunc); + } + + assert((gcInfo.gcRegGCrefSetCur & killMask) == 0); + assert((gcInfo.gcRegByrefSetCur & killMask) == 0); +#endif var_types returnType = call->TypeGet(); if (returnType != TYP_VOID) @@ -5562,6 +5565,11 @@ void CodeGen::genCallInstruction(GenTreeCall* call X86_ARG(target_ssize_t stackA #endif if (target->isContainedIndir()) { + // When CFG is enabled we should not be emitting any non-register indirect calls. + assert(!compiler->opts.IsCFGEnabled() || + call->IsHelperCall(compiler, CORINFO_HELP_VALIDATE_INDIRECT_CALL) || + call->IsHelperCall(compiler, CORINFO_HELP_DISPATCH_INDIRECT_CALL)); + if (target->AsIndir()->HasBase() && target->AsIndir()->Base()->isContainedIntOrIImmed()) { // Note that if gtControlExpr is an indir of an absolute address, we mark it as diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index d5dcf4cd92c81..10b62d7aa3dc4 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -1742,6 +1742,7 @@ struct fgArgTabEntry case NonStandardArgKind::ShiftLow: case NonStandardArgKind::ShiftHigh: case NonStandardArgKind::FixedRetBuffer: + case NonStandardArgKind::ValidateIndirectCallTarget: return false; case NonStandardArgKind::WrapperDelegateCell: case NonStandardArgKind::VirtualStubCell: @@ -9585,7 +9586,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX compMinOptsIsUsed = true; return compMinOpts; } - bool IsMinOptsSet() + bool IsMinOptsSet() const { return compMinOptsIsSet; } @@ -9594,7 +9595,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX { return compMinOpts; } - bool IsMinOptsSet() + bool IsMinOptsSet() const { return compMinOptsIsSet; } @@ -9618,23 +9619,59 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX } // true if the CLFLG_* for an optimization is set. - bool OptEnabled(unsigned optFlag) + bool OptEnabled(unsigned optFlag) const { return !!(compFlags & optFlag); } #ifdef FEATURE_READYTORUN - bool IsReadyToRun() + bool IsReadyToRun() const { return jitFlags->IsSet(JitFlags::JIT_FLAG_READYTORUN); } #else - bool IsReadyToRun() + bool IsReadyToRun() const { return false; } #endif + // Check if the compilation is control-flow guard enabled. + bool IsCFGEnabled() const + { +#if defined(TARGET_ARM64) || defined(TARGET_AMD64) + // On these platforms we assume the register that the target is + // passed in is preserved by the validator and take care to get the + // target from the register for the call (even in debug mode). + // + // TODO: Confirm that the ARM64 validator guarantees that the + // target-address register is preserved (there is conflicting + // documentation on this, but it currently does). + static_assert_no_msg((RBM_VALIDATE_INDIRECT_CALL_TRASH & (1 << REG_VALIDATE_INDIRECT_CALL_ADDR)) == 0); + if (JitConfig.JitForceControlFlowGuard()) + return true; + + return jitFlags->IsSet(JitFlags::JIT_FLAG_ENABLE_CFG); +#else + // The remaining platforms are not supported and would require some + // work to support. + // + // ARM32: + // The ARM32 validator does not preserve any volatile registers + // which means we have to take special care to allocate and use a + // callee-saved register (reloading the target from memory is a + // security issue). + // + // x86: + // On x86 some VSD calls disassemble the call site and expect an + // indirect call which is fundamentally incompatible with CFG. + // This would require a different way to pass this information + // through. + // + return false; +#endif + } + #ifdef FEATURE_ON_STACK_REPLACEMENT bool IsOSR() const { diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index ba032045e74b7..662bad7a098a0 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -3004,7 +3004,7 @@ void Compiler::fgSimpleLowering() GenTreeCall* call = tree->AsCall(); // Fast tail calls use the caller-supplied scratch // space so have no impact on this method's outgoing arg size. - if (!call->IsFastTailCall()) + if (!call->IsFastTailCall() && !call->IsHelperCall(this, CORINFO_HELP_VALIDATE_INDIRECT_CALL)) { // Update outgoing arg size to handle this call const unsigned thisCallOutAreaSize = call->fgArgInfo->GetOutArgSize(); diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index e7a03d26547d3..dc411644664b4 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -3977,6 +3977,7 @@ enum class NonStandardArgKind : unsigned FixedRetBuffer, VirtualStubCell, R2RIndirectionCell, + ValidateIndirectCallTarget, // If changing this enum also change getNonStandardArgKindName and isNonStandardArgAddedLate in fgArgInfo }; @@ -3985,6 +3986,12 @@ enum class NonStandardArgKind : unsigned const char* getNonStandardArgKindName(NonStandardArgKind kind); #endif +enum class CFGCallKind +{ + ValidateAndCall, + Dispatch, +}; + struct GenTreeCall final : public GenTree { class Use @@ -4687,6 +4694,42 @@ struct GenTreeCall final : public GenTree return NonStandardArgKind::None; } + CFGCallKind GetCFGCallKind() + { +#if defined(TARGET_AMD64) + // On x64 the dispatcher is more performant, but we cannot use it when + // we need to pass indirection cells as those go into registers that + // are clobbered by the dispatch helper. + bool mayUseDispatcher = GetIndirectionCellArgKind() == NonStandardArgKind::None; + bool shouldUseDispatcher = true; +#elif defined(TARGET_AMD64) + bool mayUseDispatcher = true; + // Branch predictors on ARM64 generally do not handle the dispatcher as + // well as on x64 hardware, so only use the validator by default. + bool shouldUseDispatcher = false; +#else + // Other platforms do not even support the dispatcher. + bool mayUseDispatcher = false; + bool shouldUseDispatcher = false; +#endif + +#ifdef DEBUG + switch (JitConfig.JitCFGUseDispatcher()) + { + case 0: + shouldUseDispatcher = false; + break; + case 1: + shouldUseDispatcher = true; + break; + default: + break; + } +#endif + + return mayUseDispatcher && shouldUseDispatcher ? CFGCallKind::Dispatch : CFGCallKind::ValidateAndCall; + } + void ResetArgInfo(); GenTreeCallFlags gtCallMoreFlags; // in addition to gtFlags diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h index 4fc4594c6fb2b..e3efd5d872d8b 100644 --- a/src/coreclr/jit/jitconfigvalues.h +++ b/src/coreclr/jit/jitconfigvalues.h @@ -494,6 +494,14 @@ CONFIG_INTEGER(JitNoteFailedExactDevirtualization, W("JitNoteFailedExactDevirtua CONFIG_INTEGER(JitExpandCallsEarly, W("JitExpandCallsEarly"), 1) // Expand Call targets early (in the global morph // phase) +// Force the generation of CFG checks +CONFIG_INTEGER(JitForceControlFlowGuard, W("JitForceControlFlowGuard"), 0); +// JitCFGUseDispatcher values: +// 0: Never use dispatcher +// 1: Use dispatcher on all platforms that support it +// 2: Default behavior, depends on platform (yes on x64, no on arm64) +CONFIG_INTEGER(JitCFGUseDispatcher, W("JitCFGUseDispatcher"), 2) + #if defined(DEBUG) // JitFunctionFile: Name of a file that contains a list of functions. If the currently compiled function is in the // file, certain other JIT config variables will be active. If the currently compiled function is not in the file, diff --git a/src/coreclr/jit/jitee.h b/src/coreclr/jit/jitee.h index 27149c824348a..58eddca01b61c 100644 --- a/src/coreclr/jit/jitee.h +++ b/src/coreclr/jit/jitee.h @@ -17,7 +17,7 @@ class JitFlags JIT_FLAG_DEBUG_EnC = 3, // We are in Edit-n-Continue mode JIT_FLAG_DEBUG_INFO = 4, // generate line and local-var info JIT_FLAG_MIN_OPT = 5, // disable all jit optimizations (not necesarily debuggable code) - JIT_FLAG_UNUSED1 = 6, + JIT_FLAG_ENABLE_CFG = 6, // generate CFG enabled code JIT_FLAG_MCJIT_BACKGROUND = 7, // Calling from multicore JIT background thread, do not call JitComplete #if defined(TARGET_X86) @@ -174,6 +174,7 @@ class JitFlags FLAGS_EQUAL(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_EnC, JIT_FLAG_DEBUG_EnC); FLAGS_EQUAL(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_INFO, JIT_FLAG_DEBUG_INFO); FLAGS_EQUAL(CORJIT_FLAGS::CORJIT_FLAG_MIN_OPT, JIT_FLAG_MIN_OPT); + FLAGS_EQUAL(CORJIT_FLAGS::CORJIT_FLAG_ENABLE_CFG, JIT_FLAG_ENABLE_CFG); FLAGS_EQUAL(CORJIT_FLAGS::CORJIT_FLAG_MCJIT_BACKGROUND, JIT_FLAG_MCJIT_BACKGROUND); #if defined(TARGET_X86) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 06d42a37f986f..3a1447a5193ee 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -1583,7 +1583,7 @@ void Lowering::LowerCall(GenTree* node) call->ClearOtherRegs(); LowerArgsForCall(call); - // note that everything generated from this point on runs AFTER the outgoing args are placed + // note that everything generated from this point might run AFTER the outgoing args are placed GenTree* controlExpr = nullptr; bool callWasExpandedEarly = false; @@ -1640,6 +1640,10 @@ void Lowering::LowerCall(GenTree* node) } } + // Indirect calls should always go through GenTreeCall::gtCallAddr and + // should never have a control expression as well. + assert((call->gtCallType != CT_INDIRECT) || (controlExpr == nullptr)); + if (call->IsTailCallViaJitHelper()) { // Either controlExpr or gtCallAddr must contain real call target. @@ -1662,40 +1666,17 @@ void Lowering::LowerCall(GenTree* node) JITDUMP("results of lowering call:\n"); DISPRANGE(controlExprRange); - GenTree* insertionPoint = call; - if (!call->IsTailCallViaJitHelper()) - { - // The controlExpr should go before the gtCallCookie and the gtCallAddr, if they exist - // - // TODO-LIR: find out what's really required here, as this is currently a tree order - // dependency. - if (call->gtCallType == CT_INDIRECT) - { - bool isClosed = false; - if (call->gtCallCookie != nullptr) - { -#ifdef DEBUG - GenTree* firstCallAddrNode = BlockRange().GetTreeRange(call->gtCallAddr, &isClosed).FirstNode(); - assert(isClosed); - assert(call->gtCallCookie->Precedes(firstCallAddrNode)); -#endif // DEBUG - - insertionPoint = BlockRange().GetTreeRange(call->gtCallCookie, &isClosed).FirstNode(); - assert(isClosed); - } - else if (call->gtCallAddr != nullptr) - { - insertionPoint = BlockRange().GetTreeRange(call->gtCallAddr, &isClosed).FirstNode(); - assert(isClosed); - } - } - } - ContainCheckRange(controlExprRange); - BlockRange().InsertBefore(insertionPoint, std::move(controlExprRange)); + BlockRange().InsertBefore(call, std::move(controlExprRange)); call->gtControlExpr = controlExpr; } + + if (comp->opts.IsCFGEnabled()) + { + LowerCFGCall(call); + } + if (call->IsFastTailCall()) { // Lower fast tail call can introduce new temps to set up args correctly for Callee. @@ -2249,6 +2230,212 @@ GenTree* Lowering::LowerTailCallViaJitHelper(GenTreeCall* call, GenTree* callTar return result; } +//------------------------------------------------------------------------ +// LowerCFGCall: Potentially lower a call to use control-flow guard. This +// expands indirect calls into either a validate+call sequence or to a dispatch +// helper taking the original target in a special register. +// +// Arguments: +// call - The call node +// +void Lowering::LowerCFGCall(GenTreeCall* call) +{ + if (call->IsHelperCall(comp, CORINFO_HELP_VALIDATE_INDIRECT_CALL) || + call->IsHelperCall(comp, CORINFO_HELP_DISPATCH_INDIRECT_CALL)) + { + return; + } + + GenTree* callTarget = call->gtCallType == CT_INDIRECT ? call->gtCallAddr : call->gtControlExpr; + if ((callTarget == nullptr) || callTarget->IsIntegralConst()) + { + // This is a direct call, no CFG check is necessary. + return; + } + + CFGCallKind cfgKind = call->GetCFGCallKind(); + + switch (cfgKind) + { + case CFGCallKind::ValidateAndCall: + { + // To safely apply CFG we need to generate a very specific pattern: + // in particular, it is a safety issue to allow the JIT to reload + // the call target from memory between calling + // CORINFO_HELP_VALIDATE_INDIRECT_CALL and the target. This is + // something that would easily occur in debug codegen if we + // produced high-level IR. Instead we will produce + // + // tx... = + // ty = callTarget + // GT_CALL CORINFO_HELP_VALIDATE_INDIRECT_CALL, ty + // tz = PHYS_REG REG_ARG_0 (preserved by helper) + // tw... = + // GT_CALL tz, tx..., tw... + // + // The most problematic thing here is that the callTarget may + // effectively null-check 'this', and should normally happen after + // late args that can have some side effects. Therefore we ensure + // in fgArgInfo::ArgsComplete that we evaluate side-effecting args + // early for CFG. + // Note that early args may place stack args, but that's ok as the + // validator has a custom calling convention. + + GenTree* regNode = PhysReg(REG_VALIDATE_INDIRECT_CALL_ADDR, TYP_I_IMPL); + LIR::Use useOfTar; + bool gotUse = BlockRange().TryGetUse(callTarget, &useOfTar); + assert(gotUse); + useOfTar.ReplaceWith(regNode); + if (callTarget->OperIs(GT_LCL_VAR) || callTarget->OperIs(GT_LCL_FLD) || callTarget->IsIntegralConst()) + { + callTarget->SetUnusedValue(); + callTarget = comp->gtClone(callTarget, false); + } + else + { + if (m_cfgCallTargetTemp == BAD_VAR_NUM) + { + m_cfgCallTargetTemp = comp->lvaGrabTemp(true DEBUGARG("CFG validate call target")); + } + + GenTree* assign = comp->gtNewTempAssign(m_cfgCallTargetTemp, callTarget); + BlockRange().InsertBefore(call, assign); + LowerNode(assign); + + callTarget = comp->gtNewLclvNode(m_cfgCallTargetTemp, callTarget->TypeGet()); + } + + // Add the call to the validator + GenTreeCall::Use* args = comp->gtNewCallArgs(callTarget); + GenTreeCall* validate = comp->gtNewHelperCallNode(CORINFO_HELP_VALIDATE_INDIRECT_CALL, TYP_VOID, args); + + comp->fgMorphTree(validate); + + LIR::Range validateRange = LIR::SeqTree(comp, validate); + GenTree* validateFirst = validateRange.FirstNode(); + GenTree* validateLast = validateRange.LastNode(); + // Insert the validator with the call target before the late args. + BlockRange().InsertBefore(call, std::move(validateRange)); + LowerRange(validateFirst, validateLast); + + // Insert the PHYSREG node that we must load right after validation. + BlockRange().InsertAfter(validate, regNode); + LowerNode(regNode); + + // Finally move all GT_PUTARG_REG nodes that the validate call may clobber to after validation. + const regMaskTP clobberedArgRegs = + RBM_VALIDATE_INDIRECT_CALL_TRASH | genRegMask(REG_VALIDATE_INDIRECT_CALL_ADDR); + if ((clobberedArgRegs & RBM_ARG_REGS) != 0) + { + for (GenTreeCall::Use& use : call->LateArgs()) + { + GenTree* node = use.GetNode(); + assert(node->OperIsPutArg()); + fgArgTabEntry* entry = comp->gtArgEntryByNode(call, node); + bool isAnyRegClobbered = false; + for (unsigned i = 0; i < entry->numRegs; i++) + { + if ((genRegMask(entry->GetRegNum(i)) & clobberedArgRegs) != 0) + { + isAnyRegClobbered = true; + break; + } + } + + // If validate call will clobber any register used by this + // arg then place the arg after the call instead. + if (isAnyRegClobbered) + { + BlockRange().Remove(node); + BlockRange().InsertBefore(call, node); + } + } + } + break; + } + case CFGCallKind::Dispatch: + { +#ifdef REG_DISPATCH_INDIRECT_CALL_ADDR + // Now insert the call target as an extra argument. + // + // First append the early placeholder arg + GenTreeCall::Use** earlySlot = &call->gtCallArgs; + unsigned int index = call->gtCallThisArg != nullptr ? 1 : 0; + while (*earlySlot != nullptr) + { + earlySlot = &(*earlySlot)->NextRef(); + index++; + } + + assert(index == call->fgArgInfo->ArgCount()); + GenTree* placeHolder = comp->gtNewArgPlaceHolderNode(callTarget->TypeGet(), NO_CLASS_HANDLE); + placeHolder->gtFlags |= GTF_LATE_ARG; + *earlySlot = comp->gtNewCallArgs(placeHolder); + + // Append the late actual arg + GenTreeCall::Use** lateSlot = &call->gtCallLateArgs; + unsigned int lateIndex = 0; + while (*lateSlot != nullptr) + { + lateSlot = &(*lateSlot)->NextRef(); + lateIndex++; + } + + *lateSlot = comp->gtNewCallArgs(callTarget); + + // Add an entry into the arg info + regNumber regNum = REG_DISPATCH_INDIRECT_CALL_ADDR; + unsigned numRegs = 1; + unsigned byteSize = TARGET_POINTER_SIZE; + unsigned byteAlignment = TARGET_POINTER_SIZE; + bool isStruct = false; + bool isFloatHfa = false; + bool isVararg = false; + + // TODO: Preallocate space for this? Potentially hard as it is + // expensive to know in morph whether a call ends up being indirect. + fgArgTabEntry* entry = + call->fgArgInfo->AddRegArg(index, placeHolder, *earlySlot, regNum, numRegs, byteSize, byteAlignment, + isStruct, isFloatHfa, + isVararg UNIX_AMD64_ABI_ONLY_ARG(REG_STK) UNIX_AMD64_ABI_ONLY_ARG(0) + UNIX_AMD64_ABI_ONLY_ARG(0) UNIX_AMD64_ABI_ONLY_ARG(nullptr)); + + entry->lateUse = *lateSlot; + entry->SetLateArgInx(lateIndex); + + // Lower the newly added args now that call is updated + LowerArg(call, &(*earlySlot)->NodeRef()); + LowerArg(call, &(*lateSlot)->NodeRef()); + + // Finally update the call to be a helper call + call->gtCallType = CT_HELPER; + call->gtCallMethHnd = comp->eeFindHelper(CORINFO_HELP_DISPATCH_INDIRECT_CALL); + call->gtFlags &= ~GTF_CALL_VIRT_KIND_MASK; +#ifdef FEATURE_READYTORUN + call->gtEntryPoint.addr = nullptr; + call->gtEntryPoint.accessType = IAT_VALUE; +#endif + + // Now relower the call target + call->gtControlExpr = LowerDirectCall(call); + + if (call->gtControlExpr != nullptr) + { + LIR::Range dispatchControlExprRange = LIR::SeqTree(comp, call->gtControlExpr); + + ContainCheckRange(dispatchControlExprRange); + BlockRange().InsertBefore(call, std::move(dispatchControlExprRange)); + } +#else + assert(!"Unexpected CFGCallKind::Dispatch for platform without dispatcher"); +#endif + break; + } + default: + unreached(); + } +} + #ifndef TARGET_64BIT //------------------------------------------------------------------------ // Lowering::DecomposeLongCompare: Decomposes a TYP_LONG compare node. @@ -3630,10 +3817,6 @@ GenTree* Lowering::LowerDirectCall(GenTreeCall* call) { noway_assert(call->gtCallType == CT_USER_FUNC || call->gtCallType == CT_HELPER); - // Don't support tail calling helper methods. - // But we might encounter tail calls dispatched via JIT helper appear as a tail call to helper. - noway_assert(!call->IsTailCall() || call->IsTailCallViaJitHelper() || call->gtCallType == CT_USER_FUNC); - // Non-virtual direct/indirect calls: Work out if the address of the // call is known at JIT time. If not it is either an indirect call // or the address must be accessed via an single/double indirection. @@ -4793,7 +4976,7 @@ GenTree* Lowering::LowerVirtualStubCall(GenTreeCall* call) // Skip inserting the indirection node to load the address that is already // computed in the VSD stub arg register as a hidden parameter. Instead during the // codegen, just load the call target from there. - shouldOptimizeVirtualStubCall = true; + shouldOptimizeVirtualStubCall = !comp->opts.IsCFGEnabled(); #endif if (!shouldOptimizeVirtualStubCall) diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index ed0ecc5661970..5edaedc0dc884 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -23,7 +23,7 @@ class Lowering final : public Phase { public: inline Lowering(Compiler* compiler, LinearScanInterface* lsra) - : Phase(compiler, PHASE_LOWERING), vtableCallTemp(BAD_VAR_NUM) + : Phase(compiler, PHASE_LOWERING), vtableCallTemp(BAD_VAR_NUM), m_cfgCallTargetTemp(BAD_VAR_NUM) { m_lsra = (LinearScan*)lsra; assert(m_lsra); @@ -123,6 +123,7 @@ class Lowering final : public Phase // Call Lowering // ------------------------------ void LowerCall(GenTree* call); + void LowerCFGCall(GenTreeCall* call); #ifndef TARGET_64BIT GenTree* DecomposeLongCompare(GenTree* cmp); #endif @@ -609,6 +610,7 @@ class Lowering final : public Phase LinearScan* m_lsra; unsigned vtableCallTemp; // local variable we use as a temp for vtable calls + unsigned m_cfgCallTargetTemp; // local variable used to evaluate call target into for CFG calls SideEffectSet m_scratchSideEffects; // SideEffectSet used for IsSafeToContainMem and isRMWIndirCandidate BasicBlock* m_block; }; diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index a48d7e7bdee57..a497add59cdac 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -859,7 +859,7 @@ regMaskTP LinearScan::getKillSetForModDiv(GenTreeOp* node) // regMaskTP LinearScan::getKillSetForCall(GenTreeCall* call) { - regMaskTP killMask = RBM_NONE; + regMaskTP killMask = RBM_CALLEE_TRASH; #ifdef TARGET_X86 if (compiler->compFloatingPointUsed) { @@ -873,38 +873,30 @@ regMaskTP LinearScan::getKillSetForCall(GenTreeCall* call) } } #endif // TARGET_X86 -#if defined(TARGET_X86) || defined(TARGET_ARM) if (call->IsHelperCall()) { CorInfoHelpFunc helpFunc = compiler->eeGetHelperNum(call->gtCallMethHnd); killMask = compiler->compHelperCallKillSet(helpFunc); } - else -#endif // defined(TARGET_X86) || defined(TARGET_ARM) + + // if there is no FP used, we can ignore the FP kills + if (!compiler->compFloatingPointUsed) { - // if there is no FP used, we can ignore the FP kills - if (compiler->compFloatingPointUsed) - { - killMask = RBM_CALLEE_TRASH; - } - else - { - killMask = RBM_INT_CALLEE_TRASH; - } + killMask &= ~RBM_FLT_CALLEE_TRASH; + } #ifdef TARGET_ARM - if (call->IsVirtualStub()) - { - killMask |= compiler->virtualStubParamInfo->GetRegMask(); - } + if (call->IsVirtualStub()) + { + killMask |= compiler->virtualStubParamInfo->GetRegMask(); + } #else // !TARGET_ARM - // Verify that the special virtual stub call registers are in the kill mask. - // We don't just add them unconditionally to the killMask because for most architectures - // they are already in the RBM_CALLEE_TRASH set, - // and we don't want to introduce extra checks and calls in this hot function. - assert(!call->IsVirtualStub() || ((killMask & compiler->virtualStubParamInfo->GetRegMask()) == - compiler->virtualStubParamInfo->GetRegMask())); + // Verify that the special virtual stub call registers are in the kill mask. + // We don't just add them unconditionally to the killMask because for most architectures + // they are already in the RBM_CALLEE_TRASH set, + // and we don't want to introduce extra checks and calls in this hot function. + assert(!call->IsVirtualStub() || + ((killMask & compiler->virtualStubParamInfo->GetRegMask()) == compiler->virtualStubParamInfo->GetRegMask())); #endif // !TARGET_ARM - } return killMask; } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 972cecf7cd2ee..c6a8a223a10b9 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -590,6 +590,8 @@ const char* getNonStandardArgKindName(NonStandardArgKind kind) return "VirtualStubCell"; case NonStandardArgKind::R2RIndirectionCell: return "R2RIndirectionCell"; + case NonStandardArgKind::ValidateIndirectCallTarget: + return "ValidateIndirectCallTarget"; default: unreached(); } @@ -859,6 +861,14 @@ fgArgTabEntry* fgArgInfo::AddRegArg(unsigned argNum, curArgTabEntry->SetByteOffset(0); hasRegArgs = true; + if (argCount >= argTableSize) + { + fgArgTabEntry** oldTable = argTable; + argTable = new (compiler, CMK_fgArgInfoPtrArr) fgArgTabEntry*[argCount + 1]; + memcpy(argTable, oldTable, argCount * sizeof(fgArgTabEntry*)); + argTableSize++; + } + AddArg(curArgTabEntry); return curArgTabEntry; } @@ -1432,6 +1442,29 @@ void fgArgInfo::ArgsComplete() } } + // When CFG is enabled and this is a delegate call or vtable call we must + // compute the call target before all late args. However this will + // effectively null-check 'this', which should happen only after all + // arguments are evaluated. Thus we must evaluate all args with side + // effects to a temp. + if (compiler->opts.IsCFGEnabled() && (callTree->IsVirtualVtable() || callTree->IsDelegateInvoke())) + { + // Always evaluate 'this' to temp. + argTable[0]->needTmp = true; + needsTemps = true; + + for (unsigned curInx = 1; curInx < argCount; curInx++) + { + fgArgTabEntry* curArgTabEntry = argTable[curInx]; + GenTree* arg = curArgTabEntry->GetNode(); + if ((arg->gtFlags & GTF_ALL_EFFECT) != 0) + { + curArgTabEntry->needTmp = true; + needsTemps = true; + } + } + } + argsComplete = true; } @@ -1882,7 +1915,7 @@ void fgArgInfo::EvalArgsToTemps() if (curArgTabEntry->needTmp) { - if (curArgTabEntry->isTmp == true) + if (curArgTabEntry->isTmp) { // Create a copy of the temp to go into the late argument list defArg = compiler->fgMakeTmpArgNode(curArgTabEntry); @@ -2566,6 +2599,14 @@ void Compiler::fgInitArgInfo(GenTreeCall* call) } #endif + if ((REG_VALIDATE_INDIRECT_CALL_ADDR != REG_ARG_0) && call->IsHelperCall(this, CORINFO_HELP_VALIDATE_INDIRECT_CALL)) + { + GenTreeCall::Use* args = call->gtCallArgs; + GenTree* tar = args->GetNode(); + assert(tar != nullptr); + nonStandardArgs.Add(tar, REG_VALIDATE_INDIRECT_CALL_ADDR, NonStandardArgKind::ValidateIndirectCallTarget); + } + // Allocate the fgArgInfo for the call node; // call->fgArgInfo = new (this, CMK_Unknown) fgArgInfo(this, call, numArgs); diff --git a/src/coreclr/jit/targetamd64.h b/src/coreclr/jit/targetamd64.h index 77df2d8547355..50ced88dcb88a 100644 --- a/src/coreclr/jit/targetamd64.h +++ b/src/coreclr/jit/targetamd64.h @@ -390,6 +390,10 @@ // The registers trashed by the CORINFO_HELP_INIT_PINVOKE_FRAME helper. #define RBM_INIT_PINVOKE_FRAME_TRASH RBM_CALLEE_TRASH + #define RBM_VALIDATE_INDIRECT_CALL_TRASH (RBM_INT_CALLEE_TRASH & ~(RBM_R10 | RBM_RCX)) + #define REG_VALIDATE_INDIRECT_CALL_ADDR REG_RCX + #define REG_DISPATCH_INDIRECT_CALL_ADDR REG_RAX + // What sort of reloc do we use for [disp32] address mode #define IMAGE_REL_BASED_DISP32 IMAGE_REL_BASED_REL32 diff --git a/src/coreclr/jit/targetarm.h b/src/coreclr/jit/targetarm.h index f0545335231e8..fd68c91295594 100644 --- a/src/coreclr/jit/targetarm.h +++ b/src/coreclr/jit/targetarm.h @@ -240,6 +240,9 @@ // The registers trashed by the CORINFO_HELP_INIT_PINVOKE_FRAME helper. #define RBM_INIT_PINVOKE_FRAME_TRASH (RBM_CALLEE_TRASH | RBM_PINVOKE_TCB | RBM_PINVOKE_SCRATCH) + #define RBM_VALIDATE_INDIRECT_CALL_TRASH (RBM_INT_CALLEE_TRASH) + #define REG_VALIDATE_INDIRECT_CALL_ADDR REG_R0 + #define REG_FPBASE REG_R11 #define RBM_FPBASE RBM_R11 #define STR_FPBASE "r11" diff --git a/src/coreclr/jit/targetarm64.h b/src/coreclr/jit/targetarm64.h index cdab21582ffce..a4c51818a9e04 100644 --- a/src/coreclr/jit/targetarm64.h +++ b/src/coreclr/jit/targetarm64.h @@ -248,6 +248,10 @@ // The registers trashed by the CORINFO_HELP_INIT_PINVOKE_FRAME helper. #define RBM_INIT_PINVOKE_FRAME_TRASH RBM_CALLEE_TRASH + #define RBM_VALIDATE_INDIRECT_CALL_TRASH (RBM_CALLEE_TRASH & ~(RBM_R0 | RBM_R1 | RBM_R2 | RBM_R3 | RBM_R4 | RBM_R5 | RBM_R6 | RBM_R7 | RBM_R8 | RBM_R15 | RBM_V0 | RBM_V1 | RBM_V2 | RBM_V3 | RBM_V4 | RBM_V5 | RBM_V6 | RBM_V7)) + #define REG_VALIDATE_INDIRECT_CALL_ADDR REG_R15 + #define REG_DISPATCH_INDIRECT_CALL_ADDR REG_R15 + #define REG_FPBASE REG_FP #define RBM_FPBASE RBM_FP #define STR_FPBASE "fp" diff --git a/src/coreclr/jit/targetx86.h b/src/coreclr/jit/targetx86.h index 776c9deece36e..ec85659d63cbc 100644 --- a/src/coreclr/jit/targetx86.h +++ b/src/coreclr/jit/targetx86.h @@ -228,6 +228,9 @@ // convention that takes EDI as argument (but doesn't trash it), trashes EAX, and returns ESI. #define RBM_INIT_PINVOKE_FRAME_TRASH (RBM_PINVOKE_SCRATCH | RBM_PINVOKE_TCB) + #define RBM_VALIDATE_INDIRECT_CALL_TRASH (RBM_INT_CALLEE_TRASH & ~RBM_ECX) + #define REG_VALIDATE_INDIRECT_CALL_ADDR REG_ECX + #define REG_FPBASE REG_EBP #define RBM_FPBASE RBM_EBP #define STR_FPBASE "ebp" diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs b/src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs index 6d8a5ec684f52..0e5f836ff2110 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs @@ -295,6 +295,14 @@ which is the right helper to use to allocate an object of a given type. */ CORINFO_HELP_STACK_PROBE, // Probes each page of the allocated stack frame + CORINFO_HELP_PATCHPOINT, // Notify runtime that code has reached a patchpoint + CORINFO_HELP_CLASSPROFILE32, // Update 32-bit class profile for a call site + CORINFO_HELP_CLASSPROFILE64, // Update 64-bit class profile for a call site + CORINFO_HELP_PARTIAL_COMPILATION_PATCHPOINT, // Notify runtime that code has reached a part of the method that wasn't originally jitted. + + CORINFO_HELP_VALIDATE_INDIRECT_CALL, // CFG: Validate function pointer + CORINFO_HELP_DISPATCH_INDIRECT_CALL, // CFG: Validate and dispatch to pointer + CORINFO_HELP_COUNT, } } diff --git a/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs b/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs index 4a7d254d583fc..ef6e8a483fb2b 100644 --- a/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs +++ b/src/coreclr/tools/Common/JitInterface/CorInfoTypes.cs @@ -1334,7 +1334,7 @@ public enum CorJitFlag : uint CORJIT_FLAG_DEBUG_EnC = 3, // We are in Edit-n-Continue mode CORJIT_FLAG_DEBUG_INFO = 4, // generate line and local-var info CORJIT_FLAG_MIN_OPT = 5, // disable all jit optimizations (not necesarily debuggable code) - CORJIT_FLAG_UNUSED1 = 6, + CORJIT_FLAG_ENABLE_CFG = 6, // generate CFG enabled code CORJIT_FLAG_MCJIT_BACKGROUND = 7, // Calling from multicore JIT background thread, do not call JitComplete CORJIT_FLAG_UNUSED2 = 8, CORJIT_FLAG_UNUSED3 = 9, diff --git a/src/coreclr/vm/amd64/JitHelpers_Fast.asm b/src/coreclr/vm/amd64/JitHelpers_Fast.asm index 219597eb350c2..0060ff036d3b0 100644 --- a/src/coreclr/vm/amd64/JitHelpers_Fast.asm +++ b/src/coreclr/vm/amd64/JitHelpers_Fast.asm @@ -436,4 +436,17 @@ ProbeLoop: LEAF_END_MARKED JIT_StackProbe, _TEXT +LEAF_ENTRY JIT_ValidateIndirectCall, _TEXT + ret +LEAF_END JIT_ValidateIndirectCall, _TEXT + +LEAF_ENTRY JIT_DispatchIndirectCall, _TEXT +ifdef _DEBUG + mov r10, 0CDCDCDCDCDCDCDCDh ; The real helper clobbers these registers, so clobber them too in the fake helper + mov r11, 0CDCDCDCDCDCDCDCDh +endif + rexw jmp rax +LEAF_END JIT_DispatchIndirectCall, _TEXT + + end diff --git a/src/coreclr/vm/amd64/jithelpers_fast.S b/src/coreclr/vm/amd64/jithelpers_fast.S index 8109886d0c969..42eb50836a526 100644 --- a/src/coreclr/vm/amd64/jithelpers_fast.S +++ b/src/coreclr/vm/amd64/jithelpers_fast.S @@ -447,3 +447,13 @@ LOCAL_LABEL(ProbeLoop): ret LEAF_END_MARKED JIT_StackProbe, _TEXT + +LEAF_ENTRY JIT_ValidateIndirectCall, _TEST + ret +LEAF_END JIT_ValidateIndirectCall, _TEST + +LEAF_ENTRY JIT_DispatchIndirectCall, _TEST + mov r10, 0xCDCDCDCDCDCDCDCD // The real helper clobbers these registers, so clobber them too in the fake helper + mov r11, 0xCDCDCDCDCDCDCDCD + rex64 jmp rax +LEAF_END JIT_DispatchIndirectCall, _TEST diff --git a/src/coreclr/vm/arm64/asmhelpers.S b/src/coreclr/vm/arm64/asmhelpers.S index e7725cd16e63b..941af8f3d00ca 100644 --- a/src/coreclr/vm/arm64/asmhelpers.S +++ b/src/coreclr/vm/arm64/asmhelpers.S @@ -1020,3 +1020,11 @@ NESTED_ENTRY OnCallCountThresholdReachedStub, _TEXT, NoHandler NESTED_END OnCallCountThresholdReachedStub, _TEXT #endif // FEATURE_TIERED_COMPILATION + +LEAF_ENTRY JIT_ValidateIndirectCall, _TEXT + ret lr +LEAF_END JIT_ValidateIndirectCall, _TEXT + +LEAF_ENTRY JIT_DispatchIndirectCall, _TEXT + br x15 +LEAF_END JIT_DispatchIndirectCall, _TEXT diff --git a/src/coreclr/vm/arm64/asmhelpers.asm b/src/coreclr/vm/arm64/asmhelpers.asm index 6bbdef2c2d936..d73b8f5ff6ff7 100644 --- a/src/coreclr/vm/arm64/asmhelpers.asm +++ b/src/coreclr/vm/arm64/asmhelpers.asm @@ -1435,5 +1435,13 @@ __HelperNakedFuncName SETS "$helper":CC:"Naked" #endif ; FEATURE_TIERED_COMPILATION + LEAF_ENTRY JIT_ValidateIndirectCall + ret lr + LEAF_END + + LEAF_ENTRY JIT_DispatchIndirectCall + br x15 + LEAF_END + ; Must be at very end of file END diff --git a/src/coreclr/vm/jithelpers.cpp b/src/coreclr/vm/jithelpers.cpp index 109f98ceecfe1..bfbd359a36b1e 100644 --- a/src/coreclr/vm/jithelpers.cpp +++ b/src/coreclr/vm/jithelpers.cpp @@ -5677,6 +5677,10 @@ HCIMPL1_RAW(void, JIT_ReversePInvokeExit, ReversePInvokeFrame* frame) } HCIMPLEND_RAW +// These two do take args but have a custom calling convention. +EXTERN_C void JIT_ValidateIndirectCall(); +EXTERN_C void JIT_DispatchIndirectCall(); + //======================================================================== // // JIT HELPERS INITIALIZATION diff --git a/src/tests/Common/testenvironment.proj b/src/tests/Common/testenvironment.proj index aae081a187126..e03e28cc6309f 100644 --- a/src/tests/Common/testenvironment.proj +++ b/src/tests/Common/testenvironment.proj @@ -63,6 +63,8 @@ COMPlus_JitRandomGuardedDevirtualization; COMPlus_JitRandomEdgeCounts; COMPlus_JitRandomOnStackReplacement; + COMPlus_JitForceControlFlowGuard; + COMPlus_JitCFGUseDispatcher; RunningIlasmRoundTrip @@ -195,6 +197,9 @@ + + + From dfa28e1303495d7264710874e64311b21360dc67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 19 Jan 2022 16:52:26 +0900 Subject: [PATCH 02/30] Pipe through the flag to NativeAOT compiler --- src/coreclr/nativeaot/Runtime/amd64/MiscStubs.asm | 12 ++++++++++++ .../Compiler/DependencyAnalysis/ObjectWriter.cs | 2 +- .../Compiler/RyuJitCompilationBuilder.cs | 3 +++ .../JitInterface/CorInfoImpl.RyuJit.cs | 5 +++++ 4 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/coreclr/nativeaot/Runtime/amd64/MiscStubs.asm b/src/coreclr/nativeaot/Runtime/amd64/MiscStubs.asm index 7331a258b94e9..4d418614f36ea 100644 --- a/src/coreclr/nativeaot/Runtime/amd64/MiscStubs.asm +++ b/src/coreclr/nativeaot/Runtime/amd64/MiscStubs.asm @@ -9,6 +9,18 @@ EXTERN memcpyGCRefsWithWriteBarrier : PROC EXTERN memcpyAnyWithWriteBarrier : PROC EXTERN RhpGetThreadStaticBaseForTypeSlow : PROC +EXTERN __guard_check_icall_fptr : QWORD +EXTERN __guard_dispatch_icall_fptr : QWORD + +LEAF_ENTRY __guard_check_icall_fptrHack, _TEXT + jmp qword ptr [__guard_check_icall_fptr] +LEAF_END __guard_check_icall_fptrHack, _TEXT + +LEAF_ENTRY __guard_dispatch_icall_fptrHack, _TEXT + jmp qword ptr [__guard_dispatch_icall_fptr] +LEAF_END __guard_dispatch_icall_fptrHack, _TEXT + + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; ;; void* RhpCopyMultibyteNoGCRefs(void*, void*, size_t) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectWriter.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectWriter.cs index e690b5d420078..3232726b49337 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectWriter.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ObjectWriter.cs @@ -792,7 +792,7 @@ public int EmitSymbolReference(ISymbolNode target, int delta, RelocType relocTyp // For now consider all method symbols address taken. // We could restrict this in the future to those that are referenced from // reflection tables, EH tables, were actually address taken in code, or are referenced from vtables. - if ((_options & ObjectWritingOptions.ControlFlowGuard) != 0 && target is IMethodNode) + if ((_options & ObjectWritingOptions.ControlFlowGuard) != 0 && (target is IMethodNode || target is AssemblyStubNode)) { flags |= SymbolRefFlags.AddressTakenFunction; } diff --git a/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilationBuilder.cs b/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilationBuilder.cs index 8b753c7103994..046fef4d50c9e 100644 --- a/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilationBuilder.cs +++ b/src/coreclr/tools/aot/ILCompiler.RyuJit/Compiler/RyuJitCompilationBuilder.cs @@ -108,7 +108,10 @@ public override ICompilation ToCompilation() options |= RyuJitCompilationOptions.MethodBodyFolding; if ((_mitigationOptions & SecurityMitigationOptions.ControlFlowGuardAnnotations) != 0) + { + jitFlagBuilder.Add(CorJitFlag.CORJIT_FLAG_ENABLE_CFG); options |= RyuJitCompilationOptions.ControlFlowGuardAnnotations; + } var factory = new RyuJitNodeFactory(_context, _compilationGroup, _metadataManager, _interopStubManager, _nameMangler, _vtableSliceProvider, _dictionaryLayoutProvider, GetPreinitializationManager()); diff --git a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs index 5966efead0066..f1cf8a1de8752 100644 --- a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs +++ b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs @@ -692,6 +692,11 @@ private ISymbolNode GetHelperFtnUncached(CorInfoHelpFunc ftnNum) id = ReadyToRunHelper.GetCurrentManagedThreadId; break; + case CorInfoHelpFunc.CORINFO_HELP_VALIDATE_INDIRECT_CALL: + return _compilation.NodeFactory.ExternSymbol("__guard_check_icall_fptrHack"); + case CorInfoHelpFunc.CORINFO_HELP_DISPATCH_INDIRECT_CALL: + return _compilation.NodeFactory.ExternSymbol("__guard_dispatch_icall_fptrHack"); + default: throw new NotImplementedException(ftnNum.ToString()); } From e07386fd1430a546cdf7fe01db84ea3637c0f8da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Wed, 19 Jan 2022 17:42:00 +0900 Subject: [PATCH 03/30] Represent the helper as an indirection --- src/coreclr/nativeaot/Runtime/amd64/MiscStubs.asm | 12 ------------ .../Compiler/DependencyAnalysis/ExternSymbolNode.cs | 12 +++++++----- .../Compiler/DependencyAnalysis/NodeFactory.cs | 11 +++++++++++ .../JitInterface/CorInfoImpl.RyuJit.cs | 4 ++-- 4 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/amd64/MiscStubs.asm b/src/coreclr/nativeaot/Runtime/amd64/MiscStubs.asm index 4d418614f36ea..7331a258b94e9 100644 --- a/src/coreclr/nativeaot/Runtime/amd64/MiscStubs.asm +++ b/src/coreclr/nativeaot/Runtime/amd64/MiscStubs.asm @@ -9,18 +9,6 @@ EXTERN memcpyGCRefsWithWriteBarrier : PROC EXTERN memcpyAnyWithWriteBarrier : PROC EXTERN RhpGetThreadStaticBaseForTypeSlow : PROC -EXTERN __guard_check_icall_fptr : QWORD -EXTERN __guard_dispatch_icall_fptr : QWORD - -LEAF_ENTRY __guard_check_icall_fptrHack, _TEXT - jmp qword ptr [__guard_check_icall_fptr] -LEAF_END __guard_check_icall_fptrHack, _TEXT - -LEAF_ENTRY __guard_dispatch_icall_fptrHack, _TEXT - jmp qword ptr [__guard_dispatch_icall_fptr] -LEAF_END __guard_dispatch_icall_fptrHack, _TEXT - - ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; ;; void* RhpCopyMultibyteNoGCRefs(void*, void*, size_t) diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ExternSymbolNode.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ExternSymbolNode.cs index 8f0e239fc1fb6..721e983ef5640 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ExternSymbolNode.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ExternSymbolNode.cs @@ -14,22 +14,24 @@ namespace ILCompiler.DependencyAnalysis /// Represents a symbol that is defined externally and statically linked to the output obj file. /// public class ExternSymbolNode : SortableDependencyNode, ISortableSymbolNode - { - private Utf8String _name; + { + private readonly Utf8String _name; + private readonly bool _isIndirection; - public ExternSymbolNode(Utf8String name) + public ExternSymbolNode(Utf8String name, bool isIndirection = false) { _name = name; + _isIndirection = isIndirection; } - protected override string GetName(NodeFactory factory) => $"ExternSymbol {_name.ToString()}"; + protected override string GetName(NodeFactory factory) => $"ExternSymbol {_name.ToString()}{(_isIndirection ? " (indirected)" : "")}"; public void AppendMangledName(NameMangler nameMangler, Utf8StringBuilder sb) { sb.Append(_name); } public int Offset => 0; - public virtual bool RepresentsIndirectionCell => false; + public virtual bool RepresentsIndirectionCell => _isIndirection; public override bool InterestingForDynamicDependencyAnalysis => false; public override bool HasDynamicDependencies => false; diff --git a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs index 67469a0f55cc6..24b45f2f1c7ed 100644 --- a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs +++ b/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NodeFactory.cs @@ -245,6 +245,10 @@ private void CreateNodeCaches() { return new ExternSymbolNode(name); }); + _externIndirectSymbols = new NodeCache((string name) => + { + return new ExternSymbolNode(name, isIndirection: true); + }); _pInvokeModuleFixups = new NodeCache((PInvokeModuleData moduleData) => { @@ -741,6 +745,13 @@ public ISortableSymbolNode ExternSymbol(string name) return _externSymbols.GetOrAdd(name); } + private NodeCache _externIndirectSymbols; + + public ISortableSymbolNode ExternIndirectSymbol(string name) + { + return _externIndirectSymbols.GetOrAdd(name); + } + private NodeCache _pInvokeModuleFixups; public ISymbolNode PInvokeModuleFixup(PInvokeModuleData moduleData) diff --git a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs index f1cf8a1de8752..d0507d25dc1de 100644 --- a/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs +++ b/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs @@ -693,9 +693,9 @@ private ISymbolNode GetHelperFtnUncached(CorInfoHelpFunc ftnNum) break; case CorInfoHelpFunc.CORINFO_HELP_VALIDATE_INDIRECT_CALL: - return _compilation.NodeFactory.ExternSymbol("__guard_check_icall_fptrHack"); + return _compilation.NodeFactory.ExternIndirectSymbol("__guard_check_icall_fptr"); case CorInfoHelpFunc.CORINFO_HELP_DISPATCH_INDIRECT_CALL: - return _compilation.NodeFactory.ExternSymbol("__guard_dispatch_icall_fptrHack"); + return _compilation.NodeFactory.ExternIndirectSymbol("__guard_dispatch_icall_fptr"); default: throw new NotImplementedException(ftnNum.ToString()); From fb293b9536b745dc70a7ff9b8f35cc7980676217 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jan 2022 13:26:14 +0100 Subject: [PATCH 04/30] Add some verbose output for moved late args --- src/coreclr/jit/lower.cpp | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 3a1447a5193ee..90823671f0dbc 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2322,33 +2322,47 @@ void Lowering::LowerCFGCall(GenTreeCall* call) BlockRange().InsertAfter(validate, regNode); LowerNode(regNode); - // Finally move all GT_PUTARG_REG nodes that the validate call may clobber to after validation. - const regMaskTP clobberedArgRegs = + // Finally move all GT_PUTARG_REG nodes that the validate call may trash to after validation. + const regMaskTP trashedByValidator = RBM_VALIDATE_INDIRECT_CALL_TRASH | genRegMask(REG_VALIDATE_INDIRECT_CALL_ADDR); - if ((clobberedArgRegs & RBM_ARG_REGS) != 0) + if ((trashedByValidator & RBM_ARG_REGS) != 0) { for (GenTreeCall::Use& use : call->LateArgs()) { GenTree* node = use.GetNode(); + JITDUMP("Checking whether late arg will be trashed by validator:\n"); + DISPTREE(node); assert(node->OperIsPutArg()); fgArgTabEntry* entry = comp->gtArgEntryByNode(call, node); - bool isAnyRegClobbered = false; + regMaskTP argRegs = 0; for (unsigned i = 0; i < entry->numRegs; i++) { - if ((genRegMask(entry->GetRegNum(i)) & clobberedArgRegs) != 0) - { - isAnyRegClobbered = true; - break; - } + argRegs |= genRegMask(entry->GetRegNum(i)); + } + + JITDUMP("Arg uses register(s) "); + if (comp->verbose) + { + dspRegMask(argRegs & trashedByValidator); } + JITDUMP("\n"); // If validate call will clobber any register used by this - // arg then place the arg after the call instead. - if (isAnyRegClobbered) + // arg then place the GT_PUTARG_REG after the call instead. + if ((argRegs & trashedByValidator) != 0) { + JITDUMP("CFG validator will trash register(s) "); + if (comp->verbose) + { + dspRegMask(argRegs & trashedByValidator); + } + JITDUMP("; moving the placement to after the validator call\n"); + BlockRange().Remove(node); BlockRange().InsertBefore(call, node); } + + JITDUMP("\n"); } } break; From 4665ea00a5766dadb80efcfbbe4e75e3212201bb Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jan 2022 13:38:33 +0100 Subject: [PATCH 05/30] Add GT_FIELD_LIST to assert --- src/coreclr/jit/lower.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 90823671f0dbc..a24b5ede23ce7 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2332,7 +2332,7 @@ void Lowering::LowerCFGCall(GenTreeCall* call) GenTree* node = use.GetNode(); JITDUMP("Checking whether late arg will be trashed by validator:\n"); DISPTREE(node); - assert(node->OperIsPutArg()); + assert(node->OperIsPutArg() || node->OperIs(GT_FIELD_LIST)); fgArgTabEntry* entry = comp->gtArgEntryByNode(call, node); regMaskTP argRegs = 0; for (unsigned i = 0; i < entry->numRegs; i++) From b19e47dafaf2f4fb3d23e0b27a932c89101781c4 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jan 2022 13:52:07 +0100 Subject: [PATCH 06/30] Fix placement moving logic for GT_FIELD_LIST --- src/coreclr/jit/gentree.h | 5 +++++ src/coreclr/jit/lower.cpp | 40 +++++++++++++++++++++++++++++++++++---- src/coreclr/jit/lower.h | 1 + 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index dc411644664b4..aeb43c7d0b8ca 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -1298,6 +1298,11 @@ struct GenTree return OperIsPutArgStk() || OperIsPutArgReg() || OperIsPutArgSplit(); } + bool OperIsFieldList() const + { + return OperIs(GT_FIELD_LIST); + } + bool OperIsMultiRegOp() const { #if !defined(TARGET_64BIT) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index a24b5ede23ce7..2d4972ed0b4ba 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2332,7 +2332,7 @@ void Lowering::LowerCFGCall(GenTreeCall* call) GenTree* node = use.GetNode(); JITDUMP("Checking whether late arg will be trashed by validator:\n"); DISPTREE(node); - assert(node->OperIsPutArg() || node->OperIs(GT_FIELD_LIST)); + assert(node->OperIsPutArg() || node->OperIsFieldList()); fgArgTabEntry* entry = comp->gtArgEntryByNode(call, node); regMaskTP argRegs = 0; for (unsigned i = 0; i < entry->numRegs; i++) @@ -2356,10 +2356,9 @@ void Lowering::LowerCFGCall(GenTreeCall* call) { dspRegMask(argRegs & trashedByValidator); } - JITDUMP("; moving the placement to after the validator call\n"); + JITDUMP("\n"); - BlockRange().Remove(node); - BlockRange().InsertBefore(call, node); + MoveCFGCallLateArg(call, node); } JITDUMP("\n"); @@ -2450,6 +2449,39 @@ void Lowering::LowerCFGCall(GenTreeCall* call) } } +void Lowering::MoveCFGCallLateArg(GenTreeCall* call, GenTree* node) +{ + assert(node->OperIsPutArg() || node->OperIsFieldList()); + + if (node->OperIsFieldList()) + { + JITDUMP("Node is a GT_FIELD_LIST; moving all operands\n"); + for (GenTreeFieldList::Use& operand : node->AsFieldList()->Uses()) + { + assert(operand.GetNode()->OperIsPutArg()); + MoveCFGCallLateArg(call, operand.GetNode()); + } + } + else + { + GenTree* operand = node->AsOp()->gtGetOp1(); + // TODO-CQ: Check for interference and move GT_LCL_VAR and GT_LCL_FLD nodes + if (operand->IsInvariant()) + { + JITDUMP("Moving following operand of late arg to after validator call\n"); + DISPTREE(operand); + BlockRange().Remove(operand); + BlockRange().InsertBefore(call, operand); + } + } + + JITDUMP("Moving\n"); + DISPTREE(node); + JITDUMP("\n"); + BlockRange().Remove(node); + BlockRange().InsertBefore(call, node); +} + #ifndef TARGET_64BIT //------------------------------------------------------------------------ // Lowering::DecomposeLongCompare: Decomposes a TYP_LONG compare node. diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 5edaedc0dc884..faf4b57a42aa2 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -124,6 +124,7 @@ class Lowering final : public Phase // ------------------------------ void LowerCall(GenTree* call); void LowerCFGCall(GenTreeCall* call); + void MoveCFGCallLateArg(GenTreeCall* call, GenTree* node); #ifndef TARGET_64BIT GenTree* DecomposeLongCompare(GenTree* cmp); #endif From 0a221c535040a3c442ad271082320034274ee528 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jan 2022 14:58:26 +0100 Subject: [PATCH 07/30] Add more general invariance check --- src/coreclr/jit/lower.cpp | 60 +++++++++++++++++++++++++++++++++++---- src/coreclr/jit/lower.h | 2 ++ 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 2d4972ed0b4ba..defc0c4e0c3cf 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2333,8 +2333,8 @@ void Lowering::LowerCFGCall(GenTreeCall* call) JITDUMP("Checking whether late arg will be trashed by validator:\n"); DISPTREE(node); assert(node->OperIsPutArg() || node->OperIsFieldList()); - fgArgTabEntry* entry = comp->gtArgEntryByNode(call, node); - regMaskTP argRegs = 0; + fgArgTabEntry* entry = comp->gtArgEntryByNode(call, node); + regMaskTP argRegs = 0; for (unsigned i = 0; i < entry->numRegs; i++) { argRegs |= genRegMask(entry->GetRegNum(i)); @@ -2449,6 +2449,50 @@ void Lowering::LowerCFGCall(GenTreeCall* call) } } +//------------------------------------------------------------------------ +// IsInvariant: Check if a node is invariant in the specified range. In other +// words, can 'node' be moved to before 'endExclusive' without its computation +// changing values? +// +// Arguments: +// node - The node. +// endExclusive - The exclusive end of the range to check invariance for. +// +bool Lowering::IsInvariant(GenTree* node, GenTree* endExclusive) +{ + assert(node->Precedes(endExclusive)); + + if (node->IsInvariant()) + { + return true; + } + + if (!node->IsValue()) + { + return false; + } + + if (node->OperIsLocal()) + { + GenTreeLclVarCommon* lcl = node->AsLclVarCommon(); + LclVarDsc* desc = comp->lvaGetDesc(lcl); + if (desc->IsAddressExposed()) + { + return false; + } + + for (GenTree* intermediate = node->gtNext; intermediate != endExclusive; intermediate = intermediate->gtNext) + { + if (intermediate->OperIsLocalStore() && intermediate->AsLclVarCommon()->GetLclNum() == lcl->GetLclNum()) + { + return false; + } + } + } + + return false; +} + void Lowering::MoveCFGCallLateArg(GenTreeCall* call, GenTree* node) { assert(node->OperIsPutArg() || node->OperIsFieldList()); @@ -2465,14 +2509,18 @@ void Lowering::MoveCFGCallLateArg(GenTreeCall* call, GenTree* node) else { GenTree* operand = node->AsOp()->gtGetOp1(); - // TODO-CQ: Check for interference and move GT_LCL_VAR and GT_LCL_FLD nodes - if (operand->IsInvariant()) + JITDUMP("Checking if we can move following operand together with the GT_PUTARG_REG\n"); + DISPTREE(operand); + if (((operand->gtFlags & GTF_ALL_EFFECT) == 0) && IsInvariant(operand, call)) { - JITDUMP("Moving following operand of late arg to after validator call\n"); - DISPTREE(operand); + JITDUMP("...yes, moving to after validator call\n"); BlockRange().Remove(operand); BlockRange().InsertBefore(call, operand); } + else + { + JITDUMP("...no, operand has side effects or is not invariant\n"); + } } JITDUMP("Moving\n"); diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index faf4b57a42aa2..a6f0801432143 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -119,6 +119,8 @@ class Lowering final : public Phase void LowerBlock(BasicBlock* block); GenTree* LowerNode(GenTree* node); + bool IsInvariant(GenTree* node, GenTree* endExclusive); + // ------------------------------ // Call Lowering // ------------------------------ From 6f68c08a23f88fb5a0221b5d66fb3fae42166ca0 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jan 2022 15:42:27 +0100 Subject: [PATCH 08/30] Fix release build --- src/coreclr/jit/lower.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index defc0c4e0c3cf..070c9846277a1 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2343,7 +2343,7 @@ void Lowering::LowerCFGCall(GenTreeCall* call) JITDUMP("Arg uses register(s) "); if (comp->verbose) { - dspRegMask(argRegs & trashedByValidator); + INDEBUG(dspRegMask(argRegs & trashedByValidator)); } JITDUMP("\n"); @@ -2354,7 +2354,7 @@ void Lowering::LowerCFGCall(GenTreeCall* call) JITDUMP("CFG validator will trash register(s) "); if (comp->verbose) { - dspRegMask(argRegs & trashedByValidator); + INDEBUG(dspRegMask(argRegs & trashedByValidator)); } JITDUMP("\n"); From acf223b7282567f7aa5a6b748c45e258941079a9 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jan 2022 15:42:40 +0100 Subject: [PATCH 09/30] Fix IsInvariant check --- src/coreclr/jit/lower.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 070c9846277a1..67bf212636fe0 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2488,6 +2488,8 @@ bool Lowering::IsInvariant(GenTree* node, GenTree* endExclusive) return false; } } + + return true; } return false; From ef465fba9d6d913788bda55600bacad92c9d3910 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jan 2022 16:19:11 +0100 Subject: [PATCH 10/30] Add a dTreeRange helper --- src/coreclr/jit/compiler.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 56701ceb08fad..2fa08d07ddd8b 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -8853,6 +8853,20 @@ void dTreeLIR(GenTree* tree) cTreeLIR(JitTls::GetCompiler(), tree); } +void dTreeRange(GenTree* first, GenTree* last) +{ + Compiler* comp = JitTls::GetCompiler(); + GenTree* cur = first; + while (true) + { + cTreeLIR(comp, cur); + if (cur == last) + break; + + cur = cur->gtNext; + } +} + void dTrees() { cTrees(JitTls::GetCompiler()); From a7a641556165885444cf3f9d73d0f0ce1e247505 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jan 2022 16:19:15 +0100 Subject: [PATCH 11/30] Skip local for call target --- src/coreclr/jit/lower.cpp | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 67bf212636fe0..411cc14f8e7e4 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2286,27 +2286,11 @@ void Lowering::LowerCFGCall(GenTreeCall* call) bool gotUse = BlockRange().TryGetUse(callTarget, &useOfTar); assert(gotUse); useOfTar.ReplaceWith(regNode); - if (callTarget->OperIs(GT_LCL_VAR) || callTarget->OperIs(GT_LCL_FLD) || callTarget->IsIntegralConst()) - { - callTarget->SetUnusedValue(); - callTarget = comp->gtClone(callTarget, false); - } - else - { - if (m_cfgCallTargetTemp == BAD_VAR_NUM) - { - m_cfgCallTargetTemp = comp->lvaGrabTemp(true DEBUGARG("CFG validate call target")); - } - GenTree* assign = comp->gtNewTempAssign(m_cfgCallTargetTemp, callTarget); - BlockRange().InsertBefore(call, assign); - LowerNode(assign); - - callTarget = comp->gtNewLclvNode(m_cfgCallTargetTemp, callTarget->TypeGet()); - } - - // Add the call to the validator - GenTreeCall::Use* args = comp->gtNewCallArgs(callTarget); + GenTree* targetPlaceholder = comp->gtNewZeroConNode(callTarget->TypeGet()); + // Add the call to the validator. Use a placeholder for the target while we + // morph, sequence and lower, to avoid redoing that for the actual target. + GenTreeCall::Use* args = comp->gtNewCallArgs(targetPlaceholder); GenTreeCall* validate = comp->gtNewHelperCallNode(CORINFO_HELP_VALIDATE_INDIRECT_CALL, TYP_VOID, args); comp->fgMorphTree(validate); @@ -2316,6 +2300,13 @@ void Lowering::LowerCFGCall(GenTreeCall* call) GenTree* validateLast = validateRange.LastNode(); // Insert the validator with the call target before the late args. BlockRange().InsertBefore(call, std::move(validateRange)); + + // Swap out the target + gotUse = BlockRange().TryGetUse(targetPlaceholder, &useOfTar); + assert(gotUse); + useOfTar.ReplaceWith(callTarget); + targetPlaceholder->SetUnusedValue(); + LowerRange(validateFirst, validateLast); // Insert the PHYSREG node that we must load right after validation. From 17e22f04751dacbc42a77c177e46e311c78ea921 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jan 2022 16:22:00 +0100 Subject: [PATCH 12/30] Add missing parentheses --- src/coreclr/jit/lower.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 411cc14f8e7e4..066436a3b2b60 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2474,7 +2474,7 @@ bool Lowering::IsInvariant(GenTree* node, GenTree* endExclusive) for (GenTree* intermediate = node->gtNext; intermediate != endExclusive; intermediate = intermediate->gtNext) { - if (intermediate->OperIsLocalStore() && intermediate->AsLclVarCommon()->GetLclNum() == lcl->GetLclNum()) + if (intermediate->OperIsLocalStore() && (intermediate->AsLclVarCommon()->GetLclNum() == lcl->GetLclNum())) { return false; } From 21e58588c8355f9ccf3ab4441719ab4a1affa4c8 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jan 2022 16:22:47 +0100 Subject: [PATCH 13/30] Rename IsInvariant -> IsInvariantInRange --- src/coreclr/jit/lower.cpp | 10 +++++----- src/coreclr/jit/lower.h | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 066436a3b2b60..b29dfa3b93b84 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2441,15 +2441,15 @@ void Lowering::LowerCFGCall(GenTreeCall* call) } //------------------------------------------------------------------------ -// IsInvariant: Check if a node is invariant in the specified range. In other -// words, can 'node' be moved to before 'endExclusive' without its computation -// changing values? +// IsInvariantInRange: Check if a node is invariant in the specified range. In +// other words, can 'node' be moved to before 'endExclusive' without its +// computation changing values? // // Arguments: // node - The node. // endExclusive - The exclusive end of the range to check invariance for. // -bool Lowering::IsInvariant(GenTree* node, GenTree* endExclusive) +bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive) { assert(node->Precedes(endExclusive)); @@ -2504,7 +2504,7 @@ void Lowering::MoveCFGCallLateArg(GenTreeCall* call, GenTree* node) GenTree* operand = node->AsOp()->gtGetOp1(); JITDUMP("Checking if we can move following operand together with the GT_PUTARG_REG\n"); DISPTREE(operand); - if (((operand->gtFlags & GTF_ALL_EFFECT) == 0) && IsInvariant(operand, call)) + if (((operand->gtFlags & GTF_ALL_EFFECT) == 0) && IsInvariantInRange(operand, call)) { JITDUMP("...yes, moving to after validator call\n"); BlockRange().Remove(operand); diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index a6f0801432143..5a0cc9d1fa452 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -119,7 +119,7 @@ class Lowering final : public Phase void LowerBlock(BasicBlock* block); GenTree* LowerNode(GenTree* node); - bool IsInvariant(GenTree* node, GenTree* endExclusive); + bool IsInvariantInRange(GenTree* node, GenTree* endExclusive); // ------------------------------ // Call Lowering From 8baddf0245d8cdde6ef5f77190c59c86a91c75dd Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 20 Jan 2022 17:58:20 +0100 Subject: [PATCH 14/30] Another build fix --- src/coreclr/jit/lower.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index b29dfa3b93b84..cf2fd68a81333 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2332,10 +2332,12 @@ void Lowering::LowerCFGCall(GenTreeCall* call) } JITDUMP("Arg uses register(s) "); +#ifdef DEBUG if (comp->verbose) { - INDEBUG(dspRegMask(argRegs & trashedByValidator)); + dspRegMask(argRegs & trashedByValidator); } +#endif JITDUMP("\n"); // If validate call will clobber any register used by this @@ -2343,10 +2345,12 @@ void Lowering::LowerCFGCall(GenTreeCall* call) if ((argRegs & trashedByValidator) != 0) { JITDUMP("CFG validator will trash register(s) "); +#ifdef DEBUG if (comp->verbose) { - INDEBUG(dspRegMask(argRegs & trashedByValidator)); + dspRegMask(argRegs & trashedByValidator); } +#endif JITDUMP("\n"); MoveCFGCallLateArg(call, node); From 092364e9b2de95d0b6ec30a6ca5a937659385fc1 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 21 Jan 2022 15:14:42 +0100 Subject: [PATCH 15/30] Fix register for ARM64 dispatcher --- src/coreclr/jit/targetarm64.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/targetarm64.h b/src/coreclr/jit/targetarm64.h index a4c51818a9e04..8bdb5e732ba41 100644 --- a/src/coreclr/jit/targetarm64.h +++ b/src/coreclr/jit/targetarm64.h @@ -250,7 +250,7 @@ #define RBM_VALIDATE_INDIRECT_CALL_TRASH (RBM_CALLEE_TRASH & ~(RBM_R0 | RBM_R1 | RBM_R2 | RBM_R3 | RBM_R4 | RBM_R5 | RBM_R6 | RBM_R7 | RBM_R8 | RBM_R15 | RBM_V0 | RBM_V1 | RBM_V2 | RBM_V3 | RBM_V4 | RBM_V5 | RBM_V6 | RBM_V7)) #define REG_VALIDATE_INDIRECT_CALL_ADDR REG_R15 - #define REG_DISPATCH_INDIRECT_CALL_ADDR REG_R15 + #define REG_DISPATCH_INDIRECT_CALL_ADDR REG_R9 #define REG_FPBASE REG_FP #define RBM_FPBASE RBM_FP From ce46dcf01efc19a32eb2db5d0c367c1334a90371 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 21 Jan 2022 15:15:01 +0100 Subject: [PATCH 16/30] Add section on CFG to the clr-abi docs --- docs/design/coreclr/botr/clr-abi.md | 39 +++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/docs/design/coreclr/botr/clr-abi.md b/docs/design/coreclr/botr/clr-abi.md index f7b01352de922..b78eb09bc7260 100644 --- a/docs/design/coreclr/botr/clr-abi.md +++ b/docs/design/coreclr/botr/clr-abi.md @@ -752,3 +752,42 @@ The return value is handled as follows: 4. All other cases require the use of a return buffer, through which the value is returned. In addition, there is a guarantee that if a return buffer is used a value is stored there only upon ordinary exit from the method. The buffer is not allowed to be used for temporary storage within the method and its contents will be unaltered if an exception occurs while executing the method. + +# Control Flow Guard (CFG) support on Windows + +Control Flow Guard (CFG) is a security mitigation available in Windows. +When CFG is enabled, the operating system maintains data structures that can be used to verify whether an address is to be considered a valid indirect call target. +This mechanism is exposed through two different helper functions, each with different characteristics. + +The first mechanism is a validator that takes the target address as an argument and fails fast if the address is not an expected indirect call target; otherwise, it does nothing and returns. +The second mechanism is a dispatcher that takes the target address in a non-standard register; on successful validation of the address, it jumps directly to the target function. +Windows makes the dispatcher available only on ARM64 and x64, while the validator is available on all platforms. +However, the JIT supports CFG only on ARM64 and x64, wth CFG by default being disabled for these platforms. + +The helpers are exposed to the JIT as standard JIT helpers `CORINFO_HELP_VALIDATE_INDIRECT_CALL` and `CORINFO_HELP_DISPATCH_INDIRECT_CALL`. + +To use the validator the JIT expands indirect calls into a call to the validator followed by a call to the validated address. +For the dispatcher the JIT will transform calls to pass the target along but otherwise set up the call as normal. + +Note that "indirect call" here refers to any call that is not to an immediate (in the instruction stream) address. +For example, even direct calls may emit indirect call instructions in JIT codegen due to e.g. tiering or if they have not been compiled yet; these are expanded with the CFG mechanism as well. + +The next sections describe the calling convention that the JIT expects from these helpers. + +## CFG details for ARM64 + +On ARM64, `CORINFO_HELP_VALIDATE_INDIRECT_CALL` takes the call address in `x15`. +In addition to the usual registers it preserves all float registers, `x0`-`x8` and `x15`. + +`CORINFO_HELP_DISPATCH_INDIRECT_CALL` takes the call address in `x9`. +The JIT does not use the dispatch helper by default due to worse branch predictor performance. +Therefore it will expand all indirect calls via the validation helper and a manual call. + +## CFG details for x64 + +On x64, `CORINFO_HELP_VALIDATE_INDIRECT_CALL` takes the call address in `rcx`. +In addition to the usual registers it also preserves all float registers and `rcx` and `r10`; furthermore, shadow stack space is not required to be allocated. + +`CORINFO_HELP_DISPATCH_INDIRECT_CALL` takes the call address in `rax` and it reserves the right to use and trash `r10` and `r11`. +The JIT uses the dispatch helper on x64 whenever possible as it is expected that the code size benefits outweighs the less accurate branch prediction. +However, note that the use of `r11` in the dispatcher makes it incompatible with VSD calls where the JIT must fall back to the validator and a manual call. From 451aaa69d44a013d9ffc74a1bbc10e5ac261fca4 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 21 Jan 2022 17:42:13 +0100 Subject: [PATCH 17/30] Fix arm64 validator trashed registers and update lowering comment --- src/coreclr/jit/lower.cpp | 36 +++++++++++++++++++++-------------- src/coreclr/jit/targetarm64.h | 2 +- 2 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index cf2fd68a81333..d2eda1b46fbed 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2264,22 +2264,30 @@ void Lowering::LowerCFGCall(GenTreeCall* call) // the call target from memory between calling // CORINFO_HELP_VALIDATE_INDIRECT_CALL and the target. This is // something that would easily occur in debug codegen if we - // produced high-level IR. Instead we will produce + // produced high-level IR. Instead we will use a GT_PHYSREG node + // to get the target back from the register that contains the target. // - // tx... = - // ty = callTarget - // GT_CALL CORINFO_HELP_VALIDATE_INDIRECT_CALL, ty - // tz = PHYS_REG REG_ARG_0 (preserved by helper) - // tw... = - // GT_CALL tz, tx..., tw... + // Additionally, since the validator may not preserve arg registers + // (e.g. on x64) we have to move all GT_PUTARG_REG nodes that would + // otherwise be trashed ahead. + // + // In total, we end up transforming + // + // ta... = + // tb... = (without trashed GT_PUTARG_REG nodes) + // tc = callTarget + // GT_CALL tc, ta..., tb... + // + // into + // + // ta... = + // tb... = (without trashed GT_PUTARG_REG nodes) + // tc = callTarget + // GT_CALL CORINFO_HELP_VALIDATE_INDIRECT_CALL, tc + // td = GT_PHYSREG REG_VALIDATE_INDIRECT_CALL_ADDR (preserved by helper) + // te = + // GT_CALL te, ta..., tb..., te... // - // The most problematic thing here is that the callTarget may - // effectively null-check 'this', and should normally happen after - // late args that can have some side effects. Therefore we ensure - // in fgArgInfo::ArgsComplete that we evaluate side-effecting args - // early for CFG. - // Note that early args may place stack args, but that's ok as the - // validator has a custom calling convention. GenTree* regNode = PhysReg(REG_VALIDATE_INDIRECT_CALL_ADDR, TYP_I_IMPL); LIR::Use useOfTar; diff --git a/src/coreclr/jit/targetarm64.h b/src/coreclr/jit/targetarm64.h index 8bdb5e732ba41..7443067c119b2 100644 --- a/src/coreclr/jit/targetarm64.h +++ b/src/coreclr/jit/targetarm64.h @@ -248,7 +248,7 @@ // The registers trashed by the CORINFO_HELP_INIT_PINVOKE_FRAME helper. #define RBM_INIT_PINVOKE_FRAME_TRASH RBM_CALLEE_TRASH - #define RBM_VALIDATE_INDIRECT_CALL_TRASH (RBM_CALLEE_TRASH & ~(RBM_R0 | RBM_R1 | RBM_R2 | RBM_R3 | RBM_R4 | RBM_R5 | RBM_R6 | RBM_R7 | RBM_R8 | RBM_R15 | RBM_V0 | RBM_V1 | RBM_V2 | RBM_V3 | RBM_V4 | RBM_V5 | RBM_V6 | RBM_V7)) + #define RBM_VALIDATE_INDIRECT_CALL_TRASH (RBM_INT_CALLEE_TRASH & ~(RBM_R0 | RBM_R1 | RBM_R2 | RBM_R3 | RBM_R4 | RBM_R5 | RBM_R6 | RBM_R7 | RBM_R8 | RBM_R15)) #define REG_VALIDATE_INDIRECT_CALL_ADDR REG_R15 #define REG_DISPATCH_INDIRECT_CALL_ADDR REG_R9 From 6df00a148ca55d7cfb6042baec4372e17f753f27 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 21 Jan 2022 17:48:39 +0100 Subject: [PATCH 18/30] Add line to disasm when CFG is enabled --- src/coreclr/jit/codegencommon.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 94c9646cfd57e..b3942296aaab3 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -2207,6 +2207,11 @@ void CodeGen::genGenerateMachineCode() compiler->fgPgoInlineePgo, compiler->fgPgoInlineeNoPgoSingleBlock, compiler->fgPgoInlineeNoPgo); } + if (compiler->opts.IsCFGEnabled()) + { + printf("; control-flow guard enabled\n"); + } + if (compiler->opts.jitFlags->IsSet(JitFlags::JIT_FLAG_ALT_JIT)) { printf("; invoked as altjit\n"); From 69d3b5f3aa160a11a1ff907db871de0e93603df8 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 1 Feb 2022 16:08:09 +0100 Subject: [PATCH 19/30] Add a workaround for reporting dead GC pointer regs too early when they are args --- src/coreclr/jit/emit.cpp | 6 ++++++ src/coreclr/jit/emitarm.cpp | 20 ++++++++++++++++++++ src/coreclr/jit/emitarm64.cpp | 20 ++++++++++++++++++++ src/coreclr/jit/emitxarch.cpp | 20 ++++++++++++++++++++ 4 files changed, 66 insertions(+) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 9f0a2cf6a897e..4cabb028b115b 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -2507,6 +2507,8 @@ bool emitter::emitNoGChelper(CorInfoHelpFunc helpFunc) case CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE_NOCTOR: case CORINFO_HELP_INIT_PINVOKE_FRAME: + + case CORINFO_HELP_VALIDATE_INDIRECT_CALL: return true; default: @@ -9323,6 +9325,10 @@ regMaskTP emitter::emitGetGCRegsKilledByNoGCCall(CorInfoHelpFunc helper) break; #endif // defined(TARGET_X86) + case CORINFO_HELP_VALIDATE_INDIRECT_CALL: + result = RBM_VALIDATE_INDIRECT_CALL_TRASH; + break; + default: result = RBM_CALLEE_TRASH_NOGC; break; diff --git a/src/coreclr/jit/emitarm.cpp b/src/coreclr/jit/emitarm.cpp index 7009826610fa1..af1617c308f46 100644 --- a/src/coreclr/jit/emitarm.cpp +++ b/src/coreclr/jit/emitarm.cpp @@ -4698,6 +4698,26 @@ void emitter::emitIns_Call(EmitCallType callType, // a sanity test. assert((unsigned)abs(argSize) <= codeGen->genStackLevel); + bool delayGCReport = + (callType == EC_FUNC_TOKEN) && (Compiler::eeGetHelperNum(methHnd) == CORINFO_HELP_VALIDATE_INDIRECT_CALL); + if (delayGCReport) + { + // The JIT usually publishes GC register kills only after calls, even + // for fully interruptible code. We currently rely on this behavior + // because we consider locals to be dead at their last use, even if + // that local is a GC pointer in an argument register that will be + // later used by a call. For some cases we need to delay the reporting + // of these GC regs, and accomplish this by using the last reported set + // here. + // + // TODO: It would be more ideal if we tracked GC pointers in argument + // registers separately from TYP_REF locals so that we knew not to kill + // them "too early". + // + gcrefRegs = emitThisGCrefRegs; + byrefRegs = emitThisByrefRegs; + } + // Trim out any callee-trashed registers from the live set. regMaskTP savedSet = emitGetGCRegsSavedOrModified(methHnd); gcrefRegs &= savedSet; diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp index 51e02c5c26895..f7e065c45b808 100644 --- a/src/coreclr/jit/emitarm64.cpp +++ b/src/coreclr/jit/emitarm64.cpp @@ -8555,6 +8555,26 @@ void emitter::emitIns_Call(EmitCallType callType, // a sanity test. assert((unsigned)abs(argSize) <= codeGen->genStackLevel); + bool delayGCReport = + (callType == EC_FUNC_TOKEN) && (Compiler::eeGetHelperNum(methHnd) == CORINFO_HELP_VALIDATE_INDIRECT_CALL); + if (delayGCReport) + { + // The JIT usually publishes GC register kills only after calls, even + // for fully interruptible code. We currently rely on this behavior + // because we consider locals to be dead at their last use, even if + // that local is a GC pointer in an argument register that will be + // later used by a call. For some cases we need to delay the reporting + // of these GC regs, and accomplish this by using the last reported set + // here. + // + // TODO: It would be more ideal if we tracked GC pointers in argument + // registers separately from TYP_REF locals so that we knew not to kill + // them "too early". + // + gcrefRegs = emitThisGCrefRegs; + byrefRegs = emitThisByrefRegs; + } + // Trim out any callee-trashed registers from the live set. regMaskTP savedSet = emitGetGCRegsSavedOrModified(methHnd); gcrefRegs &= savedSet; diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 1075c4ccd97d0..25b93f28d6196 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -7702,6 +7702,26 @@ void emitter::emitIns_Call(EmitCallType callType, // a sanity test. assert((unsigned)abs((signed)argSize) <= codeGen->genStackLevel); + bool delayGCReport = + (callType == EC_FUNC_TOKEN) && (Compiler::eeGetHelperNum(methHnd) == CORINFO_HELP_VALIDATE_INDIRECT_CALL); + if (delayGCReport) + { + // The JIT usually publishes GC register kills only after calls, even + // for fully interruptible code. We currently rely on this behavior + // because we consider locals to be dead at their last use, even if + // that local is a GC pointer in an argument register that will be + // later used by a call. For some cases we need to delay the reporting + // of these GC regs, and accomplish this by using the last reported set + // here. + // + // TODO: It would be more ideal if we tracked GC pointers in argument + // registers separately from TYP_REF locals so that we knew not to kill + // them "too early". + // + gcrefRegs = emitThisGCrefRegs; + byrefRegs = emitThisByrefRegs; + } + // Trim out any callee-trashed registers from the live set. regMaskTP savedSet = emitGetGCRegsSavedOrModified(methHnd); gcrefRegs &= savedSet; From 66595974d5f3a5d2ebfee2b5ead27e0ced6e6ac3 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 1 Feb 2022 20:19:36 +0100 Subject: [PATCH 20/30] Fix some comments --- src/coreclr/jit/lower.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 97b186e5336c5..6f44ac81114d3 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2274,7 +2274,7 @@ void Lowering::LowerCFGCall(GenTreeCall* call) // In total, we end up transforming // // ta... = - // tb... = (without trashed GT_PUTARG_REG nodes) + // tb... = // tc = callTarget // GT_CALL tc, ta..., tb... // @@ -2343,7 +2343,7 @@ void Lowering::LowerCFGCall(GenTreeCall* call) #ifdef DEBUG if (comp->verbose) { - dspRegMask(argRegs & trashedByValidator); + dspRegMask(argRegs); } #endif JITDUMP("\n"); @@ -2352,7 +2352,7 @@ void Lowering::LowerCFGCall(GenTreeCall* call) // arg then place the GT_PUTARG_REG after the call instead. if ((argRegs & trashedByValidator) != 0) { - JITDUMP("CFG validator will trash register(s) "); + JITDUMP("CFG validator will trash used register(s) "); #ifdef DEBUG if (comp->verbose) { From 866bccebc35cb594a779c0cf3dbb7520329dcad7 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 2 Feb 2022 11:34:52 +0100 Subject: [PATCH 21/30] Revert "Add a workaround for reporting dead GC pointer regs too early when they are args" This reverts commit 69d3b5f3aa160a11a1ff907db871de0e93603df8. --- src/coreclr/jit/emit.cpp | 6 ------ src/coreclr/jit/emitarm.cpp | 20 -------------------- src/coreclr/jit/emitarm64.cpp | 20 -------------------- src/coreclr/jit/emitxarch.cpp | 20 -------------------- 4 files changed, 66 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 4cabb028b115b..9f0a2cf6a897e 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -2507,8 +2507,6 @@ bool emitter::emitNoGChelper(CorInfoHelpFunc helpFunc) case CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE_NOCTOR: case CORINFO_HELP_INIT_PINVOKE_FRAME: - - case CORINFO_HELP_VALIDATE_INDIRECT_CALL: return true; default: @@ -9325,10 +9323,6 @@ regMaskTP emitter::emitGetGCRegsKilledByNoGCCall(CorInfoHelpFunc helper) break; #endif // defined(TARGET_X86) - case CORINFO_HELP_VALIDATE_INDIRECT_CALL: - result = RBM_VALIDATE_INDIRECT_CALL_TRASH; - break; - default: result = RBM_CALLEE_TRASH_NOGC; break; diff --git a/src/coreclr/jit/emitarm.cpp b/src/coreclr/jit/emitarm.cpp index af1617c308f46..7009826610fa1 100644 --- a/src/coreclr/jit/emitarm.cpp +++ b/src/coreclr/jit/emitarm.cpp @@ -4698,26 +4698,6 @@ void emitter::emitIns_Call(EmitCallType callType, // a sanity test. assert((unsigned)abs(argSize) <= codeGen->genStackLevel); - bool delayGCReport = - (callType == EC_FUNC_TOKEN) && (Compiler::eeGetHelperNum(methHnd) == CORINFO_HELP_VALIDATE_INDIRECT_CALL); - if (delayGCReport) - { - // The JIT usually publishes GC register kills only after calls, even - // for fully interruptible code. We currently rely on this behavior - // because we consider locals to be dead at their last use, even if - // that local is a GC pointer in an argument register that will be - // later used by a call. For some cases we need to delay the reporting - // of these GC regs, and accomplish this by using the last reported set - // here. - // - // TODO: It would be more ideal if we tracked GC pointers in argument - // registers separately from TYP_REF locals so that we knew not to kill - // them "too early". - // - gcrefRegs = emitThisGCrefRegs; - byrefRegs = emitThisByrefRegs; - } - // Trim out any callee-trashed registers from the live set. regMaskTP savedSet = emitGetGCRegsSavedOrModified(methHnd); gcrefRegs &= savedSet; diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp index 1845387ad6131..52f24d4524279 100644 --- a/src/coreclr/jit/emitarm64.cpp +++ b/src/coreclr/jit/emitarm64.cpp @@ -8555,26 +8555,6 @@ void emitter::emitIns_Call(EmitCallType callType, // a sanity test. assert((unsigned)abs(argSize) <= codeGen->genStackLevel); - bool delayGCReport = - (callType == EC_FUNC_TOKEN) && (Compiler::eeGetHelperNum(methHnd) == CORINFO_HELP_VALIDATE_INDIRECT_CALL); - if (delayGCReport) - { - // The JIT usually publishes GC register kills only after calls, even - // for fully interruptible code. We currently rely on this behavior - // because we consider locals to be dead at their last use, even if - // that local is a GC pointer in an argument register that will be - // later used by a call. For some cases we need to delay the reporting - // of these GC regs, and accomplish this by using the last reported set - // here. - // - // TODO: It would be more ideal if we tracked GC pointers in argument - // registers separately from TYP_REF locals so that we knew not to kill - // them "too early". - // - gcrefRegs = emitThisGCrefRegs; - byrefRegs = emitThisByrefRegs; - } - // Trim out any callee-trashed registers from the live set. regMaskTP savedSet = emitGetGCRegsSavedOrModified(methHnd); gcrefRegs &= savedSet; diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 0567de3e25cef..d90c5771ce1a9 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -7702,26 +7702,6 @@ void emitter::emitIns_Call(EmitCallType callType, // a sanity test. assert((unsigned)abs((signed)argSize) <= codeGen->genStackLevel); - bool delayGCReport = - (callType == EC_FUNC_TOKEN) && (Compiler::eeGetHelperNum(methHnd) == CORINFO_HELP_VALIDATE_INDIRECT_CALL); - if (delayGCReport) - { - // The JIT usually publishes GC register kills only after calls, even - // for fully interruptible code. We currently rely on this behavior - // because we consider locals to be dead at their last use, even if - // that local is a GC pointer in an argument register that will be - // later used by a call. For some cases we need to delay the reporting - // of these GC regs, and accomplish this by using the last reported set - // here. - // - // TODO: It would be more ideal if we tracked GC pointers in argument - // registers separately from TYP_REF locals so that we knew not to kill - // them "too early". - // - gcrefRegs = emitThisGCrefRegs; - byrefRegs = emitThisByrefRegs; - } - // Trim out any callee-trashed registers from the live set. regMaskTP savedSet = emitGetGCRegsSavedOrModified(methHnd); gcrefRegs &= savedSet; From bd1a7ec694e4f28a629ffc319bb816d692a44af8 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 2 Feb 2022 12:55:51 +0100 Subject: [PATCH 22/30] Move all GT_PUTARG_* nodes behind validator to fix GC reporting --- src/coreclr/jit/emit.cpp | 6 ++ src/coreclr/jit/lower.cpp | 114 +++++++++++++++++--------------------- src/coreclr/jit/lower.h | 2 +- 3 files changed, 57 insertions(+), 65 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 9f0a2cf6a897e..4cabb028b115b 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -2507,6 +2507,8 @@ bool emitter::emitNoGChelper(CorInfoHelpFunc helpFunc) case CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE_NOCTOR: case CORINFO_HELP_INIT_PINVOKE_FRAME: + + case CORINFO_HELP_VALIDATE_INDIRECT_CALL: return true; default: @@ -9323,6 +9325,10 @@ regMaskTP emitter::emitGetGCRegsKilledByNoGCCall(CorInfoHelpFunc helper) break; #endif // defined(TARGET_X86) + case CORINFO_HELP_VALIDATE_INDIRECT_CALL: + result = RBM_VALIDATE_INDIRECT_CALL_TRASH; + break; + default: result = RBM_CALLEE_TRASH_NOGC; break; diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 6f44ac81114d3..9b224c1eade36 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2267,11 +2267,14 @@ void Lowering::LowerCFGCall(GenTreeCall* call) // produced high-level IR. Instead we will use a GT_PHYSREG node // to get the target back from the register that contains the target. // - // Additionally, since the validator may not preserve arg registers - // (e.g. on x64) we have to move all GT_PUTARG_REG nodes that would - // otherwise be trashed ahead. + // Additionally, the validator does not preserve all arg registers, + // so we have to move all GT_PUTARG_REG nodes that would otherwise + // be trashed ahead. The JIT also has an internal invariant that + // once GT_PUTARG nodes start to appear in LIR, the call is coming + // up. To avoid breaking this invariant we move _all_ GT_PUTARG + // nodes (in particular, GC info reporting relies on this). // - // In total, we end up transforming + // To sum up, we end up transforming // // ta... = // tb... = @@ -2280,13 +2283,12 @@ void Lowering::LowerCFGCall(GenTreeCall* call) // // into // - // ta... = - // tb... = (without trashed GT_PUTARG_REG nodes) - // tc = callTarget - // GT_CALL CORINFO_HELP_VALIDATE_INDIRECT_CALL, tc - // td = GT_PHYSREG REG_VALIDATE_INDIRECT_CALL_ADDR (preserved by helper) - // te = - // GT_CALL te, ta..., tb..., te... + // ta... = (without GT_PUTARG_* nodes) + // tb = callTarget + // GT_CALL CORINFO_HELP_VALIDATE_INDIRECT_CALL, tb + // tc = GT_PHYSREG REG_VALIDATE_INDIRECT_CALL_ADDR (preserved by helper) + // td = + // GT_CALL tb, ta..., td.. // GenTree* regNode = PhysReg(REG_VALIDATE_INDIRECT_CALL_ADDR, TYP_I_IMPL); @@ -2321,51 +2323,25 @@ void Lowering::LowerCFGCall(GenTreeCall* call) BlockRange().InsertAfter(validate, regNode); LowerNode(regNode); - // Finally move all GT_PUTARG_REG nodes that the validate call may trash to after validation. - const regMaskTP trashedByValidator = - RBM_VALIDATE_INDIRECT_CALL_TRASH | genRegMask(REG_VALIDATE_INDIRECT_CALL_ADDR); - if ((trashedByValidator & RBM_ARG_REGS) != 0) + // Finally move all GT_PUTARG_* nodes + for (GenTreeCall::Use& use : call->Args()) { - for (GenTreeCall::Use& use : call->LateArgs()) + GenTree* node = use.GetNode(); + if (!node->IsValue()) { - GenTree* node = use.GetNode(); - JITDUMP("Checking whether late arg will be trashed by validator:\n"); - DISPTREE(node); - assert(node->OperIsPutArg() || node->OperIsFieldList()); - fgArgTabEntry* entry = comp->gtArgEntryByNode(call, node); - regMaskTP argRegs = 0; - for (unsigned i = 0; i < entry->numRegs; i++) - { - argRegs |= genRegMask(entry->GetRegNum(i)); - } - - JITDUMP("Arg uses register(s) "); -#ifdef DEBUG - if (comp->verbose) - { - dspRegMask(argRegs); - } -#endif - JITDUMP("\n"); - - // If validate call will clobber any register used by this - // arg then place the GT_PUTARG_REG after the call instead. - if ((argRegs & trashedByValidator) != 0) - { - JITDUMP("CFG validator will trash used register(s) "); -#ifdef DEBUG - if (comp->verbose) - { - dspRegMask(argRegs & trashedByValidator); - } -#endif - JITDUMP("\n"); + // Non-value nodes in early args are setup nodes for late args. + continue; + } - MoveCFGCallLateArg(call, node); - } + assert(node->OperIsPutArg() || node->OperIsFieldList()); + MoveCFGCallArg(call, node); + } - JITDUMP("\n"); - } + for (GenTreeCall::Use& use : call->LateArgs()) + { + GenTree* node = use.GetNode(); + assert(node->OperIsPutArg() || node->OperIsFieldList()); + MoveCFGCallArg(call, node); } break; } @@ -2454,7 +2430,7 @@ void Lowering::LowerCFGCall(GenTreeCall* call) //------------------------------------------------------------------------ // IsInvariantInRange: Check if a node is invariant in the specified range. In -// other words, can 'node' be moved to before 'endExclusive' without its +// other words, can 'node' be moved to right before 'endExclusive' without its // computation changing values? // // Arguments: @@ -2484,21 +2460,31 @@ bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive) return false; } - for (GenTree* intermediate = node->gtNext; intermediate != endExclusive; intermediate = intermediate->gtNext) - { - if (intermediate->OperIsLocalStore() && (intermediate->AsLclVarCommon()->GetLclNum() == lcl->GetLclNum())) - { - return false; - } - } - + // Currently, non-address exposed locals have the property that their + // use occurs at the user, so no further interference check is + // necessary. return true; } return false; } -void Lowering::MoveCFGCallLateArg(GenTreeCall* call, GenTree* node) +//------------------------------------------------------------------------ +// MoveCFGCallArg: Given a call that will be CFG transformed using the +// validate+call scheme, and an argument GT_PUTARG_* or GT_FIELD_LIST node, +// move that node right before the call. +// +// Arguments: +// call - The call that is being CFG transformed +// node - The argument node +// +// Remarks: +// We can always move the GT_PUTARG_* node further ahead as the side-effects +// of these nodes are handled by LSRA. However, the operands of these nodes +// are not always safe to move further ahead; for invariant operands, we +// move them ahead as well to shorten the lifetime of these values. +// +void Lowering::MoveCFGCallArg(GenTreeCall* call, GenTree* node) { assert(node->OperIsPutArg() || node->OperIsFieldList()); @@ -2508,13 +2494,13 @@ void Lowering::MoveCFGCallLateArg(GenTreeCall* call, GenTree* node) for (GenTreeFieldList::Use& operand : node->AsFieldList()->Uses()) { assert(operand.GetNode()->OperIsPutArg()); - MoveCFGCallLateArg(call, operand.GetNode()); + MoveCFGCallArg(call, operand.GetNode()); } } else { GenTree* operand = node->AsOp()->gtGetOp1(); - JITDUMP("Checking if we can move following operand together with the GT_PUTARG_REG\n"); + JITDUMP("Checking if we can move operand of GT_PUTARG_* node:\n"); DISPTREE(operand); if (((operand->gtFlags & GTF_ALL_EFFECT) == 0) && IsInvariantInRange(operand, call)) { diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 5d8b243874aa6..6ae062acf877c 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -126,7 +126,7 @@ class Lowering final : public Phase // ------------------------------ void LowerCall(GenTree* call); void LowerCFGCall(GenTreeCall* call); - void MoveCFGCallLateArg(GenTreeCall* call, GenTree* node); + void MoveCFGCallArg(GenTreeCall* call, GenTree* node); #ifndef TARGET_64BIT GenTree* DecomposeLongCompare(GenTree* cmp); #endif From f9ad18c5175f053c0b1668c0f4ece51c729b26c8 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 2 Feb 2022 13:29:52 +0100 Subject: [PATCH 23/30] Remove unused variable --- src/coreclr/jit/lower.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 6ae062acf877c..ddd99018aee44 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -23,7 +23,7 @@ class Lowering final : public Phase { public: inline Lowering(Compiler* compiler, LinearScanInterface* lsra) - : Phase(compiler, PHASE_LOWERING), vtableCallTemp(BAD_VAR_NUM), m_cfgCallTargetTemp(BAD_VAR_NUM) + : Phase(compiler, PHASE_LOWERING), vtableCallTemp(BAD_VAR_NUM) { m_lsra = (LinearScan*)lsra; assert(m_lsra); @@ -615,7 +615,6 @@ class Lowering final : public Phase LinearScan* m_lsra; unsigned vtableCallTemp; // local variable we use as a temp for vtable calls - unsigned m_cfgCallTargetTemp; // local variable used to evaluate call target into for CFG calls SideEffectSet m_scratchSideEffects; // SideEffectSet used for IsSafeToContainMem and isRMWIndirCandidate BasicBlock* m_block; }; From 1f42b99c03a07110c4e1f1c301557089c90e86ad Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 4 Feb 2022 14:00:47 +0100 Subject: [PATCH 24/30] Add a CFG + GC stress job --- eng/pipelines/common/templates/runtimes/run-test-job.yml | 1 + src/tests/Common/testenvironment.proj | 1 + 2 files changed, 2 insertions(+) diff --git a/eng/pipelines/common/templates/runtimes/run-test-job.yml b/eng/pipelines/common/templates/runtimes/run-test-job.yml index 9e9f7b944de30..602a35c4ec6c8 100644 --- a/eng/pipelines/common/templates/runtimes/run-test-job.yml +++ b/eng/pipelines/common/templates/runtimes/run-test-job.yml @@ -559,6 +559,7 @@ jobs: - jitcfg - jitcfg_dispatcher_always - jitcfg_dispatcher_never + - jitcfg_gcstress0xc ${{ if in(parameters.testGroup, 'ilasm') }}: scenarios: - ilasmroundtrip diff --git a/src/tests/Common/testenvironment.proj b/src/tests/Common/testenvironment.proj index e03e28cc6309f..97cfd88fb6ce0 100644 --- a/src/tests/Common/testenvironment.proj +++ b/src/tests/Common/testenvironment.proj @@ -200,6 +200,7 @@ + From 2eeecde61f133b3f8f0fbab0e403aa5cc971d0f8 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 8 Feb 2022 11:09:08 +0100 Subject: [PATCH 25/30] Clarify expected use of CFG --- docs/design/coreclr/botr/clr-abi.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/design/coreclr/botr/clr-abi.md b/docs/design/coreclr/botr/clr-abi.md index b78eb09bc7260..65717698e421c 100644 --- a/docs/design/coreclr/botr/clr-abi.md +++ b/docs/design/coreclr/botr/clr-abi.md @@ -762,7 +762,8 @@ This mechanism is exposed through two different helper functions, each with diff The first mechanism is a validator that takes the target address as an argument and fails fast if the address is not an expected indirect call target; otherwise, it does nothing and returns. The second mechanism is a dispatcher that takes the target address in a non-standard register; on successful validation of the address, it jumps directly to the target function. Windows makes the dispatcher available only on ARM64 and x64, while the validator is available on all platforms. -However, the JIT supports CFG only on ARM64 and x64, wth CFG by default being disabled for these platforms. +However, the JIT supports CFG only on ARM64 and x64, with CFG by default being disabled for these platforms. +The expected use of the CFG feature is for NativeAOT scenarios that are running in constrained environments where CFG is required. The helpers are exposed to the JIT as standard JIT helpers `CORINFO_HELP_VALIDATE_INDIRECT_CALL` and `CORINFO_HELP_DISPATCH_INDIRECT_CALL`. From f25ba52e6483c5baad8c2381174f718b90fc697e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 8 Feb 2022 11:10:20 +0100 Subject: [PATCH 26/30] Assert we do not lower dispatcher as CFG call --- src/coreclr/jit/lower.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 9b224c1eade36..e93130c38740f 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2240,8 +2240,8 @@ GenTree* Lowering::LowerTailCallViaJitHelper(GenTreeCall* call, GenTree* callTar // void Lowering::LowerCFGCall(GenTreeCall* call) { - if (call->IsHelperCall(comp, CORINFO_HELP_VALIDATE_INDIRECT_CALL) || - call->IsHelperCall(comp, CORINFO_HELP_DISPATCH_INDIRECT_CALL)) + assert(!call->IsHelperCall(comp, CORINFO_HELP_DISPATCH_INDIRECT_CALL)); + if (call->IsHelperCall(comp, CORINFO_HELP_VALIDATE_INDIRECT_CALL)) { return; } From 47409baece77b0cc58b3cc40a5157b151f7aea42 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 8 Feb 2022 11:20:55 +0100 Subject: [PATCH 27/30] Update jit-ee interface GUID --- src/coreclr/inc/jiteeversionguid.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/coreclr/inc/jiteeversionguid.h b/src/coreclr/inc/jiteeversionguid.h index 66de16f77a0ca..395532388fa03 100644 --- a/src/coreclr/inc/jiteeversionguid.h +++ b/src/coreclr/inc/jiteeversionguid.h @@ -43,13 +43,14 @@ typedef const GUID *LPCGUID; #define GUID_DEFINED #endif // !GUID_DEFINED -constexpr GUID JITEEVersionIdentifier = { /* ccb0c159-04b3-47f6-993e-79114c9cbef8 */ - 0xccb0c159, - 0x04b3, - 0x47f6, - {0x99, 0x3e, 0x79, 0x11, 0x4c, 0x9c, 0xbe, 0xf8} +constexpr GUID JITEEVersionIdentifier = { /* 63009f0c-662a-485b-bac1-ff67be6c7f9d */ + 0x63009f0c, + 0x662a, + 0x485b, + {0xba, 0xc1, 0xff, 0x67, 0xbe, 0x6c, 0x7f, 0x9d} }; + ////////////////////////////////////////////////////////////////////////////////////////////////////////// // // END JITEEVersionIdentifier From 13233bf1d43f54a8d4f04e360380bf3c5b514fcb Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 8 Feb 2022 11:23:48 +0100 Subject: [PATCH 28/30] Remove note about preallocating arg for dispatcher --- src/coreclr/jit/lower.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index e93130c38740f..e6cfd9c6e64f4 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -2384,8 +2384,6 @@ void Lowering::LowerCFGCall(GenTreeCall* call) bool isFloatHfa = false; bool isVararg = false; - // TODO: Preallocate space for this? Potentially hard as it is - // expensive to know in morph whether a call ends up being indirect. fgArgTabEntry* entry = call->fgArgInfo->AddRegArg(index, placeHolder, *earlySlot, regNum, numRegs, byteSize, byteAlignment, isStruct, isFloatHfa, From 447bca88e597816342ee88ea3a14010a8e06e408 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 8 Feb 2022 11:28:37 +0100 Subject: [PATCH 29/30] Remove another TODO --- src/coreclr/jit/compiler.h | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 89c9d8525bd11..a0353a023cd69 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -9644,10 +9644,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX // On these platforms we assume the register that the target is // passed in is preserved by the validator and take care to get the // target from the register for the call (even in debug mode). - // - // TODO: Confirm that the ARM64 validator guarantees that the - // target-address register is preserved (there is conflicting - // documentation on this, but it currently does). static_assert_no_msg((RBM_VALIDATE_INDIRECT_CALL_TRASH & (1 << REG_VALIDATE_INDIRECT_CALL_ADDR)) == 0); if (JitConfig.JitForceControlFlowGuard()) return true; From 95defd6d409c82477f204c7d61cc530ddcbe4e23 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 8 Feb 2022 11:37:58 +0100 Subject: [PATCH 30/30] Fix an assert --- src/coreclr/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index b6851c38e7e83..44796b7976f3f 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -2601,9 +2601,9 @@ void Compiler::fgInitArgInfo(GenTreeCall* call) if ((REG_VALIDATE_INDIRECT_CALL_ADDR != REG_ARG_0) && call->IsHelperCall(this, CORINFO_HELP_VALIDATE_INDIRECT_CALL)) { + assert(call->gtCallArgs != nullptr); GenTreeCall::Use* args = call->gtCallArgs; GenTree* tar = args->GetNode(); - assert(tar != nullptr); nonStandardArgs.Add(tar, REG_VALIDATE_INDIRECT_CALL_ADDR, NonStandardArgKind::ValidateIndirectCallTarget); }