From 78978a3d580f67ba577b274781556d374b4d56c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Thu, 13 Dec 2018 18:46:34 +0100 Subject: [PATCH 1/6] Fix stack walking and reporting of default interface methods 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. --- src/vm/frames.cpp | 11 +++++++- src/vm/frames.h | 26 +++++++++++++++++++ src/vm/gcenv.ee.common.cpp | 18 ++++++++++++- .../sharedgenerics/sharedgenerics_d.ilproj | 3 --- .../sharedgenerics/sharedgenerics_r.ilproj | 3 --- .../coreclr/15241/genericcontext.ilproj | 3 --- .../Regressions/coreclr/16355/boring.ilproj | 3 --- .../16775/sharedinterfacemethod.ilproj | 3 --- 8 files changed, 53 insertions(+), 17 deletions(-) diff --git a/src/vm/frames.cpp b/src/vm/frames.cpp index 4ca2e9261075..cea5241ef54e 100644 --- a/src/vm/frames.cpp +++ b/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/vm/frames.h b/src/vm/frames.h index a77ad63d1208..cf00dafe2f4c 100644 --- a/src/vm/frames.h +++ b/src/vm/frames.h @@ -958,6 +958,15 @@ class TransitionFrame : public Frame //--------------------------------------------------------------- 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,23 @@ class StubDispatchFrame : public FramedMethodFrame Interception GetInterception(); + virtual BOOL SuppressParamTypeArg() override + { + MethodDesc *pMD = GetFunction(); + + // + // 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 + // + return pMD->GetMethodTable()->IsInterface(); + } + private: friend class VirtualCallStubManager; diff --git a/src/vm/gcenv.ee.common.cpp b/src/vm/gcenv.ee.common.cpp index 8f247b7f2699..eaea247047e1 100644 --- a/src/vm/gcenv.ee.common.cpp +++ b/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; diff --git a/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj b/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj index 4874296a2b2c..2cdea3a3c62f 100644 --- a/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj +++ b/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj @@ -15,9 +15,6 @@ Exe BuildAndRun 0 - - - true diff --git a/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj b/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj index c1695ad77329..45d186c15f3a 100644 --- a/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj +++ b/tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj @@ -15,9 +15,6 @@ Exe BuildAndRun 0 - - - true diff --git a/tests/src/Regressions/coreclr/15241/genericcontext.ilproj b/tests/src/Regressions/coreclr/15241/genericcontext.ilproj index 40064db4a144..ea2fd35bfc2c 100644 --- a/tests/src/Regressions/coreclr/15241/genericcontext.ilproj +++ b/tests/src/Regressions/coreclr/15241/genericcontext.ilproj @@ -15,9 +15,6 @@ Exe BuildAndRun 0 - - - true diff --git a/tests/src/Regressions/coreclr/16355/boring.ilproj b/tests/src/Regressions/coreclr/16355/boring.ilproj index ce5edc1e0675..6bf3f70e6ff4 100644 --- a/tests/src/Regressions/coreclr/16355/boring.ilproj +++ b/tests/src/Regressions/coreclr/16355/boring.ilproj @@ -15,9 +15,6 @@ Exe BuildAndRun 0 - - - true diff --git a/tests/src/Regressions/coreclr/16775/sharedinterfacemethod.ilproj b/tests/src/Regressions/coreclr/16775/sharedinterfacemethod.ilproj index 25f2419039d5..7521917db5ce 100644 --- a/tests/src/Regressions/coreclr/16775/sharedinterfacemethod.ilproj +++ b/tests/src/Regressions/coreclr/16775/sharedinterfacemethod.ilproj @@ -15,9 +15,6 @@ Exe BuildAndRun 0 - - - true From 80bcf621464ad67c447ea19a7289f13213348a4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 14 Dec 2018 10:42:28 +0100 Subject: [PATCH 2/6] I hate C++ --- src/vm/frames.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vm/frames.h b/src/vm/frames.h index cf00dafe2f4c..a4110b01b92b 100644 --- a/src/vm/frames.h +++ b/src/vm/frames.h @@ -2276,7 +2276,7 @@ class StubDispatchFrame : public FramedMethodFrame Interception GetInterception(); - virtual BOOL SuppressParamTypeArg() override + virtual BOOL SuppressParamTypeArg() { MethodDesc *pMD = GetFunction(); From 211a281940e9a99ab038f47513914fed7b841b8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 14 Dec 2018 13:19:21 +0100 Subject: [PATCH 3/6] Also fixes a test I just checked in --- tests/src/Regressions/coreclr/20452/twopassvariance.ilproj | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/src/Regressions/coreclr/20452/twopassvariance.ilproj b/tests/src/Regressions/coreclr/20452/twopassvariance.ilproj index 4ffff779bc2e..a34dedab730b 100644 --- a/tests/src/Regressions/coreclr/20452/twopassvariance.ilproj +++ b/tests/src/Regressions/coreclr/20452/twopassvariance.ilproj @@ -15,9 +15,6 @@ Exe BuildAndRun 0 - - - true From 3f78926566906fa196b89029d9dff756255dfb11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Fri, 14 Dec 2018 23:31:46 +0100 Subject: [PATCH 4/6] Fix crossgen --- src/inc/corcompile.h | 3 ++- src/vm/compile.cpp | 19 +++++++++++++++++-- src/vm/compile.h | 3 ++- src/zap/zapimport.cpp | 6 +++--- src/zap/zapimport.h | 2 +- 5 files changed, 25 insertions(+), 8 deletions(-) diff --git a/src/inc/corcompile.h b/src/inc/corcompile.h index e04da79365c7..a644dd80261b 100644 --- a/src/inc/corcompile.h +++ b/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/vm/compile.cpp b/src/vm/compile.cpp index 1bd75fcffdda..ff6ce8b5160e 100644 --- a/src/vm/compile.cpp +++ b/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,22 @@ 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 + // + if (pMD->RequiresInstArg() && (!isDispatchCell || !pMD->GetMethodTable()->IsInterface())) + msig.SetHasParamTypeArg(); + ArgIterator argit(&msig); UINT nStackBytes = argit.SizeOfFrameArgumentArray(); diff --git a/src/vm/compile.h b/src/vm/compile.h index 4307d971474c..79991ddee644 100644 --- a/src/vm/compile.h +++ b/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/zap/zapimport.cpp b/src/zap/zapimport.cpp index 7b847e943037..5e1a42c11920 100644 --- a/src/zap/zapimport.cpp +++ b/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/zap/zapimport.h b/src/zap/zapimport.h index 058cb0b1459f..d9359e9df8ca 100644 --- a/src/zap/zapimport.h +++ b/src/zap/zapimport.h @@ -533,7 +533,7 @@ class ZapGCRefMapTable : public ZapNode { } - void Append(CORINFO_METHOD_HANDLE handle); + void Append(CORINFO_METHOD_HANDLE handle, bool isDispatchCell = false); virtual DWORD GetSize(); From da291cf360f5b5fd96eec67e8c7e7f28203ee064 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 17 Dec 2018 12:23:53 +0100 Subject: [PATCH 5/6] Review feedback --- src/vm/compile.cpp | 5 ++++- src/vm/frames.h | 3 ++- src/vm/gcenv.ee.common.cpp | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/vm/compile.cpp b/src/vm/compile.cpp index ff6ce8b5160e..3cec66e8332d 100644 --- a/src/vm/compile.cpp +++ b/src/vm/compile.cpp @@ -986,8 +986,11 @@ void CEECompileInfo::GetCallRefMap(CORINFO_METHOD_HANDLE hMethod, GCRefMapBuilde // methods and don't have an instantiation argument. // See code:CEEInfo::getMethodSigInternal // - if (pMD->RequiresInstArg() && (!isDispatchCell || !pMD->GetMethodTable()->IsInterface())) + assert(!isDispatchCell || !pMD->RequiresInstArg() || pMD->GetMethodTable()->IsInterface()); + if (pMD->RequiresInstArg() && !isDispatchCell) + { msig.SetHasParamTypeArg(); + } ArgIterator argit(&msig); diff --git a/src/vm/frames.h b/src/vm/frames.h index a4110b01b92b..d5b5fc2df833 100644 --- a/src/vm/frames.h +++ b/src/vm/frames.h @@ -2290,7 +2290,8 @@ class StubDispatchFrame : public FramedMethodFrame // // See code:CEEInfo::getMethodSigInternal // - return pMD->GetMethodTable()->IsInterface(); + assert(pMD->GetMethodTable()->IsInterface()); + return TRUE; } private: diff --git a/src/vm/gcenv.ee.common.cpp b/src/vm/gcenv.ee.common.cpp index eaea247047e1..8ce6709a61c5 100644 --- a/src/vm/gcenv.ee.common.cpp +++ b/src/vm/gcenv.ee.common.cpp @@ -369,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 From 7c9389a19a59ae32fd3c51f616bc8dea05be764a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?= Date: Mon, 17 Dec 2018 14:05:24 +0100 Subject: [PATCH 6/6] I hate C++ --- src/vm/frames.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/vm/frames.h b/src/vm/frames.h index d5b5fc2df833..f8bd4bec796b 100644 --- a/src/vm/frames.h +++ b/src/vm/frames.h @@ -2278,8 +2278,6 @@ class StubDispatchFrame : public FramedMethodFrame virtual BOOL SuppressParamTypeArg() { - MethodDesc *pMD = GetFunction(); - // // 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 @@ -2290,7 +2288,7 @@ class StubDispatchFrame : public FramedMethodFrame // // See code:CEEInfo::getMethodSigInternal // - assert(pMD->GetMethodTable()->IsInterface()); + assert(GetFunction()->GetMethodTable()->IsInterface()); return TRUE; }