From 800db918fe85832649b996a4fadd48db4461591d Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sun, 3 Dec 2023 16:59:51 -0800 Subject: [PATCH 01/44] allow async interruptions on safepoints --- src/coreclr/gcinfo/gcinfoencoder.cpp | 4 +++ src/coreclr/inc/gcinfodecoder.h | 2 ++ src/coreclr/jit/codegenlinear.cpp | 3 +-- .../Runtime/unix/UnixNativeCodeManager.cpp | 23 ++++++++++++++++- .../Runtime/windows/CoffNativeCodeManager.cpp | 25 +++++++++++++++++-- src/coreclr/vm/gcinfodecoder.cpp | 12 ++++++++- 6 files changed, 63 insertions(+), 6 deletions(-) diff --git a/src/coreclr/gcinfo/gcinfoencoder.cpp b/src/coreclr/gcinfo/gcinfoencoder.cpp index 115f7cdcf253ff..8a40404ace4841 100644 --- a/src/coreclr/gcinfo/gcinfoencoder.cpp +++ b/src/coreclr/gcinfo/gcinfoencoder.cpp @@ -943,6 +943,8 @@ bool GcInfoEncoder::IsAlwaysScratch(GcSlotDesc &slotDesc) else return FALSE; + // TODO: VS add ARM64 + #elif defined(TARGET_AMD64) _ASSERTE( m_SizeOfStackOutgoingAndScratchArea != (UINT32)-1 ); @@ -964,6 +966,8 @@ bool GcInfoEncoder::IsAlwaysScratch(GcSlotDesc &slotDesc) | (1 << 14) // r14 | (1 << 15); // r15 + PreservedRegMask |= 1; // rax - may contain return value + return !(PreservedRegMask & (1 << regNum)); } else if (!slotDesc.IsUntracked() && (slotDesc.Slot.Stack.Base == GC_SP_REL) && diff --git a/src/coreclr/inc/gcinfodecoder.h b/src/coreclr/inc/gcinfodecoder.h index b42f5aae8f6034..baf84f791e42d0 100644 --- a/src/coreclr/inc/gcinfodecoder.h +++ b/src/coreclr/inc/gcinfodecoder.h @@ -504,6 +504,8 @@ class GcInfoDecoder bool IsInterruptible(); #ifdef PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED + bool IsSafePoint(); + // This is used for gccoverage bool IsSafePoint(UINT32 codeOffset); diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 786f40c2f4ca7d..13290c44f3e5a1 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -714,8 +714,7 @@ void CodeGen::genCodeForBBlist() } // Do likewise for blocks that end in DOES_NOT_RETURN calls // that were not caught by the above rules. This ensures that - // gc register liveness doesn't change across call instructions - // in fully-interruptible mode. + // gc register liveness doesn't change to some random state after call instructions else { GenTree* call = block->lastNode(); diff --git a/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp b/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp index 6759662d5683a3..a3e496747cd448 100644 --- a/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp @@ -193,7 +193,13 @@ bool UnixNativeCodeManager::IsSafePoint(PTR_VOID pvAddress) codeOffset ); - return decoder.IsInterruptible(); + if (decoder.IsInterruptible()) + return true; + + if (decoder.IsSafePoint()) + return true; + + return false; } void UnixNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, @@ -219,6 +225,21 @@ void UnixNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, // the reasons for this adjustment are explained in EECodeManager::EnumGcRefs codeOffset--; } + else + { + // CONSIDER: We can optimize this by remembering the need to adjust in IsSafePoint and propagating into here. + // Or, better yet, maybe we should change the decoder to not require this adjustment. + // The scenario that adjustment tries to handle (fallthrough into BB with random liveness) + // does not seem possible. + GcInfoDecoder decoder1( + GCInfoToken(gcInfo), + GcInfoDecoderFlags(DECODE_INTERRUPTIBILITY), + codeOffset + ); + + if (decoder1.IsSafePoint()) + codeOffset--; + } GcInfoDecoder decoder( GCInfoToken(gcInfo), diff --git a/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp b/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp index 0f2aa4f73669c2..0fd111ce98377f 100644 --- a/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp @@ -415,7 +415,13 @@ bool CoffNativeCodeManager::IsSafePoint(PTR_VOID pvAddress) codeOffset ); - return decoder.IsInterruptible(); + if (decoder.IsInterruptible()) + return true; + + if (decoder.IsSafePoint()) + return true; + + return false; #else // Extract the necessary information from the info block header hdrInfo info; @@ -452,12 +458,27 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, // the reasons for this adjustment are explained in EECodeManager::EnumGcRefs codeOffset--; } + else + { + // CONSIDER: We can optimize this by remembering the need to adjust in IsSafePoint and propagating into here. + // Or, better yet, maybe we should change the decoder to not require this adjustment. + // The scenario that adjustment tries to handle (fallthrough into BB with random liveness) + // does not seem possible. + GcInfoDecoder decoder1( + GCInfoToken(gcInfo), + GcInfoDecoderFlags(DECODE_INTERRUPTIBILITY), + codeOffset + ); + + if (decoder1.IsSafePoint()) + codeOffset--; + } GcInfoDecoder decoder( GCInfoToken(gcInfo), GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG), codeOffset - ); + ); if (!decoder.EnumerateLiveSlots( pRegisterSet, diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index 855aa24f627f6b..27f11b536d5c78 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -367,7 +367,11 @@ GcInfoDecoder::GcInfoDecoder( { if(m_NumSafePoints) { - m_SafePointIndex = FindSafePoint(m_InstructionOffset); + // Safepoints are encoded with a -1 adjustment + // DECODE_GC_LIFETIMES adjusts the offset accordingly, but DECODE_INTERRUPTIBILITY does not + // adjust here + UINT32 offset = flags & DECODE_INTERRUPTIBILITY ? m_InstructionOffset - 1 : m_InstructionOffset; + m_SafePointIndex = FindSafePoint(offset); } } else if(flags & (DECODE_FOR_RANGES_CALLBACK | DECODE_INTERRUPTIBILITY)) @@ -393,6 +397,12 @@ bool GcInfoDecoder::IsInterruptible() return m_IsInterruptible; } +bool GcInfoDecoder::IsSafePoint() +{ + _ASSERTE(m_Flags & (DECODE_INTERRUPTIBILITY | DECODE_GC_LIFETIMES)); + return m_SafePointIndex != m_NumSafePoints; +} + bool GcInfoDecoder::HasMethodDescGenericsInstContext() { _ASSERTE( m_Flags & DECODE_GENERICS_INST_CONTEXT ); From 72b015fe83092fca3cc045766fc5f1159111ebed Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Mon, 4 Dec 2023 11:12:55 -0800 Subject: [PATCH 02/44] ARM64 TODO --- src/coreclr/gcinfo/gcinfoencoder.cpp | 49 +++++++++++++++++++++++----- src/coreclr/inc/gcinfoencoder.h | 4 ++- 2 files changed, 44 insertions(+), 9 deletions(-) diff --git a/src/coreclr/gcinfo/gcinfoencoder.cpp b/src/coreclr/gcinfo/gcinfoencoder.cpp index 8a40404ace4841..eb84ec3d8aa56d 100644 --- a/src/coreclr/gcinfo/gcinfoencoder.cpp +++ b/src/coreclr/gcinfo/gcinfoencoder.cpp @@ -922,7 +922,11 @@ void GcInfoEncoder::FinalizeSlotIds() #endif } -bool GcInfoEncoder::IsAlwaysScratch(GcSlotDesc &slotDesc) +#ifdef PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED + +// tells whether a slot cannot contain an object reference +// at call instruction or right after returning +bool GcInfoEncoder::DoNotTrackInPartiallyInterruptible(GcSlotDesc &slotDesc) { #if defined(TARGET_ARM) @@ -933,7 +937,10 @@ bool GcInfoEncoder::IsAlwaysScratch(GcSlotDesc &slotDesc) _ASSERTE(regNum >= 0 && regNum <= 14); _ASSERTE(regNum != 13); // sp - return ((regNum <= 3) || (regNum >= 12)); // R12 and R14/LR are both scratch registers + return ((regNum <= 3) || (regNum >= 12)) // R12 is volatile and SP/LR can't contain objects around calls + && regNum != 0 // R0 can contain return value + && regNum != 1 // R1 can contain return value + ; } else if (!slotDesc.IsUntracked() && (slotDesc.Slot.Stack.Base == GC_SP_REL) && ((UINT32)slotDesc.Slot.Stack.SpOffset < m_SizeOfStackOutgoingAndScratchArea)) @@ -943,7 +950,29 @@ bool GcInfoEncoder::IsAlwaysScratch(GcSlotDesc &slotDesc) else return FALSE; - // TODO: VS add ARM64 +#elif defined(TARGET_ARM64) + + _ASSERTE(m_SizeOfStackOutgoingAndScratchArea != (UINT32)-1); + if (slotDesc.IsRegister()) + { + int regNum = (int)slotDesc.Slot.RegisterNumber; + _ASSERTE(regNum >= 0 && regNum <= 30); + _ASSERTE(regNum != 18); + + return (regNum <= 17 || regNum >= 29) // X0 through X17 are scratch, FP/LR can't be used for objects around calls + && regNum != 0 // X0 can contain return value +#ifdef UNIX_AMD64_ABI + && regNum != 1 // X1 can contain return value +#endif + ; + } + else if (!slotDesc.IsUntracked() && (slotDesc.Slot.Stack.Base == GC_SP_REL) && + ((UINT32)slotDesc.Slot.Stack.SpOffset < m_SizeOfStackOutgoingAndScratchArea)) + { + return TRUE; + } + else + return FALSE; #elif defined(TARGET_AMD64) @@ -955,7 +984,7 @@ bool GcInfoEncoder::IsAlwaysScratch(GcSlotDesc &slotDesc) _ASSERTE(regNum != 4); // rsp UINT16 PreservedRegMask = - (1 << 3) // rbx + (1 << 3) // rbx | (1 << 5) // rbp #ifndef UNIX_AMD64_ABI | (1 << 6) // rsi @@ -964,9 +993,12 @@ bool GcInfoEncoder::IsAlwaysScratch(GcSlotDesc &slotDesc) | (1 << 12) // r12 | (1 << 13) // r13 | (1 << 14) // r14 - | (1 << 15); // r15 - - PreservedRegMask |= 1; // rax - may contain return value + | (1 << 15) // r15 + | (1 << 0) // rax - may contain return value +#ifdef UNIX_AMD64_ABI + | (1 << 2) // rdx - may contain return value +#endif + ; return !(PreservedRegMask & (1 << regNum)); } @@ -982,6 +1014,7 @@ bool GcInfoEncoder::IsAlwaysScratch(GcSlotDesc &slotDesc) return FALSE; #endif } +#endif // PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED void GcInfoEncoder::Build() { @@ -1400,7 +1433,7 @@ void GcInfoEncoder::Build() else { UINT32 slotIndex = pCurrent->SlotId; - if(!IsAlwaysScratch(m_SlotTable[slotIndex])) + if(!DoNotTrackInPartiallyInterruptible(m_SlotTable[slotIndex])) { BYTE becomesLive = pCurrent->BecomesLive; _ASSERTE((liveState.ReadBit(slotIndex) && !becomesLive) diff --git a/src/coreclr/inc/gcinfoencoder.h b/src/coreclr/inc/gcinfoencoder.h index 5085e5971a8ebd..b3199a1a956160 100644 --- a/src/coreclr/inc/gcinfoencoder.h +++ b/src/coreclr/inc/gcinfoencoder.h @@ -542,7 +542,9 @@ class GcInfoEncoder void SizeofSlotStateVarLengthVector(const BitArray& vector, UINT32 baseSkip, UINT32 baseRun, UINT32 * pSizeofSimple, UINT32 * pSizeofRLE, UINT32 * pSizeofRLENeg); UINT32 WriteSlotStateVarLengthVector(BitStreamWriter &writer, const BitArray& vector, UINT32 baseSkip, UINT32 baseRun); - bool IsAlwaysScratch(GcSlotDesc &slot); +#ifdef PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED + bool DoNotTrackInPartiallyInterruptible(GcSlotDesc &slot); +#endif // PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED // Assumes that "*ppTransitions" is has size "numTransitions", is sorted by CodeOffset then by SlotId, // and that "*ppEndTransitions" points one beyond the end of the array. If "*ppTransitions" contains From b2cf275f38cd248dd8f427188cd3ae64d47c49d7 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 9 Dec 2023 12:27:20 -0800 Subject: [PATCH 03/44] report GC ref/byref returns at partially interruptible callsites --- src/coreclr/jit/emit.cpp | 2 +- src/coreclr/jit/gcencode.cpp | 68 +++++++++---------- src/coreclr/jit/jitgcinfo.h | 8 +-- src/coreclr/jit/regset.cpp | 11 ++- src/coreclr/jit/target.h | 3 +- src/coreclr/jit/targetamd64.h | 10 +-- src/coreclr/jit/targetarm.h | 5 +- src/coreclr/jit/targetarm64.h | 8 ++- src/coreclr/jit/targetloongarch64.h | 7 +- src/coreclr/jit/targetriscv64.h | 7 +- src/coreclr/jit/targetx86.h | 7 +- .../Runtime/unix/UnixNativeCodeManager.cpp | 10 ++- .../Runtime/windows/CoffNativeCodeManager.cpp | 7 +- 13 files changed, 83 insertions(+), 70 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index f8b320c07d44cd..ef56888e89d502 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -10008,7 +10008,7 @@ void emitter::emitStackPopLargeStk(BYTE* addr, bool isCall, unsigned char callIn // We make a bitmask whose bits correspond to callee-saved register indices (in the sequence // of callee-saved registers only). - for (unsigned calleeSavedRegIdx = 0; calleeSavedRegIdx < CNT_CALLEE_SAVED; calleeSavedRegIdx++) + for (unsigned calleeSavedRegIdx = 0; calleeSavedRegIdx < CNT_CALL_GC_REGS; calleeSavedRegIdx++) { regMaskTP calleeSavedRbm = raRbmCalleeSaveOrder[calleeSavedRegIdx]; if (emitThisGCrefRegs & calleeSavedRbm) diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index e21fe864984ef7..50b06ca19456a7 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4471,8 +4471,8 @@ void GCInfo::gcMakeRegPtrTable( assert(call->u1.cdArgMask == 0 && call->cdArgCnt == 0); // Other than that, we just have to deal with the regmasks. - regMaskSmall gcrefRegMask = call->cdGCrefRegs & RBM_CALLEE_SAVED; - regMaskSmall byrefRegMask = call->cdByrefRegs & RBM_CALLEE_SAVED; + regMaskSmall gcrefRegMask = call->cdGCrefRegs & RBM_CALL_GC_REGS; + regMaskSmall byrefRegMask = call->cdByrefRegs & RBM_CALL_GC_REGS; assert((gcrefRegMask & byrefRegMask) == 0); @@ -4551,51 +4551,49 @@ void GCInfo::gcMakeRegPtrTable( for (regPtrDsc* genRegPtrTemp = gcRegPtrList; genRegPtrTemp != nullptr; genRegPtrTemp = genRegPtrTemp->rpdNext) { - if (genRegPtrTemp->rpdArg) + // Is this a call site? + if (genRegPtrTemp->rpdIsCallInstr()) { - // Is this a call site? - if (genRegPtrTemp->rpdIsCallInstr()) - { - // This is a true call site. + // This is a true call site. - regMaskSmall gcrefRegMask = genRegMaskFromCalleeSavedMask(genRegPtrTemp->rpdCallGCrefRegs); + regMaskSmall gcrefRegMask = genRegMaskFromCalleeSavedMask(genRegPtrTemp->rpdCallGCrefRegs); - regMaskSmall byrefRegMask = genRegMaskFromCalleeSavedMask(genRegPtrTemp->rpdCallByrefRegs); + regMaskSmall byrefRegMask = genRegMaskFromCalleeSavedMask(genRegPtrTemp->rpdCallByrefRegs); - assert((gcrefRegMask & byrefRegMask) == 0); + assert((gcrefRegMask & byrefRegMask) == 0); - regMaskSmall regMask = gcrefRegMask | byrefRegMask; + regMaskSmall regMask = gcrefRegMask | byrefRegMask; - // The "rpdOffs" is (apparently) the offset of the following instruction already. - // GcInfoEncoder wants the call instruction, so subtract the width of the call instruction. - assert(genRegPtrTemp->rpdOffs >= genRegPtrTemp->rpdCallInstrSize); - unsigned callOffset = genRegPtrTemp->rpdOffs - genRegPtrTemp->rpdCallInstrSize; + // The "rpdOffs" is (apparently) the offset of the following instruction already. + // GcInfoEncoder wants the call instruction, so subtract the width of the call instruction. + assert(genRegPtrTemp->rpdOffs >= genRegPtrTemp->rpdCallInstrSize); + unsigned callOffset = genRegPtrTemp->rpdOffs - genRegPtrTemp->rpdCallInstrSize; - // Tell the GCInfo encoder about these registers. We say that the registers become live - // before the call instruction, and dead after. - gcInfoRecordGCRegStateChange(gcInfoEncoder, mode, callOffset, regMask, GC_SLOT_LIVE, byrefRegMask, - nullptr); - gcInfoRecordGCRegStateChange(gcInfoEncoder, mode, genRegPtrTemp->rpdOffs, regMask, GC_SLOT_DEAD, - byrefRegMask, nullptr); + // Tell the GCInfo encoder about these registers. We say that the registers become live + // before the call instruction, and dead after. + gcInfoRecordGCRegStateChange(gcInfoEncoder, mode, callOffset, regMask, GC_SLOT_LIVE, byrefRegMask, + nullptr); + gcInfoRecordGCRegStateChange(gcInfoEncoder, mode, genRegPtrTemp->rpdOffs, regMask, GC_SLOT_DEAD, + byrefRegMask, nullptr); - // Also remember the call site. - if (mode == MAKE_REG_PTR_MODE_DO_WORK) - { - assert(pCallSites != nullptr && pCallSiteSizes != nullptr); - pCallSites[callSiteNum] = callOffset; - pCallSiteSizes[callSiteNum] = genRegPtrTemp->rpdCallInstrSize; - callSiteNum++; - } - } - else + // Also remember the call site. + if (mode == MAKE_REG_PTR_MODE_DO_WORK) { - // These are reporting outgoing stack arguments, but we don't need to report anything - // for partially interruptible - assert(genRegPtrTemp->rpdGCtypeGet() != GCT_NONE); - assert(genRegPtrTemp->rpdArgTypeGet() == rpdARG_PUSH); + assert(pCallSites != nullptr && pCallSiteSizes != nullptr); + pCallSites[callSiteNum] = callOffset; + pCallSiteSizes[callSiteNum] = genRegPtrTemp->rpdCallInstrSize; + callSiteNum++; } } + else if (genRegPtrTemp->rpdArg) + { + // These are reporting outgoing stack arguments, but we don't need to report anything + // for partially interruptible + assert(genRegPtrTemp->rpdGCtypeGet() != GCT_NONE); + assert(genRegPtrTemp->rpdArgTypeGet() == rpdARG_PUSH); + } } + // The routine is fully interruptible. if (mode == MAKE_REG_PTR_MODE_DO_WORK) { diff --git a/src/coreclr/jit/jitgcinfo.h b/src/coreclr/jit/jitgcinfo.h index 02fd49cead9cb3..3d2a9b2047ee4f 100644 --- a/src/coreclr/jit/jitgcinfo.h +++ b/src/coreclr/jit/jitgcinfo.h @@ -183,10 +183,10 @@ class GCInfo } unsigned short rpdIsThis : 1; // is it the 'this' pointer - unsigned short rpdCall : 1; // is this a true call site? - unsigned short : 1; // Padding bit, so next two start on a byte boundary - unsigned short rpdCallGCrefRegs : CNT_CALLEE_SAVED; // Callee-saved registers containing GC pointers. - unsigned short rpdCallByrefRegs : CNT_CALLEE_SAVED; // Callee-saved registers containing byrefs. + unsigned short rpdCall : 1; // is this a true call site? + unsigned short : 1; // Padding bit, so next two start on a byte boundary + unsigned short rpdCallGCrefRegs : CNT_CALL_GC_REGS; // Callee-saved and return registers containing GC pointers. + unsigned short rpdCallByrefRegs : CNT_CALL_GC_REGS; // Callee-saved and return registers containing byrefs. #ifndef JIT32_GCENCODER bool rpdIsCallInstr() diff --git a/src/coreclr/jit/regset.cpp b/src/coreclr/jit/regset.cpp index 12975850a404ba..dfc6df891bcf85 100644 --- a/src/coreclr/jit/regset.cpp +++ b/src/coreclr/jit/regset.cpp @@ -946,19 +946,16 @@ regNumber genRegArgNext(regNumber argReg) /***************************************************************************** * - * The following table determines the order in which callee-saved registers - * are encoded in GC information at call sites (perhaps among other things). - * In any case, they establish a mapping from ordinal callee-save reg "indices" to - * register numbers and corresponding bitmaps. + * The following table determines the order in which callee registers + * are encoded in GC information at call sites. */ -const regNumber raRegCalleeSaveOrder[] = {REG_CALLEE_SAVED_ORDER}; -const regMaskTP raRbmCalleeSaveOrder[] = {RBM_CALLEE_SAVED_ORDER}; +const regMaskTP raRbmCalleeSaveOrder[] = {RBM_CALL_GC_REGS_ORDER}; regMaskSmall genRegMaskFromCalleeSavedMask(unsigned short calleeSaveMask) { regMaskSmall res = 0; - for (int i = 0; i < CNT_CALLEE_SAVED; i++) + for (int i = 0; i < CNT_CALL_GC_REGS; i++) { if ((calleeSaveMask & ((regMaskTP)1 << i)) != 0) { diff --git a/src/coreclr/jit/target.h b/src/coreclr/jit/target.h index 06777fa9d5f709..d57d9baade310f 100644 --- a/src/coreclr/jit/target.h +++ b/src/coreclr/jit/target.h @@ -673,8 +673,7 @@ inline regMaskTP genRegMask(regNumber regNum, var_types type) * These arrays list the callee-saved register numbers (and bitmaps, respectively) for * the current architecture. */ -extern const regNumber raRegCalleeSaveOrder[CNT_CALLEE_SAVED]; -extern const regMaskTP raRbmCalleeSaveOrder[CNT_CALLEE_SAVED]; +extern const regMaskTP raRbmCalleeSaveOrder[CNT_CALL_GC_REGS]; // This method takes a "compact" bitset of the callee-saved registers, and "expands" it to a full register mask. regMaskSmall genRegMaskFromCalleeSavedMask(unsigned short); diff --git a/src/coreclr/jit/targetamd64.h b/src/coreclr/jit/targetamd64.h index 7e72da9cf2ccdc..417239dcc8d431 100644 --- a/src/coreclr/jit/targetamd64.h +++ b/src/coreclr/jit/targetamd64.h @@ -295,13 +295,14 @@ #define CNT_CALLEE_SAVED (5 + REG_ETW_FRAMED_EBP_COUNT) #define CNT_CALLEE_TRASH (9) #define CNT_CALLEE_ENREG (CNT_CALLEE_SAVED) + #define CNT_CALL_GC_REGS (CNT_CALLEE_SAVED + 2) #define CNT_CALLEE_SAVED_FLOAT (0) #define CNT_CALLEE_TRASH_FLOAT_INIT (16) #define CNT_CALLEE_TRASH_HIGHFLOAT (16) /* NOTE: Sync with variable name defined in compiler.h */ - #define REG_CALLEE_SAVED_ORDER REG_EBX,REG_ETW_FRAMED_EBP_LIST REG_R12,REG_R13,REG_R14,REG_R15 - #define RBM_CALLEE_SAVED_ORDER RBM_EBX,RBM_ETW_FRAMED_EBP_LIST RBM_R12,RBM_R13,RBM_R14,RBM_R15 + #define RBM_CALL_GC_REGS_ORDER RBM_EBX,RBM_ETW_FRAMED_EBP_LIST RBM_R12,RBM_R13,RBM_R14,RBM_R15,RBM_RAX,RBM_RDX + #define RBM_CALL_GC_REGS (RBM_EBX|RBM_ETW_FRAMED_EBP|RBM_R12|RBM_R13|RBM_R14|RBM_R15|RBM_RAX|RBM_RDX) // For SysV we have more volatile registers so we do not save any callee saves for EnC. #define RBM_ENC_CALLEE_SAVED 0 @@ -309,13 +310,14 @@ #define CNT_CALLEE_SAVED (7 + REG_ETW_FRAMED_EBP_COUNT) #define CNT_CALLEE_TRASH (7) #define CNT_CALLEE_ENREG (CNT_CALLEE_SAVED) + #define CNT_CALL_GC_REGS (CNT_CALLEE_SAVED + 1) #define CNT_CALLEE_SAVED_FLOAT (10) #define CNT_CALLEE_TRASH_FLOAT_INIT (6) #define CNT_CALLEE_TRASH_HIGHFLOAT (16) /* NOTE: Sync with variable name defined in compiler.h */ - #define REG_CALLEE_SAVED_ORDER REG_EBX,REG_ESI,REG_EDI,REG_ETW_FRAMED_EBP_LIST REG_R12,REG_R13,REG_R14,REG_R15 - #define RBM_CALLEE_SAVED_ORDER RBM_EBX,RBM_ESI,RBM_EDI,RBM_ETW_FRAMED_EBP_LIST RBM_R12,RBM_R13,RBM_R14,RBM_R15 + #define RBM_CALL_GC_REGS_ORDER RBM_EBX,RBM_ESI,RBM_EDI,RBM_ETW_FRAMED_EBP_LIST RBM_R12,RBM_R13,RBM_R14,RBM_R15,RBM_RAX + #define RBM_CALL_GC_REGS (RBM_EBX|RBM_ESI|RBM_EDI|RBM_ETW_FRAMED_EBP|RBM_R12|RBM_R13|RBM_R14|RBM_R15|RBM_RAX) // Callee-preserved registers we always save and allow use of for EnC code, since there are quite few volatile registers. #define RBM_ENC_CALLEE_SAVED (RBM_RSI | RBM_RDI) diff --git a/src/coreclr/jit/targetarm.h b/src/coreclr/jit/targetarm.h index 0f56ebe1ce989a..53ce844b17cc92 100644 --- a/src/coreclr/jit/targetarm.h +++ b/src/coreclr/jit/targetarm.h @@ -90,12 +90,13 @@ #define RBM_LOW_REGS (RBM_R0|RBM_R1|RBM_R2|RBM_R3|RBM_R4|RBM_R5|RBM_R6|RBM_R7) #define RBM_HIGH_REGS (RBM_R8|RBM_R9|RBM_R10|RBM_R11|RBM_R12|RBM_SP|RBM_LR|RBM_PC) - #define REG_CALLEE_SAVED_ORDER REG_R4,REG_R5,REG_R6,REG_R7,REG_R8,REG_R9,REG_R10,REG_R11 - #define RBM_CALLEE_SAVED_ORDER RBM_R4,RBM_R5,RBM_R6,RBM_R7,RBM_R8,RBM_R9,RBM_R10,RBM_R11 + #define RBM_CALL_GC_REGS_ORDER RBM_R4,RBM_R5,RBM_R6,RBM_R7,RBM_R8,RBM_R9,RBM_R10,RBM_R11,RBM_R0 + #define RBM_CALL_GC_REGS (RBM_R4|RBM_R5|RBM_R6|RBM_R7|RBM_R8|RBM_R9|RBM_R10|RBM_R11|RBM_R0) #define CNT_CALLEE_SAVED (8) #define CNT_CALLEE_TRASH (6) #define CNT_CALLEE_ENREG (CNT_CALLEE_SAVED-1) + #define CNT_CALL_GC_REGS (CNT_CALLEE_SAVED+1) #define CNT_CALLEE_SAVED_FLOAT (16) #define CNT_CALLEE_TRASH_FLOAT (16) diff --git a/src/coreclr/jit/targetarm64.h b/src/coreclr/jit/targetarm64.h index 6d33d378bcd96e..590b0c05927df6 100644 --- a/src/coreclr/jit/targetarm64.h +++ b/src/coreclr/jit/targetarm64.h @@ -90,7 +90,8 @@ REG_R6, REG_R7, REG_R8, REG_R9, REG_R10, \ REG_R11, REG_R13, REG_R14, \ REG_R12, REG_R15, REG_IP0, REG_IP1, \ - REG_CALLEE_SAVED_ORDER, REG_LR + REG_R19,REG_R20,REG_R21,REG_R22,REG_R23,REG_R24,REG_R25,REG_R26,REG_R27,REG_R28,\ + REG_LR #define REG_VAR_ORDER_FLT REG_V16, REG_V17, REG_V18, REG_V19, \ REG_V20, REG_V21, REG_V22, REG_V23, \ @@ -101,12 +102,13 @@ REG_V12, REG_V13, REG_V14, REG_V15, \ REG_V3, REG_V2, REG_V1, REG_V0 - #define REG_CALLEE_SAVED_ORDER REG_R19,REG_R20,REG_R21,REG_R22,REG_R23,REG_R24,REG_R25,REG_R26,REG_R27,REG_R28 - #define RBM_CALLEE_SAVED_ORDER RBM_R19,RBM_R20,RBM_R21,RBM_R22,RBM_R23,RBM_R24,RBM_R25,RBM_R26,RBM_R27,RBM_R28 + #define RBM_CALL_GC_REGS_ORDER RBM_R19,RBM_R20,RBM_R21,RBM_R22,RBM_R23,RBM_R24,RBM_R25,RBM_R26,RBM_R27,RBM_R28,RBM_R0,RBM_R1 + #define RBM_CALL_GC_REGS (RBM_R19|RBM_R20|RBM_R21|RBM_R22|RBM_R23|RBM_R24|RBM_R25|RBM_R26|RBM_R27|RBM_R28|RBM_R0|RBM_R1) #define CNT_CALLEE_SAVED (11) #define CNT_CALLEE_TRASH (17) #define CNT_CALLEE_ENREG (CNT_CALLEE_SAVED-1) + #define CNT_CALL_GC_REGS (CNT_CALLEE_SAVED+2) #define CNT_CALLEE_SAVED_FLOAT (8) #define CNT_CALLEE_TRASH_FLOAT (24) diff --git a/src/coreclr/jit/targetloongarch64.h b/src/coreclr/jit/targetloongarch64.h index d27bffa3aa698f..dcc87944beb163 100644 --- a/src/coreclr/jit/targetloongarch64.h +++ b/src/coreclr/jit/targetloongarch64.h @@ -83,7 +83,7 @@ // REG_VAR_ORDER is: (CALLEE_TRASH & ~CALLEE_TRASH_NOGC), CALLEE_TRASH_NOGC, CALLEE_SAVED #define REG_VAR_ORDER REG_A0,REG_A1,REG_A2,REG_A3,REG_A4,REG_A5,REG_A6,REG_A7, \ REG_T0,REG_T1,REG_T2,REG_T3,REG_T4,REG_T5,REG_T6,REG_T7,REG_T8, \ - REG_CALLEE_SAVED_ORDER + REG_S0,REG_S1,REG_S2,REG_S3,REG_S4,REG_S5,REG_S6,REG_S7,REG_S8 #define REG_VAR_ORDER_FLT REG_F12,REG_F13,REG_F14,REG_F15,REG_F16,REG_F17,REG_F18,REG_F19, \ REG_F2,REG_F3,REG_F4,REG_F5,REG_F6,REG_F7,REG_F8,REG_F9,REG_F10, \ @@ -91,12 +91,13 @@ REG_F24,REG_F25,REG_F26,REG_F27,REG_F28,REG_F29,REG_F30,REG_F31, \ REG_F1,REG_F0 - #define REG_CALLEE_SAVED_ORDER REG_S0,REG_S1,REG_S2,REG_S3,REG_S4,REG_S5,REG_S6,REG_S7,REG_S8 - #define RBM_CALLEE_SAVED_ORDER RBM_S0,RBM_S1,RBM_S2,RBM_S3,RBM_S4,RBM_S5,RBM_S6,RBM_S7,RBM_S8 + #define RBM_CALL_GC_REGS_ORDER RBM_S0,RBM_S1,RBM_S2,RBM_S3,RBM_S4,RBM_S5,RBM_S6,RBM_S7,RBM_S8,RBM_INTRET,RBM_INTRET_1 + #define RBM_CALL_GC_REGS (RBM_S0|RBM_S1|RBM_S2|RBM_S3|RBM_S4|RBM_S5|RBM_S6|RBM_S7|RBM_S8|RBM_INTRET|RBM_INTRET_1) #define CNT_CALLEE_SAVED (10) //s0-s8,fp. #define CNT_CALLEE_TRASH (17) #define CNT_CALLEE_ENREG (CNT_CALLEE_SAVED-1) + #define CNT_CALL_GC_REGS (CNT_CALLEE_SAVED+2) #define CNT_CALLEE_SAVED_FLOAT (8) #define CNT_CALLEE_TRASH_FLOAT (24) diff --git a/src/coreclr/jit/targetriscv64.h b/src/coreclr/jit/targetriscv64.h index 33c1b0d4919096..a0d5254dc5d174 100644 --- a/src/coreclr/jit/targetriscv64.h +++ b/src/coreclr/jit/targetriscv64.h @@ -78,7 +78,7 @@ // REG_VAR_ORDER is: (CALLEE_TRASH & ~CALLEE_TRASH_NOGC), CALLEE_TRASH_NOGC, CALLEE_SAVED #define REG_VAR_ORDER REG_A0,REG_A1,REG_A2,REG_A3,REG_A4,REG_A5,REG_A6,REG_A7, \ REG_T0,REG_T1,REG_T2,REG_T3,REG_T4,REG_T5,REG_T6, \ - REG_CALLEE_SAVED_ORDER + REG_S1,REG_S2,REG_S3,REG_S4,REG_S5,REG_S6,REG_S7,REG_S8,REG_S9,REG_S10,REG_S11 #define REG_VAR_ORDER_FLT REG_F12,REG_F13,REG_F14,REG_F15,REG_F16,REG_F17,REG_F18,REG_F19, \ REG_F2,REG_F3,REG_F4,REG_F5,REG_F6,REG_F7,REG_F8,REG_F9,REG_F10, \ @@ -86,12 +86,13 @@ REG_F24,REG_F25,REG_F26,REG_F27,REG_F28,REG_F29,REG_F30,REG_F31, \ REG_F1,REG_F0 - #define REG_CALLEE_SAVED_ORDER REG_S1,REG_S2,REG_S3,REG_S4,REG_S5,REG_S6,REG_S7,REG_S8,REG_S9,REG_S10,REG_S11 - #define RBM_CALLEE_SAVED_ORDER RBM_S1,RBM_S2,RBM_S3,RBM_S4,RBM_S5,RBM_S6,RBM_S7,RBM_S8,RBM_S9,RBM_S10,RBM_S11 + #define RBM_CALL_GC_REGS_ORDER RBM_S1,RBM_S2,RBM_S3,RBM_S4,RBM_S5,RBM_S6,RBM_S7,RBM_S8,RBM_S9,RBM_S10,RBM_S11,RBM_INTRET,RBM_INTRET_1 + #define RBM_CALL_GC_REGS (RBM_S1|RBM_S2|RBM_S3|RBM_S4|RBM_S5|RBM_S6|RBM_S7|RBM_S8|RBM_S9|RBM_S10|RBM_S11|RBM_INTRET|RBM_INTRET_1) #define CNT_CALLEE_SAVED (11) #define CNT_CALLEE_TRASH (15) #define CNT_CALLEE_ENREG (CNT_CALLEE_SAVED-1) + #define CNT_CALL_GC_REGS (CNT_CALLEE_SAVED+2) #define CNT_CALLEE_SAVED_FLOAT (12) #define CNT_CALLEE_TRASH_FLOAT (20) diff --git a/src/coreclr/jit/targetx86.h b/src/coreclr/jit/targetx86.h index dfeb96ae9e977c..aa142f6cff0de4 100644 --- a/src/coreclr/jit/targetx86.h +++ b/src/coreclr/jit/targetx86.h @@ -145,12 +145,15 @@ #define MAX_VAR_ORDER_SIZE 6 // The order here is fixed: it must agree with an order assumed in eetwain... - #define REG_CALLEE_SAVED_ORDER REG_EDI,REG_ESI,REG_EBX,REG_EBP - #define RBM_CALLEE_SAVED_ORDER RBM_EDI,RBM_ESI,RBM_EBX,RBM_EBP + // NB: x86 GC decoder does not report return registers at call sites. + #define RBM_CALL_GC_REGS_ORDER RBM_EDI,RBM_ESI,RBM_EBX,RBM_EBP + #define RBM_CALL_GC_REGS (RBM_EDI|RBM_ESI|RBM_EBX|RBM_EBP) #define CNT_CALLEE_SAVED (4) #define CNT_CALLEE_TRASH (3) #define CNT_CALLEE_ENREG (CNT_CALLEE_SAVED-1) + // NB: x86 GC decoder does not report return registers at call sites. + #define CNT_CALL_GC_REGS (CNT_CALLEE_SAVED) #define CNT_CALLEE_SAVED_FLOAT (0) #define CNT_CALLEE_TRASH_FLOAT (6) diff --git a/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp b/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp index a3e496747cd448..c83b45f4e1e0aa 100644 --- a/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp @@ -196,8 +196,9 @@ bool UnixNativeCodeManager::IsSafePoint(PTR_VOID pvAddress) if (decoder.IsInterruptible()) return true; - if (decoder.IsSafePoint()) - return true; + // TODO: VS multireg returns + //if (decoder.IsSafePoint()) + // return true; return false; } @@ -237,8 +238,11 @@ void UnixNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, codeOffset ); - if (decoder1.IsSafePoint()) + if (!decoder1.IsInterruptible()) + { + assert(decoder1.IsSafePoint()); codeOffset--; + } } GcInfoDecoder decoder( diff --git a/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp b/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp index 0fd111ce98377f..ca9a5ef3db8a18 100644 --- a/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp @@ -418,8 +418,10 @@ bool CoffNativeCodeManager::IsSafePoint(PTR_VOID pvAddress) if (decoder.IsInterruptible()) return true; +#if !defined(TARGET_ARM64) if (decoder.IsSafePoint()) return true; +#endif return false; #else @@ -470,8 +472,11 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, codeOffset ); - if (decoder1.IsSafePoint()) + if (!decoder1.IsInterruptible()) + { + assert(decoder1.IsSafePoint()); codeOffset--; + } } GcInfoDecoder decoder( From 8cf3e0c28c3218197c43fa28de422b30ee38fd81 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 9 Dec 2023 12:28:58 -0800 Subject: [PATCH 04/44] enable on all platforms --- src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp | 5 ++--- .../nativeaot/Runtime/windows/CoffNativeCodeManager.cpp | 2 -- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp b/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp index c83b45f4e1e0aa..ef0a3178f94be5 100644 --- a/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp @@ -196,9 +196,8 @@ bool UnixNativeCodeManager::IsSafePoint(PTR_VOID pvAddress) if (decoder.IsInterruptible()) return true; - // TODO: VS multireg returns - //if (decoder.IsSafePoint()) - // return true; + if (decoder.IsSafePoint()) + return true; return false; } diff --git a/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp b/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp index ca9a5ef3db8a18..4c27ceb3bcf8c8 100644 --- a/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp @@ -418,10 +418,8 @@ bool CoffNativeCodeManager::IsSafePoint(PTR_VOID pvAddress) if (decoder.IsInterruptible()) return true; -#if !defined(TARGET_ARM64) if (decoder.IsSafePoint()) return true; -#endif return false; #else From 76b4053972a9344004b5855ef47887d93d69adaa Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 9 Dec 2023 17:26:34 -0800 Subject: [PATCH 05/44] tweak --- src/coreclr/inc/gcinfodecoder.h | 1 + .../Runtime/unix/UnixNativeCodeManager.cpp | 30 +++++++++---------- .../Runtime/windows/CoffNativeCodeManager.cpp | 30 +++++++++---------- src/coreclr/vm/gcinfodecoder.cpp | 6 ++++ 4 files changed, 35 insertions(+), 32 deletions(-) diff --git a/src/coreclr/inc/gcinfodecoder.h b/src/coreclr/inc/gcinfodecoder.h index baf84f791e42d0..e546111f1889c9 100644 --- a/src/coreclr/inc/gcinfodecoder.h +++ b/src/coreclr/inc/gcinfodecoder.h @@ -502,6 +502,7 @@ class GcInfoDecoder //------------------------------------------------------------------------ bool IsInterruptible(); + bool HasInterruptibleRanges(); #ifdef PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED bool IsSafePoint(); diff --git a/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp b/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp index ef0a3178f94be5..0ca6a1769fc131 100644 --- a/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp @@ -225,31 +225,29 @@ void UnixNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, // the reasons for this adjustment are explained in EECodeManager::EnumGcRefs codeOffset--; } - else + + GcInfoDecoder decoder( + GCInfoToken(gcInfo), + GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG), + codeOffset + ); + + if (isActiveStackFrame) { // CONSIDER: We can optimize this by remembering the need to adjust in IsSafePoint and propagating into here. // Or, better yet, maybe we should change the decoder to not require this adjustment. // The scenario that adjustment tries to handle (fallthrough into BB with random liveness) // does not seem possible. - GcInfoDecoder decoder1( - GCInfoToken(gcInfo), - GcInfoDecoderFlags(DECODE_INTERRUPTIBILITY), - codeOffset - ); - - if (!decoder1.IsInterruptible()) + if (!decoder.HasInterruptibleRanges()) { - assert(decoder1.IsSafePoint()); - codeOffset--; + decoder = GcInfoDecoder( + GCInfoToken(gcInfo), + GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG), + codeOffset - 1 + ); } } - GcInfoDecoder decoder( - GCInfoToken(gcInfo), - GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG), - codeOffset - ); - ICodeManagerFlags flags = (ICodeManagerFlags)0; if (executionAborted) flags = ICodeManagerFlags::ExecutionAborted; diff --git a/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp b/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp index 4c27ceb3bcf8c8..ec08de488adb91 100644 --- a/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp @@ -458,31 +458,29 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, // the reasons for this adjustment are explained in EECodeManager::EnumGcRefs codeOffset--; } - else + + GcInfoDecoder decoder( + GCInfoToken(gcInfo), + GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG), + codeOffset + ); + + if (isActiveStackFrame) { // CONSIDER: We can optimize this by remembering the need to adjust in IsSafePoint and propagating into here. // Or, better yet, maybe we should change the decoder to not require this adjustment. // The scenario that adjustment tries to handle (fallthrough into BB with random liveness) // does not seem possible. - GcInfoDecoder decoder1( - GCInfoToken(gcInfo), - GcInfoDecoderFlags(DECODE_INTERRUPTIBILITY), - codeOffset - ); - - if (!decoder1.IsInterruptible()) + if (!decoder.HasInterruptibleRanges()) { - assert(decoder1.IsSafePoint()); - codeOffset--; + decoder = GcInfoDecoder( + GCInfoToken(gcInfo), + GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG), + codeOffset - 1 + ); } } - GcInfoDecoder decoder( - GCInfoToken(gcInfo), - GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG), - codeOffset - ); - if (!decoder.EnumerateLiveSlots( pRegisterSet, isActiveStackFrame /* reportScratchSlots */, diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index 27f11b536d5c78..b5762021034d3f 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -397,6 +397,12 @@ bool GcInfoDecoder::IsInterruptible() return m_IsInterruptible; } +bool GcInfoDecoder::HasInterruptibleRanges() +{ + _ASSERTE(m_Flags & (DECODE_INTERRUPTIBILITY | DECODE_GC_LIFETIMES)); + return m_NumInterruptibleRanges > 0; +} + bool GcInfoDecoder::IsSafePoint() { _ASSERTE(m_Flags & (DECODE_INTERRUPTIBILITY | DECODE_GC_LIFETIMES)); From fbaf77eb172256729efea4cf97009cc1b42b419a Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 27 Jan 2024 00:51:53 -0800 Subject: [PATCH 06/44] fix after rebasing --- src/coreclr/vm/gcinfodecoder.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index b5762021034d3f..db29e8ded3e4eb 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -363,7 +363,7 @@ GcInfoDecoder::GcInfoDecoder( } #ifdef PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED - if(flags & (DECODE_GC_LIFETIMES)) + if(flags & (DECODE_GC_LIFETIMES | DECODE_INTERRUPTIBILITY)) { if(m_NumSafePoints) { @@ -374,7 +374,8 @@ GcInfoDecoder::GcInfoDecoder( m_SafePointIndex = FindSafePoint(offset); } } - else if(flags & (DECODE_FOR_RANGES_CALLBACK | DECODE_INTERRUPTIBILITY)) + + if(flags & (DECODE_FOR_RANGES_CALLBACK | DECODE_INTERRUPTIBILITY)) { // Note that normalization as a code offset can be different than // normalization as code length @@ -385,7 +386,8 @@ GcInfoDecoder::GcInfoDecoder( } #endif - if(!m_IsInterruptible && (flags & DECODE_INTERRUPTIBILITY)) + _ASSERTE(!m_IsInterruptible); + if(flags & DECODE_INTERRUPTIBILITY) { EnumerateInterruptibleRanges(&SetIsInterruptibleCB, this); } From 0a13211538d01f80f6bda69826a6c65dcb2ff96c Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 15 Feb 2024 21:14:59 -0800 Subject: [PATCH 07/44] do not record tailcalls --- src/coreclr/jit/emitxarch.cpp | 109 ++++++++++++++++++---------------- 1 file changed, 59 insertions(+), 50 deletions(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 6fe7f00dddc359..cfce3158deb4f5 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -16637,74 +16637,83 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) } #endif // DEBUG - // If the method returns a GC ref, mark EAX appropriately - if (id->idGCref() == GCT_GCREF) - { - gcrefRegs |= RBM_EAX; - } - else if (id->idGCref() == GCT_BYREF) - { - byrefRegs |= RBM_EAX; - } + // Now deal with post-call state. + // Compute the liveness set, record a call for gec purposes. + // We do not need to do that though if we have a tail call as the following + // instruction would not even be reachable from here. -#ifdef UNIX_AMD64_ABI - // If is a multi-register return method is called, mark RDX appropriately (for System V AMD64). - if (id->idIsLargeCall()) + assert((ins == INS_call) || (ins == INS_tail_i_jmp) || (ins == INS_l_jmp)); + if (ins == INS_call) { - instrDescCGCA* idCall = (instrDescCGCA*)id; - if (idCall->idSecondGCref() == GCT_GCREF) + // If the method returns a GC ref, mark EAX appropriately + if (id->idGCref() == GCT_GCREF) { - gcrefRegs |= RBM_RDX; + gcrefRegs |= RBM_EAX; } - else if (idCall->idSecondGCref() == GCT_BYREF) + else if (id->idGCref() == GCT_BYREF) { - byrefRegs |= RBM_RDX; + byrefRegs |= RBM_EAX; + } + +#ifdef UNIX_AMD64_ABI + // If is a multi-register return method is called, mark RDX appropriately (for System V AMD64). + if (id->idIsLargeCall()) + { + instrDescCGCA* idCall = (instrDescCGCA*)id; + if (idCall->idSecondGCref() == GCT_GCREF) + { + gcrefRegs |= RBM_RDX; + } + else if (idCall->idSecondGCref() == GCT_BYREF) + { + byrefRegs |= RBM_RDX; + } } - } #endif // UNIX_AMD64_ABI - // If the GC register set has changed, report the new set - if (gcrefRegs != emitThisGCrefRegs) - { - emitUpdateLiveGCregs(GCT_GCREF, gcrefRegs, dst); - } + // If the GC register set has changed, report the new set + if (gcrefRegs != emitThisGCrefRegs) + { + emitUpdateLiveGCregs(GCT_GCREF, gcrefRegs, dst); + } - if (byrefRegs != emitThisByrefRegs) - { - emitUpdateLiveGCregs(GCT_BYREF, byrefRegs, dst); - } + if (byrefRegs != emitThisByrefRegs) + { + emitUpdateLiveGCregs(GCT_BYREF, byrefRegs, dst); + } - if (recCall || args) - { - // For callee-pop, all arguments will be popped after the call. - // For caller-pop, any GC arguments will go dead after the call. + if (recCall || args) + { + // For callee-pop, all arguments will be popped after the call. + // For caller-pop, any GC arguments will go dead after the call. - assert(callInstrSize != 0); + assert(callInstrSize != 0); - if (args >= 0) - { - emitStackPop(dst, /*isCall*/ true, callInstrSize, args); + if (args >= 0) + { + emitStackPop(dst, /*isCall*/ true, callInstrSize, args); + } + else + { + emitStackKillArgs(dst, -args, callInstrSize); + } } - else + + // Do we need to record a call location for GC purposes? + if (!emitFullGCinfo && recCall) { - emitStackKillArgs(dst, -args, callInstrSize); + assert(callInstrSize != 0); + emitRecordGCcall(dst, callInstrSize); } - } - - // Do we need to record a call location for GC purposes? - if (!emitFullGCinfo && recCall) - { - assert(callInstrSize != 0); - emitRecordGCcall(dst, callInstrSize); - } #ifdef DEBUG - if ((ins == INS_call) && !id->idIsTlsGD()) - { - emitRecordCallSite(emitCurCodeOffs(*dp), id->idDebugOnlyInfo()->idCallSig, - (CORINFO_METHOD_HANDLE)id->idDebugOnlyInfo()->idMemCookie); - } + if ((ins == INS_call) && !id->idIsTlsGD()) + { + emitRecordCallSite(emitCurCodeOffs(*dp), id->idDebugOnlyInfo()->idCallSig, + (CORINFO_METHOD_HANDLE)id->idDebugOnlyInfo()->idMemCookie); + } #endif // DEBUG + } break; } From 30a8524a96c312fe0b20241eaad744349d5d6d06 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 16 Feb 2024 10:37:25 -0800 Subject: [PATCH 08/44] IsInterruptibleSafePoint --- src/coreclr/inc/gcinfodecoder.h | 2 ++ .../Runtime/unix/UnixNativeCodeManager.cpp | 4 +++- .../Runtime/windows/CoffNativeCodeManager.cpp | 4 +++- src/coreclr/vm/eetwain.cpp | 22 +++++++++++++++++-- src/coreclr/vm/gcinfodecoder.cpp | 10 +++++++++ 5 files changed, 38 insertions(+), 4 deletions(-) diff --git a/src/coreclr/inc/gcinfodecoder.h b/src/coreclr/inc/gcinfodecoder.h index e546111f1889c9..749d71f9fa8092 100644 --- a/src/coreclr/inc/gcinfodecoder.h +++ b/src/coreclr/inc/gcinfodecoder.h @@ -506,6 +506,8 @@ class GcInfoDecoder #ifdef PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED bool IsSafePoint(); + bool AreSafePointsInterruptible(); + bool IsInterruptibleSafePoint(); // This is used for gccoverage bool IsSafePoint(UINT32 codeOffset); diff --git a/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp b/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp index 0ca6a1769fc131..3ccf59f611ecba 100644 --- a/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp @@ -196,7 +196,7 @@ bool UnixNativeCodeManager::IsSafePoint(PTR_VOID pvAddress) if (decoder.IsInterruptible()) return true; - if (decoder.IsSafePoint()) + if (decoder.IsInterruptibleSafePoint()) return true; return false; @@ -245,6 +245,8 @@ void UnixNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG), codeOffset - 1 ); + + assert(decoder.IsInterruptibleSafePoint()); } } diff --git a/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp b/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp index ec08de488adb91..b0eb24759bb526 100644 --- a/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp @@ -418,7 +418,7 @@ bool CoffNativeCodeManager::IsSafePoint(PTR_VOID pvAddress) if (decoder.IsInterruptible()) return true; - if (decoder.IsSafePoint()) + if (decoder.IsInterruptibleSafePoint()) return true; return false; @@ -478,6 +478,8 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG), codeOffset - 1 ); + + assert(decoder.IsInterruptibleSafePoint()); } } diff --git a/src/coreclr/vm/eetwain.cpp b/src/coreclr/vm/eetwain.cpp index 3f28d4b7431530..1d38348d6e8775 100644 --- a/src/coreclr/vm/eetwain.cpp +++ b/src/coreclr/vm/eetwain.cpp @@ -1421,7 +1421,7 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY pRD, DECODE_INTERRUPTIBILITY, curOffs ); - if(!_gcInfoDecoder.IsInterruptible()) + if(!_gcInfoDecoder.IsInterruptible() && !_gcInfoDecoder.IsInterruptibleSafePoint()) { // This must be the offset after a call #ifdef _DEBUG @@ -1441,7 +1441,7 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY pRD, DECODE_INTERRUPTIBILITY, curOffs ); - _ASSERTE(_gcInfoDecoder.IsInterruptible()); + _ASSERTE(_gcInfoDecoder.IsInterruptible() || _gcInfoDecoder.IsInterruptibleSafePoint()); } #endif @@ -1530,6 +1530,24 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY pRD, curOffs ); + if ((flags & ActiveStackFrame) != 0) + { + // CONSIDER: We can optimize this by remembering the need to adjust in IsSafePoint and propagating into here. + // Or, better yet, maybe we should change the decoder to not require this adjustment. + // The scenario that adjustment tries to handle (fallthrough into BB with random liveness) + // does not seem possible. + if (!gcInfoDecoder.HasInterruptibleRanges()) + { + gcInfoDecoder = GcInfoDecoder( + gcInfoToken, + GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG), + curOffs - 1 + ); + + _ASSERTE(gcInfoDecoder.IsInterruptibleSafePoint()); + } + } + if (!gcInfoDecoder.EnumerateLiveSlots( pRD, reportScratchSlots, diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index db29e8ded3e4eb..e24f733d41d2c8 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -411,6 +411,16 @@ bool GcInfoDecoder::IsSafePoint() return m_SafePointIndex != m_NumSafePoints; } +bool GcInfoDecoder::AreSafePointsInterruptible() +{ + return true; +} + +bool GcInfoDecoder::IsInterruptibleSafePoint() +{ + return IsSafePoint() && AreSafePointsInterruptible(); +} + bool GcInfoDecoder::HasMethodDescGenericsInstContext() { _ASSERTE( m_Flags & DECODE_GENERICS_INST_CONTEXT ); From df581c67a4587b6b65ea5c109fa726ac3d6e93c9 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 17 Feb 2024 10:36:35 -0800 Subject: [PATCH 09/44] update gccover --- src/coreclr/vm/gccover.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/coreclr/vm/gccover.cpp b/src/coreclr/vm/gccover.cpp index 44ccfd4c46bec7..22f90f6378b1f7 100644 --- a/src/coreclr/vm/gccover.cpp +++ b/src/coreclr/vm/gccover.cpp @@ -264,15 +264,6 @@ void SetupGcCoverage(NativeCodeVersion nativeCodeVersion, BYTE* methodStartPtr) void ReplaceInstrAfterCall(PBYTE instrToReplace, MethodDesc* callMD) { ReturnKind returnKind = callMD->GetReturnKind(true); - if (!IsValidReturnKind(returnKind)) - { -#if defined(TARGET_AMD64) && defined(TARGET_UNIX) - _ASSERTE(!"Unexpected return kind for x64 Unix."); -#else - // SKip GC coverage after the call. - return; -#endif - } _ASSERTE(IsValidReturnKind(returnKind)); bool ispointerKind = IsPointerReturnKind(returnKind); @@ -548,7 +539,8 @@ void GCCoverageInfo::SprinkleBreakpoints( { case InstructionType::Call_IndirectUnconditional: #ifdef TARGET_AMD64 - if(safePointDecoder.IsSafePoint((UINT32)(cur + len - codeStart + regionOffsetAdj))) + if(!safePointDecoder.AreSafePointsInterruptible() && + safePointDecoder.IsSafePoint((UINT32)(cur + len - codeStart + regionOffsetAdj))) #endif { *(cur + writeableOffset) = INTERRUPT_INSTR_CALL; // return value. May need to protect @@ -559,7 +551,8 @@ void GCCoverageInfo::SprinkleBreakpoints( if(fGcStressOnDirectCalls.val(CLRConfig::INTERNAL_GcStressOnDirectCalls)) { #ifdef TARGET_AMD64 - if(safePointDecoder.IsSafePoint((UINT32)(cur + len - codeStart + regionOffsetAdj))) + if(!safePointDecoder.AreSafePointsInterruptible() && + safePointDecoder.IsSafePoint((UINT32)(cur + len - codeStart + regionOffsetAdj))) #endif { PBYTE nextInstr; @@ -599,6 +592,11 @@ void GCCoverageInfo::SprinkleBreakpoints( { *(cur + writeableOffset) = INTERRUPT_INSTR; } + else if (safePointDecoder.AreSafePointsInterruptible() && + safePointDecoder.IsSafePoint((UINT32)dwRelOffset)) + { + *(cur + writeableOffset) = INTERRUPT_INSTR; + } #ifdef TARGET_X86 // we will whack every instruction in the prolog and epilog to make certain From d08552b871184dff15c307347784d26ab7e1e905 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 17 Feb 2024 10:31:37 -0800 Subject: [PATCH 10/44] turn on new behavior on a gcinfo version --- src/coreclr/inc/gcinfo.h | 2 +- src/coreclr/vm/gcinfodecoder.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/inc/gcinfo.h b/src/coreclr/inc/gcinfo.h index 16bff25525a97d..09763fde2e3ed9 100644 --- a/src/coreclr/inc/gcinfo.h +++ b/src/coreclr/inc/gcinfo.h @@ -36,7 +36,7 @@ const unsigned this_OFFSET_FLAG = 0x2; // the offset is "this" // The current GCInfo Version //----------------------------------------------------------------------------- -#define GCINFO_VERSION 2 +#define GCINFO_VERSION 3 //----------------------------------------------------------------------------- // GCInfoToken: A wrapper that contains the GcInfo data and version number. diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index e24f733d41d2c8..1152fd3015f22b 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -413,7 +413,7 @@ bool GcInfoDecoder::IsSafePoint() bool GcInfoDecoder::AreSafePointsInterruptible() { - return true; + return m_Version == 3; } bool GcInfoDecoder::IsInterruptibleSafePoint() From 45f74af446a5018b830da6c59aac8bfecb9664df Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sun, 18 Feb 2024 15:19:42 -0800 Subject: [PATCH 11/44] tailcalls tweak --- src/coreclr/jit/emitxarch.cpp | 45 ++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index cfce3158deb4f5..7531b62cc4f467 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -16616,35 +16616,36 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) DONE_CALL: - /* We update the variable (not register) GC info before the call as the variables cannot be - used by the call. Killing variables before the call helps with - boundary conditions if the call is CORINFO_HELP_THROW - see bug 50029. - If we ever track aliased variables (which could be used by the - call), we would have to keep them alive past the call. - */ - assert(FitsIn(dst - *dp)); - callInstrSize = static_cast(dst - *dp); - - // Note the use of address `*dp`, the call instruction address, instead of `dst`, the post-call-instruction - // address. - emitUpdateLiveGCvars(GCvars, *dp); - -#ifdef DEBUG - // Output any delta in GC variable info, corresponding to the before-call GC var updates done above. - if (EMIT_GC_VERBOSE || emitComp->opts.disasmWithGC) - { - emitDispGCVarDelta(); - } -#endif // DEBUG - // Now deal with post-call state. - // Compute the liveness set, record a call for gec purposes. + // Compute the liveness set, record a call for gc purposes. // We do not need to do that though if we have a tail call as the following // instruction would not even be reachable from here. assert((ins == INS_call) || (ins == INS_tail_i_jmp) || (ins == INS_l_jmp)); if (ins == INS_call) { + + /* We update the variable (not register) GC info before the call as the variables cannot be + used by the call. Killing variables before the call helps with + boundary conditions if the call is CORINFO_HELP_THROW - see bug 50029. + If we ever track aliased variables (which could be used by the + call), we would have to keep them alive past the call. + */ + assert(FitsIn(dst - *dp)); + callInstrSize = static_cast(dst - *dp); + + // Note the use of address `*dp`, the call instruction address, instead of `dst`, the post-call-instruction + // address. + emitUpdateLiveGCvars(GCvars, *dp); + + #ifdef DEBUG + // Output any delta in GC variable info, corresponding to the before-call GC var updates done above. + if (EMIT_GC_VERBOSE || emitComp->opts.disasmWithGC) + { + emitDispGCVarDelta(); + } + #endif // DEBUG + // If the method returns a GC ref, mark EAX appropriately if (id->idGCref() == GCT_GCREF) { From f656ba4619a6363acee1ed03778188f6e82ea26b Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 21 Feb 2024 14:51:00 -0800 Subject: [PATCH 12/44] do not report unused returns --- src/coreclr/jit/codegenxarch.cpp | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index a2a17e735c9038..aa8d9426b84db0 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -6212,22 +6212,26 @@ void CodeGen::genCallInstruction(GenTreeCall* call X86_ARG(target_ssize_t stackA emitAttr retSize = EA_PTRSIZE; emitAttr secondRetSize = EA_UNKNOWN; - if (call->HasMultiRegRetVal()) + // unused values are of no interest to GC. + if (!call->IsUnusedValue()) { - retSize = emitTypeSize(retTypeDesc->GetReturnRegType(0)); - secondRetSize = emitTypeSize(retTypeDesc->GetReturnRegType(1)); - } - else - { - assert(!varTypeIsStruct(call)); - - if (call->gtType == TYP_REF) + if (call->HasMultiRegRetVal()) { - retSize = EA_GCREF; + retSize = emitTypeSize(retTypeDesc->GetReturnRegType(0)); + secondRetSize = emitTypeSize(retTypeDesc->GetReturnRegType(1)); } - else if (call->gtType == TYP_BYREF) + else { - retSize = EA_BYREF; + assert(!varTypeIsStruct(call)); + + if (call->gtType == TYP_REF) + { + retSize = EA_GCREF; + } + else if (call->gtType == TYP_BYREF) + { + retSize = EA_BYREF; + } } } From 54878187609ddf06cc69fb9c4e0df2010e6e3cd9 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 21 Feb 2024 15:11:45 -0800 Subject: [PATCH 13/44] CORINFO_HELP_FAIL_FAST should not be a safepoint --- src/coreclr/jit/emit.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index ef56888e89d502..158b6699506f76 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -2834,6 +2834,8 @@ bool emitter::emitNoGChelper(CorInfoHelpFunc helpFunc) case CORINFO_HELP_INIT_PINVOKE_FRAME: + case CORINFO_HELP_FAIL_FAST: + case CORINFO_HELP_VALIDATE_INDIRECT_CALL: return true; From 604f8649b799ab4c4ad9678d3546f69fd5bc2d75 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 21 Feb 2024 15:30:23 -0800 Subject: [PATCH 14/44] treat tailcalls as emitNoGChelper --- src/coreclr/jit/emitxarch.cpp | 157 ++++++++++++++++------------------ 1 file changed, 74 insertions(+), 83 deletions(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 7531b62cc4f467..c67321d5eae706 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -9598,7 +9598,8 @@ void emitter::emitIns_Call(EmitCallType callType, } id->idIns(ins); - id->idSetIsNoGC(emitNoGChelper(methHnd)); + // for the purpose of GC safepointing tail-calls are not real calls + id->idSetIsNoGC(isJump || emitNoGChelper(methHnd)); UNATIVE_OFFSET sz; @@ -16554,9 +16555,9 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) #ifdef TARGET_X86 dst += emitOutputWord(dst, code | 0x0500); #else // TARGET_AMD64 - // Amd64: addr fits within 32-bits and can be encoded as a displacement relative to zero. - // This addr mode should never be used while generating relocatable ngen code nor if - // the addr can be encoded as pc-relative address. + // Amd64: addr fits within 32-bits and can be encoded as a displacement relative to zero. + // This addr mode should never be used while generating relocatable ngen code nor if + // the addr can be encoded as pc-relative address. noway_assert(!emitComp->opts.compReloc); noway_assert(codeGen->genAddrRelocTypeHint((size_t)addr) != IMAGE_REL_BASED_REL32); noway_assert(static_cast(reinterpret_cast(addr)) == (ssize_t)addr); @@ -16616,105 +16617,95 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) DONE_CALL: - // Now deal with post-call state. - // Compute the liveness set, record a call for gc purposes. - // We do not need to do that though if we have a tail call as the following - // instruction would not even be reachable from here. - - assert((ins == INS_call) || (ins == INS_tail_i_jmp) || (ins == INS_l_jmp)); - if (ins == INS_call) - { + /* We update the variable (not register) GC info before the call as the variables cannot be + used by the call. Killing variables before the call helps with + boundary conditions if the call is CORINFO_HELP_THROW - see bug 50029. + If we ever track aliased variables (which could be used by the + call), we would have to keep them alive past the call. + */ + assert(FitsIn(dst - *dp)); + callInstrSize = static_cast(dst - *dp); - /* We update the variable (not register) GC info before the call as the variables cannot be - used by the call. Killing variables before the call helps with - boundary conditions if the call is CORINFO_HELP_THROW - see bug 50029. - If we ever track aliased variables (which could be used by the - call), we would have to keep them alive past the call. - */ - assert(FitsIn(dst - *dp)); - callInstrSize = static_cast(dst - *dp); + // Note the use of address `*dp`, the call instruction address, instead of `dst`, the post-call-instruction + // address. + emitUpdateLiveGCvars(GCvars, *dp); - // Note the use of address `*dp`, the call instruction address, instead of `dst`, the post-call-instruction - // address. - emitUpdateLiveGCvars(GCvars, *dp); +#ifdef DEBUG + // Output any delta in GC variable info, corresponding to the before-call GC var updates done above. + if (EMIT_GC_VERBOSE || emitComp->opts.disasmWithGC) + { + emitDispGCVarDelta(); + } +#endif // DEBUG - #ifdef DEBUG - // Output any delta in GC variable info, corresponding to the before-call GC var updates done above. - if (EMIT_GC_VERBOSE || emitComp->opts.disasmWithGC) - { - emitDispGCVarDelta(); - } - #endif // DEBUG + // If the method returns a GC ref, mark EAX appropriately + if (id->idGCref() == GCT_GCREF) + { + gcrefRegs |= RBM_EAX; + } + else if (id->idGCref() == GCT_BYREF) + { + byrefRegs |= RBM_EAX; + } - // If the method returns a GC ref, mark EAX appropriately - if (id->idGCref() == GCT_GCREF) - { - gcrefRegs |= RBM_EAX; - } - else if (id->idGCref() == GCT_BYREF) +#ifdef UNIX_AMD64_ABI + // If is a multi-register return method is called, mark RDX appropriately (for System V AMD64). + if (id->idIsLargeCall()) + { + instrDescCGCA* idCall = (instrDescCGCA*)id; + if (idCall->idSecondGCref() == GCT_GCREF) { - byrefRegs |= RBM_EAX; + gcrefRegs |= RBM_RDX; } - -#ifdef UNIX_AMD64_ABI - // If is a multi-register return method is called, mark RDX appropriately (for System V AMD64). - if (id->idIsLargeCall()) + else if (idCall->idSecondGCref() == GCT_BYREF) { - instrDescCGCA* idCall = (instrDescCGCA*)id; - if (idCall->idSecondGCref() == GCT_GCREF) - { - gcrefRegs |= RBM_RDX; - } - else if (idCall->idSecondGCref() == GCT_BYREF) - { - byrefRegs |= RBM_RDX; - } + byrefRegs |= RBM_RDX; } + } #endif // UNIX_AMD64_ABI - // If the GC register set has changed, report the new set - if (gcrefRegs != emitThisGCrefRegs) - { - emitUpdateLiveGCregs(GCT_GCREF, gcrefRegs, dst); - } + // If the GC register set has changed, report the new set + if (gcrefRegs != emitThisGCrefRegs) + { + emitUpdateLiveGCregs(GCT_GCREF, gcrefRegs, dst); + } - if (byrefRegs != emitThisByrefRegs) - { - emitUpdateLiveGCregs(GCT_BYREF, byrefRegs, dst); - } + if (byrefRegs != emitThisByrefRegs) + { + emitUpdateLiveGCregs(GCT_BYREF, byrefRegs, dst); + } - if (recCall || args) - { - // For callee-pop, all arguments will be popped after the call. - // For caller-pop, any GC arguments will go dead after the call. + if (recCall || args) + { + // For callee-pop, all arguments will be popped after the call. + // For caller-pop, any GC arguments will go dead after the call. - assert(callInstrSize != 0); + assert(callInstrSize != 0); - if (args >= 0) - { - emitStackPop(dst, /*isCall*/ true, callInstrSize, args); - } - else - { - emitStackKillArgs(dst, -args, callInstrSize); - } + if (args >= 0) + { + emitStackPop(dst, /*isCall*/ true, callInstrSize, args); } - - // Do we need to record a call location for GC purposes? - if (!emitFullGCinfo && recCall) + else { - assert(callInstrSize != 0); - emitRecordGCcall(dst, callInstrSize); + emitStackKillArgs(dst, -args, callInstrSize); } + } + + // Do we need to record a call location for GC purposes? + if (!emitFullGCinfo && recCall) + { + assert(callInstrSize != 0); + emitRecordGCcall(dst, callInstrSize); + } #ifdef DEBUG - if ((ins == INS_call) && !id->idIsTlsGD()) - { - emitRecordCallSite(emitCurCodeOffs(*dp), id->idDebugOnlyInfo()->idCallSig, - (CORINFO_METHOD_HANDLE)id->idDebugOnlyInfo()->idMemCookie); - } -#endif // DEBUG + if ((ins == INS_call) && !id->idIsTlsGD()) + { + emitRecordCallSite(emitCurCodeOffs(*dp), id->idDebugOnlyInfo()->idCallSig, + (CORINFO_METHOD_HANDLE)id->idDebugOnlyInfo()->idMemCookie); } +#endif // DEBUG break; } From ee6129c36c69bfad653aba63c425969c8a362032 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 21 Feb 2024 15:50:05 -0800 Subject: [PATCH 15/44] versioning tweak --- src/coreclr/inc/gcinfo.h | 6 ++++-- src/coreclr/vm/codeman.cpp | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/coreclr/inc/gcinfo.h b/src/coreclr/inc/gcinfo.h index 09763fde2e3ed9..131c77c0265d87 100644 --- a/src/coreclr/inc/gcinfo.h +++ b/src/coreclr/inc/gcinfo.h @@ -65,9 +65,11 @@ struct GCInfoToken } #endif - static uint32_t ReadyToRunVersionToGcInfoVersion(uint32_t readyToRunMajorVersion) + static uint32_t ReadyToRunVersionToGcInfoVersion(uint32_t readyToRunMajorVersion, uint32_t readyToRunMinorVersion)) { - // GcInfo version is current from ReadyToRun version 2.0 + // TODO: VS versioning. For now assume the latest. + // 2.0 => 2 + // 2.1 => 3 return GCINFO_VERSION; } }; diff --git a/src/coreclr/vm/codeman.cpp b/src/coreclr/vm/codeman.cpp index 3bb10ac6c6bb64..ff096681a07421 100644 --- a/src/coreclr/vm/codeman.cpp +++ b/src/coreclr/vm/codeman.cpp @@ -5555,7 +5555,7 @@ UINT32 ReadyToRunJitManager::JitTokenToGCInfoVersion(const METHODTOKEN& MethodTo READYTORUN_HEADER * header = JitTokenToReadyToRunInfo(MethodToken)->GetReadyToRunHeader(); - return GCInfoToken::ReadyToRunVersionToGcInfoVersion(header->MajorVersion); + return GCInfoToken::ReadyToRunVersionToGcInfoVersion(header->MajorVersion, header->MinorVersion); } PTR_RUNTIME_FUNCTION ReadyToRunJitManager::JitTokenToRuntimeFunction(const METHODTOKEN& MethodToken) From ee75d3587a5e4f22c8746a264c9ed474ba115e67 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 21 Feb 2024 15:59:45 -0800 Subject: [PATCH 16/44] enable in CoreCLR (not just for GC stress scenarios) --- src/coreclr/vm/eetwain.cpp | 8 +++++++- src/coreclr/vm/gcinfodecoder.cpp | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/eetwain.cpp b/src/coreclr/vm/eetwain.cpp index 1d38348d6e8775..0f9e2ab891225c 100644 --- a/src/coreclr/vm/eetwain.cpp +++ b/src/coreclr/vm/eetwain.cpp @@ -843,7 +843,13 @@ bool EECodeManager::IsGcSafe( EECodeInfo *pCodeInfo, dwRelOffset ); - return gcInfoDecoder.IsInterruptible(); + if (gcInfoDecoder.IsInterruptible()) + return true; + + if (gcInfoDecoder.IsInterruptibleSafePoint()) + return true; + + return false; } #if defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index 1152fd3015f22b..3050ce0120613b 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -413,6 +413,7 @@ bool GcInfoDecoder::IsSafePoint() bool GcInfoDecoder::AreSafePointsInterruptible() { + // TODO: VS add a DOTNET_InterruptibleCallSites knob? return m_Version == 3; } From bb98cd9fc14306658938747b88bf6ccb41e648c1 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 21 Feb 2024 16:07:00 -0800 Subject: [PATCH 17/44] fix x86 build --- src/coreclr/vm/gccover.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/vm/gccover.cpp b/src/coreclr/vm/gccover.cpp index 22f90f6378b1f7..d31894c43dc7b0 100644 --- a/src/coreclr/vm/gccover.cpp +++ b/src/coreclr/vm/gccover.cpp @@ -592,11 +592,13 @@ void GCCoverageInfo::SprinkleBreakpoints( { *(cur + writeableOffset) = INTERRUPT_INSTR; } +#ifdef TARGET_AMD64 else if (safePointDecoder.AreSafePointsInterruptible() && safePointDecoder.IsSafePoint((UINT32)dwRelOffset)) { *(cur + writeableOffset) = INTERRUPT_INSTR; } +#endif #ifdef TARGET_X86 // we will whack every instruction in the prolog and epilog to make certain From a8d9b43e4761219f4f5276ff3ab6cc4a36e2f68a Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 21 Feb 2024 16:09:58 -0800 Subject: [PATCH 18/44] other architectures --- src/coreclr/jit/codegenarmarch.cpp | 28 +++++++++++++++----------- src/coreclr/jit/codegenloongarch64.cpp | 28 +++++++++++++++----------- src/coreclr/jit/codegenriscv64.cpp | 28 +++++++++++++++----------- src/coreclr/jit/emitarm.cpp | 3 ++- src/coreclr/jit/emitarm64.cpp | 3 ++- src/coreclr/jit/emitloongarch64.cpp | 3 ++- src/coreclr/jit/emitriscv64.cpp | 3 ++- 7 files changed, 56 insertions(+), 40 deletions(-) diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index 90447292dbe837..cbd74f8dc016bc 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -3503,22 +3503,26 @@ void CodeGen::genCallInstruction(GenTreeCall* call) emitAttr retSize = EA_PTRSIZE; emitAttr secondRetSize = EA_UNKNOWN; - if (call->HasMultiRegRetVal()) + // unused values are of no interest to GC. + if (!call->IsUnusedValue()) { - retSize = emitTypeSize(pRetTypeDesc->GetReturnRegType(0)); - secondRetSize = emitTypeSize(pRetTypeDesc->GetReturnRegType(1)); - } - else - { - assert(call->gtType != TYP_STRUCT); - - if (call->gtType == TYP_REF) + if (call->HasMultiRegRetVal()) { - retSize = EA_GCREF; + retSize = emitTypeSize(pRetTypeDesc->GetReturnRegType(0)); + secondRetSize = emitTypeSize(pRetTypeDesc->GetReturnRegType(1)); } - else if (call->gtType == TYP_BYREF) + else { - retSize = EA_BYREF; + assert(call->gtType != TYP_STRUCT); + + if (call->gtType == TYP_REF) + { + retSize = EA_GCREF; + } + else if (call->gtType == TYP_BYREF) + { + retSize = EA_BYREF; + } } } diff --git a/src/coreclr/jit/codegenloongarch64.cpp b/src/coreclr/jit/codegenloongarch64.cpp index 6fceb11807ed33..6090ad39e555dc 100644 --- a/src/coreclr/jit/codegenloongarch64.cpp +++ b/src/coreclr/jit/codegenloongarch64.cpp @@ -6356,22 +6356,26 @@ void CodeGen::genCallInstruction(GenTreeCall* call) emitAttr retSize = EA_PTRSIZE; emitAttr secondRetSize = EA_UNKNOWN; - if (call->HasMultiRegRetVal()) + // unused values are of no interest to GC. + if (!call->IsUnusedValue()) { - retSize = emitTypeSize(pRetTypeDesc->GetReturnRegType(0)); - secondRetSize = emitTypeSize(pRetTypeDesc->GetReturnRegType(1)); - } - else - { - assert(call->gtType != TYP_STRUCT); - - if (call->gtType == TYP_REF) + if (call->HasMultiRegRetVal()) { - retSize = EA_GCREF; + retSize = emitTypeSize(pRetTypeDesc->GetReturnRegType(0)); + secondRetSize = emitTypeSize(pRetTypeDesc->GetReturnRegType(1)); } - else if (call->gtType == TYP_BYREF) + else { - retSize = EA_BYREF; + assert(call->gtType != TYP_STRUCT); + + if (call->gtType == TYP_REF) + { + retSize = EA_GCREF; + } + else if (call->gtType == TYP_BYREF) + { + retSize = EA_BYREF; + } } } diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 84a2663dcbcce5..fd9ce985cd25e0 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -6500,22 +6500,26 @@ void CodeGen::genCallInstruction(GenTreeCall* call) emitAttr retSize = EA_PTRSIZE; emitAttr secondRetSize = EA_UNKNOWN; - if (call->HasMultiRegRetVal()) + // unused values are of no interest to GC. + if (!call->IsUnusedValue()) { - retSize = emitTypeSize(pRetTypeDesc->GetReturnRegType(0)); - secondRetSize = emitTypeSize(pRetTypeDesc->GetReturnRegType(1)); - } - else - { - assert(call->gtType != TYP_STRUCT); - - if (call->gtType == TYP_REF) + if (call->HasMultiRegRetVal()) { - retSize = EA_GCREF; + retSize = emitTypeSize(pRetTypeDesc->GetReturnRegType(0)); + secondRetSize = emitTypeSize(pRetTypeDesc->GetReturnRegType(1)); } - else if (call->gtType == TYP_BYREF) + else { - retSize = EA_BYREF; + assert(call->gtType != TYP_STRUCT); + + if (call->gtType == TYP_REF) + { + retSize = EA_GCREF; + } + else if (call->gtType == TYP_BYREF) + { + retSize = EA_BYREF; + } } } diff --git a/src/coreclr/jit/emitarm.cpp b/src/coreclr/jit/emitarm.cpp index 65a6e098b2976a..87c29c7f5333d6 100644 --- a/src/coreclr/jit/emitarm.cpp +++ b/src/coreclr/jit/emitarm.cpp @@ -4758,7 +4758,8 @@ void emitter::emitIns_Call(EmitCallType callType, emitThisGCrefRegs = gcrefRegs; emitThisByrefRegs = byrefRegs; - id->idSetIsNoGC(emitNoGChelper(methHnd)); + // for the purpose of GC safepointing tail-calls are not real calls + id->idSetIsNoGC(isJump || emitNoGChelper(methHnd)); /* Set the instruction - special case jumping a function */ instruction ins; diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp index 3c988633e798b9..2f1470ea2ad37e 100644 --- a/src/coreclr/jit/emitarm64.cpp +++ b/src/coreclr/jit/emitarm64.cpp @@ -9024,7 +9024,8 @@ void emitter::emitIns_Call(EmitCallType callType, emitThisGCrefRegs = gcrefRegs; emitThisByrefRegs = byrefRegs; - id->idSetIsNoGC(emitNoGChelper(methHnd)); + // for the purpose of GC safepointing tail-calls are not real calls + id->idSetIsNoGC(isJump || emitNoGChelper(methHnd)); /* Set the instruction - special case jumping a function */ instruction ins; diff --git a/src/coreclr/jit/emitloongarch64.cpp b/src/coreclr/jit/emitloongarch64.cpp index 43281e536e8b31..cd1d5e22df3871 100644 --- a/src/coreclr/jit/emitloongarch64.cpp +++ b/src/coreclr/jit/emitloongarch64.cpp @@ -2468,7 +2468,8 @@ void emitter::emitIns_Call(EmitCallType callType, emitThisGCrefRegs = gcrefRegs; emitThisByrefRegs = byrefRegs; - id->idSetIsNoGC(emitNoGChelper(methHnd)); + // for the purpose of GC safepointing tail-calls are not real calls + id->idSetIsNoGC(isJump || emitNoGChelper(methHnd)); /* Set the instruction - special case jumping a function */ instruction ins; diff --git a/src/coreclr/jit/emitriscv64.cpp b/src/coreclr/jit/emitriscv64.cpp index 932c9a1125b016..62c156c8093153 100644 --- a/src/coreclr/jit/emitriscv64.cpp +++ b/src/coreclr/jit/emitriscv64.cpp @@ -1377,7 +1377,8 @@ void emitter::emitIns_Call(EmitCallType callType, emitThisGCrefRegs = gcrefRegs; emitThisByrefRegs = byrefRegs; - id->idSetIsNoGC(emitNoGChelper(methHnd)); + // for the purpose of GC safepointing tail-calls are not real calls + id->idSetIsNoGC(isJump || emitNoGChelper(methHnd)); /* Set the instruction - special case jumping a function */ instruction ins; From 5a332b0e50e638a90dea254f236f529dec66afbe Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 22 Feb 2024 14:11:57 -0800 Subject: [PATCH 19/44] added a knob DOTNET_InterruptibleCallSites --- src/coreclr/inc/clrconfigvalues.h | 1 + src/coreclr/vm/gcinfodecoder.cpp | 25 +++++++++++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/coreclr/inc/clrconfigvalues.h b/src/coreclr/inc/clrconfigvalues.h index ddc7c79506ad4e..7005e25226ce4f 100644 --- a/src/coreclr/inc/clrconfigvalues.h +++ b/src/coreclr/inc/clrconfigvalues.h @@ -505,6 +505,7 @@ CONFIG_DWORD_INFO(INTERNAL_DiagnosticSuspend, W("DiagnosticSuspend"), 0, "") CONFIG_DWORD_INFO(INTERNAL_SuspendDeadlockTimeout, W("SuspendDeadlockTimeout"), 40000, "") CONFIG_DWORD_INFO(INTERNAL_SuspendThreadDeadlockTimeoutMs, W("SuspendThreadDeadlockTimeoutMs"), 2000, "") RETAIL_CONFIG_DWORD_INFO(INTERNAL_ThreadSuspendInjection, W("INTERNAL_ThreadSuspendInjection"), 1, "Specifies whether to inject activations for thread suspension on Unix") +RETAIL_CONFIG_DWORD_INFO(INTERNAL_InterruptibleCallSites, W("InterruptibleCallSites"), 1, "Specifies whether to allow asynchronous thread interruptions at call sites (requires GCInfo v3)") /// /// Thread (miscellaneous) diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index 3050ce0120613b..e04a523b6c82e4 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -3,6 +3,10 @@ #include "common.h" +#ifdef FEATURE_NATIVEAOT +#include "RhConfig.h" +#endif + #include "gcinfodecoder.h" #ifdef USE_GC_INFO_DECODER @@ -413,8 +417,25 @@ bool GcInfoDecoder::IsSafePoint() bool GcInfoDecoder::AreSafePointsInterruptible() { - // TODO: VS add a DOTNET_InterruptibleCallSites knob? - return m_Version == 3; + if (m_Version < 3) + return false; + + // zero initialized + static bool fInterruptibleCallSitesInited; + static uint64_t fInterruptibleCallSites; + + if (!fInterruptibleCallSitesInited) + { +#ifdef FEATURE_NATIVEAOT + fInterruptibleCallSites = 1; + g_pRhConfig->ReadConfigValue("InterruptibleCallSites", &fInterruptibleCallSites); +#else + fInterruptibleCallSites = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_InterruptibleCallSites); +#endif + fInterruptibleCallSitesInited = true; + } + + return fInterruptibleCallSites != 0; } bool GcInfoDecoder::IsInterruptibleSafePoint() From 3f6c088b84fcfbbb5641645ad020da72ddc4a634 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 23 Feb 2024 10:03:40 -0800 Subject: [PATCH 20/44] moved DOTNET_InterruptibleCallSites check to the code manager --- src/coreclr/inc/eetwain.h | 6 ++++++ src/coreclr/vm/eetwain.cpp | 19 +++++++++++++++---- src/coreclr/vm/gccover.cpp | 21 ++++++++------------- src/coreclr/vm/gcinfodecoder.cpp | 24 +----------------------- 4 files changed, 30 insertions(+), 40 deletions(-) diff --git a/src/coreclr/inc/eetwain.h b/src/coreclr/inc/eetwain.h index bee2f658ee7c08..41f43708887b7c 100644 --- a/src/coreclr/inc/eetwain.h +++ b/src/coreclr/inc/eetwain.h @@ -214,6 +214,9 @@ virtual bool UnwindStackFrame(PREGDISPLAY pContext, virtual bool IsGcSafe(EECodeInfo *pCodeInfo, DWORD dwRelOffset) = 0; +virtual +bool InterruptibleSafePointsEnabled() = 0; + #if defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) virtual bool HasTailCalls(EECodeInfo *pCodeInfo) = 0; #endif // TARGET_ARM || TARGET_ARM64 || TARGET_LOONGARCH64 || TARGET_RISCV64 @@ -457,6 +460,9 @@ virtual bool IsGcSafe( EECodeInfo *pCodeInfo, DWORD dwRelOffset); +virtual +bool InterruptibleSafePointsEnabled(); + #if defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) virtual bool HasTailCalls(EECodeInfo *pCodeInfo); diff --git a/src/coreclr/vm/eetwain.cpp b/src/coreclr/vm/eetwain.cpp index 0f9e2ab891225c..42a6776123f1d4 100644 --- a/src/coreclr/vm/eetwain.cpp +++ b/src/coreclr/vm/eetwain.cpp @@ -846,12 +846,21 @@ bool EECodeManager::IsGcSafe( EECodeInfo *pCodeInfo, if (gcInfoDecoder.IsInterruptible()) return true; - if (gcInfoDecoder.IsInterruptibleSafePoint()) + if (InterruptibleSafePointsEnabled() && gcInfoDecoder.IsInterruptibleSafePoint()) return true; return false; } +bool EECodeManager::InterruptibleSafePointsEnabled() +{ + LIMITED_METHOD_CONTRACT; + + // zero initialized + static ConfigDWORD interruptibleCallSitesEnabled; + return interruptibleCallSitesEnabled.val(CLRConfig::INTERNAL_InterruptibleCallSites) != 0; +} + #if defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) bool EECodeManager::HasTailCalls( EECodeInfo *pCodeInfo) { @@ -1427,7 +1436,8 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY pRD, DECODE_INTERRUPTIBILITY, curOffs ); - if(!_gcInfoDecoder.IsInterruptible() && !_gcInfoDecoder.IsInterruptibleSafePoint()) + if(!_gcInfoDecoder.IsInterruptible() && + !(InterruptibleSafePointsEnabled() && _gcInfoDecoder.IsInterruptibleSafePoint())) { // This must be the offset after a call #ifdef _DEBUG @@ -1447,7 +1457,8 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY pRD, DECODE_INTERRUPTIBILITY, curOffs ); - _ASSERTE(_gcInfoDecoder.IsInterruptible() || _gcInfoDecoder.IsInterruptibleSafePoint()); + _ASSERTE(_gcInfoDecoder.IsInterruptible() || + (InterruptibleSafePointsEnabled() && _gcInfoDecoder.IsInterruptibleSafePoint())); } #endif @@ -1550,7 +1561,7 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY pRD, curOffs - 1 ); - _ASSERTE(gcInfoDecoder.IsInterruptibleSafePoint()); + _ASSERTE((InterruptibleSafePointsEnabled() && gcInfoDecoder.IsInterruptibleSafePoint())); } } diff --git a/src/coreclr/vm/gccover.cpp b/src/coreclr/vm/gccover.cpp index d31894c43dc7b0..cede54465ebce2 100644 --- a/src/coreclr/vm/gccover.cpp +++ b/src/coreclr/vm/gccover.cpp @@ -535,11 +535,15 @@ void GCCoverageInfo::SprinkleBreakpoints( _ASSERTE(len > 0); _ASSERTE(len <= (size_t)(codeEnd-cur)); + // For non-fully interruptible code, we want to at least + // patch the return sites after the call instructions. + // Specially so that we can verify stack-walking through the call site via a simulated hijack. + // We would need to know the return kind of the callee, so this may not always be possible. switch(instructionType) { case InstructionType::Call_IndirectUnconditional: #ifdef TARGET_AMD64 - if(!safePointDecoder.AreSafePointsInterruptible() && + if(!(codeMan->InterruptibleSafePointsEnabled() && safePointDecoder.AreSafePointsInterruptible()) && safePointDecoder.IsSafePoint((UINT32)(cur + len - codeStart + regionOffsetAdj))) #endif { @@ -551,7 +555,7 @@ void GCCoverageInfo::SprinkleBreakpoints( if(fGcStressOnDirectCalls.val(CLRConfig::INTERNAL_GcStressOnDirectCalls)) { #ifdef TARGET_AMD64 - if(!safePointDecoder.AreSafePointsInterruptible() && + if(!(codeMan->InterruptibleSafePointsEnabled() && safePointDecoder.AreSafePointsInterruptible()) && safePointDecoder.IsSafePoint((UINT32)(cur + len - codeStart + regionOffsetAdj))) #endif { @@ -582,23 +586,14 @@ void GCCoverageInfo::SprinkleBreakpoints( ReplaceInstrAfterCall(cur + writeableOffset, prevDirectCallTargetMD); } - // For fully interruptible code, we end up whacking every instruction - // to INTERRUPT_INSTR. For non-fully interruptible code, we end - // up only touching the call instructions (specially so that we - // can really do the GC on the instruction just after the call). + // For fully interruptible locations, we end up whacking every instruction + // to INTERRUPT_INSTR. size_t dwRelOffset = (cur - codeStart) + regionOffsetAdj; _ASSERTE(FitsIn(dwRelOffset)); if (codeMan->IsGcSafe(&codeInfo, static_cast(dwRelOffset))) { *(cur + writeableOffset) = INTERRUPT_INSTR; } -#ifdef TARGET_AMD64 - else if (safePointDecoder.AreSafePointsInterruptible() && - safePointDecoder.IsSafePoint((UINT32)dwRelOffset)) - { - *(cur + writeableOffset) = INTERRUPT_INSTR; - } -#endif #ifdef TARGET_X86 // we will whack every instruction in the prolog and epilog to make certain diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index e04a523b6c82e4..214659424db997 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -3,10 +3,6 @@ #include "common.h" -#ifdef FEATURE_NATIVEAOT -#include "RhConfig.h" -#endif - #include "gcinfodecoder.h" #ifdef USE_GC_INFO_DECODER @@ -417,25 +413,7 @@ bool GcInfoDecoder::IsSafePoint() bool GcInfoDecoder::AreSafePointsInterruptible() { - if (m_Version < 3) - return false; - - // zero initialized - static bool fInterruptibleCallSitesInited; - static uint64_t fInterruptibleCallSites; - - if (!fInterruptibleCallSitesInited) - { -#ifdef FEATURE_NATIVEAOT - fInterruptibleCallSites = 1; - g_pRhConfig->ReadConfigValue("InterruptibleCallSites", &fInterruptibleCallSites); -#else - fInterruptibleCallSites = CLRConfig::GetConfigValue(CLRConfig::INTERNAL_InterruptibleCallSites); -#endif - fInterruptibleCallSitesInited = true; - } - - return fInterruptibleCallSites != 0; + return m_Version >= 3; } bool GcInfoDecoder::IsInterruptibleSafePoint() From 4d1895db886fa2dc71643812850011b572efea11 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 23 Feb 2024 12:25:02 -0800 Subject: [PATCH 21/44] JIT_StackProbe should not be a safepoint (stack is not cleaned yet) --- src/coreclr/jit/emit.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 158b6699506f76..2b1227f7e45fd8 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -2835,6 +2835,7 @@ bool emitter::emitNoGChelper(CorInfoHelpFunc helpFunc) case CORINFO_HELP_INIT_PINVOKE_FRAME: case CORINFO_HELP_FAIL_FAST: + case CORINFO_HELP_STACK_PROBE: case CORINFO_HELP_VALIDATE_INDIRECT_CALL: return true; From 93eede9d704e927e1ba18441f6af426f676c1622 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 23 Feb 2024 13:45:51 -0800 Subject: [PATCH 22/44] Hooked up GCInfo version to R2R file version --- src/coreclr/inc/gcinfo.h | 13 ++++++++++--- src/coreclr/inc/readytorun.h | 2 +- src/coreclr/nativeaot/Runtime/TypeManager.cpp | 9 ++++++--- .../Amd64/GcInfo.cs | 16 ++++++++++++---- .../ReadyToRunMethod.cs | 9 +++++++-- .../x86/GcInfo.cs | 2 +- 6 files changed, 37 insertions(+), 14 deletions(-) diff --git a/src/coreclr/inc/gcinfo.h b/src/coreclr/inc/gcinfo.h index 131c77c0265d87..2fa83d6de30b71 100644 --- a/src/coreclr/inc/gcinfo.h +++ b/src/coreclr/inc/gcinfo.h @@ -67,9 +67,16 @@ struct GCInfoToken static uint32_t ReadyToRunVersionToGcInfoVersion(uint32_t readyToRunMajorVersion, uint32_t readyToRunMinorVersion)) { - // TODO: VS versioning. For now assume the latest. - // 2.0 => 2 - // 2.1 => 3 + static_assert(MINIMUM_READYTORUN_MAJOR_VERSION == 9, + "when min version moves ahead, revisit the following logic, also check https://github.com/dotnet/runtime/issues/98871"); + + _ASSERT(readyToRunMajorVersion >= MINIMUM_READYTORUN_MAJOR_VERSION); + + // R2R 9.0 and 9.1 use GCInfo v2 + // R2R 9.2 uses GCInfo v3 + if (readyToRunMajorVersion == 9 && readyToRunMinorVersion < 2) + return 2; + return GCINFO_VERSION; } }; diff --git a/src/coreclr/inc/readytorun.h b/src/coreclr/inc/readytorun.h index 1c3ce8237ef7fc..a9050dae85f7ae 100644 --- a/src/coreclr/inc/readytorun.h +++ b/src/coreclr/inc/readytorun.h @@ -35,7 +35,7 @@ // R2R Version 9.1 adds new helpers to allocate objects on frozen segments // R2R Version 9.2 adds MemZero and NativeMemSet helpers // R2R Version 9.3 adds BulkWriteBarrier helper - +// uses GCInfo v3, which makes safe points in partially interruptible code interruptible. struct READYTORUN_CORE_HEADER { diff --git a/src/coreclr/nativeaot/Runtime/TypeManager.cpp b/src/coreclr/nativeaot/Runtime/TypeManager.cpp index 77eda2c4c60405..96dc357136d90a 100644 --- a/src/coreclr/nativeaot/Runtime/TypeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/TypeManager.cpp @@ -29,9 +29,12 @@ TypeManager * TypeManager::Create(HANDLE osModule, void * pModuleHeader, void** if (pReadyToRunHeader->Signature != ReadyToRunHeaderConstants::Signature) return nullptr; - // Only the current major version is supported currently - ASSERT(pReadyToRunHeader->MajorVersion == ReadyToRunHeaderConstants::CurrentMajorVersion); - if (pReadyToRunHeader->MajorVersion != ReadyToRunHeaderConstants::CurrentMajorVersion) + // Only the current version is supported currently + ASSERT((pReadyToRunHeader->MajorVersion == ReadyToRunHeaderConstants::CurrentMajorVersion) && + (pReadyToRunHeader->MinorVersion == ReadyToRunHeaderConstants::CurrentMinorVersion)); + + if ((pReadyToRunHeader->MajorVersion != ReadyToRunHeaderConstants::CurrentMajorVersion) || + (pReadyToRunHeader->MinorVersion != ReadyToRunHeaderConstants::CurrentMinorVersion)) return nullptr; return new (nothrow) TypeManager(osModule, pReadyToRunHeader, pClasslibFunctions, nClasslibFunctions); diff --git a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs index d01c636ef3b39e..fee7af776a16e2 100644 --- a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs +++ b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs @@ -86,7 +86,7 @@ public GcInfo() { } /// /// based on GcInfoDecoder::GcInfoDecoder /// - public GcInfo(byte[] image, int offset, Machine machine, ushort majorVersion) + public GcInfo(byte[] image, int offset, Machine machine, ushort majorVersion, ushort minorVersion) { Offset = offset; _gcInfoTypes = new GcInfoTypes(machine); @@ -101,7 +101,7 @@ public GcInfo(byte[] image, int offset, Machine machine, ushort majorVersion) SizeOfEditAndContinuePreservedArea = 0xffffffff; ReversePInvokeFrameStackSlot = -1; - Version = ReadyToRunVersionToGcInfoVersion(majorVersion); + Version = ReadyToRunVersionToGcInfoVersion(majorVersion, minorVersion); int bitOffset = offset * 8; ParseHeaderFlags(image, ref bitOffset); @@ -388,9 +388,17 @@ private List EnumerateInterruptibleRanges(byte[] image, int /// GcInfo version is 1 up to ReadyTorun version 1.x. /// GcInfo version is current from ReadyToRun version 2.0 /// - private int ReadyToRunVersionToGcInfoVersion(int readyToRunMajorVersion) + private int ReadyToRunVersionToGcInfoVersion(int readyToRunMajorVersion, int readyToRunMinorVersion) { - return (readyToRunMajorVersion == 1) ? 1 : GCINFO_VERSION; + if (readyToRunMajorVersion == 1) + return 1; + + // R2R 9.0 and 9.1 use GCInfo v2 + // R2R 9.2 uses GCInfo v3 + if (readyToRunMajorVersion == 9 && readyToRunMinorVersion < 2) + return 2; + + return GCINFO_VERSION; } private List> GetLiveSlotsAtSafepoints(byte[] image, ref int bitOffset) diff --git a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunMethod.cs b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunMethod.cs index 790c7142475b54..982d678cdce12e 100644 --- a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunMethod.cs +++ b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunMethod.cs @@ -492,12 +492,17 @@ private void EnsureInitialized() int gcInfoOffset = _readyToRunReader.CompositeReader.GetOffset(GcInfoRva); if (_readyToRunReader.Machine == Machine.I386) { - _gcInfo = new x86.GcInfo(_readyToRunReader.Image, gcInfoOffset, _readyToRunReader.Machine, _readyToRunReader.ReadyToRunHeader.MajorVersion); + _gcInfo = new x86.GcInfo(_readyToRunReader.Image, gcInfoOffset); } else { // Arm, Arm64, LoongArch64 and RISCV64 use the same GcInfo format as Amd64 - _gcInfo = new Amd64.GcInfo(_readyToRunReader.Image, gcInfoOffset, _readyToRunReader.Machine, _readyToRunReader.ReadyToRunHeader.MajorVersion); + _gcInfo = new Amd64.GcInfo( + _readyToRunReader.Image, + gcInfoOffset, + _readyToRunReader.Machine, + _readyToRunReader.ReadyToRunHeader.MajorVersion, + _readyToRunReader.ReadyToRunHeader.MinorVersion); } } } diff --git a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/x86/GcInfo.cs b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/x86/GcInfo.cs index 197fcfb1ee0324..ddb44a1d312ad1 100644 --- a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/x86/GcInfo.cs +++ b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/x86/GcInfo.cs @@ -20,7 +20,7 @@ public GcInfo() { } /// /// based on GCDump::DumpGCTable /// - public GcInfo(byte[] image, int offset, Machine machine, ushort majorVersion) + public GcInfo(byte[] image, int offset) { Offset = offset; From 8fa6a6f32dfd99225754b1c87344e22ec64e38db Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Fri, 23 Feb 2024 17:41:13 -0800 Subject: [PATCH 23/44] formatting --- src/coreclr/vm/gcinfodecoder.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index 214659424db997..c6a9660d475293 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -413,7 +413,7 @@ bool GcInfoDecoder::IsSafePoint() bool GcInfoDecoder::AreSafePointsInterruptible() { - return m_Version >= 3; + return m_Version >= 3; } bool GcInfoDecoder::IsInterruptibleSafePoint() From 5ac10c5c61206cb63104674bed72784e4243f2cb Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sun, 25 Feb 2024 14:37:50 -0800 Subject: [PATCH 24/44] GCStress support for RISC architectures --- src/coreclr/gcinfo/gcinfoencoder.cpp | 2 -- src/coreclr/inc/gcinfodecoder.h | 2 +- src/coreclr/vm/gccover.cpp | 26 ++++++++++++++++++++++++-- src/coreclr/vm/gcinfodecoder.cpp | 2 +- 4 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/coreclr/gcinfo/gcinfoencoder.cpp b/src/coreclr/gcinfo/gcinfoencoder.cpp index eb84ec3d8aa56d..c98802dd5602b4 100644 --- a/src/coreclr/gcinfo/gcinfoencoder.cpp +++ b/src/coreclr/gcinfo/gcinfoencoder.cpp @@ -961,9 +961,7 @@ bool GcInfoEncoder::DoNotTrackInPartiallyInterruptible(GcSlotDesc &slotDesc) return (regNum <= 17 || regNum >= 29) // X0 through X17 are scratch, FP/LR can't be used for objects around calls && regNum != 0 // X0 can contain return value -#ifdef UNIX_AMD64_ABI && regNum != 1 // X1 can contain return value -#endif ; } else if (!slotDesc.IsUntracked() && (slotDesc.Slot.Stack.Base == GC_SP_REL) && diff --git a/src/coreclr/inc/gcinfodecoder.h b/src/coreclr/inc/gcinfodecoder.h index 749d71f9fa8092..8534bc2508c5a4 100644 --- a/src/coreclr/inc/gcinfodecoder.h +++ b/src/coreclr/inc/gcinfodecoder.h @@ -512,7 +512,7 @@ class GcInfoDecoder // This is used for gccoverage bool IsSafePoint(UINT32 codeOffset); - typedef void EnumerateSafePointsCallback (UINT32 offset, void * hCallback); + typedef void EnumerateSafePointsCallback (GcInfoDecoder* decoder, UINT32 offset, void * hCallback); void EnumerateSafePoints(EnumerateSafePointsCallback * pCallback, void * hCallback); #endif diff --git a/src/coreclr/vm/gccover.cpp b/src/coreclr/vm/gccover.cpp index cede54465ebce2..b1c219d3317e67 100644 --- a/src/coreclr/vm/gccover.cpp +++ b/src/coreclr/vm/gccover.cpp @@ -37,7 +37,7 @@ MethodDesc* AsMethodDesc(size_t addr); static PBYTE getTargetOfCall(PBYTE instrPtr, PCONTEXT regs, PBYTE*nextInstr); #if defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) -static void replaceSafePointInstructionWithGcStressInstr(UINT32 safePointOffset, LPVOID codeStart); +static void replaceSafePointInstructionWithGcStressInstr(GcInfoDecoder* decoder, UINT32 safePointOffset, LPVOID codeStart); static bool replaceInterruptibleRangesWithGcStressInstr (UINT32 startOffset, UINT32 stopOffset, LPVOID codeStart); #endif @@ -684,7 +684,7 @@ enum #ifdef PARTIALLY_INTERRUPTIBLE_GC_SUPPORTED -void replaceSafePointInstructionWithGcStressInstr(UINT32 safePointOffset, LPVOID pGCCover) +void replaceSafePointInstructionWithGcStressInstr(GcInfoDecoder* decoder, UINT32 safePointOffset, LPVOID pGCCover) { PCODE pCode = NULL; IJitManager::MethodRegionInfo *ptr = &(((GCCoverageInfo*)pGCCover)->methodRegion); @@ -707,6 +707,28 @@ void replaceSafePointInstructionWithGcStressInstr(UINT32 safePointOffset, LPVOID PBYTE instrPtr = (BYTE*)PCODEToPINSTR(pCode); + // if this is an interruptible safe point, just replace it with an interrupt instr and we are done. + if (((GCCoverageInfo*)pGCCover)->codeMan->InterruptibleSafePointsEnabled() && decoder->AreSafePointsInterruptible()) + { + // The instruction about to be replaced cannot already be a gcstress instruction + _ASSERTE(!IsGcCoverageInterruptInstruction(instrPtr)); + + ExecutableWriterHolder instrPtrWriterHolder(instrPtr, sizeof(DWORD)); +#if defined(TARGET_ARM) + size_t instrLen = GetARMInstructionLength(instrPtr); + + if (instrLen == 2) + *((WORD*)instrPtrWriterHolder.GetRW()) = INTERRUPT_INSTR; + else + { + *((DWORD*)instrPtrWriterHolder.GetRW()) = INTERRUPT_INSTR_32; + } +#elif defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) + *((DWORD*)instrPtrWriterHolder.GetRW()) = INTERRUPT_INSTR; +#endif // TARGET_XXXX_ + return; + } + // For code sequences of the type // BL func1 // BL func2 // Safe point 1 diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index c6a9660d475293..c4fffc80e5035d 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -543,7 +543,7 @@ void GcInfoDecoder::EnumerateSafePoints(EnumerateSafePointsCallback *pCallback, offset--; #endif - pCallback(offset, hCallback); + pCallback(this, offset, hCallback); } } #endif From 43fb760f460094bbaebbfd972e55273e2c9cfe31 Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Sun, 25 Feb 2024 14:39:15 -0800 Subject: [PATCH 25/44] Update src/coreclr/inc/gcinfo.h Co-authored-by: Jan Kotas --- src/coreclr/inc/gcinfo.h | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/coreclr/inc/gcinfo.h b/src/coreclr/inc/gcinfo.h index 2fa83d6de30b71..81ef941a79f445 100644 --- a/src/coreclr/inc/gcinfo.h +++ b/src/coreclr/inc/gcinfo.h @@ -67,15 +67,13 @@ struct GCInfoToken static uint32_t ReadyToRunVersionToGcInfoVersion(uint32_t readyToRunMajorVersion, uint32_t readyToRunMinorVersion)) { - static_assert(MINIMUM_READYTORUN_MAJOR_VERSION == 9, - "when min version moves ahead, revisit the following logic, also check https://github.com/dotnet/runtime/issues/98871"); - - _ASSERT(readyToRunMajorVersion >= MINIMUM_READYTORUN_MAJOR_VERSION); - +// Delete once MINIMUM_READYTORUN_MAJOR_VERSION is bumped to 10+ +#if MINIMUM_READYTORUN_MAJOR_VERSION < 10 // R2R 9.0 and 9.1 use GCInfo v2 // R2R 9.2 uses GCInfo v3 if (readyToRunMajorVersion == 9 && readyToRunMinorVersion < 2) return 2; +#endif return GCINFO_VERSION; } From b17b42edd4191925d436e749c2a7f0d122d87df7 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sun, 25 Feb 2024 14:47:14 -0800 Subject: [PATCH 26/44] make InterruptibleSafePointsEnabled static --- src/coreclr/inc/eetwain.h | 5 +---- src/coreclr/vm/gccover.cpp | 6 +++--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/coreclr/inc/eetwain.h b/src/coreclr/inc/eetwain.h index 41f43708887b7c..c7b1be02e5c638 100644 --- a/src/coreclr/inc/eetwain.h +++ b/src/coreclr/inc/eetwain.h @@ -214,9 +214,6 @@ virtual bool UnwindStackFrame(PREGDISPLAY pContext, virtual bool IsGcSafe(EECodeInfo *pCodeInfo, DWORD dwRelOffset) = 0; -virtual -bool InterruptibleSafePointsEnabled() = 0; - #if defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) virtual bool HasTailCalls(EECodeInfo *pCodeInfo) = 0; #endif // TARGET_ARM || TARGET_ARM64 || TARGET_LOONGARCH64 || TARGET_RISCV64 @@ -460,7 +457,7 @@ virtual bool IsGcSafe( EECodeInfo *pCodeInfo, DWORD dwRelOffset); -virtual +static bool InterruptibleSafePointsEnabled(); #if defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) diff --git a/src/coreclr/vm/gccover.cpp b/src/coreclr/vm/gccover.cpp index b1c219d3317e67..a5656c5391be31 100644 --- a/src/coreclr/vm/gccover.cpp +++ b/src/coreclr/vm/gccover.cpp @@ -543,7 +543,7 @@ void GCCoverageInfo::SprinkleBreakpoints( { case InstructionType::Call_IndirectUnconditional: #ifdef TARGET_AMD64 - if(!(codeMan->InterruptibleSafePointsEnabled() && safePointDecoder.AreSafePointsInterruptible()) && + if(!(EECodeManager::InterruptibleSafePointsEnabled() && safePointDecoder.AreSafePointsInterruptible()) && safePointDecoder.IsSafePoint((UINT32)(cur + len - codeStart + regionOffsetAdj))) #endif { @@ -555,7 +555,7 @@ void GCCoverageInfo::SprinkleBreakpoints( if(fGcStressOnDirectCalls.val(CLRConfig::INTERNAL_GcStressOnDirectCalls)) { #ifdef TARGET_AMD64 - if(!(codeMan->InterruptibleSafePointsEnabled() && safePointDecoder.AreSafePointsInterruptible()) && + if(!(EECodeManager::InterruptibleSafePointsEnabled() && safePointDecoder.AreSafePointsInterruptible()) && safePointDecoder.IsSafePoint((UINT32)(cur + len - codeStart + regionOffsetAdj))) #endif { @@ -708,7 +708,7 @@ void replaceSafePointInstructionWithGcStressInstr(GcInfoDecoder* decoder, UINT32 PBYTE instrPtr = (BYTE*)PCODEToPINSTR(pCode); // if this is an interruptible safe point, just replace it with an interrupt instr and we are done. - if (((GCCoverageInfo*)pGCCover)->codeMan->InterruptibleSafePointsEnabled() && decoder->AreSafePointsInterruptible()) + if (EECodeManager::InterruptibleSafePointsEnabled() && decoder->AreSafePointsInterruptible()) { // The instruction about to be replaced cannot already be a gcstress instruction _ASSERTE(!IsGcCoverageInterruptInstruction(instrPtr)); From 855dd1f8f559c32a42349bb1c7fdaf85089abd6b Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sun, 25 Feb 2024 15:48:59 -0800 Subject: [PATCH 27/44] fix linux-x86 build. --- src/coreclr/inc/gcinfo.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/inc/gcinfo.h b/src/coreclr/inc/gcinfo.h index 81ef941a79f445..297c25f9346cb2 100644 --- a/src/coreclr/inc/gcinfo.h +++ b/src/coreclr/inc/gcinfo.h @@ -67,13 +67,13 @@ struct GCInfoToken static uint32_t ReadyToRunVersionToGcInfoVersion(uint32_t readyToRunMajorVersion, uint32_t readyToRunMinorVersion)) { -// Delete once MINIMUM_READYTORUN_MAJOR_VERSION is bumped to 10+ -#if MINIMUM_READYTORUN_MAJOR_VERSION < 10 + // Once MINIMUM_READYTORUN_MAJOR_VERSION is bumped to 10+ + // delete the following and just return GCINFO_VERSION + // // R2R 9.0 and 9.1 use GCInfo v2 // R2R 9.2 uses GCInfo v3 if (readyToRunMajorVersion == 9 && readyToRunMinorVersion < 2) return 2; -#endif return GCINFO_VERSION; } From 8fb65ba757979672d9d823be5da3cead7455dd7b Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 27 Feb 2024 14:42:34 -0800 Subject: [PATCH 28/44] ARM32 actually can`t return 2 GC references, so can filter out R1 early --- src/coreclr/gcinfo/gcinfoencoder.cpp | 1 - src/coreclr/jit/codegenarmarch.cpp | 6 ++++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/coreclr/gcinfo/gcinfoencoder.cpp b/src/coreclr/gcinfo/gcinfoencoder.cpp index c98802dd5602b4..0abf65047b8d9a 100644 --- a/src/coreclr/gcinfo/gcinfoencoder.cpp +++ b/src/coreclr/gcinfo/gcinfoencoder.cpp @@ -939,7 +939,6 @@ bool GcInfoEncoder::DoNotTrackInPartiallyInterruptible(GcSlotDesc &slotDesc) return ((regNum <= 3) || (regNum >= 12)) // R12 is volatile and SP/LR can't contain objects around calls && regNum != 0 // R0 can contain return value - && regNum != 1 // R1 can contain return value ; } else if (!slotDesc.IsUntracked() && (slotDesc.Slot.Stack.Base == GC_SP_REL) && diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index cbd74f8dc016bc..1cb7ae7985c274 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -3526,6 +3526,12 @@ void CodeGen::genCallInstruction(GenTreeCall* call) } } +#ifdef TARGET_ARM + // ARM32 support multireg returns, but only to return 64bit primitives. + assert(secondRetSize != EA_GCREF); + assert(secondRetSize != EA_BYREF); +#endif + DebugInfo di; // We need to propagate the debug information to the call instruction, so we can emit // an IL to native mapping record for the call, to support managed return value debugging. From cb84de742c621fedb7e5fb5dbcf6017bfaa4008a Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 28 Feb 2024 15:38:22 -0800 Subject: [PATCH 29/44] revert unnecessary change --- src/coreclr/jit/gcencode.cpp | 64 +++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 31 deletions(-) diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index 50b06ca19456a7..6e1fa8bab9b7cf 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4551,49 +4551,51 @@ void GCInfo::gcMakeRegPtrTable( for (regPtrDsc* genRegPtrTemp = gcRegPtrList; genRegPtrTemp != nullptr; genRegPtrTemp = genRegPtrTemp->rpdNext) { - // Is this a call site? - if (genRegPtrTemp->rpdIsCallInstr()) + if (genRegPtrTemp->rpdArg) { - // This is a true call site. + // Is this a call site? + if (genRegPtrTemp->rpdIsCallInstr()) + { + // This is a true call site. - regMaskSmall gcrefRegMask = genRegMaskFromCalleeSavedMask(genRegPtrTemp->rpdCallGCrefRegs); + regMaskSmall gcrefRegMask = genRegMaskFromCalleeSavedMask(genRegPtrTemp->rpdCallGCrefRegs); - regMaskSmall byrefRegMask = genRegMaskFromCalleeSavedMask(genRegPtrTemp->rpdCallByrefRegs); + regMaskSmall byrefRegMask = genRegMaskFromCalleeSavedMask(genRegPtrTemp->rpdCallByrefRegs); - assert((gcrefRegMask & byrefRegMask) == 0); + assert((gcrefRegMask & byrefRegMask) == 0); - regMaskSmall regMask = gcrefRegMask | byrefRegMask; + regMaskSmall regMask = gcrefRegMask | byrefRegMask; - // The "rpdOffs" is (apparently) the offset of the following instruction already. - // GcInfoEncoder wants the call instruction, so subtract the width of the call instruction. - assert(genRegPtrTemp->rpdOffs >= genRegPtrTemp->rpdCallInstrSize); - unsigned callOffset = genRegPtrTemp->rpdOffs - genRegPtrTemp->rpdCallInstrSize; + // The "rpdOffs" is (apparently) the offset of the following instruction already. + // GcInfoEncoder wants the call instruction, so subtract the width of the call instruction. + assert(genRegPtrTemp->rpdOffs >= genRegPtrTemp->rpdCallInstrSize); + unsigned callOffset = genRegPtrTemp->rpdOffs - genRegPtrTemp->rpdCallInstrSize; - // Tell the GCInfo encoder about these registers. We say that the registers become live - // before the call instruction, and dead after. - gcInfoRecordGCRegStateChange(gcInfoEncoder, mode, callOffset, regMask, GC_SLOT_LIVE, byrefRegMask, - nullptr); - gcInfoRecordGCRegStateChange(gcInfoEncoder, mode, genRegPtrTemp->rpdOffs, regMask, GC_SLOT_DEAD, - byrefRegMask, nullptr); + // Tell the GCInfo encoder about these registers. We say that the registers become live + // before the call instruction, and dead after. + gcInfoRecordGCRegStateChange(gcInfoEncoder, mode, callOffset, regMask, GC_SLOT_LIVE, byrefRegMask, + nullptr); + gcInfoRecordGCRegStateChange(gcInfoEncoder, mode, genRegPtrTemp->rpdOffs, regMask, GC_SLOT_DEAD, + byrefRegMask, nullptr); - // Also remember the call site. - if (mode == MAKE_REG_PTR_MODE_DO_WORK) + // Also remember the call site. + if (mode == MAKE_REG_PTR_MODE_DO_WORK) + { + assert(pCallSites != nullptr && pCallSiteSizes != nullptr); + pCallSites[callSiteNum] = callOffset; + pCallSiteSizes[callSiteNum] = genRegPtrTemp->rpdCallInstrSize; + callSiteNum++; + } + } + else { - assert(pCallSites != nullptr && pCallSiteSizes != nullptr); - pCallSites[callSiteNum] = callOffset; - pCallSiteSizes[callSiteNum] = genRegPtrTemp->rpdCallInstrSize; - callSiteNum++; + // These are reporting outgoing stack arguments, but we don't need to report anything + // for partially interruptible + assert(genRegPtrTemp->rpdGCtypeGet() != GCT_NONE); + assert(genRegPtrTemp->rpdArgTypeGet() == rpdARG_PUSH); } } - else if (genRegPtrTemp->rpdArg) - { - // These are reporting outgoing stack arguments, but we don't need to report anything - // for partially interruptible - assert(genRegPtrTemp->rpdGCtypeGet() != GCT_NONE); - assert(genRegPtrTemp->rpdArgTypeGet() == rpdARG_PUSH); - } } - // The routine is fully interruptible. if (mode == MAKE_REG_PTR_MODE_DO_WORK) { From d1cb7795af63ed30e9439c9ed9b751e76a6ec9c8 Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Fri, 1 Mar 2024 21:14:18 -0800 Subject: [PATCH 30/44] Update src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs Co-authored-by: Filip Navara --- .../tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs index fee7af776a16e2..54c5cd615c7103 100644 --- a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs +++ b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs @@ -395,7 +395,7 @@ private int ReadyToRunVersionToGcInfoVersion(int readyToRunMajorVersion, int rea // R2R 9.0 and 9.1 use GCInfo v2 // R2R 9.2 uses GCInfo v3 - if (readyToRunMajorVersion == 9 && readyToRunMinorVersion < 2) + if (readyToRunMajorVersion < 9 || (readyToRunMajorVersion == 9 && readyToRunMinorVersion < 2)) return 2; return GCINFO_VERSION; From 7ad04bcc6b47da8281fd6e7b5dad4449476afb8d Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Fri, 1 Mar 2024 21:22:22 -0800 Subject: [PATCH 31/44] removed GCINFO_VERSION cons from GcInfo.cs --- .../tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs index 54c5cd615c7103..0b1c98a939e138 100644 --- a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs +++ b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs @@ -46,7 +46,6 @@ public SafePointOffset(int index, uint value) } } - private const int GCINFO_VERSION = 2; private const int MIN_GCINFO_VERSION_WITH_RETURN_KIND = 2; private const int MIN_GCINFO_VERSION_WITH_REV_PINVOKE_FRAME = 2; @@ -398,7 +397,7 @@ private int ReadyToRunVersionToGcInfoVersion(int readyToRunMajorVersion, int rea if (readyToRunMajorVersion < 9 || (readyToRunMajorVersion == 9 && readyToRunMinorVersion < 2)) return 2; - return GCINFO_VERSION; + return 3; } private List> GetLiveSlotsAtSafepoints(byte[] image, ref int bitOffset) From 3702386b81e4c06fbfdb4fa522748767e6ef944e Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Fri, 1 Mar 2024 22:02:05 -0800 Subject: [PATCH 32/44] Use RBM_INTRET/RBM_INTRET_1 --- src/coreclr/jit/targetamd64.h | 8 ++++---- src/coreclr/jit/targetarm.h | 4 ++-- src/coreclr/jit/targetarm64.h | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/targetamd64.h b/src/coreclr/jit/targetamd64.h index 417239dcc8d431..1575177643eb1e 100644 --- a/src/coreclr/jit/targetamd64.h +++ b/src/coreclr/jit/targetamd64.h @@ -301,8 +301,8 @@ #define CNT_CALLEE_TRASH_FLOAT_INIT (16) #define CNT_CALLEE_TRASH_HIGHFLOAT (16) /* NOTE: Sync with variable name defined in compiler.h */ - #define RBM_CALL_GC_REGS_ORDER RBM_EBX,RBM_ETW_FRAMED_EBP_LIST RBM_R12,RBM_R13,RBM_R14,RBM_R15,RBM_RAX,RBM_RDX - #define RBM_CALL_GC_REGS (RBM_EBX|RBM_ETW_FRAMED_EBP|RBM_R12|RBM_R13|RBM_R14|RBM_R15|RBM_RAX|RBM_RDX) + #define RBM_CALL_GC_REGS_ORDER RBM_EBX,RBM_ETW_FRAMED_EBP_LIST RBM_R12,RBM_R13,RBM_R14,RBM_R15,RBM_INTRET,RBM_INTRET_1 + #define RBM_CALL_GC_REGS (RBM_EBX|RBM_ETW_FRAMED_EBP|RBM_R12|RBM_R13|RBM_R14|RBM_R15|RBM_INTRET|RBM_INTRET_1) // For SysV we have more volatile registers so we do not save any callee saves for EnC. #define RBM_ENC_CALLEE_SAVED 0 @@ -316,8 +316,8 @@ #define CNT_CALLEE_TRASH_FLOAT_INIT (6) #define CNT_CALLEE_TRASH_HIGHFLOAT (16) /* NOTE: Sync with variable name defined in compiler.h */ - #define RBM_CALL_GC_REGS_ORDER RBM_EBX,RBM_ESI,RBM_EDI,RBM_ETW_FRAMED_EBP_LIST RBM_R12,RBM_R13,RBM_R14,RBM_R15,RBM_RAX - #define RBM_CALL_GC_REGS (RBM_EBX|RBM_ESI|RBM_EDI|RBM_ETW_FRAMED_EBP|RBM_R12|RBM_R13|RBM_R14|RBM_R15|RBM_RAX) + #define RBM_CALL_GC_REGS_ORDER RBM_EBX,RBM_ESI,RBM_EDI,RBM_ETW_FRAMED_EBP_LIST RBM_R12,RBM_R13,RBM_R14,RBM_R15,RBM_INTRET + #define RBM_CALL_GC_REGS (RBM_EBX|RBM_ESI|RBM_EDI|RBM_ETW_FRAMED_EBP|RBM_R12|RBM_R13|RBM_R14|RBM_R15|RBM_INTRET) // Callee-preserved registers we always save and allow use of for EnC code, since there are quite few volatile registers. #define RBM_ENC_CALLEE_SAVED (RBM_RSI | RBM_RDI) diff --git a/src/coreclr/jit/targetarm.h b/src/coreclr/jit/targetarm.h index 53ce844b17cc92..46c41329b0565b 100644 --- a/src/coreclr/jit/targetarm.h +++ b/src/coreclr/jit/targetarm.h @@ -90,8 +90,8 @@ #define RBM_LOW_REGS (RBM_R0|RBM_R1|RBM_R2|RBM_R3|RBM_R4|RBM_R5|RBM_R6|RBM_R7) #define RBM_HIGH_REGS (RBM_R8|RBM_R9|RBM_R10|RBM_R11|RBM_R12|RBM_SP|RBM_LR|RBM_PC) - #define RBM_CALL_GC_REGS_ORDER RBM_R4,RBM_R5,RBM_R6,RBM_R7,RBM_R8,RBM_R9,RBM_R10,RBM_R11,RBM_R0 - #define RBM_CALL_GC_REGS (RBM_R4|RBM_R5|RBM_R6|RBM_R7|RBM_R8|RBM_R9|RBM_R10|RBM_R11|RBM_R0) + #define RBM_CALL_GC_REGS_ORDER RBM_R4,RBM_R5,RBM_R6,RBM_R7,RBM_R8,RBM_R9,RBM_R10,RBM_R11,RBM_INTRET + #define RBM_CALL_GC_REGS (RBM_R4|RBM_R5|RBM_R6|RBM_R7|RBM_R8|RBM_R9|RBM_R10|RBM_R11|RBM_INTRET) #define CNT_CALLEE_SAVED (8) #define CNT_CALLEE_TRASH (6) diff --git a/src/coreclr/jit/targetarm64.h b/src/coreclr/jit/targetarm64.h index 590b0c05927df6..8f5f1d68a6ef43 100644 --- a/src/coreclr/jit/targetarm64.h +++ b/src/coreclr/jit/targetarm64.h @@ -102,8 +102,8 @@ REG_V12, REG_V13, REG_V14, REG_V15, \ REG_V3, REG_V2, REG_V1, REG_V0 - #define RBM_CALL_GC_REGS_ORDER RBM_R19,RBM_R20,RBM_R21,RBM_R22,RBM_R23,RBM_R24,RBM_R25,RBM_R26,RBM_R27,RBM_R28,RBM_R0,RBM_R1 - #define RBM_CALL_GC_REGS (RBM_R19|RBM_R20|RBM_R21|RBM_R22|RBM_R23|RBM_R24|RBM_R25|RBM_R26|RBM_R27|RBM_R28|RBM_R0|RBM_R1) + #define RBM_CALL_GC_REGS_ORDER RBM_R19,RBM_R20,RBM_R21,RBM_R22,RBM_R23,RBM_R24,RBM_R25,RBM_R26,RBM_R27,RBM_R28,RBM_INTRET,RBM_INTRET_1 + #define RBM_CALL_GC_REGS (RBM_R19|RBM_R20|RBM_R21|RBM_R22|RBM_R23|RBM_R24|RBM_R25|RBM_R26|RBM_R27|RBM_R28|RBM_INTRET|RBM_INTRET_1) #define CNT_CALLEE_SAVED (11) #define CNT_CALLEE_TRASH (17) From 7b6506d38ea1e15f3e4c2e6558a94c03fb06873e Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Mon, 4 Mar 2024 11:16:01 -0800 Subject: [PATCH 33/44] Update src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs Co-authored-by: Jan Kotas --- .../aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs index 0b1c98a939e138..b934e36719e8a2 100644 --- a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs +++ b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs @@ -392,8 +392,8 @@ private int ReadyToRunVersionToGcInfoVersion(int readyToRunMajorVersion, int rea if (readyToRunMajorVersion == 1) return 1; - // R2R 9.0 and 9.1 use GCInfo v2 - // R2R 9.2 uses GCInfo v3 + // R2R 2.0+ uses GCInfo v2 + // R2R 9.2+ uses GCInfo v3 if (readyToRunMajorVersion < 9 || (readyToRunMajorVersion == 9 && readyToRunMinorVersion < 2)) return 2; From a816dc06220a312ef5ab1e8266cd4f357450654a Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Mon, 4 Mar 2024 10:50:08 -0800 Subject: [PATCH 34/44] do not skip safe points twice (stress failure) --- src/coreclr/vm/gcinfodecoder.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index c4fffc80e5035d..66347d8bf8d28c 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -374,8 +374,7 @@ GcInfoDecoder::GcInfoDecoder( m_SafePointIndex = FindSafePoint(offset); } } - - if(flags & (DECODE_FOR_RANGES_CALLBACK | DECODE_INTERRUPTIBILITY)) + else if(flags & DECODE_FOR_RANGES_CALLBACK) { // Note that normalization as a code offset can be different than // normalization as code length @@ -386,6 +385,11 @@ GcInfoDecoder::GcInfoDecoder( } #endif + // we do not support both DECODE_INTERRUPTIBILITY and DECODE_FOR_RANGES_CALLBACK at the same time + // as both will enumerate and consume interruptible ranges. + _ASSERTE((flags & (DECODE_INTERRUPTIBILITY | DECODE_FOR_RANGES_CALLBACK)) != + (DECODE_INTERRUPTIBILITY | DECODE_FOR_RANGES_CALLBACK)); + _ASSERTE(!m_IsInterruptible); if(flags & DECODE_INTERRUPTIBILITY) { From ef9bd7d7ad0fc01a23a0db1a716b9a93a497d10a Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Mon, 4 Mar 2024 23:37:31 -0800 Subject: [PATCH 35/44] revert unnecessary change in gccover. --- src/coreclr/vm/gccover.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/coreclr/vm/gccover.cpp b/src/coreclr/vm/gccover.cpp index a5656c5391be31..70ab39ea681b02 100644 --- a/src/coreclr/vm/gccover.cpp +++ b/src/coreclr/vm/gccover.cpp @@ -264,6 +264,15 @@ void SetupGcCoverage(NativeCodeVersion nativeCodeVersion, BYTE* methodStartPtr) void ReplaceInstrAfterCall(PBYTE instrToReplace, MethodDesc* callMD) { ReturnKind returnKind = callMD->GetReturnKind(true); + if (!IsValidReturnKind(returnKind)) + { +#if defined(TARGET_AMD64) && defined(TARGET_UNIX) + _ASSERTE(!"Unexpected return kind for x64 Unix."); +#else + // SKip GC coverage after the call. + return; +#endif + } _ASSERTE(IsValidReturnKind(returnKind)); bool ispointerKind = IsPointerReturnKind(returnKind); From d6b2b9f271d8c87f6547c6f5f2c5d2d55e808286 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 6 Mar 2024 10:26:24 -0800 Subject: [PATCH 36/44] fix after rebase --- src/coreclr/inc/gcinfo.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/inc/gcinfo.h b/src/coreclr/inc/gcinfo.h index 297c25f9346cb2..d526405c9f2cff 100644 --- a/src/coreclr/inc/gcinfo.h +++ b/src/coreclr/inc/gcinfo.h @@ -65,7 +65,7 @@ struct GCInfoToken } #endif - static uint32_t ReadyToRunVersionToGcInfoVersion(uint32_t readyToRunMajorVersion, uint32_t readyToRunMinorVersion)) + static uint32_t ReadyToRunVersionToGcInfoVersion(uint32_t readyToRunMajorVersion, uint32_t readyToRunMinorVersion) { // Once MINIMUM_READYTORUN_MAJOR_VERSION is bumped to 10+ // delete the following and just return GCINFO_VERSION From 06d681ee55c3e5bc37c0e7ff4b766119a9c9727e Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 6 Mar 2024 19:57:27 -0800 Subject: [PATCH 37/44] make sure to check `idIsNoGC` on all codepaths in `emitOutputInstr` --- src/coreclr/jit/emitxarch.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index c67321d5eae706..715c4a26fb3f40 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -16497,9 +16497,6 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) case IF_METHOD: case IF_METHPTR: { - // Assume we'll be recording this call - recCall = true; - // Get hold of the argument count and field Handle args = emitGetInsCDinfo(id); @@ -16526,12 +16523,6 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) addr = (BYTE*)id->idAddr()->iiaAddr; assert(addr != nullptr); - // Some helpers don't get recorded in GC tables - if (id->idIsNoGC()) - { - recCall = false; - } - // What kind of a call do we have here? if (id->idInsFmt() == IF_METHPTR) { @@ -16617,6 +16608,15 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) DONE_CALL: + // Assume we'll be recording this call + recCall = true; + + // Some helpers don't get recorded in GC tables + if (id->idIsNoGC()) + { + recCall = false; + } + /* We update the variable (not register) GC info before the call as the variables cannot be used by the call. Killing variables before the call helps with boundary conditions if the call is CORINFO_HELP_THROW - see bug 50029. @@ -16989,8 +16989,6 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) sz = sizeof(instrDesc); } - recCall = true; - goto DONE_CALL; default: From 52e2fd60cba4d509af3c79270b9502797bbed0e4 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Thu, 7 Mar 2024 15:22:48 -0800 Subject: [PATCH 38/44] make CORINFO_HELP_CHECK_OBJ a no-gc helper (because we can) --- src/coreclr/jit/emit.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 2b1227f7e45fd8..331887c52bdefa 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -2837,6 +2837,8 @@ bool emitter::emitNoGChelper(CorInfoHelpFunc helpFunc) case CORINFO_HELP_FAIL_FAST: case CORINFO_HELP_STACK_PROBE: + case CORINFO_HELP_CHECK_OBJ: + case CORINFO_HELP_VALIDATE_INDIRECT_CALL: return true; From 0a938e0ec94c959a5f6abae26c1b9df95c552035 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 9 Mar 2024 08:08:00 -0800 Subject: [PATCH 39/44] mark a test that tests WaitForPendingFinalizers as GCStressIncompatible --- src/tests/JIT/Directed/lifetime/lifetime2.csproj | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tests/JIT/Directed/lifetime/lifetime2.csproj b/src/tests/JIT/Directed/lifetime/lifetime2.csproj index 7913879515ebfc..0e79bac317c9b6 100644 --- a/src/tests/JIT/Directed/lifetime/lifetime2.csproj +++ b/src/tests/JIT/Directed/lifetime/lifetime2.csproj @@ -10,6 +10,7 @@ None True True + true From dc123798aa4587ed5691bf71732cc82892ff0e01 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 9 Apr 2024 10:14:36 -0700 Subject: [PATCH 40/44] NOP --- src/coreclr/jit/codegencommon.cpp | 4 +-- src/coreclr/jit/codegenlinear.cpp | 2 +- src/coreclr/jit/emit.cpp | 48 +++++++++++++++++++++++++++-- src/coreclr/jit/emit.h | 34 ++++++++++++++------ src/coreclr/jit/emitarm.cpp | 10 ++++++ src/coreclr/jit/emitarm64.cpp | 20 ++++++++++++ src/coreclr/jit/emitloongarch64.cpp | 20 ++++++++++++ src/coreclr/jit/emitriscv64.cpp | 20 ++++++++++++ src/coreclr/jit/emitxarch.cpp | 28 +++++++++++++++-- src/coreclr/jit/jitgcinfo.h | 4 +-- 10 files changed, 169 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 6dcbba13b48533..27fd021ba3d2c2 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -997,8 +997,8 @@ void CodeGen::genLogLabel(BasicBlock* bb) void CodeGen::genDefineTempLabel(BasicBlock* label) { genLogLabel(label); - label->bbEmitCookie = GetEmitter()->emitAddLabel(gcInfo.gcVarPtrSetCur, gcInfo.gcRegGCrefSetCur, - gcInfo.gcRegByrefSetCur DEBUG_ARG(label)); + label->bbEmitCookie = + GetEmitter()->emitAddLabel(gcInfo.gcVarPtrSetCur, gcInfo.gcRegGCrefSetCur, gcInfo.gcRegByrefSetCur); } // genDefineInlineTempLabel: Define an inline label that does not affect the GC diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 13290c44f3e5a1..4c3d53acfa5db8 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -348,7 +348,7 @@ void CodeGen::genCodeForBBlist() // Mark a label and update the current set of live GC refs block->bbEmitCookie = GetEmitter()->emitAddLabel(gcInfo.gcVarPtrSetCur, gcInfo.gcRegGCrefSetCur, - gcInfo.gcRegByrefSetCur DEBUG_ARG(block)); + gcInfo.gcRegByrefSetCur, block->Prev()); } if (block->IsFirstColdBlock(compiler)) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 331887c52bdefa..d0b14daea201d2 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -2217,6 +2217,10 @@ void emitter::emitCreatePlaceholderIG(insGroupPlaceholderType igType, emitCurIG->igFlags &= ~IGF_PROPAGATE_MASK; } + // since we have emitted a placeholder, the last ins is not longer the last. + emitLastIns = nullptr; + emitLastInsIG = nullptr; + #ifdef DEBUG if (emitComp->verbose) { @@ -2873,10 +2877,36 @@ bool emitter::emitNoGChelper(CORINFO_METHOD_HANDLE methHnd) * Mark the current spot as having a label. */ -void* emitter::emitAddLabel(VARSET_VALARG_TP GCvars, - regMaskTP gcrefRegs, - regMaskTP byrefRegs DEBUG_ARG(BasicBlock* block)) +void* emitter::emitAddLabel(VARSET_VALARG_TP GCvars, regMaskTP gcrefRegs, regMaskTP byrefRegs, BasicBlock* prevBlock) { + if (prevBlock != NULL && emitLastInsIsCallWithGC()) + { + // We have just emitted a call that can do GC and conservatively recorded what is alive after the call. + // Now we see that the next instruction may be reachable by a branch with a different liveness. + // We want to maintain the invariant that the GC info at IP after a GC-capable call is the same + // regardless how it is reached. + // One way to fix this is to fish out the call instruction and patch its GC info, but we must be + // certain that the current IP is indeed reachable after the call. + // Another way it to add an instruction (NOP or BRK) after the call. + if (emitThisGCrefRegs != gcrefRegs || emitThisByrefRegs != byrefRegs || + !VarSetOps::Equal(emitComp, emitThisGCrefVars, GCvars)) + { + if (prevBlock->KindIs(BBJ_THROW)) + { + emitIns(INS_BREAKPOINT); + } + else + { + // other block kinds should emit something at the end that is not a call. + assert(prevBlock->KindIs(BBJ_ALWAYS)); + // CONSIDER: We could patch up the previous call instruction with new GC info instead. + // But that will need to be coordinated with how the GC info vor variables is used. + // We currently apply that info to the instruction before the call. It may need to change. + emitIns(INS_nop); + } + } + } + /* Create a new IG if the current one is non-empty */ if (emitCurIGnonEmpty()) @@ -3637,6 +3667,7 @@ emitter::instrDesc* emitter::emitNewInstrCallInd(int argCnt, /* Make sure we didn't waste space unexpectedly */ assert(!id->idIsLargeCns()); + id->idSetIsCall(); #ifdef TARGET_XARCH /* Store the displacement and make sure the value fit */ @@ -3716,6 +3747,7 @@ emitter::instrDesc* emitter::emitNewInstrCallDir(int argCnt, /* Make sure we didn't waste space unexpectedly */ assert(!id->idIsLargeCns()); + id->idSetIsCall(); /* Save the live GC registers in the unused register fields */ assert((gcrefRegs & RBM_CALLEE_TRASH) == 0); @@ -8725,6 +8757,16 @@ void emitter::emitUpdateLiveGCvars(VARSET_VALARG_TP vars, BYTE* addr) emitThisGCrefVset = true; } +/***************************************************************************** + * + * Last emitted instruction is a call and it is not a NoGC call. + */ + +bool emitter::emitLastInsIsCallWithGC() +{ + return emitLastIns != nullptr && emitLastIns->idIsCall() && !emitLastIns->idIsNoGC(); +} + /***************************************************************************** * * Record a call location for GC purposes (we know that this is a method that diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 4fb37df5149111..3e74bca557263d 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -762,10 +762,10 @@ class emitter // loongarch64: 28 bits // risc-v: 28 bits - unsigned _idSmallDsc : 1; // is this a "small" descriptor? - unsigned _idLargeCns : 1; // does a large constant follow? - unsigned _idLargeDsp : 1; // does a large displacement follow? - unsigned _idLargeCall : 1; // large call descriptor used + unsigned _idSmallDsc : 1; // is this a "small" descriptor? + unsigned _idLargeCns : 1; // does a large constant follow? (or if large call descriptor used) + unsigned _idLargeDsp : 1; // does a large displacement follow? + unsigned _idCall : 1; // this is a call // We have several pieces of information we need to encode but which are only applicable // to a subset of instrDescs. To accommodate that, we define a several _idCustom# bitfields @@ -1565,7 +1565,7 @@ class emitter bool idIsLargeCns() const { - return _idLargeCns != 0; + return _idLargeCns != 0 && !idIsCall(); } void idSetIsLargeCns() { @@ -1585,13 +1585,23 @@ class emitter _idLargeDsp = 0; } + bool idIsCall() const + { + return _idCall != 0; + } + void idSetIsCall() + { + _idCall = 1; + } + bool idIsLargeCall() const { - return _idLargeCall != 0; + return idIsCall() && _idLargeCns == 1; } void idSetIsLargeCall() { - _idLargeCall = 1; + idSetIsCall(); + _idLargeCns = 1; } bool idIsBound() const @@ -2857,9 +2867,11 @@ class emitter // Mark this instruction group as having a label; return the new instruction group. // Sets the emitter's record of the currently live GC variables // and registers. - void* emitAddLabel(VARSET_VALARG_TP GCvars, - regMaskTP gcrefRegs, - regMaskTP byrefRegs DEBUG_ARG(BasicBlock* block = nullptr)); + // prevBlock is passed when starting a new block. + void* emitAddLabel(VARSET_VALARG_TP GCvars, + regMaskTP gcrefRegs, + regMaskTP byrefRegs, + BasicBlock* prevBlock = nullptr); // Same as above, except the label is added and is conceptually "inline" in // the current block. Thus it extends the previous block and the emitter @@ -3190,6 +3202,8 @@ class emitter void emitRecordGCcall(BYTE* codePos, unsigned char callInstrSize); + bool emitLastInsIsCallWithGC(); + // Helpers for the above void emitStackPushLargeStk(BYTE* addr, GCtype gcType, unsigned count = 1); diff --git a/src/coreclr/jit/emitarm.cpp b/src/coreclr/jit/emitarm.cpp index 87c29c7f5333d6..ea88e01adbd90b 100644 --- a/src/coreclr/jit/emitarm.cpp +++ b/src/coreclr/jit/emitarm.cpp @@ -4754,6 +4754,16 @@ void emitter::emitIns_Call(EmitCallType callType, /* Update the emitter's live GC ref sets */ + // If the method returns a GC ref, mark R0 appropriately + if (retSize == EA_GCREF) + { + gcrefRegs |= RBM_R0; + } + else if (retSize == EA_BYREF) + { + byrefRegs |= RBM_R0; + } + VarSetOps::Assign(emitComp, emitThisGCrefVars, ptrVars); emitThisGCrefRegs = gcrefRegs; emitThisByrefRegs = byrefRegs; diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp index 2f1470ea2ad37e..b0a63e41a07eca 100644 --- a/src/coreclr/jit/emitarm64.cpp +++ b/src/coreclr/jit/emitarm64.cpp @@ -9020,6 +9020,26 @@ void emitter::emitIns_Call(EmitCallType callType, /* Update the emitter's live GC ref sets */ + // If the method returns a GC ref, mark RBM_INTRET appropriately + if (retSize == EA_GCREF) + { + gcrefRegs |= RBM_INTRET; + } + else if (retSize == EA_BYREF) + { + byrefRegs |= RBM_INTRET; + } + + // If is a multi-register return method is called, mark RBM_INTRET_1 appropriately + if (secondRetSize == EA_GCREF) + { + gcrefRegs |= RBM_INTRET_1; + } + else if (secondRetSize == EA_BYREF) + { + byrefRegs |= RBM_INTRET_1; + } + VarSetOps::Assign(emitComp, emitThisGCrefVars, ptrVars); emitThisGCrefRegs = gcrefRegs; emitThisByrefRegs = byrefRegs; diff --git a/src/coreclr/jit/emitloongarch64.cpp b/src/coreclr/jit/emitloongarch64.cpp index cd1d5e22df3871..6647d935f9e3b7 100644 --- a/src/coreclr/jit/emitloongarch64.cpp +++ b/src/coreclr/jit/emitloongarch64.cpp @@ -2464,6 +2464,26 @@ void emitter::emitIns_Call(EmitCallType callType, /* Update the emitter's live GC ref sets */ + // If the method returns a GC ref, mark RBM_INTRET appropriately + if (retSize == EA_GCREF) + { + gcrefRegs |= RBM_INTRET; + } + else if (retSize == EA_BYREF) + { + byrefRegs |= RBM_INTRET; + } + + // If is a multi-register return method is called, mark RBM_INTRET_1 appropriately + if (secondRetSize == EA_GCREF) + { + gcrefRegs |= RBM_INTRET_1; + } + else if (secondRetSize == EA_BYREF) + { + byrefRegs |= RBM_INTRET_1; + } + VarSetOps::Assign(emitComp, emitThisGCrefVars, ptrVars); emitThisGCrefRegs = gcrefRegs; emitThisByrefRegs = byrefRegs; diff --git a/src/coreclr/jit/emitriscv64.cpp b/src/coreclr/jit/emitriscv64.cpp index 62c156c8093153..a05831c473d6f0 100644 --- a/src/coreclr/jit/emitriscv64.cpp +++ b/src/coreclr/jit/emitriscv64.cpp @@ -1373,6 +1373,26 @@ void emitter::emitIns_Call(EmitCallType callType, /* Update the emitter's live GC ref sets */ + // If the method returns a GC ref, mark RBM_INTRET appropriately + if (retSize == EA_GCREF) + { + gcrefRegs |= RBM_INTRET; + } + else if (retSize == EA_BYREF) + { + byrefRegs |= RBM_INTRET; + } + + // If is a multi-register return method is called, mark RBM_INTRET_1 appropriately + if (secondRetSize == EA_GCREF) + { + gcrefRegs |= RBM_INTRET_1; + } + else if (secondRetSize == EA_BYREF) + { + byrefRegs |= RBM_INTRET_1; + } + VarSetOps::Assign(emitComp, emitThisGCrefVars, ptrVars); emitThisGCrefRegs = gcrefRegs; emitThisByrefRegs = byrefRegs; diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 715c4a26fb3f40..637db7ba08b6f6 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -9578,6 +9578,28 @@ void emitter::emitIns_Call(EmitCallType callType, /* Update the emitter's live GC ref sets */ + // If the method returns a GC ref, mark EAX appropriately + if (retSize == EA_GCREF) + { + gcrefRegs |= RBM_EAX; + } + else if (retSize == EA_BYREF) + { + byrefRegs |= RBM_EAX; + } + +#ifdef UNIX_AMD64_ABI + // If is a multi-register return method is called, mark RDX appropriately (for System V AMD64). + if (secondRetSize == EA_GCREF) + { + gcrefRegs |= RBM_RDX; + } + else if (secondRetSize == EA_BYREF) + { + byrefRegs |= RBM_RDX; + } +#endif // UNIX_AMD64_ABI + VarSetOps::Assign(emitComp, emitThisGCrefVars, ptrVars); emitThisGCrefRegs = gcrefRegs; emitThisByrefRegs = byrefRegs; @@ -16546,9 +16568,9 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) #ifdef TARGET_X86 dst += emitOutputWord(dst, code | 0x0500); #else // TARGET_AMD64 - // Amd64: addr fits within 32-bits and can be encoded as a displacement relative to zero. - // This addr mode should never be used while generating relocatable ngen code nor if - // the addr can be encoded as pc-relative address. + // Amd64: addr fits within 32-bits and can be encoded as a displacement relative to zero. + // This addr mode should never be used while generating relocatable ngen code nor if + // the addr can be encoded as pc-relative address. noway_assert(!emitComp->opts.compReloc); noway_assert(codeGen->genAddrRelocTypeHint((size_t)addr) != IMAGE_REL_BASED_REL32); noway_assert(static_cast(reinterpret_cast(addr)) == (ssize_t)addr); diff --git a/src/coreclr/jit/jitgcinfo.h b/src/coreclr/jit/jitgcinfo.h index 3d2a9b2047ee4f..6c62a0816113a5 100644 --- a/src/coreclr/jit/jitgcinfo.h +++ b/src/coreclr/jit/jitgcinfo.h @@ -183,8 +183,8 @@ class GCInfo } unsigned short rpdIsThis : 1; // is it the 'this' pointer - unsigned short rpdCall : 1; // is this a true call site? - unsigned short : 1; // Padding bit, so next two start on a byte boundary + unsigned short rpdCall : 1; // is this a true call site? + unsigned short : 1; // Padding bit, so next two start on a byte boundary unsigned short rpdCallGCrefRegs : CNT_CALL_GC_REGS; // Callee-saved and return registers containing GC pointers. unsigned short rpdCallByrefRegs : CNT_CALL_GC_REGS; // Callee-saved and return registers containing byrefs. From c8f1ba163880282da1cef0cebbc015e9be3dc788 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sat, 20 Apr 2024 11:29:01 -0700 Subject: [PATCH 41/44] these helpers should not form GC safe points --- src/coreclr/jit/emit.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index d0b14daea201d2..df6d108e0d5939 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -2843,6 +2843,13 @@ bool emitter::emitNoGChelper(CorInfoHelpFunc helpFunc) case CORINFO_HELP_CHECK_OBJ: + // never present on stack at the time of GC + case CORINFO_HELP_TAILCALL: + + // called in prolog + case CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER: + case CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER_TRACK_TRANSITIONS: + case CORINFO_HELP_VALIDATE_INDIRECT_CALL: return true; From e99529cb49f253f859f5019d5276032653c0380c Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sun, 21 Apr 2024 13:57:42 -0700 Subject: [PATCH 42/44] require that the new block has BBF_HAS_LABEL --- src/coreclr/jit/emit.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index df6d108e0d5939..ec15bea4ed1fa2 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -2886,15 +2886,17 @@ bool emitter::emitNoGChelper(CORINFO_METHOD_HANDLE methHnd) void* emitter::emitAddLabel(VARSET_VALARG_TP GCvars, regMaskTP gcrefRegs, regMaskTP byrefRegs, BasicBlock* prevBlock) { - if (prevBlock != NULL && emitLastInsIsCallWithGC()) + // if starting a new block that can be a target of a branch and the last instruction was GC-capable call. + if (prevBlock != NULL && emitComp->compCurBB->HasFlag(BBF_HAS_LABEL) && emitLastInsIsCallWithGC()) { + // no GC-capable calls expected in prolog + assert(!emitIGisInEpilog(emitLastInsIG)); + // We have just emitted a call that can do GC and conservatively recorded what is alive after the call. // Now we see that the next instruction may be reachable by a branch with a different liveness. // We want to maintain the invariant that the GC info at IP after a GC-capable call is the same // regardless how it is reached. - // One way to fix this is to fish out the call instruction and patch its GC info, but we must be - // certain that the current IP is indeed reachable after the call. - // Another way it to add an instruction (NOP or BRK) after the call. + // One way to ensure that is by adding an instruction (NOP or BRK) after the call. if (emitThisGCrefRegs != gcrefRegs || emitThisByrefRegs != byrefRegs || !VarSetOps::Equal(emitComp, emitThisGCrefVars, GCvars)) { @@ -2906,9 +2908,7 @@ void* emitter::emitAddLabel(VARSET_VALARG_TP GCvars, regMaskTP gcrefRegs, regMas { // other block kinds should emit something at the end that is not a call. assert(prevBlock->KindIs(BBJ_ALWAYS)); - // CONSIDER: We could patch up the previous call instruction with new GC info instead. - // But that will need to be coordinated with how the GC info vor variables is used. - // We currently apply that info to the instruction before the call. It may need to change. + // CONSIDER: In many cases we could instead patch up the GC info for previous call instruction. emitIns(INS_nop); } } From 53c99d3c18d29ca91c1b0991f8eba4175d19c28c Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Wed, 1 May 2024 09:52:45 -0700 Subject: [PATCH 43/44] Apply suggestions from code review Co-authored-by: Jakob Botsch Nielsen --- src/coreclr/jit/emit.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index ec15bea4ed1fa2..61fcf9447137de 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -2845,8 +2845,6 @@ bool emitter::emitNoGChelper(CorInfoHelpFunc helpFunc) // never present on stack at the time of GC case CORINFO_HELP_TAILCALL: - - // called in prolog case CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER: case CORINFO_HELP_JIT_REVERSE_PINVOKE_ENTER_TRACK_TRANSITIONS: @@ -2887,7 +2885,7 @@ bool emitter::emitNoGChelper(CORINFO_METHOD_HANDLE methHnd) void* emitter::emitAddLabel(VARSET_VALARG_TP GCvars, regMaskTP gcrefRegs, regMaskTP byrefRegs, BasicBlock* prevBlock) { // if starting a new block that can be a target of a branch and the last instruction was GC-capable call. - if (prevBlock != NULL && emitComp->compCurBB->HasFlag(BBF_HAS_LABEL) && emitLastInsIsCallWithGC()) + if ((prevBlock != nullptr) && emitComp->compCurBB->HasFlag(BBF_HAS_LABEL) && emitLastInsIsCallWithGC()) { // no GC-capable calls expected in prolog assert(!emitIGisInEpilog(emitLastInsIG)); @@ -2897,7 +2895,7 @@ void* emitter::emitAddLabel(VARSET_VALARG_TP GCvars, regMaskTP gcrefRegs, regMas // We want to maintain the invariant that the GC info at IP after a GC-capable call is the same // regardless how it is reached. // One way to ensure that is by adding an instruction (NOP or BRK) after the call. - if (emitThisGCrefRegs != gcrefRegs || emitThisByrefRegs != byrefRegs || + if ((emitThisGCrefRegs != gcrefRegs) || (emitThisByrefRegs != byrefRegs) || !VarSetOps::Equal(emitComp, emitThisGCrefVars, GCvars)) { if (prevBlock->KindIs(BBJ_THROW)) From 40e452600f4f4cc03d68d283f705ab1d88d8079b Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Mon, 6 May 2024 11:58:57 -0700 Subject: [PATCH 44/44] updated JITEEVersionIdentifier GUID --- src/coreclr/inc/jiteeversionguid.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/inc/jiteeversionguid.h b/src/coreclr/inc/jiteeversionguid.h index 122906341d00b4..762656a3dbbcff 100644 --- a/src/coreclr/inc/jiteeversionguid.h +++ b/src/coreclr/inc/jiteeversionguid.h @@ -43,11 +43,11 @@ typedef const GUID *LPCGUID; #define GUID_DEFINED #endif // !GUID_DEFINED -constexpr GUID JITEEVersionIdentifier = { /* bd8c41d4-8531-49c1-a600-0ae9bfe05de1 */ - 0xbd8c41d4, - 0x8531, - 0x49c1, - {0xa6, 0x00, 0x0a, 0xe9, 0xbf, 0xe0, 0x5d, 0xe1} +constexpr GUID JITEEVersionIdentifier = { /* 43854594-cd60-45df-a89f-5b7697586f46 */ + 0x43854594, + 0xcd60, + 0x45df, + {0xa8, 0x9f, 0x5b, 0x76, 0x97, 0x58, 0x6f, 0x46} }; //////////////////////////////////////////////////////////////////////////////////////////////////////////