Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partial re-revert of #104336. Only JIT fixes are included. #105596

Merged
merged 5 commits into from
Aug 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/coreclr/jit/codegenarmarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
53 changes: 19 additions & 34 deletions src/coreclr/jit/codegencommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 -
Expand All @@ -1540,6 +1554,7 @@ void CodeGen::genExitCode(BasicBlock* block)
GetEmitter()->emitThisGCrefRegs = GetEmitter()->emitInitGCrefRegs = gcInfo.gcRegGCrefSetCur;
GetEmitter()->emitThisByrefRegs = GetEmitter()->emitInitByrefRegs = gcInfo.gcRegByrefSetCur;
}
#endif
}

genReserveEpilog(block);
Expand Down Expand Up @@ -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());
}

/*****************************************************************************
Expand Down
7 changes: 1 addition & 6 deletions src/coreclr/jit/codegenloongarch64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 1 addition & 6 deletions src/coreclr/jit/codegenriscv64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 6 additions & 6 deletions src/coreclr/jit/codegenxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -126,6 +123,9 @@ void CodeGen::genEmitGSCookieCheck(bool pushReg)
}
}
}
#else
assert(GetEmitter()->emitGCDisabled());
#endif

regNumber regGSCheck;
regMaskTP regMaskGSCheck = RBM_NONE;
Expand Down
11 changes: 8 additions & 3 deletions src/coreclr/jit/emit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down Expand Up @@ -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.
//
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/emit.h
Original file line number Diff line number Diff line change
Expand Up @@ -2745,6 +2745,7 @@ class emitter
#if !defined(JIT32_GCENCODER)
void emitDisableGC();
void emitEnableGC();
bool emitGCDisabled();
#endif // !defined(JIT32_GCENCODER)

#if defined(TARGET_XARCH)
Expand Down
3 changes: 1 addition & 2 deletions src/coreclr/jit/emitinl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
Copy link
Member Author

@VSadov VSadov Aug 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main part of the change. The epilog is no different from other No-GC regions and its first instruction should be interruptible.
See: https://github.com/dotnet/runtime/pull/104336/files#r1664973906

The rest of the changes is dealing with GS cookie check that is not always tracking GC info correctly and tweaking an assert in NativeAOT.

{
return false;
}
Expand Down
7 changes: 3 additions & 4 deletions src/coreclr/jit/gcencode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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;
}
Expand Down
6 changes: 6 additions & 0 deletions src/coreclr/nativeaot/Runtime/thread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading