From 5cd729cd70abf1129764b754e727f1514d4fd0a8 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/39] 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 115f7cdcf253..8a40404ace48 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 b42f5aae8f60..baf84f791e42 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 0b373a94fd3f..55db3ccabba1 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 6759662d5683..a3e496747cd4 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 0f2aa4f73669..0fd111ce9837 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 855aa24f627f..27f11b536d5c 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 2434466ee429211cefe042259ca3cb8963862de6 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/39] 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 8a40404ace48..eb84ec3d8aa5 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 5085e5971a8e..b3199a1a9561 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 cbaafbed6eaa0bf3b1303572b601c483af8e4201 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/39] 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 f8b320c07d44..ef56888e89d5 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 e21fe864984e..50b06ca19456 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 02fd49cead9c..3d2a9b2047ee 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 12975850a404..dfc6df891bcf 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 06777fa9d5f7..d57d9baade31 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 7e72da9cf2cc..417239dcc8d4 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 0f56ebe1ce98..53ce844b17cc 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 6d33d378bcd9..590b0c05927d 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 d27bffa3aa69..dcc87944beb1 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 33c1b0d49190..a0d5254dc5d1 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 dfeb96ae9e97..aa142f6cff0d 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 a3e496747cd4..c83b45f4e1e0 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 0fd111ce9837..ca9a5ef3db8a 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 ac41f7bda760be712b5e2ec79d59f56b310beb1b 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/39] 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 c83b45f4e1e0..ef0a3178f94b 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 ca9a5ef3db8a..4c27ceb3bcf8 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 590a165e2334c443a53ffc839af43bb67445d92e 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/39] 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 baf84f791e42..e546111f1889 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 ef0a3178f94b..0ca6a1769fc1 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 4c27ceb3bcf8..ec08de488adb 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 27f11b536d5c..b5762021034d 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 5b15ad624df4cf719f6ce62f36314e3901b16331 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/39] 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 b5762021034d..db29e8ded3e4 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 372f26819586bf1328303516675495d85aa0f1b9 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/39] 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 6bf148cf2d88..f4b9106ae9bf 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 77aca7b59a520e516b660d2a142fe0110caee52e 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/39] 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 e546111f1889..749d71f9fa80 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 0ca6a1769fc1..3ccf59f611ec 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 ec08de488adb..b0eb24759bb5 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 3f28d4b74315..1d38348d6e87 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 db29e8ded3e4..e24f733d41d2 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 eed56d9ca8161620b1f615ba782a08cf7fd9d61e 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/39] 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 44ccfd4c46be..22f90f6378b1 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 8d85757de40534ab9f3acd478d4ba03e40a0ae73 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/39] 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 16bff25525a9..09763fde2e3e 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 e24f733d41d2..1152fd3015f2 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 f7b7fc0a779d6c665957e132d0f6bfde8fcc0112 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/39] 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 f4b9106ae9bf..e41aabd81588 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 8c7dd4801a783a8d6dddbebc6cacae4069980dd2 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/39] 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 4901ee9e3aa0..9d5dc34a55a4 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 250f985e5ae00019653fb695efeaaf37ca0f914d 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/39] 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 ef56888e89d5..158b6699506f 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 1d78a87bfd3582166ec84b8a13384ae3a29e9c27 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/39] 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 e41aabd81588..a4fa7cbfae85 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 b9fccd4132816314af8878e168b599a4fac223e2 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/39] 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 09763fde2e3e..131c77c0265d 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 3bb10ac6c6bb..ff096681a074 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 86a4fa468e205e9207c810edc3f5bbaa561f74d8 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/39] 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 1d38348d6e87..0f9e2ab89122 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 1152fd3015f2..3050ce012061 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 ccd5cc543cbf409df0345b850136a87b7f69a2a8 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/39] 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 22f90f6378b1..d31894c43dc7 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 fc0352a17482a0a2cb55de9fe9ecc44dd68e55ba 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/39] 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 d015332a76d8..61ca86776564 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 fdb99f1c29f5..d8addc4da9ef 100644 --- a/src/coreclr/jit/codegenloongarch64.cpp +++ b/src/coreclr/jit/codegenloongarch64.cpp @@ -6485,22 +6485,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 546ba7b31808..026047ad7420 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -6561,22 +6561,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 81331547b4a9..262ab3d9c897 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 181b9706e416..aec4b0b03de3 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 c69ea7c5a36e..3e2d54748f4b 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 71fd3e323d51..e40be073ffa3 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 efe24c88c32323cb7367f33bf7ede56f773effb3 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/39] 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 ddc7c79506ad..7005e25226ce 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 3050ce012061..e04a523b6c82 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 1bab7f2d119db44b1f981b60c86984318a19086d 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/39] 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 bee2f658ee7c..41f43708887b 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 0f9e2ab89122..42a6776123f1 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 d31894c43dc7..cede54465ebc 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 e04a523b6c82..214659424db9 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 dc56a7a57e07444c95c2ac36e56271e8ffb254f0 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/39] 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 158b6699506f..2b1227f7e45f 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 0cc6a4c6c6377b35fbc4804f8fd2f6d09da93dff 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/39] Hooked up GCInfo version to R2R file version --- src/coreclr/inc/gcinfo.h | 13 ++++++++++--- src/coreclr/inc/readytorun.h | 4 ++-- src/coreclr/nativeaot/Runtime/TypeManager.cpp | 9 ++++++--- .../Amd64/GcInfo.cs | 16 ++++++++++++---- .../ReadyToRunMethod.cs | 9 +++++++-- .../x86/GcInfo.cs | 2 +- 6 files changed, 38 insertions(+), 15 deletions(-) diff --git a/src/coreclr/inc/gcinfo.h b/src/coreclr/inc/gcinfo.h index 131c77c0265d..2fa83d6de30b 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 88219146a123..c950f1599ccc 100644 --- a/src/coreclr/inc/readytorun.h +++ b/src/coreclr/inc/readytorun.h @@ -33,8 +33,8 @@ // R2R Version 8.0 Changes the alignment of the Int128 type // R2R Version 9.0 adds support for the Vector512 type // 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.2 adds MemZero and NativeMemSet helpers, +// 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 77eda2c4c604..96dc357136d9 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 d01c636ef3b3..fee7af776a16 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 SafePointOffset(int index, uint value) /// /// 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 790c7142475b..982d678cdce1 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 197fcfb1ee03..ddb44a1d312a 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 class GcInfo : BaseGcInfo /// /// based on GCDump::DumpGCTable /// - public GcInfo(byte[] image, int offset, Machine machine, ushort majorVersion) + public GcInfo(byte[] image, int offset) { Offset = offset; From 59a4baa3e1f612f6af314535c7576b10ad3bc6e8 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/39] 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 214659424db9..c6a9660d4752 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 35d8e6349b3062d6c48484d99384b1ffa66d8fd4 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/39] 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 eb84ec3d8aa5..c98802dd5602 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 749d71f9fa80..8534bc2508c5 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 cede54465ebc..b1c219d3317e 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 c6a9660d4752..c4fffc80e503 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 55a06e101fb2911b6e8ca60a961e77106368908f Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Sun, 25 Feb 2024 14:39:15 -0800 Subject: [PATCH 25/39] 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 2fa83d6de30b..81ef941a79f4 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 4004d6d5c3aa179d30b0bc60b46c71562888953b 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/39] 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 41f43708887b..c7b1be02e5c6 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 b1c219d3317e..a5656c5391be 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 c5b72962e594aeb942efe7f3b9fc98b745be5d51 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/39] 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 81ef941a79f4..297c25f9346c 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 11425c5df2bc014933f4b4024bbc108d82e436b9 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/39] 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 c98802dd5602..0abf65047b8d 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 61ca86776564..afca2cd0c879 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 0743c7b5a7491b66ba7516669e42aae8801267f1 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/39] 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 50b06ca19456..6e1fa8bab9b7 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 ef760e524e2c991262f1ab39b351db7fd0774503 Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Fri, 1 Mar 2024 21:14:18 -0800 Subject: [PATCH 30/39] 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 fee7af776a16..54c5cd615c71 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 854c3aaa09001e7f5bef94c13a5797489a93d957 Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Fri, 1 Mar 2024 21:22:22 -0800 Subject: [PATCH 31/39] 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 54c5cd615c71..0b1c98a939e1 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 4a117b1ce5629b27607a3401c3ce0493ce773f24 Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Fri, 1 Mar 2024 22:02:05 -0800 Subject: [PATCH 32/39] 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 417239dcc8d4..1575177643eb 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 53ce844b17cc..46c41329b056 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 590b0c05927d..8f5f1d68a6ef 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 5ae3e4b8ba5a378971e6dd575a93bd611ddc9165 Mon Sep 17 00:00:00 2001 From: Vladimir Sadov Date: Mon, 4 Mar 2024 11:16:01 -0800 Subject: [PATCH 33/39] 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 0b1c98a939e1..b934e36719e8 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 cae83d9db964eec9f35f5ebbe12a011469faad59 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/39] 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 c4fffc80e503..66347d8bf8d2 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 411f2045426ca495704dfa2655f1960b94df01fb 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/39] 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 a5656c5391be..70ab39ea681b 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 e0a31e182786cde331c4e30fffdc5dbd957a2073 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/39] 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 297c25f9346c..d526405c9f2c 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 d68e09474f2055a07b2c057bdb5818530732db89 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/39] 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 a4fa7cbfae85..dfd3b3e08281 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 7ed82b333bb80bc7e5d4040617f6229741a2a411 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/39] 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 2b1227f7e45f..331887c52bde 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 a149e6a6a94a97eee63214605832b3f9f71e30ba 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/39] 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 7913879515eb..0e79bac317c9 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