Do not encode safe points with -1 offset. (#104336)

* Do not record safe points with -1 adjustmnt

* fix for arm64

* suppress an assert

* comments about  emitDisableGC

* JIT format

* NORMALIZE_CODE_OFFSET  on RISC

* JIT format again

* do not resurrect random regs after GS check

* denormalize code offsets in ILCompiler.Reflection.ReadyToRun.Amd64

Author: VSadov

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);

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)

src/coreclr/jit/codegencommon.cpp

@@ -1517,29 +1517,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);

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()
 {

src/coreclr/jit/emitinl.h

@@ -594,8 +594,7 @@ bool emitter::emitGenNoGCLst(Callback& cb)
             emitter::instrDesc* id = emitFirstInstrDesc(ig->igData);
             assert(id != nullptr);
             assert(id->idCodeSize() > 0);
-            if (!cb(ig->igFuncIdx, ig->igOffs, ig->igSize, id->idCodeSize(),
-                    ig->igFlags & (IGF_FUNCLET_PROLOG | IGF_FUNCLET_EPILOG | IGF_EPILOG)))
+            if (!cb(ig->igFuncIdx, ig->igOffs, ig->igSize, id->idCodeSize(), ig->igFlags & (IGF_FUNCLET_PROLOG)))
             {
                 return false;
             }

src/coreclr/jit/gcencode.cpp

@@ -4027,8 +4027,7 @@ class InterruptibleRangeReporter
     // Report everything between the previous region and the current
     // region as interruptible.
 
-    bool operator()(
-        unsigned igFuncIdx, unsigned igOffs, unsigned igSize, unsigned firstInstrSize, bool isInPrologOrEpilog)
+    bool operator()(unsigned igFuncIdx, unsigned igOffs, unsigned igSize, unsigned firstInstrSize, bool isInProlog)
     {
         if (igOffs < m_uninterruptibleEnd)
         {
@@ -4044,7 +4043,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;
             }

src/coreclr/nativeaot/Runtime/thread.cpp

@@ -675,10 +675,16 @@ void Thread::HijackCallback(NATIVE_CONTEXT* pThreadContext, void* pThreadToHijac
     if (runtime->IsConservativeStackReportingEnabled() ||
         codeManager->IsSafePoint(pvAddress))
     {
+        // IsUnwindable is precise on arm64, but can give false negatives on other architectures.
+        // (when IP is on the first instruction of an epilog, we still can unwind,
+        // but we can tell if the instruction is the first only if we can navigate instructions backwards and check)
+        // The preciseness of IsUnwindable is tracked in https://github.com/dotnet/runtime/issues/101932
+#if defined(TARGET_ARM64)
         // we may not be able to unwind in some locations, such as epilogs.
         // such locations should not contain safe points.
         // when scanning conservatively we do not need to unwind
         ASSERT(codeManager->IsUnwindable(pvAddress) || runtime->IsConservativeStackReportingEnabled());
+#endif
 
         // if we are not given a thread to hijack
         // perform in-line wait on the current thread

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;
 

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,36 +452,13 @@ 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),
         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());
-        }
-    }
-
     if (!decoder.EnumerateLiveSlots(
         pRegisterSet,
         isActiveStackFrame /* reportScratchSlots */,