Delete RETURNTYPE and change how we get ReturnKind for gccover. (#24600)

* Move GetReturnKindFromMethodTable to method.hpp.

We would need this in other places in the next commits.

* Delete unnecessary checks from callhelpers.

* Do not check return types in CanDeduplicateCode.

GC info v.2 has this information and it is checked in another place.

* Change ComPlusMethodFrame to use the new function.

* Change gccover.cpp to use GetReturnKindFromMethodTable.

* Delete RETURNTYPE.

* Add check to ComPlusMethodFrame.

* Delete check from threadsuspend.

codeInfo->GetCodeManager()->GetReturnKind(gcInfoToken) must always return a valid kind nowdays (it could return an invalid lind only when GC Info v2 was not available).

* Rename functions/arguments.

* Add check for IsValidReturnKind.

* delete unused var.

src/inc/gcinfotypes.h

@@ -267,6 +267,20 @@ inline bool IsStructReturnKind(ReturnKind returnKind)
     return returnKind > 3;
 }
 
+inline bool IsScalarReturnKind(ReturnKind returnKind)
+{
+    return (returnKind == RT_Scalar)
+#ifdef _TARGET_X86_
+        || (returnKind == RT_Float)
+#endif // _TARGET_X86_
+        ;
+}
+
+inline bool IsPointerReturnKind(ReturnKind returnKind)
+{
+    return IsValidReturnKind(returnKind) && !IsScalarReturnKind(returnKind);
+}
+
 // Helpers for combining/extracting individual ReturnKinds from/to Struct ReturnKinds.
 // Encoding is two bits per register
 

src/vm/callhelpers.h

@@ -362,22 +362,13 @@ class MethodDescCallSite
 #define MDCALLDEF_ARGSLOT(wrappedmethod, ext)                                       \
         FORCEINLINE void wrappedmethod##ext (const ARG_SLOT* pArguments, ARG_SLOT *pReturnValue, int cbReturnValue) \
         {                                                                           \
-            WRAPPER_NO_CONTRACT;                                                    \
-            {                                                                       \
-                GCX_FORBID();  /* arg array is not protected */                     \
-            }                                                                       \
             CallTargetWorker(pArguments, pReturnValue, cbReturnValue);              \
             /* Bigendian layout not support */                                      \
         }
 
 #define MDCALLDEF_REFTYPE(wrappedmethod,  permitvaluetypes, ext, ptrtype, reftype)              \
         FORCEINLINE reftype wrappedmethod##ext (const ARG_SLOT* pArguments)                     \
         {                                                                                       \
-            WRAPPER_NO_CONTRACT;                                                                \
-            {                                                                                   \
-                GCX_FORBID();  /* arg array is not protected */                                 \
-                CONSISTENCY_CHECK(MetaSig::RETOBJ == m_pMD->ReturnsObject(true));               \
-            }                                                                                   \
             ARG_SLOT retval;                                                                    \
             CallTargetWorker(pArguments, &retval, sizeof(retval));                              \
             return ObjectTo##reftype(*(ptrtype *)                                               \

src/vm/compile.cpp

@@ -1355,30 +1355,6 @@ BOOL CanDeduplicateCode(CORINFO_METHOD_HANDLE method, CORINFO_METHOD_HANDLE dupl
     DynamicMethodDesc * pMethod = GetMethod(method)->AsDynamicMethodDesc();
     DynamicMethodDesc * pDuplicateMethod = GetMethod(duplicateMethod)->AsDynamicMethodDesc();
 
-    //
-    // Make sure that the return types match (for code:Thread::HijackThread)
-    //
-
-#ifdef _TARGET_X86_
-    MetaSig msig1(pMethod);
-    MetaSig msig2(pDuplicateMethod);
-    if (!msig1.HasFPReturn() != !msig2.HasFPReturn())
-        return FALSE;
-#endif // _TARGET_X86_
-
-    MetaSig::RETURNTYPE returnType = pMethod->ReturnsObject();
-    MetaSig::RETURNTYPE returnTypeDuplicate = pDuplicateMethod->ReturnsObject();
-
-    if (returnType != returnTypeDuplicate)
-        return FALSE;
-
-    //
-    // Do not enable deduplication of structs returned in registers
-    //
-
-    if (returnType == MetaSig::RETVALUETYPE)
-        return FALSE;
-
     //
     // Make sure that the IL stub flags match
     //

src/vm/frames.cpp

@@ -1422,7 +1422,7 @@ ComPlusMethodFrame::ComPlusMethodFrame(TransitionBlock * pTransitionBlock, Metho
 #endif // #ifndef DACCESS_COMPILE
 
 //virtual
-void ComPlusMethodFrame::GcScanRoots(promote_func *fn, ScanContext* sc)
+void ComPlusMethodFrame::GcScanRoots(promote_func* fn, ScanContext* sc)
 {
     WRAPPER_NO_CONTRACT;
 
@@ -1432,13 +1432,23 @@ void ComPlusMethodFrame::GcScanRoots(promote_func *fn, ScanContext* sc)
     FramedMethodFrame::GcScanRoots(fn, sc);
     PromoteCallerStack(fn, sc);
 
-    MetaSig::RETURNTYPE returnType = GetFunction()->ReturnsObject();
 
     // Promote the returned object
-    if(returnType == MetaSig::RETOBJ)
+    MethodDesc* methodDesc = GetFunction();
+    ReturnKind returnKind = methodDesc->GetReturnKind();
+    if (returnKind == RT_Object)
+    {
         (*fn)(GetReturnObjectPtr(), sc, CHECK_APP_DOMAIN);
-    else if (returnType == MetaSig::RETBYREF)
-        PromoteCarefully(fn, GetReturnObjectPtr(), sc, GC_CALL_INTERIOR|CHECK_APP_DOMAIN);
+    }
+    else if (returnKind == RT_ByRef)
+    {
+        PromoteCarefully(fn, GetReturnObjectPtr(), sc, GC_CALL_INTERIOR | CHECK_APP_DOMAIN);
+    }
+    else
+    {
+        _ASSERTE_MSG(!IsStructReturnKind(returnKind), "NYI: We can't promote multiregs struct returns");
+        _ASSERTE_MSG(IsScalarReturnKind(returnKind), "Non-scalar types must be promoted.");
+    }
 }
 #endif // FEATURE_COMINTEROP
 

src/vm/gccover.cpp

@@ -532,7 +532,8 @@ void GCCoverageInfo::SprinkleBreakpoints(
 
         if (prevDirectCallTargetMD != 0)
         {
-            if (prevDirectCallTargetMD->ReturnsObject(true) != MetaSig::RETNONOBJ)
+            ReturnKind returnKind = prevDirectCallTargetMD->GetReturnKind(true);
+            if (IsPointerReturnKind(returnKind))
                 *cur = INTERRUPT_INSTR_PROTECT_RET;  
             else
                 *cur = INTERRUPT_INSTR;
@@ -759,16 +760,16 @@ void replaceSafePointInstructionWithGcStressInstr(UINT32 safePointOffset, LPVOID
                 // never requires restore, however it is possible that it is initially in an invalid state
                 // and remains invalid until one or more eager fixups are applied.
                 //
-                // MethodDesc::ReturnsObject() consults the method signature, meaning it consults the
+                // GetReturnKind consults the method signature, meaning it consults the
                 // metadata in the owning module.  For generic instantiations stored in non-preferred
                 // modules, reaching the owning module requires following the module override pointer for
                 // the enclosing MethodTable.  In this case, the module override pointer is generally
                 // invalid until an associated eager fixup is applied.
                 //
-                // In situations like this, MethodDesc::ReturnsObject() will try to dereference an
+                // In situations like this, GetReturnKind will try to dereference an
                 // unresolved fixup and will AV.
                 //
-                // Given all of this, skip the MethodDesc::ReturnsObject() call by default to avoid
+                // Given all of this, skip the GetReturnKind call by default to avoid
                 // unexpected AVs.  This implies leaving out the GC coverage breakpoints for direct calls
                 // unless COMPlus_GcStressOnDirectCalls=1 is explicitly set in the environment.
                 //
@@ -777,8 +778,10 @@ void replaceSafePointInstructionWithGcStressInstr(UINT32 safePointOffset, LPVOID
 
                 if (fGcStressOnDirectCalls.val(CLRConfig::INTERNAL_GcStressOnDirectCalls))
                 {
+                    ReturnKind returnKind = targetMD->GetReturnKind(true);
+
                     // If the method returns an object then should protect the return object
-                    if (targetMD->ReturnsObject(true) != MetaSig::RETNONOBJ)
+                    if (IsPointerReturnKind(returnKind))
                     {
                         // replace with corresponding 2 or 4 byte illegal instruction (which roots the return value)
 #if defined(_TARGET_ARM_)
@@ -1607,17 +1610,16 @@ void DoGcStress (PCONTEXT regs, MethodDesc *pMD)
 
                 if (targetMD != 0)
                 {
-                    // Mark that we are performing a stackwalker like operation on the current thread.
-                    // This is necessary to allow the ReturnsObject function to work without triggering any loads
-                    ClrFlsValueSwitch _threadStackWalking(TlsIdx_StackWalkerWalkingThread, pThread);
-
                     // @Todo: possible race here, might need to be fixed  if it become a problem.
                     // It could become a problem if 64bit does partially interrupt work.
                     // OK, we have the MD, mark the instruction after the CALL
                     // appropriately
+
+                    ReturnKind returnKind = targetMD->GetReturnKind(true);
+                    bool protectReturn = IsPointerReturnKind(returnKind);
 #ifdef _TARGET_ARM_
                     size_t instrLen = GetARMInstructionLength(nextInstr);
-                    if (targetMD->ReturnsObject(true) != MetaSig::RETNONOBJ)
+                    if (protectReturn)
                         if (instrLen == 2)
                             *(WORD*)nextInstr  = INTERRUPT_INSTR_PROTECT_RET;
                         else
@@ -1628,12 +1630,12 @@ void DoGcStress (PCONTEXT regs, MethodDesc *pMD)
                         else
                             *(DWORD*)nextInstr = INTERRUPT_INSTR_32;
 #elif defined(_TARGET_ARM64_)
-                    if (targetMD->ReturnsObject(true) != MetaSig::RETNONOBJ)
+                    if (protectReturn)
                         *(DWORD *)nextInstr = INTERRUPT_INSTR_PROTECT_RET;  
                     else
                         *(DWORD *)nextInstr = INTERRUPT_INSTR;
 #else
-                    if (targetMD->ReturnsObject(true) != MetaSig::RETNONOBJ)
+                    if (protectReturn)
                         *nextInstr = INTERRUPT_INSTR_PROTECT_RET;  
                     else
                         *nextInstr = INTERRUPT_INSTR;

src/vm/method.cpp

@@ -1213,12 +1213,7 @@ COR_ILMETHOD* MethodDesc::GetILHeader(BOOL fAllowOverrides /*=FALSE*/)
 }
 
 //*******************************************************************************
-MetaSig::RETURNTYPE MethodDesc::ReturnsObject(
-#ifdef _DEBUG
-    bool supportStringConstructors,
-#endif
-    MethodTable** pMT
-    )
+ReturnKind MethodDesc::ParseReturnKindFromSig(INDEBUG(bool supportStringConstructors))
 {
     CONTRACTL
     {
@@ -1243,7 +1238,7 @@ MetaSig::RETURNTYPE MethodDesc::ReturnsObject(
         case ELEMENT_TYPE_ARRAY:
         case ELEMENT_TYPE_OBJECT:
         case ELEMENT_TYPE_VAR:
-            return(MetaSig::RETOBJ);
+            return RT_Object;
 
 #ifdef ENREGISTERED_RETURNTYPE_INTEGER_MAXSIZE
         case ELEMENT_TYPE_VALUETYPE:
@@ -1258,22 +1253,39 @@ MetaSig::RETURNTYPE MethodDesc::ReturnsObject(
                     if (!thValueType.IsTypeDesc())
                     {
                         MethodTable * pReturnTypeMT = thValueType.AsMethodTable();
-                        if (pMT != NULL)
-                        {
-                            *pMT = pReturnTypeMT;
-                        }
-
 #ifdef UNIX_AMD64_ABI
                         if (pReturnTypeMT->IsRegPassedStruct())
                         {
-                            return MetaSig::RETVALUETYPE;
+                            // The Multi-reg return case using the classhandle is only implemented for AMD64 SystemV ABI.
+                            // On other platforms, multi-reg return is not supported with GcInfo v1.
+                            // So, the relevant information must be obtained from the GcInfo tables (which requires version2).
+                            EEClass* eeClass = pReturnTypeMT->GetClass();
+                            ReturnKind regKinds[2] = { RT_Unset, RT_Unset };
+                            int orefCount = 0;
+                            for (int i = 0; i < 2; i++)
+                            {
+                                if (eeClass->GetEightByteClassification(i) == SystemVClassificationTypeIntegerReference)
+                                {
+                                    regKinds[i] = RT_Object;
+                                }
+                                else if (eeClass->GetEightByteClassification(i) == SystemVClassificationTypeIntegerByRef)
+                                {
+                                    regKinds[i] = RT_ByRef;
+                                }
+                                else
+                                {
+                                    regKinds[i] = RT_Scalar;
+                                }
+                            }
+                            ReturnKind structReturnKind = GetStructReturnKind(regKinds[0], regKinds[1]);
+                            return structReturnKind;
                         }
-#endif // !UNIX_AMD64_ABI
+#endif // UNIX_AMD64_ABI
 
                         if (pReturnTypeMT->ContainsPointers())
                         {
                             _ASSERTE(pReturnTypeMT->GetNumInstanceFieldBytes() == sizeof(void*));
-                            return MetaSig::RETOBJ;
+                            return RT_Object;
                         }
                     }
                 }
@@ -1290,19 +1302,47 @@ MetaSig::RETURNTYPE MethodDesc::ReturnsObject(
             if (IsCtor() && GetMethodTable()->HasComponentSize())
             {
                 _ASSERTE(supportStringConstructors);
-                return MetaSig::RETOBJ;
+                return RT_Object;
             }
             break;
 #endif // _DEBUG
 
         case ELEMENT_TYPE_BYREF:
-            return(MetaSig::RETBYREF);
+            return RT_ByRef;
 
         default:
             break;
     }
 
-    return(MetaSig::RETNONOBJ);
+    return RT_Scalar;
+}
+
+ReturnKind MethodDesc::GetReturnKind(INDEBUG(bool supportStringConstructors))
+{
+#ifdef _WIN64
+    // For simplicity, we don't hijack in funclets, but if you ever change that, 
+    // be sure to choose the OnHijack... callback type to match that of the FUNCLET
+    // not the main method (it would probably be Scalar).
+#endif // _WIN64
+
+    ENABLE_FORBID_GC_LOADER_USE_IN_THIS_SCOPE();
+    // Mark that we are performing a stackwalker like operation on the current thread.
+    // This is necessary to allow the signature parsing functions to work without triggering any loads
+    ClrFlsValueSwitch threadStackWalking(TlsIdx_StackWalkerWalkingThread, GetThread());
+
+#ifdef _TARGET_X86_
+    MetaSig msig(this);
+    if (msig.HasFPReturn())
+    {
+        // Figuring out whether the function returns FP or not is hard to do
+        // on-the-fly, so we use a different callback helper on x86 where this
+        // piece of information is needed in order to perform the right save &
+        // restore of the return value around the call to OnHijackScalarWorker.
+        return RT_Float;
+    }
+#endif // _TARGET_X86_
+
+    return ParseReturnKindFromSig(INDEBUG(supportStringConstructors));
 }
 
 #ifdef FEATURE_COMINTEROP

src/vm/method.hpp

@@ -1662,18 +1662,11 @@ class MethodDesc
     MethodDesc *ResolveGenericVirtualMethod(OBJECTREF *orThis);
 
 
-public:
-
-    // does this function return an object reference?
-    MetaSig::RETURNTYPE ReturnsObject(
-#ifdef _DEBUG 
-        bool supportStringConstructors = false,
-#endif
-        MethodTable** pMT = NULL
-        );
-
+private:
+    ReturnKind ParseReturnKindFromSig(INDEBUG(bool supportStringConstructors = false));
 
-    void Destruct();
+public:
+    ReturnKind GetReturnKind(INDEBUG(bool supportStringConstructors = false));
 
 public:
     // In general you don't want to call GetCallTarget - you want to

src/vm/siginfo.hpp

@@ -887,9 +887,6 @@ class MetaSig
 
         BOOL IsReturnTypeVoid() const;
 
-
-        enum RETURNTYPE {RETOBJ, RETBYREF, RETNONOBJ, RETVALUETYPE};
-
         CorElementType GetReturnTypeNormalized(TypeHandle * pthValueType = NULL) const;
 
         //------------------------------------------------------------------