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

Default Interface Method Prototype #10505

Merged
merged 23 commits into from
Apr 6, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
eecb802
allow non-zero RVA on abstract interface method in ilasm
Mar 11, 2017
83ba711
Revert "allow non-zero RVA on abstract interface method in ilasm"
Mar 12, 2017
675e681
add a test case and allow virtual non-abstract method in ilasm
Mar 12, 2017
c986dbe
allow non-abstract methods in the loader
Mar 13, 2017
6e36e29
fixup dispatch map
Mar 14, 2017
b6db058
support for simple default interface method scenario
Mar 16, 2017
a2a95a7
fix a bug with incorrect usage of MethodIterator skpping the first me…
Mar 16, 2017
205e7f5
add another simple test case for base class
Mar 16, 2017
60afcd5
allow private/internal methods in ilasm and add a explict impl test
Mar 17, 2017
f6e3c08
update reference to mscorlib in il test
Mar 17, 2017
c48881f
add shared generics and variance case
Mar 17, 2017
ed5c9d6
allow interface dispatch to return instantiating stubs with the right…
Mar 18, 2017
adf1a09
simple factoring and add a valuetype test case
Mar 18, 2017
546df05
add a test case for generic virtual methods
Mar 20, 2017
97108f6
a bit more refactoring by moving the CALLCONV_PARAMTYPE logic inside …
Mar 25, 2017
7efe42e
support explicit methodimpl and remove implicit methodimpl test case
Mar 26, 2017
a52dd22
update test cases to give more clear output
Mar 26, 2017
5078548
Fix a bug where GetMethodDescForSlot chokes on interface MethodDesc w…
Mar 26, 2017
4d9e3cf
cleanup code after review and add a bit more comments
Mar 26, 2017
be54957
update comments
Mar 27, 2017
9fdf872
only use custom ILAsm for default interface methods tests - some test…
Mar 28, 2017
f8295a2
address comments and allow instance methods, and add a constraint val…
Apr 6, 2017
a1ceb77
disable the failing protected method scenario
Apr 6, 2017
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
1 change: 0 additions & 1 deletion src/ilasm/assem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,6 @@ BOOL Assembler::AddMethod(Method *pMethod)
{
char sz[1024];
sz[0] = 0;
if(fIsInterface && (!IsMdStatic(pMethod->m_Attr))) strcat_s(sz,1024," non-static declared in interface");
if(fIsImport) strcat_s(sz,1024," imported");
if(IsMdAbstract(pMethod->m_Attr)) strcat_s(sz,1024," abstract");
if(IsMdPinvokeImpl(pMethod->m_Attr)) strcat_s(sz,1024," pinvoke");
Expand Down
18 changes: 0 additions & 18 deletions src/ilasm/assembler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -692,24 +692,6 @@ void Assembler::StartMethod(__in __nullterminated char* name, BinStr* sig, CorMe
{
flags = (CorMethodAttr)(flags | mdSpecialName);
if(IsTdInterface(m_pCurClass->m_Attr)) report->error("Instance constructor in interface\n");

}
if(!IsMdStatic(flags))
{
if(IsTdInterface(m_pCurClass->m_Attr))
{
if(!IsMdPublic(flags)) report->error("Non-public instance method in interface\n");
if((!(IsMdVirtual(flags) && IsMdAbstract(flags))))
{
if(OnErrGo) report->error("Non-virtual, non-abstract instance method in interface\n");
else
{
report->warn("Non-virtual, non-abstract instance method in interface, set to such\n");
flags = (CorMethodAttr)(flags |mdVirtual | mdAbstract);
}
}

}
}
m_pCurMethod = new Method(this, m_pCurClass, name, sig, flags);
}
Expand Down
31 changes: 7 additions & 24 deletions src/vm/classcompat.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2620,26 +2620,6 @@ VOID MethodTableBuilder::EnumerateClassMethods()
}
}

// Some interface checks.
if (IsInterface())
{
if (IsMdVirtual(dwMemberAttrs))
{
if (!IsMdAbstract(dwMemberAttrs))
{
BuildMethodTableThrowException(BFA_VIRTUAL_NONAB_INT_METHOD);
}
}
else
{
// Instance field/method
if (!IsMdStatic(dwMemberAttrs))
{
BuildMethodTableThrowException(BFA_NONVIRT_INST_INT_METHOD);
}
}
}

// No synchronized methods in ValueTypes
if(fIsClassValueType && IsMiSynchronized(dwImplFlags))
{
Expand Down Expand Up @@ -2805,17 +2785,20 @@ VOID MethodTableBuilder::EnumerateClassMethods()
// If the interface is a standard managed interface then allocate space for an FCall method desc.
Classification = mcFCall;
}
else
else if (IsMdAbstract(dwMemberAttrs))
{
// If COM interop is supported then all other interface MDs may be
// accessed via COM interop <TODO> mcComInterop MDs are BIG -
// this is very often a waste of space </TODO>
// @DIM_TODO - What if default interface method is called through COM interop?
Classification = mcComInterop;
}
#else // !FEATURE_COMINTEROP
// This codepath is used by remoting
Classification = mcIL;
else
#endif // !FEATURE_COMINTEROP
{
// This codepath is used by remoting and default interface methods
Classification = mcIL;
}
}
else
{
Expand Down
15 changes: 12 additions & 3 deletions src/vm/jitinterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5708,7 +5708,7 @@ void CEEInfo::getCallInfo(
pResult->classFlags = getClassAttribsInternal(pResolvedToken->hClass);

pResult->methodFlags = getMethodAttribsInternal(pResult->hMethod);
getMethodSigInternal(pResult->hMethod, &pResult->sig, (pResult->hMethod == pResolvedToken->hMethod) ? pResolvedToken->hClass : NULL);
getMethodSigInternal(pResult->hMethod, &pResult->sig, (pResult->hMethod == pResolvedToken->hMethod) ? pResolvedToken->hClass : NULL, /* isCallSite = */ TRUE);

if (flags & CORINFO_CALLINFO_VERIFICATION)
{
Expand Down Expand Up @@ -8471,7 +8471,8 @@ void
CEEInfo::getMethodSigInternal(
CORINFO_METHOD_HANDLE ftnHnd,
CORINFO_SIG_INFO * sigRet,
CORINFO_CLASS_HANDLE owner)
CORINFO_CLASS_HANDLE owner,
BOOL isCallSite)
{
STANDARD_VM_CONTRACT;

Expand All @@ -8497,7 +8498,15 @@ CEEInfo::getMethodSigInternal(
// Shared generic methods and shared methods on generic structs take an extra argument representing their instantiation
if (ftn->RequiresInstArg())
{
sigRet->callConv = (CorInfoCallConv) (sigRet->callConv | CORINFO_CALLCONV_PARAMTYPE);
//
// If we are making an interface call that is a default interface method, we need to lie to the JIT.
// The reason being that we already made sure target is always directly callable (through instantiation stubs),
// JIT should not generate shared generics aware call code and insert the secret argument again at the callsite.
// Otherwise we would end up with two secret generic dictionary arguments (since the stub also provides one).
//
BOOL isDefaultInterfaceMethodCallSite = isCallSite && ftn->IsDefaultInterfaceMethod();
if (!isDefaultInterfaceMethodCallSite)
sigRet->callConv = (CorInfoCallConv) (sigRet->callConv | CORINFO_CALLCONV_PARAMTYPE);
}

// We want the calling convention bit to be consistant with the method attribute bit
Expand Down
3 changes: 2 additions & 1 deletion src/vm/jitinterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,8 @@ class CEEInfo : public ICorJitInfo
void getMethodSigInternal (
CORINFO_METHOD_HANDLE ftnHnd,
CORINFO_SIG_INFO* sigInfo,
CORINFO_CLASS_HANDLE owner = NULL
CORINFO_CLASS_HANDLE owner = NULL,
BOOL isCallSite = FALSE
);

void getEHinfo(
Expand Down
18 changes: 13 additions & 5 deletions src/vm/method.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1763,15 +1763,19 @@ BOOL MethodDesc::IsSharedByGenericMethodInstantiations()
// Does this method require an extra MethodTable argument for instantiation information?
// This is the case for
// * per-inst static methods in shared-code instantiated generic classes (e.g. static void MyClass<string>::m())
// - there is no this pointer providing generic dictionary info
// * shared-code instance methods in instantiated generic structs (e.g. void MyValueType<string>::m())
// - unboxed 'this' pointer in value-type instance methods don't have MethodTable pointer by definition
// * default interface method called via interface dispatch (e. g. IFoo<string>.Foo calling into IFoo<object>::Foo())
// - this pointer is ambiguous as it can implement more than one IFoo<T>
BOOL MethodDesc::RequiresInstMethodTableArg()
{
LIMITED_METHOD_DAC_CONTRACT;

return
IsSharedByGenericInstantiations() &&
!HasMethodInstantiation() &&
(IsStatic() || GetMethodTable()->IsValueType());
(IsStatic() || GetMethodTable()->IsValueType() || IsDefaultInterfaceMethod());
}

//*******************************************************************************
Expand All @@ -1793,7 +1797,7 @@ BOOL MethodDesc::RequiresInstArg()
LIMITED_METHOD_DAC_CONTRACT;

BOOL fRet = IsSharedByGenericInstantiations() &&
(HasMethodInstantiation() || IsStatic() || GetMethodTable()->IsValueType());
(HasMethodInstantiation() || IsStatic() || GetMethodTable()->IsValueType() || IsDefaultInterfaceMethod());

_ASSERT(fRet == (RequiresInstMethodTableArg() || RequiresInstMethodDescArg()));
return fRet;
Expand Down Expand Up @@ -1869,7 +1873,8 @@ BOOL MethodDesc::AcquiresInstMethodTableFromThis() {
IsSharedByGenericInstantiations() &&
!HasMethodInstantiation() &&
!IsStatic() &&
!GetMethodTable()->IsValueType();
!GetMethodTable()->IsValueType() &&
!IsDefaultInterfaceMethod();
Copy link
Member

Choose a reason for hiding this comment

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

IsDefaultInterfaceMethod [](start = 9, length = 24)

Could we rename this predicate to be IsInterfaceMethodWithImplementation()? We may support non-virtual interface methods, and calling them "default" interface methods would be confusing.

Copy link
Author

Choose a reason for hiding this comment

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

I'm a bit unsure about this one. Default interface method feature do have wider implications (such as non-virtual instance methods, etc). But the specific default interface method scenario in the context of runtime should be referring to non-abstract virtual instance method. You can also argue that IsInterfaceMethodWithImplementation can also refer to instance methods in interface, which is not what we want here.
I'll add a bit of comment in the method header to explain / clarify that it is non-abstract virtual instance method. I can change it if you feel strongly, though.

}

//*******************************************************************************
Expand Down Expand Up @@ -2734,7 +2739,10 @@ BOOL MethodDesc::RequiresStableEntryPoint(BOOL fEstimateForChunk /*=FALSE*/)
return TRUE;

// TODO: Can we avoid early allocation of precodes for interfaces and cominterop?
if ((IsInterface() && !IsStatic()) || IsComPlusCall())
// Only abstract virtual interface method need precode
// @DIM_TODO - We need to decide what is the right approach for precode for default
// interface methods
if ((IsInterface() && IsAbstract() && IsVirtual() && !IsStatic()) || IsComPlusCall())
return TRUE;
}

Expand Down Expand Up @@ -2830,7 +2838,7 @@ BOOL MethodDesc::MayHaveNativeCode()

_ASSERTE(IsIL());

if ((IsInterface() && !IsStatic()) || IsWrapperStub() || ContainsGenericVariables() || IsAbstract())
if (IsWrapperStub() || ContainsGenericVariables() || IsAbstract())
{
return FALSE;
}
Expand Down
8 changes: 8 additions & 0 deletions src/vm/method.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1087,6 +1087,14 @@ class MethodDesc
&& GetSlot() < pMT->GetNumVirtuals();
}

// Is this a default interface method (virtual non-abstract instance method)
inline BOOL IsDefaultInterfaceMethod()
{
LIMITED_METHOD_CONTRACT;

return (GetMethodTable()->IsInterface() && !IsStatic() && IsVirtual() && !IsAbstract());
}

inline BOOL HasNonVtableSlot();

void SetHasNonVtableSlot()
Expand Down
Loading