From d78082bca2a967f7cb8e42a492f58d874f164f79 Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Mon, 5 Aug 2024 11:34:01 -0700 Subject: [PATCH] Partial re-revert of #104336. Only JIT fixes are included. (#105596) * Partial re-revert of #104336. Only JIT fixes are included. * fix for stress issues * comments and some cleanup * JIT format * keep existing code for x86 --- src/coreclr/jit/codegenarmarch.cpp | 5 +-- src/coreclr/jit/codegencommon.cpp | 53 +++++++++--------------- src/coreclr/jit/codegenloongarch64.cpp | 7 +--- src/coreclr/jit/codegenriscv64.cpp | 7 +--- src/coreclr/jit/codegenxarch.cpp | 12 +++--- src/coreclr/jit/emit.cpp | 11 +++-- src/coreclr/jit/emit.h | 1 + src/coreclr/jit/emitinl.h | 3 +- src/coreclr/jit/gcencode.cpp | 7 ++-- src/coreclr/nativeaot/Runtime/thread.cpp | 6 +++ 10 files changed, 47 insertions(+), 65 deletions(-) diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index b0d5d5727fc50..5617c6dfc9135 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -605,10 +605,7 @@ void CodeGen::genEmitGSCookieCheck(bool pushReg) { noway_assert(compiler->gsGlobalSecurityCookieAddr || compiler->gsGlobalSecurityCookieVal); - // Make sure that the return register is reported as live GC-ref so that any GC that kicks in while - // executing GS cookie check will not collect the object pointed to by REG_INTRET (R0). - if (!pushReg && (compiler->info.compRetNativeType == TYP_REF)) - gcInfo.gcRegGCrefSetCur |= RBM_INTRET; + assert(GetEmitter()->emitGCDisabled()); // We need two temporary registers, to load the GS cookie values and compare them. We can't use // any argument registers if 'pushReg' is true (meaning we have a JMP call). They should be diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 3c590feb06edb..0598d968e7388 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1516,8 +1516,22 @@ void CodeGen::genExitCode(BasicBlock* block) bool jmpEpilog = block->HasFlag(BBF_HAS_JMP); if (compiler->getNeedsGSSecurityCookie()) { +#ifndef JIT32_GCENCODER + // At this point the gc info that we track in codegen is often incorrect, + // as it could be missing return registers or arg registers (in a case of tail call). + // GS cookie check will emit a call and that will pass our GC info to emit and potentially mess things up. + // While we could infer returns/args and force them to be live and it seems to work in JIT32_GCENCODER case, + // it appears to be nontrivial in more general case. + // So, instead, we just claim that the whole thing is not GC-interruptible. + // Effectively this starts the epilog a few instructions earlier. + // + // CONSIDER: is that a good place to be that codegen loses track of returns/args at this point? + GetEmitter()->emitDisableGC(); +#endif + genEmitGSCookieCheck(jmpEpilog); +#ifdef JIT32_GCENCODER if (jmpEpilog) { // Dev10 642944 - @@ -1540,6 +1554,7 @@ void CodeGen::genExitCode(BasicBlock* block) GetEmitter()->emitThisGCrefRegs = GetEmitter()->emitInitGCrefRegs = gcInfo.gcRegGCrefSetCur; GetEmitter()->emitThisByrefRegs = GetEmitter()->emitInitByrefRegs = gcInfo.gcRegByrefSetCur; } +#endif } genReserveEpilog(block); @@ -4728,43 +4743,13 @@ void CodeGen::genReserveProlog(BasicBlock* block) void CodeGen::genReserveEpilog(BasicBlock* block) { - regMaskTP gcrefRegsArg = gcInfo.gcRegGCrefSetCur; - regMaskTP byrefRegsArg = gcInfo.gcRegByrefSetCur; - - /* The return value is special-cased: make sure it goes live for the epilog */ - - bool jmpEpilog = block->HasFlag(BBF_HAS_JMP); - - if (IsFullPtrRegMapRequired() && !jmpEpilog) - { - if (varTypeIsGC(compiler->info.compRetNativeType)) - { - noway_assert(genTypeStSz(compiler->info.compRetNativeType) == genTypeStSz(TYP_I_IMPL)); - - gcInfo.gcMarkRegPtrVal(REG_INTRET, compiler->info.compRetNativeType); - - switch (compiler->info.compRetNativeType) - { - case TYP_REF: - gcrefRegsArg |= RBM_INTRET; - break; - case TYP_BYREF: - byrefRegsArg |= RBM_INTRET; - break; - default: - break; - } - - JITDUMP("Extending return value GC liveness to epilog\n"); - } - } - JITDUMP("Reserving epilog IG for block " FMT_BB "\n", block->bbNum); assert(block != nullptr); - const VARSET_TP& gcrefVarsArg(GetEmitter()->emitThisGCrefVars); - GetEmitter()->emitCreatePlaceholderIG(IGPT_EPILOG, block, gcrefVarsArg, gcrefRegsArg, byrefRegsArg, - block->IsLast()); + // We pass empty GC info, because epilog is always an extend IG and will ignore what we pass. + // Besides, at this point the GC info that we track in CodeGen is often incorrect. + // See comments in genExitCode for more info. + GetEmitter()->emitCreatePlaceholderIG(IGPT_EPILOG, block, VarSetOps::MakeEmpty(compiler), 0, 0, block->IsLast()); } /***************************************************************************** diff --git a/src/coreclr/jit/codegenloongarch64.cpp b/src/coreclr/jit/codegenloongarch64.cpp index 91623af10b7cc..5815d5720422d 100644 --- a/src/coreclr/jit/codegenloongarch64.cpp +++ b/src/coreclr/jit/codegenloongarch64.cpp @@ -4610,12 +4610,7 @@ void CodeGen::genEmitGSCookieCheck(bool pushReg) { noway_assert(compiler->gsGlobalSecurityCookieAddr || compiler->gsGlobalSecurityCookieVal); - // Make sure that the return register is reported as live GC-ref so that any GC that kicks in while - // executing GS cookie check will not collect the object pointed to by REG_INTRET (A0). - if (!pushReg && (compiler->info.compRetNativeType == TYP_REF)) - { - gcInfo.gcRegGCrefSetCur |= RBM_INTRET; - } + assert(GetEmitter()->emitGCDisabled()); // We need two temporary registers, to load the GS cookie values and compare them. We can't use // any argument registers if 'pushReg' is true (meaning we have a JMP call). They should be diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 7b06287c742c9..2ed15edb0e3fc 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -4664,12 +4664,7 @@ void CodeGen::genEmitGSCookieCheck(bool pushReg) { noway_assert(compiler->gsGlobalSecurityCookieAddr || compiler->gsGlobalSecurityCookieVal); - // Make sure that the return register is reported as live GC-ref so that any GC that kicks in while - // executing GS cookie check will not collect the object pointed to by REG_INTRET (A0). - if (!pushReg && (compiler->info.compRetNativeType == TYP_REF)) - { - gcInfo.gcRegGCrefSetCur |= RBM_INTRET; - } + assert(GetEmitter()->emitGCDisabled()); // We need two temporary registers, to load the GS cookie values and compare them. We can't use // any argument registers if 'pushReg' is true (meaning we have a JMP call). They should be diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 8504f2fb3ec52..4102d4c771115 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -96,14 +96,11 @@ void CodeGen::genEmitGSCookieCheck(bool pushReg) { noway_assert(compiler->gsGlobalSecurityCookieAddr || compiler->gsGlobalSecurityCookieVal); - // Make sure that EAX is reported as live GC-ref so that any GC that kicks in while - // executing GS cookie check will not collect the object pointed to by EAX. - // - // For Amd64 System V, a two-register-returned struct could be returned in RAX and RDX - // In such case make sure that the correct GC-ness of RDX is reported as well, so - // a GC object pointed by RDX will not be collected. +#ifdef JIT32_GCENCODER if (!pushReg) { + // Make sure that EAX is reported as live GC-ref so that any GC that kicks in while + // executing GS cookie check will not collect the object pointed to by EAX. if (compiler->compMethodReturnsRetBufAddr()) { // This is for returning in an implicit RetBuf. @@ -126,6 +123,9 @@ void CodeGen::genEmitGSCookieCheck(bool pushReg) } } } +#else + assert(GetEmitter()->emitGCDisabled()); +#endif regNumber regGSCheck; regMaskTP regMaskGSCheck = RBM_NONE; diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index d91b822afa11a..6331adf3b420c 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -10427,9 +10427,9 @@ regMaskTP emitter::emitGetGCRegsKilledByNoGCCall(CorInfoHelpFunc helper) // of the last instruction in the region makes GC safe again. // In particular - once the IP is on the first instruction, but not executed it yet, // it is still safe to do GC. -// The only special case is when NoGC region is used for prologs/epilogs. -// In such case the GC info could be incorrect until the prolog completes and epilogs -// may have unwindability restrictions, so the first instruction cannot have GC. +// The only special case is when NoGC region is used for prologs. +// In such case the GC info could be incorrect until the prolog completes, so the first +// instruction cannot have GC. void emitter::emitDisableGC() { @@ -10459,6 +10459,11 @@ void emitter::emitDisableGC() } } +bool emitter::emitGCDisabled() +{ + return emitNoGCIG == true; +} + //------------------------------------------------------------------------ // emitEnableGC(): Removes a request that the following instruction groups are not GC-interruptible. // diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 83c08395118ef..b227fb182d58b 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -2745,6 +2745,7 @@ class emitter #if !defined(JIT32_GCENCODER) void emitDisableGC(); void emitEnableGC(); + bool emitGCDisabled(); #endif // !defined(JIT32_GCENCODER) #if defined(TARGET_XARCH) diff --git a/src/coreclr/jit/emitinl.h b/src/coreclr/jit/emitinl.h index 022064073d908..a586193dd5b71 100644 --- a/src/coreclr/jit/emitinl.h +++ b/src/coreclr/jit/emitinl.h @@ -594,8 +594,7 @@ bool emitter::emitGenNoGCLst(Callback& cb) emitter::instrDesc* id = emitFirstInstrDesc(ig->igData); assert(id != nullptr); assert(id->idCodeSize() > 0); - if (!cb(ig->igFuncIdx, ig->igOffs, ig->igSize, id->idCodeSize(), - ig->igFlags & (IGF_FUNCLET_PROLOG | IGF_FUNCLET_EPILOG | IGF_EPILOG))) + if (!cb(ig->igFuncIdx, ig->igOffs, ig->igSize, id->idCodeSize(), ig->igFlags & (IGF_FUNCLET_PROLOG))) { return false; } diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index 4863c95a7f59d..ae05bf797f86a 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4027,8 +4027,7 @@ class InterruptibleRangeReporter // Report everything between the previous region and the current // region as interruptible. - bool operator()( - unsigned igFuncIdx, unsigned igOffs, unsigned igSize, unsigned firstInstrSize, bool isInPrologOrEpilog) + bool operator()(unsigned igFuncIdx, unsigned igOffs, unsigned igSize, unsigned firstInstrSize, bool isInProlog) { if (igOffs < m_uninterruptibleEnd) { @@ -4042,9 +4041,9 @@ class InterruptibleRangeReporter if (igOffs > m_uninterruptibleEnd) { // Once the first instruction in IG executes, we cannot have GC. - // But it is ok to have GC while the IP is on the first instruction, unless we are in prolog/epilog. + // But it is ok to have GC while the IP is on the first instruction, unless we are in prolog. unsigned interruptibleEnd = igOffs; - if (!isInPrologOrEpilog) + if (!isInProlog) { interruptibleEnd += firstInstrSize; } diff --git a/src/coreclr/nativeaot/Runtime/thread.cpp b/src/coreclr/nativeaot/Runtime/thread.cpp index b796b05218226..c2e94a1dc8f38 100644 --- a/src/coreclr/nativeaot/Runtime/thread.cpp +++ b/src/coreclr/nativeaot/Runtime/thread.cpp @@ -675,10 +675,16 @@ void Thread::HijackCallback(NATIVE_CONTEXT* pThreadContext, void* pThreadToHijac if (runtime->IsConservativeStackReportingEnabled() || codeManager->IsSafePoint(pvAddress)) { + // IsUnwindable is precise on arm64, but can give false negatives on other architectures. + // (when IP is on the first instruction of an epilog, we still can unwind, + // but we can tell if the instruction is the first only if we can navigate instructions backwards and check) + // The preciseness of IsUnwindable is tracked in https://github.com/dotnet/runtime/issues/101932 +#if defined(TARGET_ARM64) // we may not be able to unwind in some locations, such as epilogs. // such locations should not contain safe points. // when scanning conservatively we do not need to unwind ASSERT(codeManager->IsUnwindable(pvAddress) || runtime->IsConservativeStackReportingEnabled()); +#endif // if we are not given a thread to hijack // perform in-line wait on the current thread