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

* Do not record safe points with -1 adjustmnt * NORMALIZE_CODE_OFFSET on RISC * denormalize code offsets in ILCompiler.Reflection.ReadyToRun.Amd64 * bump the min R2R version and GCInfo version * Do not record Return Kind * m_ReturnKind is not needed in non-legacy GC encoder, regardless X86 or not.
CodeBlanch · Dec 22, 2024 · 0cba4a1 · 0cba4a1
1 parent 004f205
commit 0cba4a1
Show file tree

Hide file tree

Showing 14 changed files with 105 additions and 271 deletions.
diff --git a/src/coreclr/gcinfo/gcinfoencoder.cpp b/src/coreclr/gcinfo/gcinfoencoder.cpp
@@ -490,13 +490,6 @@ GcInfoEncoder::GcInfoEncoder(
     m_IsSlotTableFrozen = FALSE;
 #endif //_DEBUG
 
-#ifndef TARGET_X86
-    // If the compiler doesn't set the GCInfo, report RT_Unset.
-    // This is used for compatibility with JITs that aren't updated to use the new API.
-    m_ReturnKind = RT_Unset;
-#else
-    m_ReturnKind = RT_Illegal;
-#endif // TARGET_X86
     m_CodeLength = 0;
 #ifdef FIXED_STACK_PARAMETER_SCRATCH_AREA
     m_SizeOfStackOutgoingAndScratchArea = -1;
@@ -776,13 +769,6 @@ void GcInfoEncoder::SetReversePInvokeFrameSlot(INT32 spOffset)
     m_ReversePInvokeFrameSlot = spOffset;
 }
 
-void GcInfoEncoder::SetReturnKind(ReturnKind returnKind)
-{
-    _ASSERTE(IsValidReturnKind(returnKind));
-
-    m_ReturnKind = returnKind;
-}
-
 struct GcSlotDescAndId
 {
     GcSlotDesc m_SlotDesc;
@@ -1045,16 +1031,15 @@ void GcInfoEncoder::Build()
     BOOL slimHeader = (!m_IsVarArg && !hasGSCookie && (m_PSPSymStackSlot == NO_PSP_SYM) &&
         !hasContextParamType && (m_InterruptibleRanges.Count() == 0) && !hasReversePInvokeFrame &&
         ((m_StackBaseRegister == NO_STACK_BASE_REGISTER) || (NORMALIZE_STACK_BASE_REGISTER(m_StackBaseRegister) == 0))) &&
-        (m_SizeOfEditAndContinuePreservedArea == NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA) &&
 #ifdef TARGET_AMD64
         !m_WantsReportOnlyLeaf &&
 #elif defined(TARGET_ARM) || defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)
         !m_HasTailCalls &&
 #endif // TARGET_AMD64
-        !IsStructReturnKind(m_ReturnKind);
+        (m_SizeOfEditAndContinuePreservedArea == NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA);
 
     // All new code is generated for the latest GCINFO_VERSION.
-    // So, always encode RetunrKind and encode ReversePInvokeFrameSlot where applicable.
+    // So, always encode ReversePInvokeFrameSlot where applicable.
     if (slimHeader)
     {
         // Slim encoding means nothing special, partially interruptible, maybe a default frame register
@@ -1065,8 +1050,6 @@ void GcInfoEncoder::Build()
         assert(m_StackBaseRegister == 8 || 2 == m_StackBaseRegister);
 #endif
         GCINFO_WRITE(m_Info1, (m_StackBaseRegister == NO_STACK_BASE_REGISTER) ? 0 : 1, 1, FlagsSize);
-
-        GCINFO_WRITE(m_Info1, m_ReturnKind, SIZE_OF_RETURN_KIND_IN_SLIM_HEADER, RetKindSize);
     }
     else
     {
@@ -1089,8 +1072,6 @@ void GcInfoEncoder::Build()
 #endif // TARGET_AMD64
         GCINFO_WRITE(m_Info1, ((m_SizeOfEditAndContinuePreservedArea != NO_SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA) ? 1 : 0), 1, FlagsSize);
         GCINFO_WRITE(m_Info1, (hasReversePInvokeFrame ? 1 : 0), 1, FlagsSize);
-
-        GCINFO_WRITE(m_Info1, m_ReturnKind, SIZE_OF_RETURN_KIND_IN_FAT_HEADER, RetKindSize);
     }
 
     _ASSERTE( m_CodeLength > 0 );
@@ -1217,42 +1198,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);
@@ -1419,7 +1377,7 @@ void GcInfoEncoder::Build()
 
         for(pCurrent = pTransitions; pCurrent < pEndTransitions; )
         {
-            if(pCurrent->CodeOffset > callSite)
+            if(pCurrent->CodeOffset >= callSite)
             {
                 couldBeLive |= liveState;
 
@@ -1774,7 +1732,7 @@ void GcInfoEncoder::Build()
         {
             for(pCurrent = pTransitions; pCurrent < pEndTransitions; )
             {
-                if(pCurrent->CodeOffset > callSite)
+                if(pCurrent->CodeOffset >= callSite)
                 {
                     // Time to record the call site
 
@@ -1873,7 +1831,7 @@ void GcInfoEncoder::Build()
 
             for(pCurrent = pTransitions; pCurrent < pEndTransitions; )
             {
-                if(pCurrent->CodeOffset > callSite)
+                if(pCurrent->CodeOffset >= callSite)
                 {
                     // Time to encode the call site
 
@@ -1920,7 +1878,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/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 3
+#define GCINFO_VERSION 4
 
 //-----------------------------------------------------------------------------
 // GCInfoToken: A wrapper that contains the GcInfo data and version number.

diff --git a/src/coreclr/inc/gcinfoencoder.h b/src/coreclr/inc/gcinfoencoder.h
@@ -409,13 +409,6 @@ class GcInfoEncoder
                                     GcSlotState slotState
                                     );
 
-
-    //------------------------------------------------------------------------
-    // ReturnKind
-    //------------------------------------------------------------------------
-
-    void SetReturnKind(ReturnKind returnKind);
-
     //------------------------------------------------------------------------
     // Miscellaneous method information
     //------------------------------------------------------------------------
@@ -509,7 +502,6 @@ class GcInfoEncoder
     INT32  m_PSPSymStackSlot;
     INT32  m_GenericsInstContextStackSlot;
     GENERIC_CONTEXTPARAM_TYPE m_contextParamType;
-    ReturnKind m_ReturnKind;
     UINT32 m_CodeLength;
     UINT32 m_StackBaseRegister;
     UINT32 m_SizeOfEditAndContinuePreservedArea;

diff --git a/src/coreclr/inc/gcinfotypes.h b/src/coreclr/inc/gcinfotypes.h
@@ -637,8 +637,6 @@ void FASTCALL decodeCallPattern(int         pattern,
 #define SECURITY_OBJECT_STACK_SLOT_ENCBASE 6
 #define GS_COOKIE_STACK_SLOT_ENCBASE 6
 #define CODE_LENGTH_ENCBASE 8
-#define SIZE_OF_RETURN_KIND_IN_SLIM_HEADER 2
-#define SIZE_OF_RETURN_KIND_IN_FAT_HEADER  4
 #define STACK_BASE_REGISTER_ENCBASE 3
 #define SIZE_OF_STACK_AREA_ENCBASE 3
 #define SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA_ENCBASE 4
@@ -679,8 +677,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)
@@ -695,8 +693,6 @@ void FASTCALL decodeCallPattern(int         pattern,
 #define SECURITY_OBJECT_STACK_SLOT_ENCBASE 5
 #define GS_COOKIE_STACK_SLOT_ENCBASE 5
 #define CODE_LENGTH_ENCBASE 7
-#define SIZE_OF_RETURN_KIND_IN_SLIM_HEADER 2
-#define SIZE_OF_RETURN_KIND_IN_FAT_HEADER  2
 #define STACK_BASE_REGISTER_ENCBASE 1
 #define SIZE_OF_STACK_AREA_ENCBASE 3
 #define SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA_ENCBASE 3
@@ -735,9 +731,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)
@@ -750,8 +746,6 @@ void FASTCALL decodeCallPattern(int         pattern,
 #define SECURITY_OBJECT_STACK_SLOT_ENCBASE 6
 #define GS_COOKIE_STACK_SLOT_ENCBASE 6
 #define CODE_LENGTH_ENCBASE 8
-#define SIZE_OF_RETURN_KIND_IN_SLIM_HEADER 2
-#define SIZE_OF_RETURN_KIND_IN_FAT_HEADER  4
 #define STACK_BASE_REGISTER_ENCBASE 2 // FP encoded as 0, SP as 2.
 #define SIZE_OF_STACK_AREA_ENCBASE 3
 #define SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA_ENCBASE 4
@@ -790,9 +784,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)
@@ -805,8 +799,6 @@ void FASTCALL decodeCallPattern(int         pattern,
 #define SECURITY_OBJECT_STACK_SLOT_ENCBASE 6
 #define GS_COOKIE_STACK_SLOT_ENCBASE 6
 #define CODE_LENGTH_ENCBASE 8
-#define SIZE_OF_RETURN_KIND_IN_SLIM_HEADER 2
-#define SIZE_OF_RETURN_KIND_IN_FAT_HEADER  4
 // FP/SP encoded as 0 or 1.
 #define STACK_BASE_REGISTER_ENCBASE 2
 #define SIZE_OF_STACK_AREA_ENCBASE 3
@@ -845,9 +837,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)
@@ -860,8 +852,6 @@ void FASTCALL decodeCallPattern(int         pattern,
 #define SECURITY_OBJECT_STACK_SLOT_ENCBASE 6
 #define GS_COOKIE_STACK_SLOT_ENCBASE 6
 #define CODE_LENGTH_ENCBASE 8
-#define SIZE_OF_RETURN_KIND_IN_SLIM_HEADER 2
-#define SIZE_OF_RETURN_KIND_IN_FAT_HEADER  4
 #define STACK_BASE_REGISTER_ENCBASE 2
 // FP encoded as 0, SP as 1
 #define SIZE_OF_STACK_AREA_ENCBASE 3
@@ -924,8 +914,6 @@ PORTABILITY_WARNING("Please specialize these definitions for your platform!")
 #define SECURITY_OBJECT_STACK_SLOT_ENCBASE 6
 #define GS_COOKIE_STACK_SLOT_ENCBASE 6
 #define CODE_LENGTH_ENCBASE 6
-#define SIZE_OF_RETURN_KIND_IN_SLIM_HEADER 2
-#define SIZE_OF_RETURN_KIND_IN_FAT_HEADER  2
 #define STACK_BASE_REGISTER_ENCBASE 3
 #define SIZE_OF_STACK_AREA_ENCBASE 6
 #define SIZE_OF_EDIT_AND_CONTINUE_PRESERVED_AREA_ENCBASE 3

diff --git a/src/coreclr/inc/readytorun.h b/src/coreclr/inc/readytorun.h
@@ -19,10 +19,10 @@
 //  src/coreclr/nativeaot/Runtime/inc/ModuleHeaders.h
 // If you update this, ensure you run `git grep MINIMUM_READYTORUN_MAJOR_VERSION`
 // and handle pending work.
-#define READYTORUN_MAJOR_VERSION 10
-#define READYTORUN_MINOR_VERSION 0x0001
+#define READYTORUN_MAJOR_VERSION 11
+#define READYTORUN_MINOR_VERSION 0x0000
 
-#define MINIMUM_READYTORUN_MAJOR_VERSION 10
+#define MINIMUM_READYTORUN_MAJOR_VERSION 11
 
 // R2R Version 2.1 adds the InliningInfo section
 // R2R Version 2.2 adds the ProfileDataInfo section
@@ -38,6 +38,7 @@
 //                 uses GCInfo v3, which makes safe points in partially interruptible code interruptible.
 // R2R Version 10.0 adds support for the statics being allocated on a per type basis instead of on a per module basis, disable support for LogMethodEnter helper
 // R2R Version 10.1 adds Unbox_TypeTest helper
+// R2R Version 11 uses GCInfo v4, which encodes safe points without -1 offset and does not track return kinds in GCInfo
 
 struct READYTORUN_CORE_HEADER
 {

diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp
@@ -3709,15 +3709,6 @@ class GcInfoEncoderWithLogging
         }
     }
 
-    void SetReturnKind(ReturnKind returnKind)
-    {
-        m_gcInfoEncoder->SetReturnKind(returnKind);
-        if (m_doLogging)
-        {
-            printf("Set ReturnKind to %s.\n", ReturnKindToString(returnKind));
-        }
-    }
-
     void SetStackBaseRegister(UINT32 registerNumber)
     {
         m_gcInfoEncoder->SetStackBaseRegister(registerNumber);
@@ -3832,8 +3823,6 @@ void GCInfo::gcInfoBlockHdrSave(GcInfoEncoder* gcInfoEncoder, unsigned methodSiz
 
     gcInfoEncoderWithLog->SetCodeLength(methodSize);
 
-    gcInfoEncoderWithLog->SetReturnKind(getReturnKind());
-
     if (compiler->isFramePointerUsed())
     {
         gcInfoEncoderWithLog->SetStackBaseRegister(REG_FPBASE);

diff --git a/src/coreclr/nativeaot/Runtime/inc/ModuleHeaders.h b/src/coreclr/nativeaot/Runtime/inc/ModuleHeaders.h
@@ -11,8 +11,8 @@ struct ReadyToRunHeaderConstants
 {
     static const uint32_t Signature = 0x00525452; // 'RTR'
 
-    static const uint32_t CurrentMajorVersion = 10;
-    static const uint32_t CurrentMinorVersion = 1;
+    static const uint32_t CurrentMajorVersion = 11;
+    static const uint32_t CurrentMinorVersion = 0;
 };
 
 struct ReadyToRunHeader

diff --git 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.IsSafePoint());
-        }
-    }
-
     ICodeManagerFlags flags = (ICodeManagerFlags)0;
+    bool executionAborted = ((UnixNativeMethodInfo*)pMethodInfo)->executionAborted;
     if (executionAborted)
         flags = ICodeManagerFlags::ExecutionAborted;