Fix stack walking and reporting of default interface methods (dotnet/coreclr#21525)

Default interface methods in their unresolved state don't have a generic context. The generic context is only added once the method is resolved to its implementation.

Commit migrated from https://github.com/dotnet/coreclr/commit/352a5cb1b358ba7e3a5addbf7bd4a5fd73499683

diff --git a/src/coreclr/src/inc/corcompile.h b/src/coreclr/src/inc/corcompile.h
index e04da79..a644dd8 100644 (file)
--- a/src/coreclr/src/inc/corcompile.h
+++ b/src/coreclr/src/inc/corcompile.h
@@ -1711,7 +1711,8 @@ class ICorCompileInfo
     //
     virtual void GetCallRefMap(
             CORINFO_METHOD_HANDLE hMethod,
-            GCRefMapBuilder * pBuilder) = 0;
+            GCRefMapBuilder * pBuilder,
+            bool isDispatchCell) = 0;
 
     // Returns a compressed block of debug information
     //

diff --git a/src/coreclr/src/vm/compile.cpp b/src/coreclr/src/vm/compile.cpp
index 1bd75fc..3cec66e 100644 (file)
--- a/src/coreclr/src/vm/compile.cpp
+++ b/src/coreclr/src/vm/compile.cpp
@@ -964,7 +964,7 @@ void FakeGcScanRoots(MetaSig& msig, ArgIterator& argit, MethodDesc * pMD, BYTE *
     }
 }
 
-void CEECompileInfo::GetCallRefMap(CORINFO_METHOD_HANDLE hMethod, GCRefMapBuilder * pBuilder)
+void CEECompileInfo::GetCallRefMap(CORINFO_METHOD_HANDLE hMethod, GCRefMapBuilder * pBuilder, bool isDispatchCell)
 {
 #ifdef _DEBUG
     DWORD dwInitialLength = pBuilder->GetBlobLength();
@@ -973,7 +973,25 @@ void CEECompileInfo::GetCallRefMap(CORINFO_METHOD_HANDLE hMethod, GCRefMapBuilde
 
     MethodDesc *pMD = (MethodDesc *)hMethod;
 
-    MetaSig msig(pMD);
+    SigTypeContext typeContext(pMD);
+    PCCOR_SIGNATURE pSig;
+    DWORD cbSigSize;
+    pMD->GetSig(&pSig, &cbSigSize);
+    MetaSig msig(pSig, cbSigSize, pMD->GetModule(), &typeContext);
+
+    //
+    // Shared default interface methods (i.e. virtual interface methods with an implementation) require
+    // an instantiation argument. But if we're in a situation where we haven't resolved the method yet
+    // we need to pretent that unresolved default interface methods are like any other interface
+    // methods and don't have an instantiation argument.
+    // See code:CEEInfo::getMethodSigInternal
+    //
+    assert(!isDispatchCell || !pMD->RequiresInstArg() || pMD->GetMethodTable()->IsInterface());
+    if (pMD->RequiresInstArg() && !isDispatchCell)
+    {
+        msig.SetHasParamTypeArg();
+    }
+
     ArgIterator argit(&msig);
 
     UINT nStackBytes = argit.SizeOfFrameArgumentArray();

diff --git a/src/coreclr/src/vm/compile.h b/src/coreclr/src/vm/compile.h
index 4307d97..79991dd 100644 (file)
--- a/src/coreclr/src/vm/compile.h
+++ b/src/coreclr/src/vm/compile.h
@@ -338,7 +338,8 @@ class CEECompileInfo : public ICorCompileInfo
                              SString                &result);
 
     void GetCallRefMap(CORINFO_METHOD_HANDLE hMethod, 
-                       GCRefMapBuilder * pBuilder);
+                       GCRefMapBuilder * pBuilder,
+                       bool isDispatchCell);
 
     void CompressDebugInfo(
                                     IN ICorDebugInfo::OffsetMapping * pOffsetMapping,

diff --git a/src/coreclr/src/vm/frames.cpp b/src/coreclr/src/vm/frames.cpp
index 4ca2e92..cea5241 100644 (file)
--- a/src/coreclr/src/vm/frames.cpp
+++ b/src/coreclr/src/vm/frames.cpp
@@ -1238,7 +1238,16 @@ void TransitionFrame::PromoteCallerStack(promote_func* fn, ScanContext* sc)
     //If not "vararg" calling convention, assume "default" calling convention
     if (!MetaSig::IsVarArg(pFunction->GetModule(), callSignature))
     {
-        MetaSig msig(pFunction);
+        SigTypeContext typeContext(pFunction);
+        PCCOR_SIGNATURE pSig;
+        DWORD cbSigSize;
+        pFunction->GetSig(&pSig, &cbSigSize);
+
+        MetaSig msig(pSig, cbSigSize, pFunction->GetModule(), &typeContext);
+
+        if (pFunction->RequiresInstArg() && !SuppressParamTypeArg())
+            msig.SetHasParamTypeArg();
+
         PromoteCallerStackHelper (fn, sc, pFunction, &msig);
     }
     else

diff --git a/src/coreclr/src/vm/frames.h b/src/coreclr/src/vm/frames.h
index a77ad63..f8bd4be 100644 (file)
--- a/src/coreclr/src/vm/frames.h
+++ b/src/coreclr/src/vm/frames.h
@@ -958,6 +958,15 @@ public:
     //---------------------------------------------------------------
     PTR_VOID GetParamTypeArg();
 
+    //---------------------------------------------------------------
+    // Gets value indicating whether the generic parameter type
+    // argument should be supressed.
+    //---------------------------------------------------------------
+    virtual BOOL SuppressParamTypeArg()
+    {
+        return FALSE;
+    }
+
 protected:  // we don't want people using this directly
     //---------------------------------------------------------------
     // Get the address of the "this" object. WARNING!!! Whether or not "this"
@@ -2267,6 +2276,22 @@ public:
 
     Interception GetInterception();
 
+    virtual BOOL SuppressParamTypeArg()
+    {
+        //
+        // Shared default interface methods (i.e. virtual interface methods with an implementation) require
+        // an instantiation argument. But if we're in the stub dispatch frame, we haven't actually resolved
+        // the method yet (we could end up in class's override of this method, for example).
+        //
+        // So we need to pretent that unresolved default interface methods are like any other interface
+        // methods and don't have an instantiation argument.
+        //
+        // See code:CEEInfo::getMethodSigInternal
+        //
+        assert(GetFunction()->GetMethodTable()->IsInterface());
+        return TRUE;
+    }
+
 private:
     friend class VirtualCallStubManager;
 

diff --git a/src/coreclr/src/vm/gcenv.ee.common.cpp b/src/coreclr/src/vm/gcenv.ee.common.cpp
index 8f247b7..8ce6709 100644 (file)
--- a/src/coreclr/src/vm/gcenv.ee.common.cpp
+++ b/src/coreclr/src/vm/gcenv.ee.common.cpp
@@ -81,7 +81,9 @@ unsigned FindFirstInterruptiblePoint(CrawlFrame* pCF, unsigned offs, unsigned en
 //-----------------------------------------------------------------------------
 // Determine whether we should report the generic parameter context
 // 
-// This is meant to detect the situation where a ThreadAbortException is raised
+// This is meant to detect following situations:
+//
+// When a ThreadAbortException is raised
 // in the prolog of a managed method, before the location for the generics 
 // context has been initialized; when such a TAE is raised, we are open to a
 // race with the GC (e.g. while creating the managed object for the TAE).
@@ -90,9 +92,23 @@ unsigned FindFirstInterruptiblePoint(CrawlFrame* pCF, unsigned offs, unsigned en
 // The long term solution is to avoid raising TAEs in any non-GC safe points, 
 // and to additionally ensure that we do not expose the runtime to TAE 
 // starvation.
+//
+// When we're in the process of resolution of an interface method and the
+// interface method happens to have a default implementation. Normally,
+// such methods require a generic context, but since we didn't resolve the
+// method to an implementation yet, we don't have the right context (in fact,
+// there's no context provided by the caller).
+// See code:CEEInfo::getMethodSigInternal
+//
 inline bool SafeToReportGenericParamContext(CrawlFrame* pCF)
 {
     LIMITED_METHOD_CONTRACT;
+
+    if (!pCF->IsFrameless() && pCF->GetFrame()->GetVTablePtr() == StubDispatchFrame::GetMethodFrameVPtr())
+    {
+        return !((StubDispatchFrame*)pCF->GetFrame())->SuppressParamTypeArg();
+    }
+
     if (!pCF->IsFrameless() || !(pCF->IsActiveFrame() || pCF->IsInterrupted()))
     {
         return true;
@@ -353,7 +369,7 @@ StackWalkAction GcStackCrawlCallBack(CrawlFrame* pCF, VOID* pData)
                         paramContextType = GENERIC_PARAM_CONTEXT_METHODTABLE;
                 }
 
-                if (SafeToReportGenericParamContext(pCF))
+                if (paramContextType != GENERIC_PARAM_CONTEXT_NONE && SafeToReportGenericParamContext(pCF))
                 {
                     // Handle the case where the method is a static shared generic method and we need to keep the type 
                     // of the generic parameters alive

diff --git a/src/coreclr/src/zap/zapimport.cpp b/src/coreclr/src/zap/zapimport.cpp
index 7b847e9..5e1a42c 100644 (file)
--- a/src/coreclr/src/zap/zapimport.cpp
+++ b/src/coreclr/src/zap/zapimport.cpp
@@ -658,7 +658,7 @@ void ZapImportSectionSignatures::PlaceStubDispatchCell(ZapImport * pImport)
 
     m_pImage->GetImportTable()->PlaceImportBlob(pCell);
 
-    m_pGCRefMapTable->Append(pCell->GetMethod());
+    m_pGCRefMapTable->Append(pCell->GetMethod(), true);
 }
 
 //
@@ -871,9 +871,9 @@ ZapImport * ZapImportTable::GetVirtualImportThunk(CORINFO_METHOD_HANDLE handle,
 // GCRefMapTable is used to encode for GC references locations for lazily resolved calls
 //
 
-void ZapGCRefMapTable::Append(CORINFO_METHOD_HANDLE handle)
+void ZapGCRefMapTable::Append(CORINFO_METHOD_HANDLE handle, bool isDispatchCell)
 {
-    m_pImage->GetCompileInfo()->GetCallRefMap(handle, &m_GCRefMapBuilder);
+    m_pImage->GetCompileInfo()->GetCallRefMap(handle, &m_GCRefMapBuilder, isDispatchCell);
     m_nCount++;
 }
 

diff --git a/src/coreclr/src/zap/zapimport.h b/src/coreclr/src/zap/zapimport.h
index 058cb0b..d9359e9 100644 (file)
--- a/src/coreclr/src/zap/zapimport.h
+++ b/src/coreclr/src/zap/zapimport.h
@@ -533,7 +533,7 @@ public:
     {
     }
 
-    void Append(CORINFO_METHOD_HANDLE handle);
+    void Append(CORINFO_METHOD_HANDLE handle, bool isDispatchCell = false);
 
     virtual DWORD GetSize();
 

diff --git a/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj b/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj
index 4874296..2cdea3a 100644 (file)
--- a/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj
+++ b/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj
@@ -15,9 +15,6 @@
     <OutputType>Exe</OutputType>
     <CLRTestKind>BuildAndRun</CLRTestKind>
     <CLRTestPriority>0</CLRTestPriority>
-    
-    <!-- Hitting failures in GCStress: https://github.com/dotnet/coreclr/issues/16376 --> 
-    <GCStressIncompatible>true</GCStressIncompatible>
   </PropertyGroup>
 
   <ItemGroup>

diff --git a/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj b/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj
index c1695ad..45d186c 100644 (file)
--- a/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj
+++ b/src/coreclr/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj
@@ -15,9 +15,6 @@
     <OutputType>Exe</OutputType>
     <CLRTestKind>BuildAndRun</CLRTestKind>
     <CLRTestPriority>0</CLRTestPriority>
-    
-    <!-- Hitting failures in GCStress: https://github.com/dotnet/coreclr/issues/16376 -->
-    <GCStressIncompatible>true</GCStressIncompatible>
   </PropertyGroup>
 
   <ItemGroup>

diff --git a/src/coreclr/tests/src/Regressions/coreclr/15241/genericcontext.ilproj b/src/coreclr/tests/src/Regressions/coreclr/15241/genericcontext.ilproj
index 40064db..ea2fd35 100644 (file)
--- a/src/coreclr/tests/src/Regressions/coreclr/15241/genericcontext.ilproj
+++ b/src/coreclr/tests/src/Regressions/coreclr/15241/genericcontext.ilproj
@@ -15,9 +15,6 @@
     <OutputType>Exe</OutputType>
     <CLRTestKind>BuildAndRun</CLRTestKind>
     <CLRTestPriority>0</CLRTestPriority>
-        
-    <!-- Fails GCStress: https://github.com/dotnet/coreclr/issues/16898 --> 
-    <GCStressIncompatible>true</GCStressIncompatible>
   </PropertyGroup>
 
   <ItemGroup>

diff --git a/src/coreclr/tests/src/Regressions/coreclr/16355/boring.ilproj b/src/coreclr/tests/src/Regressions/coreclr/16355/boring.ilproj
index ce5edc1..6bf3f70 100644 (file)
--- a/src/coreclr/tests/src/Regressions/coreclr/16355/boring.ilproj
+++ b/src/coreclr/tests/src/Regressions/coreclr/16355/boring.ilproj
@@ -15,9 +15,6 @@
     <OutputType>Exe</OutputType>
     <CLRTestKind>BuildAndRun</CLRTestKind>
     <CLRTestPriority>0</CLRTestPriority>
-       
-    <!-- Issue 21044, https://github.com/dotnet/coreclr/issues/21044 -->
-    <GCStressIncompatible>true</GCStressIncompatible>
   </PropertyGroup>
 
   <ItemGroup>

diff --git a/src/coreclr/tests/src/Regressions/coreclr/16775/sharedinterfacemethod.ilproj b/src/coreclr/tests/src/Regressions/coreclr/16775/sharedinterfacemethod.ilproj
index 25f2419..7521917 100644 (file)
--- a/src/coreclr/tests/src/Regressions/coreclr/16775/sharedinterfacemethod.ilproj
+++ b/src/coreclr/tests/src/Regressions/coreclr/16775/sharedinterfacemethod.ilproj
@@ -15,9 +15,6 @@
     <OutputType>Exe</OutputType>
     <CLRTestKind>BuildAndRun</CLRTestKind>
     <CLRTestPriority>0</CLRTestPriority>
-       
-    <!-- Issue 21044, https://github.com/dotnet/coreclr/issues/21044 -->
-    <GCStressIncompatible>true</GCStressIncompatible>
   </PropertyGroup>
 
   <ItemGroup>

diff --git a/src/coreclr/tests/src/Regressions/coreclr/20452/twopassvariance.ilproj b/src/coreclr/tests/src/Regressions/coreclr/20452/twopassvariance.ilproj
index 4ffff77..a34deda 100644 (file)
--- a/src/coreclr/tests/src/Regressions/coreclr/20452/twopassvariance.ilproj
+++ b/src/coreclr/tests/src/Regressions/coreclr/20452/twopassvariance.ilproj
@@ -15,9 +15,6 @@
     <OutputType>Exe</OutputType>
     <CLRTestKind>BuildAndRun</CLRTestKind>
     <CLRTestPriority>0</CLRTestPriority>
-       
-    <!-- Issue 21044, https://github.com/dotnet/coreclr/issues/21044 -->
-    <GCStressIncompatible>true</GCStressIncompatible>
   </PropertyGroup>
 
   <ItemGroup>