Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix stack walking and reporting of default interface methods #21525

Merged
MichalStrehovsky merged 7 commits into from
Dec 18, 2018
Merged

Fix stack walking and reporting of default interface methods #21525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
78978a3
Fix stack walking and reporting of default interface methods
MichalStrehovsky Dec 13, 2018
80bcf62
I hate C++
MichalStrehovsky Dec 14, 2018
e210c0b
Merge branch 'master' into gcStress
MichalStrehovsky Dec 14, 2018
211a281
Also fixes a test I just checked in
MichalStrehovsky Dec 14, 2018
3f78926
Fix crossgen
MichalStrehovsky Dec 14, 2018
da291cf
Review feedback
MichalStrehovsky Dec 17, 2018
7c9389a
I hate C++
MichalStrehovsky Dec 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/inc/corcompile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
//
Expand Down
22 changes: 20 additions & 2 deletions src/vm/compile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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();
Expand Down
3 changes: 2 additions & 1 deletion src/vm/compile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
11 changes: 10 additions & 1 deletion src/vm/frames.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

@jkotas jkotas Dec 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a matching fix for PromoteCallerStackUsingGCRefMap - adjust the GCRef map that gets saved into R2R image at crossgen time?

You should be able to verify it by compiling the affected tests using crossgen before running them.

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Dec 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it didn't work. Thanks for spotting! I pushed out a fix.


if (pFunction->RequiresInstArg() && !SuppressParamTypeArg())
msig.SetHasParamTypeArg();

PromoteCallerStackHelper (fn, sc, pFunction, &msig);
}
else
Expand Down
25 changes: 25 additions & 0 deletions src/vm/frames.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -2267,6 +2276,22 @@ class StubDispatchFrame : public FramedMethodFrame

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;

Expand Down
20 changes: 18 additions & 2 deletions src/vm/gcenv.ee.common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions src/zap/zapimport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

//
Expand Down Expand Up @@ -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++;
}

Expand Down
2 changes: 1 addition & 1 deletion src/zap/zapimport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
3 changes: 0 additions & 3 deletions tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_d.ilproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
3 changes: 0 additions & 3 deletions tests/src/Loader/classloader/DefaultInterfaceMethods/sharedgenerics/sharedgenerics_r.ilproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
3 changes: 0 additions & 3 deletions tests/src/Regressions/coreclr/15241/genericcontext.ilproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
3 changes: 0 additions & 3 deletions tests/src/Regressions/coreclr/16355/boring.ilproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
3 changes: 0 additions & 3 deletions tests/src/Regressions/coreclr/16775/sharedinterfacemethod.ilproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
3 changes: 0 additions & 3 deletions tests/src/Regressions/coreclr/20452/twopassvariance.ilproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down