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

Handle generics in methodimpls for default interface methods #20404

Merged
merged 6 commits into from Nov 13, 2018
Merged

Handle generics in methodimpls for default interface methods #20404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
52a4c2f
Handle generics in methodimpls for default interface methods
MichalStrehovsky Oct 12, 2018
c1492d4
Merge branch 'master' into fix16355
MichalStrehovsky Oct 13, 2018
ccb3d5f
Fix the test
MichalStrehovsky Oct 13, 2018
1336469
Maybe also fix 20281
MichalStrehovsky Oct 16, 2018
7d1fe6f
Minor simplification
MichalStrehovsky Oct 16, 2018
e0ad8f1
Add one more test
MichalStrehovsky Oct 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 8 additions & 4 deletions src/vm/methodimpl.cpp
View file
Open in desktop
Expand Up @@ -164,7 +164,8 @@ void MethodImpl::SetSize(LoaderHeap *pHeap, AllocMemTracker *pamTracker, DWORD s
if(size > 0) {
// An array of DWORDs, the first entry representing count, and the rest representing slot numbers
S_SIZE_T cbCountAndSlots = S_SIZE_T(sizeof(DWORD)) + // DWORD for the total count of slots
S_SIZE_T(size) * S_SIZE_T(sizeof(DWORD)); // DWORD each for the slot numbers
S_SIZE_T(size) * S_SIZE_T(sizeof(DWORD)) + // DWORD each for the slot numbers
S_SIZE_T(size) * S_SIZE_T(sizeof(mdToken)); // Token each for the method tokens

// MethodDesc* for each of the implemented methods
S_SIZE_T cbMethodDescs = S_SIZE_T(size) * S_SIZE_T(sizeof(RelativePointer<MethodDesc *>));
Expand All @@ -190,7 +191,7 @@ void MethodImpl::SetSize(LoaderHeap *pHeap, AllocMemTracker *pamTracker, DWORD s
}

///////////////////////////////////////////////////////////////////////////////////////
void MethodImpl::SetData(DWORD* slots, RelativePointer<MethodDesc*>* md)
void MethodImpl::SetData(DWORD* slots, mdToken* tokens, RelativePointer<MethodDesc*>* md)
{
CONTRACTL {
NOTHROW;
Expand All @@ -202,6 +203,9 @@ void MethodImpl::SetData(DWORD* slots, RelativePointer<MethodDesc*>* md)
DWORD *pdwSize = pdwSlots.GetValue();
DWORD dwSize = *pdwSize;
memcpy(&(pdwSize[1]), slots, dwSize*sizeof(DWORD));

// Copy tokens that correspond to the slots above
memcpy(&(pdwSize[1 + dwSize]), tokens, dwSize*sizeof(mdToken));

RelativePointer<MethodDesc *> *pImplMD = pImplementedMD.GetValue();

Expand All @@ -219,7 +223,7 @@ void MethodImpl::Save(DataImage *image)
DWORD size = GetSize();
_ASSERTE(size > 0);

image->StoreStructure(pdwSlots.GetValue(), (size+1)*sizeof(DWORD),
image->StoreStructure(pdwSlots.GetValue(), (size+1)*sizeof(DWORD)+size*sizeof(mdToken),
DataImage::ITEM_METHOD_DESC_COLD,
sizeof(DWORD));
image->StoreStructure(pImplementedMD.GetValue(), size*sizeof(RelativePointer<MethodDesc*>),
Expand Down Expand Up @@ -277,7 +281,7 @@ MethodImpl::EnumMemoryRegions(CLRDataEnumMemoryFlags flags)
{
ULONG32 numSlots = GetSize();
DacEnumMemoryRegion(dac_cast<TADDR>(GetSlotsRawNonNull()),
(numSlots + 1) * sizeof(DWORD));
(numSlots + 1) * sizeof(DWORD) + numSlots * sizeof(mdToken));

if (GetImpMDs().IsValid())
{
Expand Down
22 changes: 20 additions & 2 deletions src/vm/methodimpl.h
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@ class MethodImpl
friend class NativeImageDumper;
#endif

RelativePointer<PTR_DWORD> pdwSlots; // Maintains the slots in sorted order, the first entry is the size
RelativePointer<PTR_DWORD> pdwSlots; // Maintains the slots and tokens in sorted order, the first entry is the size
RelativePointer<DPTR( RelativePointer<PTR_MethodDesc> )> pImplementedMD;

public:
Expand All @@ -46,6 +46,8 @@ class MethodImpl
{ WRAPPER_NO_CONTRACT; if (IsValid()) m_iCur++; }
inline WORD GetSlot()
{ WRAPPER_NO_CONTRACT; CONSISTENCY_CHECK(IsValid()); _ASSERTE(FitsIn<WORD>(m_pImpl->GetSlots()[m_iCur])); return static_cast<WORD>(m_pImpl->GetSlots()[m_iCur]); }
inline mdToken GetToken()
{ WRAPPER_NO_CONTRACT; CONSISTENCY_CHECK(IsValid()); return m_pImpl->GetTokens()[m_iCur]; }
inline MethodDesc *GetMethodDesc()
{ WRAPPER_NO_CONTRACT; return m_pImpl->GetMethodDesc(m_iCur, (PTR_MethodDesc) m_pMD); }
};
Expand Down Expand Up @@ -108,11 +110,27 @@ class MethodImpl

#ifndef DACCESS_COMPILE

///////////////////////////////////////////////////////////////////////////////////////
inline mdToken* GetTokens()
{
CONTRACTL{
NOTHROW;
GC_NOTRIGGER;
PRECONDITION(CheckPointer(this));
SUPPORTS_DAC;
} CONTRACTL_END;

if (pdwSlots.IsNull())
return NULL;
else
return (mdToken*)(GetSlotsRawNonNull() + 1 + *GetSlotsRawNonNull());
}

///////////////////////////////////////////////////////////////////////////////////////
void SetSize(LoaderHeap *pHeap, AllocMemTracker *pamTracker, DWORD size);

///////////////////////////////////////////////////////////////////////////////////////
void SetData(DWORD* slots, RelativePointer<MethodDesc*> * md);
void SetData(DWORD* slots, mdToken* tokens, RelativePointer<MethodDesc*> * md);

#endif // !DACCESS_COMPILE

Expand Down
80 changes: 51 additions & 29 deletions src/vm/methodtable.cpp
View file
Open in desktop
Expand Up @@ -7161,8 +7161,11 @@ BOOL MethodTable::FindDefaultInterfaceImplementation(
{
if (pCurMT->HasSameTypeDefAs(pInterfaceMT))
{
// Generic variance match - we'll instantiate pCurMD with the right type arguments later
pCurMD = pInterfaceMD;
if (!pInterfaceMD->IsAbstract())
{
// Generic variance match - we'll instantiate pCurMD with the right type arguments later
pCurMD = pInterfaceMD;
}
}
else
{
Expand All @@ -7171,43 +7174,62 @@ BOOL MethodTable::FindDefaultInterfaceImplementation(
// Implicit override in default interface methods are not allowed
//
MethodIterator methodIt(pCurMT);
for (; methodIt.IsValid(); methodIt.Next())
for (; methodIt.IsValid() && pCurMD == NULL; methodIt.Next())
{
MethodDesc *pMD = methodIt.GetMethodDesc();
int targetSlot = pInterfaceMD->GetSlot();

if (pMD->IsMethodImpl())
// If this is not a MethodImpl, it can't be implementing the method we're looking for
if (!pMD->IsMethodImpl())
continue;

// We have a MethodImpl - iterate over all the declarations it's implementing,
// looking for the interface method we need.
MethodImpl::Iterator it(pMD);
for (; it.IsValid() && pCurMD == NULL; it.Next())
{
MethodImpl::Iterator it(pMD);
for (; it.IsValid(); it.Next())
{
MethodDesc *pDeclMD = it.GetMethodDesc();
MethodDesc *pDeclMD = it.GetMethodDesc();

if (pDeclMD->GetSlot() != targetSlot)
continue;
// Is this the right slot?
if (pDeclMD->GetSlot() != targetSlot)
continue;

MethodTable *pDeclMT = pDeclMD->GetMethodTable();
if (pDeclMT->ContainsGenericVariables())
{
TypeHandle thInstDeclMT = ClassLoader::LoadGenericInstantiationThrowing(
pDeclMT->GetModule(),
pDeclMT->GetCl(),
pCurMT->GetInstantiation());
MethodTable *pInstDeclMT = thInstDeclMT.GetMethodTable();
if (pInstDeclMT == pInterfaceMT)
{
// This is a matching override. We'll instantiate pCurMD later
pCurMD = pMD;
break;
}
}
else if (pDeclMD == pInterfaceMD)
// Is this the right interface?
if (!pDeclMD->HasSameMethodDefAs(pInterfaceMD))
continue;

if (pInterfaceMD->HasClassInstantiation())
{
// pInterfaceMD will be in the canonical form, so we need to check the specific
// instantiation against pInterfaceMT.
//
// The parent of pDeclMD is unreliable for this purpose because it may or
// may not be canonicalized. Let's go from the metadata.

SigTypeContext typeContext = SigTypeContext(pCurMT);

mdTypeRef tkParent;
IfFailThrow(pMD->GetModule()->GetMDImport()->GetParentToken(it.GetToken(), &tkParent));

MethodTable* pDeclMT = ClassLoader::LoadTypeDefOrRefOrSpecThrowing(
pMD->GetModule(),
tkParent,
&typeContext).AsMethodTable();

// We do CanCastToInterface to also cover variance.
// We already know this is a method on the same type definition as the (generic)
// interface but we need to make sure the instantiations match.
Copy link
Member

Choose a reason for hiding this comment

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

Variant dispatch is typically done via a two pass technique. Wherein we first iterate to see if there is an exact match, and then a second iteration through the interfaces (of a particular type) to attempt to do a variant dispatch. Looking at the logic in this function, it appears that the variant matching rules here may not be followed, and the variance test cases added do not appear to cover the exact/inexact matching issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue of it ignoring variant interface dispatch rules and not doing two passes is orthogonal to the thing I'm fixing here.

I would prefer to track that in a separate issue/pull request for accounting purposes.

Copy link
Member Author

Choose a reason for hiding this comment

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

#20452

if (pDeclMT->CanCastToInterface(pInterfaceMT))
{
// Exact match override
// We have a match
pCurMD = pMD;
break;
}
}
}
else
{
// No generics involved. If the method definitions match, it's a match.
pCurMD = pMD;
}
}
}
}
Expand Down