From 4bf5e039a2298a87e7c7f6dca275621fed861630 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Sun, 30 Jun 2024 15:34:16 -0700 Subject: [PATCH 1/9] Do not record safe points with -1 adjustmnt --- src/coreclr/gcinfo/gcinfoencoder.cpp | 39 +++--------- .../Runtime/unix/UnixNativeCodeManager.cpp | 30 +-------- .../Runtime/windows/CoffNativeCodeManager.cpp | 26 +------- src/coreclr/vm/eetwain.cpp | 50 --------------- src/coreclr/vm/gcinfodecoder.cpp | 63 +++++-------------- 5 files changed, 28 insertions(+), 180 deletions(-) diff --git a/src/coreclr/gcinfo/gcinfoencoder.cpp b/src/coreclr/gcinfo/gcinfoencoder.cpp index 0abf65047b8d9..c116d6d0dbc7b 100644 --- a/src/coreclr/gcinfo/gcinfoencoder.cpp +++ b/src/coreclr/gcinfo/gcinfoencoder.cpp @@ -1216,42 +1216,19 @@ void GcInfoEncoder::Build() /////////////////////////////////////////////////////////////////////// // Normalize call sites - // Eliminate call sites that fall inside interruptible ranges /////////////////////////////////////////////////////////////////////// + _ASSERTE(m_NumCallSites == 0 || numInterruptibleRanges == 0); + UINT32 numCallSites = 0; for(UINT32 callSiteIndex = 0; callSiteIndex < m_NumCallSites; callSiteIndex++) { UINT32 callSite = m_pCallSites[callSiteIndex]; - // There's a contract with the EE that says for non-leaf stack frames, where the - // method is stopped at a call site, the EE will not query with the return PC, but - // rather the return PC *minus 1*. - // The reason is that variable/register liveness may change at the instruction immediately after the - // call, so we want such frames to appear as if they are "within" the call. - // Since we use "callSite" as the "key" when we search for the matching descriptor, also subtract 1 here - // (after, of course, adding the size of the call instruction to get the return PC). - callSite += m_pCallSiteSizes[callSiteIndex] - 1; + callSite += m_pCallSiteSizes[callSiteIndex]; _ASSERTE(DENORMALIZE_CODE_OFFSET(NORMALIZE_CODE_OFFSET(callSite)) == callSite); UINT32 normOffset = NORMALIZE_CODE_OFFSET(callSite); - - BOOL keepIt = TRUE; - - for(UINT32 intRangeIndex = 0; intRangeIndex < numInterruptibleRanges; intRangeIndex++) - { - InterruptibleRange *pRange = &pRanges[intRangeIndex]; - if(pRange->NormStopOffset > normOffset) - { - if(pRange->NormStartOffset <= normOffset) - { - keepIt = FALSE; - } - break; - } - } - - if(keepIt) - m_pCallSites[numCallSites++] = normOffset; + m_pCallSites[numCallSites++] = normOffset; } GCINFO_WRITE_VARL_U(m_Info1, NORMALIZE_NUM_SAFE_POINTS(numCallSites), NUM_SAFE_POINTS_ENCBASE, NumCallSitesSize); @@ -1418,7 +1395,7 @@ void GcInfoEncoder::Build() for(pCurrent = pTransitions; pCurrent < pEndTransitions; ) { - if(pCurrent->CodeOffset > callSite) + if(pCurrent->CodeOffset >= callSite) { couldBeLive |= liveState; @@ -1773,7 +1750,7 @@ void GcInfoEncoder::Build() { for(pCurrent = pTransitions; pCurrent < pEndTransitions; ) { - if(pCurrent->CodeOffset > callSite) + if(pCurrent->CodeOffset >= callSite) { // Time to record the call site @@ -1872,7 +1849,7 @@ void GcInfoEncoder::Build() for(pCurrent = pTransitions; pCurrent < pEndTransitions; ) { - if(pCurrent->CodeOffset > callSite) + if(pCurrent->CodeOffset >= callSite) { // Time to encode the call site @@ -1919,7 +1896,7 @@ void GcInfoEncoder::Build() for(pCurrent = pTransitions; pCurrent < pEndTransitions; ) { - if(pCurrent->CodeOffset > callSite) + if(pCurrent->CodeOffset >= callSite) { // Time to encode the call site GCINFO_WRITE_VECTOR(m_Info1, liveState, CallSiteStateSize); diff --git a/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp b/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp index 3ccf59f611ecb..19f7a753043f6 100644 --- a/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/unix/UnixNativeCodeManager.cpp @@ -213,44 +213,18 @@ void UnixNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, #ifdef TARGET_ARM // Ensure that code offset doesn't have the Thumb bit set. We need - // it to be aligned to instruction start to make the !isActiveStackFrame - // branch below work. + // it to be aligned to instruction start ASSERT(((uintptr_t)codeOffset & 1) == 0); #endif - bool executionAborted = ((UnixNativeMethodInfo*)pMethodInfo)->executionAborted; - - if (!isActiveStackFrame && !executionAborted) - { - // the reasons for this adjustment are explained in EECodeManager::EnumGcRefs - codeOffset--; - } - 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. - if (!decoder.HasInterruptibleRanges()) - { - decoder = GcInfoDecoder( - GCInfoToken(gcInfo), - GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG), - codeOffset - 1 - ); - - assert(decoder.IsInterruptibleSafePoint()); - } - } - ICodeManagerFlags flags = (ICodeManagerFlags)0; + bool executionAborted = ((UnixNativeMethodInfo*)pMethodInfo)->executionAborted; if (executionAborted) flags = ICodeManagerFlags::ExecutionAborted; diff --git a/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp b/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp index 0bd9fbb34f9a5..fed9853b72ad2 100644 --- a/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp +++ b/src/coreclr/nativeaot/Runtime/windows/CoffNativeCodeManager.cpp @@ -440,9 +440,8 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, PTR_uint8_t gcInfo; uint32_t codeOffset = GetCodeOffset(pMethodInfo, safePointAddress, &gcInfo); - bool executionAborted = ((CoffNativeMethodInfo *)pMethodInfo)->executionAborted; - ICodeManagerFlags flags = (ICodeManagerFlags)0; + bool executionAborted = ((CoffNativeMethodInfo *)pMethodInfo)->executionAborted; if (executionAborted) flags = ICodeManagerFlags::ExecutionAborted; @@ -453,11 +452,6 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, flags = (ICodeManagerFlags)(flags | ICodeManagerFlags::ActiveStackFrame); #ifdef USE_GC_INFO_DECODER - if (!isActiveStackFrame && !executionAborted) - { - // the reasons for this adjustment are explained in EECodeManager::EnumGcRefs - codeOffset--; - } GcInfoDecoder decoder( GCInfoToken(gcInfo), @@ -465,24 +459,6 @@ void CoffNativeCodeManager::EnumGcRefs(MethodInfo * pMethodInfo, 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. - if (!decoder.HasInterruptibleRanges()) - { - decoder = GcInfoDecoder( - GCInfoToken(gcInfo), - GcInfoDecoderFlags(DECODE_GC_LIFETIMES | DECODE_SECURITY_OBJECT | DECODE_VARARG), - codeOffset - 1 - ); - - assert(decoder.IsInterruptibleSafePoint()); - } - } - if (!decoder.EnumerateLiveSlots( pRegisterSet, isActiveStackFrame /* reportScratchSlots */, diff --git a/src/coreclr/vm/eetwain.cpp b/src/coreclr/vm/eetwain.cpp index 5746c44de4a77..cf2867f38fe5a 100644 --- a/src/coreclr/vm/eetwain.cpp +++ b/src/coreclr/vm/eetwain.cpp @@ -1436,37 +1436,6 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY pRD, } #endif - /* If we are not in the active method, we are currently pointing - * to the return address; at the return address stack variables - * can become dead if the call is the last instruction of a try block - * and the return address is the jump around the catch block. Therefore - * we simply assume an offset inside of call instruction. - * NOTE: The GcInfoDecoder depends on this; if you change it, you must - * revisit the GcInfoEncoder/Decoder - */ - - if (!(flags & ExecutionAborted)) - { - if (!(flags & ActiveStackFrame)) - { - curOffs--; - LOG((LF_GCINFO, LL_INFO1000, "Adjusted GC reporting offset due to flags !ExecutionAborted && !ActiveStackFrame. Now reporting GC refs for %s at offset %04x.\n", - methodName, curOffs)); - } - } - else - { - // Since we are aborting execution, we are either in a frame that actually faulted or in a throwing call. - // * We do not need to adjust in a leaf - // * A throwing call will have unreachable after it, thus GC info is the same as before the call. - // - // Either way we do not need to adjust. - - // NOTE: only fully interruptible methods may need to report anything here as without - // exception handling all current local variables are already unreachable. - // EnumerateLiveSlots will shortcircuit the partially interruptible case just a bit later. - } - // Check if we have been given an override value for relOffset if (relOffsetOverride != NO_OVERRIDE_OFFSET) { @@ -1510,7 +1479,6 @@ bool EECodeManager::EnumGcRefs( PREGDISPLAY pRD, // A frame is non-leaf if we are executing a call, or a fault occurred in the function. // The only case in which we need to report scratch slots for a non-leaf frame // is when execution has to be resumed at the point of interruption (via ResumableFrame) - //Implement ResumableFrame _ASSERTE( sizeof( BOOL ) >= sizeof( ActiveStackFrame ) ); reportScratchSlots = (flags & ActiveStackFrame) != 0; @@ -1521,24 +1489,6 @@ 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((InterruptibleSafePointsEnabled() && gcInfoDecoder.CouldBeInterruptibleSafePoint())); - } - } - if (!gcInfoDecoder.EnumerateLiveSlots( pRD, reportScratchSlots, diff --git a/src/coreclr/vm/gcinfodecoder.cpp b/src/coreclr/vm/gcinfodecoder.cpp index 32c74bab8b409..97588ca0c9313 100644 --- a/src/coreclr/vm/gcinfodecoder.cpp +++ b/src/coreclr/vm/gcinfodecoder.cpp @@ -367,11 +367,7 @@ GcInfoDecoder::GcInfoDecoder( { if(m_NumSafePoints) { - // 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); + m_SafePointIndex = FindSafePoint(m_InstructionOffset); } } else if(flags & DECODE_FOR_RANGES_CALLBACK) @@ -457,10 +453,6 @@ bool GcInfoDecoder::IsSafePoint(UINT32 codeOffset) if(m_NumSafePoints == 0) return false; -#if defined(TARGET_AMD64) || defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) - // Safepoints are encoded with a -1 adjustment - codeOffset--; -#endif size_t savedPos = m_Reader.GetCurrentPos(); UINT32 safePointIndex = FindSafePoint(codeOffset); m_Reader.SetCurrentPos(savedPos); @@ -507,32 +499,26 @@ UINT32 GcInfoDecoder::FindSafePoint(UINT32 breakOffset) const size_t savedPos = m_Reader.GetCurrentPos(); const UINT32 numBitsPerOffset = CeilOfLog2(NORMALIZE_CODE_OFFSET(m_CodeLength)); -#if defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) - // Safepoints are encoded with a -1 adjustment - if ((breakOffset & 1) != 0) -#endif + const UINT32 normBreakOffset = NORMALIZE_CODE_OFFSET(breakOffset); + UINT32 linearSearchStart = 0; + UINT32 linearSearchEnd = m_NumSafePoints; + if (linearSearchEnd - linearSearchStart > MAX_LINEAR_SEARCH) { - const UINT32 normBreakOffset = NORMALIZE_CODE_OFFSET(breakOffset); - UINT32 linearSearchStart = 0; - UINT32 linearSearchEnd = m_NumSafePoints; - if (linearSearchEnd - linearSearchStart > MAX_LINEAR_SEARCH) + linearSearchStart = NarrowSafePointSearch(savedPos, normBreakOffset, &linearSearchEnd); + } + + for (UINT32 i = linearSearchStart; i < linearSearchEnd; i++) + { + UINT32 spOffset = (UINT32)m_Reader.Read(numBitsPerOffset); + if (spOffset == normBreakOffset) { - linearSearchStart = NarrowSafePointSearch(savedPos, normBreakOffset, &linearSearchEnd); + result = i; + break; } - for (UINT32 i = linearSearchStart; i < linearSearchEnd; i++) + if (spOffset > normBreakOffset) { - UINT32 spOffset = (UINT32)m_Reader.Read(numBitsPerOffset); - if (spOffset == normBreakOffset) - { - result = i; - break; - } - - if (spOffset > normBreakOffset) - { - break; - } + break; } } @@ -553,13 +539,7 @@ void GcInfoDecoder::EnumerateSafePoints(EnumerateSafePointsCallback *pCallback, for(UINT32 i = 0; i < m_NumSafePoints; i++) { UINT32 normOffset = (UINT32)m_Reader.Read(numBitsPerOffset); - UINT32 offset = DENORMALIZE_CODE_OFFSET(normOffset) + 2; - -#if defined(TARGET_AMD64) || defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) - // Safepoints are encoded with a -1 adjustment - offset--; -#endif - + UINT32 offset = DENORMALIZE_CODE_OFFSET(normOffset); pCallback(this, offset, hCallback); } } @@ -729,15 +709,6 @@ bool GcInfoDecoder::EnumerateLiveSlots( return true; } - // - // If this is a non-leaf frame and we are executing a call, the unwinder has given us the PC - // of the call instruction. We should adjust it to the PC of the instruction after the call in order to - // obtain transition information for scratch slots. However, we always assume scratch slots to be - // dead for non-leaf frames (except for ResumableFrames), so we don't need to adjust the PC. - // If this is a non-leaf frame and we are not executing a call (i.e.: a fault occurred in the function), - // then it would be incorrect to adjust the PC - // - _ASSERTE(GC_SLOT_INTERIOR == GC_CALL_INTERIOR); _ASSERTE(GC_SLOT_PINNED == GC_CALL_PINNED); From 7ecaab7bd5c3cc2d9df3259bd36c3c7047d83100 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Mon, 1 Jul 2024 14:14:42 -0700 Subject: [PATCH 2/9] fix for arm64 --- src/coreclr/jit/emitinl.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/emitinl.h b/src/coreclr/jit/emitinl.h index 022064073d908..8cdbee39d94eb 100644 --- a/src/coreclr/jit/emitinl.h +++ b/src/coreclr/jit/emitinl.h @@ -595,7 +595,7 @@ bool emitter::emitGenNoGCLst(Callback& cb) assert(id != nullptr); assert(id->idCodeSize() > 0); if (!cb(ig->igFuncIdx, ig->igOffs, ig->igSize, id->idCodeSize(), - ig->igFlags & (IGF_FUNCLET_PROLOG | IGF_FUNCLET_EPILOG | IGF_EPILOG))) + ig->igFlags & (IGF_FUNCLET_PROLOG))) { return false; } From 91bdf6330d0818f2deb58d6d2ca697dacd54bcf3 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Mon, 1 Jul 2024 15:52:37 -0700 Subject: [PATCH 3/9] suppress an assert --- src/coreclr/nativeaot/Runtime/thread.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/coreclr/nativeaot/Runtime/thread.cpp b/src/coreclr/nativeaot/Runtime/thread.cpp index b796b05218226..370f7417d85a3 100644 --- a/src/coreclr/nativeaot/Runtime/thread.cpp +++ b/src/coreclr/nativeaot/Runtime/thread.cpp @@ -675,10 +675,16 @@ void Thread::HijackCallback(NATIVE_CONTEXT* pThreadContext, void* pThreadToHijac if (runtime->IsConservativeStackReportingEnabled() || codeManager->IsSafePoint(pvAddress)) { + // IsUnwindable is precise on arm64, but can give false negatives on other architectures. + // (when IP is on the first instruction of an epilog, we still can unwind, + // but we cannot tell if the instruction is the first only if we can navigate instructions backwards) + // The preciseness of IsUnwindable is tracked in https://github.com/dotnet/runtime/issues/101932 +#if defined(TARGET_ARM64) // we may not be able to unwind in some locations, such as epilogs. // such locations should not contain safe points. // when scanning conservatively we do not need to unwind ASSERT(codeManager->IsUnwindable(pvAddress) || runtime->IsConservativeStackReportingEnabled()); +#endif // if we are not given a thread to hijack // perform in-line wait on the current thread From 09e0dfb60b364d67a29e47440f252690733ee97d Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 2 Jul 2024 18:12:10 -0700 Subject: [PATCH 4/9] comments about emitDisableGC --- src/coreclr/jit/emit.cpp | 6 +++--- src/coreclr/jit/gcencode.cpp | 4 ++-- src/coreclr/nativeaot/Runtime/thread.cpp | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 206066200a47c..9d736c3aa2406 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -10502,9 +10502,9 @@ regMaskTP emitter::emitGetGCRegsKilledByNoGCCall(CorInfoHelpFunc helper) // of the last instruction in the region makes GC safe again. // In particular - once the IP is on the first instruction, but not executed it yet, // it is still safe to do GC. -// The only special case is when NoGC region is used for prologs/epilogs. -// In such case the GC info could be incorrect until the prolog completes and epilogs -// may have unwindability restrictions, so the first instruction cannot have GC. +// The only special case is when NoGC region is used for prologs. +// In such case the GC info could be incorrect until the prolog completes, so the first +// instruction cannot have GC. void emitter::emitDisableGC() { diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index 4863c95a7f59d..1d3afcbe80bc0 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4028,7 +4028,7 @@ class InterruptibleRangeReporter // region as interruptible. bool operator()( - unsigned igFuncIdx, unsigned igOffs, unsigned igSize, unsigned firstInstrSize, bool isInPrologOrEpilog) + unsigned igFuncIdx, unsigned igOffs, unsigned igSize, unsigned firstInstrSize, bool isInProlog) { if (igOffs < m_uninterruptibleEnd) { @@ -4044,7 +4044,7 @@ class InterruptibleRangeReporter // Once the first instruction in IG executes, we cannot have GC. // But it is ok to have GC while the IP is on the first instruction, unless we are in prolog/epilog. unsigned interruptibleEnd = igOffs; - if (!isInPrologOrEpilog) + if (!isInProlog) { interruptibleEnd += firstInstrSize; } diff --git a/src/coreclr/nativeaot/Runtime/thread.cpp b/src/coreclr/nativeaot/Runtime/thread.cpp index 370f7417d85a3..c2e94a1dc8f38 100644 --- a/src/coreclr/nativeaot/Runtime/thread.cpp +++ b/src/coreclr/nativeaot/Runtime/thread.cpp @@ -677,7 +677,7 @@ void Thread::HijackCallback(NATIVE_CONTEXT* pThreadContext, void* pThreadToHijac { // IsUnwindable is precise on arm64, but can give false negatives on other architectures. // (when IP is on the first instruction of an epilog, we still can unwind, - // but we cannot tell if the instruction is the first only if we can navigate instructions backwards) + // but we can tell if the instruction is the first only if we can navigate instructions backwards and check) // The preciseness of IsUnwindable is tracked in https://github.com/dotnet/runtime/issues/101932 #if defined(TARGET_ARM64) // we may not be able to unwind in some locations, such as epilogs. From 7d2fd227b17037e233ccdec229bee5e5a8342bbe Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 2 Jul 2024 18:14:39 -0700 Subject: [PATCH 5/9] JIT format --- src/coreclr/jit/emitinl.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/jit/emitinl.h b/src/coreclr/jit/emitinl.h index 8cdbee39d94eb..a586193dd5b71 100644 --- a/src/coreclr/jit/emitinl.h +++ b/src/coreclr/jit/emitinl.h @@ -594,8 +594,7 @@ bool emitter::emitGenNoGCLst(Callback& cb) emitter::instrDesc* id = emitFirstInstrDesc(ig->igData); assert(id != nullptr); assert(id->idCodeSize() > 0); - if (!cb(ig->igFuncIdx, ig->igOffs, ig->igSize, id->idCodeSize(), - ig->igFlags & (IGF_FUNCLET_PROLOG))) + if (!cb(ig->igFuncIdx, ig->igOffs, ig->igSize, id->idCodeSize(), ig->igFlags & (IGF_FUNCLET_PROLOG))) { return false; } From ee1a050356845d6af155fbd209164e9a81191d34 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 2 Jul 2024 18:55:09 -0700 Subject: [PATCH 6/9] NORMALIZE_CODE_OFFSET on RISC --- src/coreclr/inc/gcinfotypes.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/coreclr/inc/gcinfotypes.h b/src/coreclr/inc/gcinfotypes.h index 7457063d47eb3..0727e5e1a7b3e 100644 --- a/src/coreclr/inc/gcinfotypes.h +++ b/src/coreclr/inc/gcinfotypes.h @@ -678,8 +678,8 @@ void FASTCALL decodeCallPattern(int pattern, #define NORMALIZE_SIZE_OF_STACK_AREA(x) ((x)>>2) #define DENORMALIZE_SIZE_OF_STACK_AREA(x) ((x)<<2) #define CODE_OFFSETS_NEED_NORMALIZATION 1 -#define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 2/4 bytes long in Thumb/ARM states, -#define DENORMALIZE_CODE_OFFSET(x) (x) // but the safe-point offsets are encoded with a -1 adjustment. +#define NORMALIZE_CODE_OFFSET(x) ((x)>>1) // Instructions are 2/4 bytes long in Thumb/ARM states, +#define DENORMALIZE_CODE_OFFSET(x) ((x)<<1) #define NORMALIZE_REGISTER(x) (x) #define DENORMALIZE_REGISTER(x) (x) #define NORMALIZE_NUM_SAFE_POINTS(x) (x) @@ -734,9 +734,9 @@ void FASTCALL decodeCallPattern(int pattern, #define DENORMALIZE_STACK_BASE_REGISTER(x) ((x)^29) #define NORMALIZE_SIZE_OF_STACK_AREA(x) ((x)>>3) #define DENORMALIZE_SIZE_OF_STACK_AREA(x) ((x)<<3) -#define CODE_OFFSETS_NEED_NORMALIZATION 0 -#define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 4 bytes long, but the safe-point -#define DENORMALIZE_CODE_OFFSET(x) (x) // offsets are encoded with a -1 adjustment. +#define CODE_OFFSETS_NEED_NORMALIZATION 1 +#define NORMALIZE_CODE_OFFSET(x) ((x)>>2) // Instructions are 4 bytes long +#define DENORMALIZE_CODE_OFFSET(x) ((x)<<2) #define NORMALIZE_REGISTER(x) (x) #define DENORMALIZE_REGISTER(x) (x) #define NORMALIZE_NUM_SAFE_POINTS(x) (x) @@ -789,9 +789,9 @@ void FASTCALL decodeCallPattern(int pattern, #define DENORMALIZE_STACK_BASE_REGISTER(x) ((x) == 0 ? 22 : 3) #define NORMALIZE_SIZE_OF_STACK_AREA(x) ((x)>>3) #define DENORMALIZE_SIZE_OF_STACK_AREA(x) ((x)<<3) -#define CODE_OFFSETS_NEED_NORMALIZATION 0 -#define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 4 bytes long, but the safe-point -#define DENORMALIZE_CODE_OFFSET(x) (x) // offsets are encoded with a -1 adjustment. +#define CODE_OFFSETS_NEED_NORMALIZATION 1 +#define NORMALIZE_CODE_OFFSET(x) ((x)>>2) // Instructions are 4 bytes long +#define DENORMALIZE_CODE_OFFSET(x) ((x)<<2) #define NORMALIZE_REGISTER(x) (x) #define DENORMALIZE_REGISTER(x) (x) #define NORMALIZE_NUM_SAFE_POINTS(x) (x) @@ -844,9 +844,9 @@ void FASTCALL decodeCallPattern(int pattern, #define DENORMALIZE_STACK_BASE_REGISTER(x) ((x) == 0 ? 8 : 2) #define NORMALIZE_SIZE_OF_STACK_AREA(x) ((x)>>3) #define DENORMALIZE_SIZE_OF_STACK_AREA(x) ((x)<<3) -#define CODE_OFFSETS_NEED_NORMALIZATION 0 -#define NORMALIZE_CODE_OFFSET(x) (x) // Instructions are 4 bytes long, but the safe-point -#define DENORMALIZE_CODE_OFFSET(x) (x) // offsets are encoded with a -1 adjustment. +#define CODE_OFFSETS_NEED_NORMALIZATION 1 +#define NORMALIZE_CODE_OFFSET(x) ((x)>>2) // Instructions are 4 bytes long +#define DENORMALIZE_CODE_OFFSET(x) ((x)<<2) #define NORMALIZE_REGISTER(x) (x) #define DENORMALIZE_REGISTER(x) (x) #define NORMALIZE_NUM_SAFE_POINTS(x) (x) From 8b84d1f1d1f2d084e3bfbe33c3ddc0e0696e972b Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 2 Jul 2024 21:43:53 -0700 Subject: [PATCH 7/9] JIT format again --- src/coreclr/jit/gcencode.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp index 1d3afcbe80bc0..f99407e90b4e2 100644 --- a/src/coreclr/jit/gcencode.cpp +++ b/src/coreclr/jit/gcencode.cpp @@ -4027,8 +4027,7 @@ class InterruptibleRangeReporter // Report everything between the previous region and the current // region as interruptible. - bool operator()( - unsigned igFuncIdx, unsigned igOffs, unsigned igSize, unsigned firstInstrSize, bool isInProlog) + bool operator()(unsigned igFuncIdx, unsigned igOffs, unsigned igSize, unsigned firstInstrSize, bool isInProlog) { if (igOffs < m_uninterruptibleEnd) { From 649fabb82f8c7b82be19652f20168c1935ad50f3 Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Wed, 3 Jul 2024 12:43:49 -0700 Subject: [PATCH 8/9] do not resurrect random regs after GS check --- src/coreclr/jit/codegencommon.cpp | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 4a0d11282c5b3..f8d54a3c95b25 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1515,29 +1515,6 @@ void CodeGen::genExitCode(BasicBlock* block) if (compiler->getNeedsGSSecurityCookie()) { genEmitGSCookieCheck(jmpEpilog); - - if (jmpEpilog) - { - // Dev10 642944 - - // The GS cookie check created a temp label that has no live - // incoming GC registers, we need to fix that - - unsigned varNum; - LclVarDsc* varDsc; - - /* Figure out which register parameters hold pointers */ - - for (varNum = 0, varDsc = compiler->lvaTable; varNum < compiler->lvaCount && varDsc->lvIsRegArg; - varNum++, varDsc++) - { - noway_assert(varDsc->lvIsParam); - - gcInfo.gcMarkRegPtrVal(varDsc->GetArgReg(), varDsc->TypeGet()); - } - - GetEmitter()->emitThisGCrefRegs = GetEmitter()->emitInitGCrefRegs = gcInfo.gcRegGCrefSetCur; - GetEmitter()->emitThisByrefRegs = GetEmitter()->emitInitByrefRegs = gcInfo.gcRegByrefSetCur; - } } genReserveEpilog(block); From 698dc6107be300fe495daae1e80b02a6a7f63bdd Mon Sep 17 00:00:00 2001 From: vsadov <8218165+VSadov@users.noreply.github.com> Date: Tue, 16 Jul 2024 16:35:30 -0700 Subject: [PATCH 9/9] denormalize code offsets in ILCompiler.Reflection.ReadyToRun.Amd64 --- .../Amd64/GcInfo.cs | 28 +++++++++------- .../GCInfoTypes.cs | 32 ++++++++++++++++++- 2 files changed, 48 insertions(+), 12 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 b934e36719e8a..aa635e1b06158 100644 --- a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs +++ b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Amd64/GcInfo.cs @@ -48,6 +48,7 @@ public SafePointOffset(int index, uint value) private const int MIN_GCINFO_VERSION_WITH_RETURN_KIND = 2; private const int MIN_GCINFO_VERSION_WITH_REV_PINVOKE_FRAME = 2; + private const int MIN_GCINFO_VERSION_WITH_NORMALIZED_CODE_OFFSETS = 3; private bool _slimHeader; private bool _hasSecurityObject; @@ -88,7 +89,9 @@ public GcInfo() { } public GcInfo(byte[] image, int offset, Machine machine, ushort majorVersion, ushort minorVersion) { Offset = offset; - _gcInfoTypes = new GcInfoTypes(machine); + Version = ReadyToRunVersionToGcInfoVersion(majorVersion, minorVersion); + bool denormalizeCodeOffsets = Version > MIN_GCINFO_VERSION_WITH_NORMALIZED_CODE_OFFSETS; + _gcInfoTypes = new GcInfoTypes(machine, denormalizeCodeOffsets); _machine = machine; SecurityObjectStackSlot = -1; @@ -100,7 +103,6 @@ public GcInfo(byte[] image, int offset, Machine machine, ushort majorVersion, us SizeOfEditAndContinuePreservedArea = 0xffffffff; ReversePInvokeFrameStackSlot = -1; - Version = ReadyToRunVersionToGcInfoVersion(majorVersion, minorVersion); int bitOffset = offset * 8; ParseHeaderFlags(image, ref bitOffset); @@ -118,12 +120,13 @@ public GcInfo(byte[] image, int offset, Machine machine, ushort majorVersion, us uint normPrologSize = NativeReader.DecodeVarLengthUnsigned(image, _gcInfoTypes.NORM_PROLOG_SIZE_ENCBASE, ref bitOffset) + 1; uint normEpilogSize = NativeReader.DecodeVarLengthUnsigned(image, _gcInfoTypes.NORM_EPILOG_SIZE_ENCBASE, ref bitOffset); - ValidRangeStart = normPrologSize; - ValidRangeEnd = (uint)CodeLength - normEpilogSize; + ValidRangeStart = _gcInfoTypes.DenormalizeCodeOffset(normPrologSize); + ValidRangeEnd = (uint)CodeLength - _gcInfoTypes.DenormalizeCodeOffset(normEpilogSize); } else if (_hasSecurityObject || _hasGenericsInstContext) { - ValidRangeStart = NativeReader.DecodeVarLengthUnsigned(image, _gcInfoTypes.NORM_PROLOG_SIZE_ENCBASE, ref bitOffset) + 1; + uint normValidRangeStart = NativeReader.DecodeVarLengthUnsigned(image, _gcInfoTypes.NORM_PROLOG_SIZE_ENCBASE, ref bitOffset) + 1; + ValidRangeStart = _gcInfoTypes.DenormalizeCodeOffset(normValidRangeStart); ValidRangeEnd = ValidRangeStart + 1; } @@ -352,11 +355,11 @@ private void ParseHeaderFlags(byte[] image, ref int bitOffset) private List EnumerateSafePoints(byte[] image, ref int bitOffset) { List safePoints = new List(); - uint numBitsPerOffset = GcInfoTypes.CeilOfLog2(CodeLength); + uint numBitsPerOffset = GcInfoTypes.CeilOfLog2((int)_gcInfoTypes.NormalizeCodeOffset((uint)CodeLength)); for (int i = 0; i < NumSafePoints; i++) { uint normOffset = (uint)NativeReader.ReadBits(image, (int)numBitsPerOffset, ref bitOffset); - safePoints.Add(new SafePointOffset(i, normOffset)); + safePoints.Add(new SafePointOffset(i, _gcInfoTypes.DenormalizeCodeOffset(normOffset))); } return safePoints; } @@ -367,18 +370,21 @@ private List EnumerateSafePoints(byte[] image, ref int bitOffse private List EnumerateInterruptibleRanges(byte[] image, int interruptibleRangeDelta1EncBase, int interruptibleRangeDelta2EncBase, ref int bitOffset) { List ranges = new List(); - uint lastinterruptibleRangeStopOffset = 0; + uint normLastinterruptibleRangeStopOffset = 0; for (uint i = 0; i < NumInterruptibleRanges; i++) { uint normStartDelta = NativeReader.DecodeVarLengthUnsigned(image, interruptibleRangeDelta1EncBase, ref bitOffset); uint normStopDelta = NativeReader.DecodeVarLengthUnsigned(image, interruptibleRangeDelta2EncBase, ref bitOffset) + 1; - uint rangeStartOffset = lastinterruptibleRangeStopOffset + normStartDelta; - uint rangeStopOffset = rangeStartOffset + normStopDelta; + uint normRangeStartOffset = normLastinterruptibleRangeStopOffset + normStartDelta; + uint normRangeStopOffset = normRangeStartOffset + normStopDelta; + + uint rangeStartOffset = _gcInfoTypes.DenormalizeCodeOffset(normRangeStopOffset); + uint rangeStopOffset = _gcInfoTypes.DenormalizeCodeOffset(normRangeStartOffset); ranges.Add(new InterruptibleRange(i, rangeStartOffset, rangeStopOffset)); - lastinterruptibleRangeStopOffset = rangeStopOffset; + normLastinterruptibleRangeStopOffset = normRangeStopOffset; } return ranges; } diff --git a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/GCInfoTypes.cs b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/GCInfoTypes.cs index d3adc46240958..653425781a7cf 100644 --- a/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/GCInfoTypes.cs +++ b/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/GCInfoTypes.cs @@ -73,6 +73,7 @@ enum InfoHdrAdjust public class GcInfoTypes { private Machine _target; + private bool _denormalizeCodeOffsets; internal int SIZE_OF_RETURN_KIND_SLIM { get; } = 2; internal int SIZE_OF_RETURN_KIND_FAT { get; } = 2; @@ -105,9 +106,10 @@ public class GcInfoTypes internal int LIVESTATE_RLE_SKIP_ENCBASE { get; } = 4; internal int NUM_NORM_CODE_OFFSETS_PER_CHUNK_LOG2 { get; } = 6; - internal GcInfoTypes(Machine machine) + internal GcInfoTypes(Machine machine, bool denormalizeCodeOffsets) { _target = machine; + _denormalizeCodeOffsets = denormalizeCodeOffsets; switch (machine) { @@ -176,6 +178,34 @@ internal int DenormalizeCodeLength(int x) return x; } + internal int NormalizeCodeLength(int x) + { + switch (_target) + { + case Machine.ArmThumb2: + return (x >> 1); + case Machine.Arm64: + case Machine.LoongArch64: + case Machine.RiscV64: + return (x >> 2); + } + return x; + } + + internal uint DenormalizeCodeOffset(uint x) + { + return _denormalizeCodeOffsets ? + (uint)DenormalizeCodeLength((int)x) : + x; + } + + internal uint NormalizeCodeOffset(uint x) + { + return _denormalizeCodeOffsets ? + (uint)NormalizeCodeLength((int)x) : + x; + } + internal int DenormalizeStackSlot(int x) { switch (_target)