Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default Interface Method Prototype #10505

Merged
merged 23 commits into from Apr 6, 2017
Merged

Conversation

@yizhang82
Copy link

yizhang82 commented Mar 27, 2017

CoreCLR Issue - https://github.com/dotnet/coreclr/issues/10504
C# Language Spec (WIP) - https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md

As suggested by @jkotas, I'm moving my work to dev/defaultintf branch in the main CoreCLR repo. I'm going to keep submitting periodically submit new PRs to dev/defaultintf branch going forward - this is going to make collaboration between various runtime and language team a bit easier.

What this PR has:

  • Allow non-abstract non-public interface methods in ILDAsm and change our IL.targets to use ILDAsm we built for the time being, until we have a good set of toolsets that has the new ILDAsm.
  • Allow type loader to ignore not implemented interfaces (we may need to add more validation later)
  • Hooked up VSD interface dispatch to look for default interface method in the absence of proper implementation from the class itself.
  • A naive implementation of default interface method look up based on InterfaceMap, from derived class to base class - just enough for prototyping on both CLR and C# sides. Once we finalize the design, it'll be replaced by the real thing.
  • Support for shared generics by properly instantiate MethodDesc which inserts instantiating stubs to pass the secret MethodTable argument for the particular generic instantiation. One particular interesting aspect of this is that we need to lie to the JIT about those interface methods at call site requires secret argument so that it emits the correct code at callsite.
  • Support for explicit override by looking up methodimpls (implicit method override is not supported)
  • Support for interface variance
  • Temporary workaround for interface precode. Today we always assume interface has precode
  • Added tests
    • Note that the C# code is simply there to make writing IL a bit easier - the IL are disassembled from C# generated code and then modified by hand. Once things got more stable we can delete them, or see if it makes sense to switch over to a new C# compiler for positive cases that we care about.

TODO - See https://github.com/dotnet/coreclr/issues/10504

@jkotas @davidwrighton PTAL

//cc @fadimounir

Yi Zhang added 20 commits Mar 11, 2017
…thod. add a test case for overriding but it may not be what we want
…ithout precode. This is accdentally discovered by methodimpl test case when iterating methods on a default interface method that has already been JITted
@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Mar 27, 2017

…s are choking on CoreCLR ilasm for security related stuff
report->warn("Non-virtual, non-abstract instance method in interface, set to such\n");
flags = (CorMethodAttr)(flags |mdVirtual | mdAbstract);
report->warn("Non-virtual instance method in interface, set to such\n");
flags = (CorMethodAttr)(flags | mdVirtual);

This comment has been minimized.

Copy link
@davidwrighton

davidwrighton Apr 4, 2017

Member

I think that in ILASM we should permit non-virtuals. This would be to allow either the feature to be implemented in the runtime, or for tests that the feature is blocked by the typeloader to be put in place. In either case, we shouldn't mystically mark the method as virtual. (However, we should consider spewing a warning)

This comment has been minimized.

Copy link
@yizhang82

yizhang82 Apr 5, 2017

Author

Yep. I haven't got to supporting instance methods yet. I'm looking into it now and have removed these checks from ILAsm.

@@ -1869,7 +1873,8 @@ BOOL MethodDesc::AcquiresInstMethodTableFromThis() {
IsSharedByGenericInstantiations() &&
!HasMethodInstantiation() &&
!IsStatic() &&
!GetMethodTable()->IsValueType();
!GetMethodTable()->IsValueType() &&
!IsDefaultInterfaceMethod();

This comment has been minimized.

Copy link
@davidwrighton

davidwrighton Apr 4, 2017

Member

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.

This comment has been minimized.

Copy link
@yizhang82

yizhang82 Apr 6, 2017

Author

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.

BuildMethodTableThrowException(BFA_NONAB_NONCCTOR_METHOD_ON_INT);
}
}
}

This comment has been minimized.

Copy link
@davidwrighton

davidwrighton Apr 4, 2017

Member

Why are you removing the check against cctor's on interfaces? It seems to me that since we've not talked positively about adding static fields to interfaces, we should keep cctors disabled.

This comment has been minimized.

Copy link
@yizhang82

yizhang82 Apr 6, 2017

Author

If I read the code correctly, we already allow static cctors in interfaces (since we allow static methods already), and the check is for non-cctor that are virtual or special name. I can keep the special name check for now.

@@ -7667,6 +7704,23 @@ MethodTableBuilder::PlaceInterfaceMethods()
PlaceMethodFromParentEquivalentInterfaceIntoInterfaceSlot(itfSlotIt, pCurItfEntry, &rgInterfaceDispatchMapTypeIDs, dwCurInterface);
}
#endif

/*

This comment has been minimized.

Copy link
@davidwrighton

davidwrighton Apr 4, 2017

Member

Please don't check in commented out code without a comment as to what's going on here.

This comment has been minimized.

Copy link
@yizhang82

yizhang82 Apr 6, 2017

Author

I've removed the code for now. It might be useful going forward when we go for a new implementation but I can easily add it back.

MethodDesc *pMD = it.GetDeclMethodDesc();
BuildMethodTableThrowException(IDS_CLASSLOAD_NOTIMPLEMENTED, pMD->GetNameOnNonArrayClass());
}
// @DESIGN - What is the right level of check if the interface itself does not have default implementation

This comment has been minimized.

Copy link
@davidwrighton

davidwrighton Apr 4, 2017

Member

Please add a DIM_TODO to the comment so we can find these bits of commented out logic.

This comment has been minimized.

Copy link
@yizhang82

yizhang82 Apr 6, 2017

Author

Thanks. Fixed/Replaced with DIM_TODO

@davidwrighton

This comment has been minimized.

Copy link
Member

davidwrighton commented Apr 4, 2017

:shipit:

@yizhang82 yizhang82 merged commit d793d5e into dotnet:dev/defaultintf Apr 6, 2017
@yizhang82

This comment has been minimized.

Copy link
Author

yizhang82 commented Apr 6, 2017

Thanks @davidwrighton for review!

@yizhang82 yizhang82 deleted the yizhang82:defaultintf branch Apr 6, 2017
yizhang82 added a commit that referenced this pull request May 23, 2017
* allow non-zero RVA on abstract interface method in ilasm

* Revert "allow non-zero RVA on abstract interface method in ilasm"

This reverts commit eecb802.

* add a test case and allow virtual non-abstract method in ilasm

* allow non-abstract methods in the loader

* fixup dispatch map

* support for simple default interface method scenario

* fix a bug with incorrect usage of MethodIterator skpping the first method. add a test case for overriding but it may not be what we want

* add another simple test case for base class

* allow private/internal methods in ilasm and add a explict impl test

* update reference to mscorlib in il test

* add shared generics and variance case

* allow interface dispatch to return instantiating stubs with the right PARAM_TYPE calling conv

* simple factoring and add a valuetype test case

* add a test case for generic virtual methods

* a bit more refactoring by moving the CALLCONV_PARAMTYPE logic inside getMethodSigInternal

* support explicit methodimpl and remove implicit methodimpl test case

* update test cases to give more clear output

* Fix a bug where GetMethodDescForSlot chokes on interface MethodDesc without precode. This is accdentally discovered by methodimpl test case when iterating methods on a default interface method that has already been JITted

* cleanup code after review and add a bit more comments

* update comments

* only use custom ILAsm for default interface methods tests - some tests are choking on CoreCLR ilasm for security related stuff

* address comments and allow instance methods, and add a constraint value type call test scenario

* disable the failing protected method scenario
yizhang82 added a commit that referenced this pull request Jun 8, 2017
* allow non-zero RVA on abstract interface method in ilasm

* Revert "allow non-zero RVA on abstract interface method in ilasm"

This reverts commit eecb802.

* add a test case and allow virtual non-abstract method in ilasm

* allow non-abstract methods in the loader

* fixup dispatch map

* support for simple default interface method scenario

* fix a bug with incorrect usage of MethodIterator skpping the first method. add a test case for overriding but it may not be what we want

* add another simple test case for base class

* allow private/internal methods in ilasm and add a explict impl test

* update reference to mscorlib in il test

* add shared generics and variance case

* allow interface dispatch to return instantiating stubs with the right PARAM_TYPE calling conv

* simple factoring and add a valuetype test case

* add a test case for generic virtual methods

* a bit more refactoring by moving the CALLCONV_PARAMTYPE logic inside getMethodSigInternal

* support explicit methodimpl and remove implicit methodimpl test case

* update test cases to give more clear output

* Fix a bug where GetMethodDescForSlot chokes on interface MethodDesc without precode. This is accdentally discovered by methodimpl test case when iterating methods on a default interface method that has already been JITted

* cleanup code after review and add a bit more comments

* update comments

* only use custom ILAsm for default interface methods tests - some tests are choking on CoreCLR ilasm for security related stuff

* address comments and allow instance methods, and add a constraint value type call test scenario

* disable the failing protected method scenario
yizhang82 added a commit that referenced this pull request Jun 9, 2017
* allow non-zero RVA on abstract interface method in ilasm

* Revert "allow non-zero RVA on abstract interface method in ilasm"

This reverts commit eecb802.

* add a test case and allow virtual non-abstract method in ilasm

* allow non-abstract methods in the loader

* fixup dispatch map

* support for simple default interface method scenario

* fix a bug with incorrect usage of MethodIterator skpping the first method. add a test case for overriding but it may not be what we want

* add another simple test case for base class

* allow private/internal methods in ilasm and add a explict impl test

* update reference to mscorlib in il test

* add shared generics and variance case

* allow interface dispatch to return instantiating stubs with the right PARAM_TYPE calling conv

* simple factoring and add a valuetype test case

* add a test case for generic virtual methods

* a bit more refactoring by moving the CALLCONV_PARAMTYPE logic inside getMethodSigInternal

* support explicit methodimpl and remove implicit methodimpl test case

* update test cases to give more clear output

* Fix a bug where GetMethodDescForSlot chokes on interface MethodDesc without precode. This is accdentally discovered by methodimpl test case when iterating methods on a default interface method that has already been JITted

* cleanup code after review and add a bit more comments

* update comments

* only use custom ILAsm for default interface methods tests - some tests are choking on CoreCLR ilasm for security related stuff

* address comments and allow instance methods, and add a constraint value type call test scenario

* disable the failing protected method scenario
yizhang82 added a commit that referenced this pull request Jun 26, 2017
* allow non-zero RVA on abstract interface method in ilasm

* Revert "allow non-zero RVA on abstract interface method in ilasm"

This reverts commit eecb802.

* add a test case and allow virtual non-abstract method in ilasm

* allow non-abstract methods in the loader

* fixup dispatch map

* support for simple default interface method scenario

* fix a bug with incorrect usage of MethodIterator skpping the first method. add a test case for overriding but it may not be what we want

* add another simple test case for base class

* allow private/internal methods in ilasm and add a explict impl test

* update reference to mscorlib in il test

* add shared generics and variance case

* allow interface dispatch to return instantiating stubs with the right PARAM_TYPE calling conv

* simple factoring and add a valuetype test case

* add a test case for generic virtual methods

* a bit more refactoring by moving the CALLCONV_PARAMTYPE logic inside getMethodSigInternal

* support explicit methodimpl and remove implicit methodimpl test case

* update test cases to give more clear output

* Fix a bug where GetMethodDescForSlot chokes on interface MethodDesc without precode. This is accdentally discovered by methodimpl test case when iterating methods on a default interface method that has already been JITted

* cleanup code after review and add a bit more comments

* update comments

* only use custom ILAsm for default interface methods tests - some tests are choking on CoreCLR ilasm for security related stuff

* address comments and allow instance methods, and add a constraint value type call test scenario

* disable the failing protected method scenario
yizhang82 added a commit that referenced this pull request Jun 28, 2017
* allow non-zero RVA on abstract interface method in ilasm

* Revert "allow non-zero RVA on abstract interface method in ilasm"

This reverts commit eecb802.

* add a test case and allow virtual non-abstract method in ilasm

* allow non-abstract methods in the loader

* fixup dispatch map

* support for simple default interface method scenario

* fix a bug with incorrect usage of MethodIterator skpping the first method. add a test case for overriding but it may not be what we want

* add another simple test case for base class

* allow private/internal methods in ilasm and add a explict impl test

* update reference to mscorlib in il test

* add shared generics and variance case

* allow interface dispatch to return instantiating stubs with the right PARAM_TYPE calling conv

* simple factoring and add a valuetype test case

* add a test case for generic virtual methods

* a bit more refactoring by moving the CALLCONV_PARAMTYPE logic inside getMethodSigInternal

* support explicit methodimpl and remove implicit methodimpl test case

* update test cases to give more clear output

* Fix a bug where GetMethodDescForSlot chokes on interface MethodDesc without precode. This is accdentally discovered by methodimpl test case when iterating methods on a default interface method that has already been JITted

* cleanup code after review and add a bit more comments

* update comments

* only use custom ILAsm for default interface methods tests - some tests are choking on CoreCLR ilasm for security related stuff

* address comments and allow instance methods, and add a constraint value type call test scenario

* disable the failing protected method scenario
yizhang82 added a commit to yizhang82/coreclr that referenced this pull request Jul 10, 2017
* allow non-zero RVA on abstract interface method in ilasm

* Revert "allow non-zero RVA on abstract interface method in ilasm"

This reverts commit eecb802.

* add a test case and allow virtual non-abstract method in ilasm

* allow non-abstract methods in the loader

* fixup dispatch map

* support for simple default interface method scenario

* fix a bug with incorrect usage of MethodIterator skpping the first method. add a test case for overriding but it may not be what we want

* add another simple test case for base class

* allow private/internal methods in ilasm and add a explict impl test

* update reference to mscorlib in il test

* add shared generics and variance case

* allow interface dispatch to return instantiating stubs with the right PARAM_TYPE calling conv

* simple factoring and add a valuetype test case

* add a test case for generic virtual methods

* a bit more refactoring by moving the CALLCONV_PARAMTYPE logic inside getMethodSigInternal

* support explicit methodimpl and remove implicit methodimpl test case

* update test cases to give more clear output

* Fix a bug where GetMethodDescForSlot chokes on interface MethodDesc without precode. This is accdentally discovered by methodimpl test case when iterating methods on a default interface method that has already been JITted

* cleanup code after review and add a bit more comments

* update comments

* only use custom ILAsm for default interface methods tests - some tests are choking on CoreCLR ilasm for security related stuff

* address comments and allow instance methods, and add a constraint value type call test scenario

* disable the failing protected method scenario
yizhang82 added a commit that referenced this pull request Jul 10, 2017
* allow non-zero RVA on abstract interface method in ilasm

* Revert "allow non-zero RVA on abstract interface method in ilasm"

This reverts commit eecb802.

* add a test case and allow virtual non-abstract method in ilasm

* allow non-abstract methods in the loader

* fixup dispatch map

* support for simple default interface method scenario

* fix a bug with incorrect usage of MethodIterator skpping the first method. add a test case for overriding but it may not be what we want

* add another simple test case for base class

* allow private/internal methods in ilasm and add a explict impl test

* update reference to mscorlib in il test

* add shared generics and variance case

* allow interface dispatch to return instantiating stubs with the right PARAM_TYPE calling conv

* simple factoring and add a valuetype test case

* add a test case for generic virtual methods

* a bit more refactoring by moving the CALLCONV_PARAMTYPE logic inside getMethodSigInternal

* support explicit methodimpl and remove implicit methodimpl test case

* update test cases to give more clear output

* Fix a bug where GetMethodDescForSlot chokes on interface MethodDesc without precode. This is accdentally discovered by methodimpl test case when iterating methods on a default interface method that has already been JITted

* cleanup code after review and add a bit more comments

* update comments

* only use custom ILAsm for default interface methods tests - some tests are choking on CoreCLR ilasm for security related stuff

* address comments and allow instance methods, and add a constraint value type call test scenario

* disable the failing protected method scenario
yizhang82 added a commit that referenced this pull request Jul 19, 2017
* allow non-zero RVA on abstract interface method in ilasm

* Revert "allow non-zero RVA on abstract interface method in ilasm"

This reverts commit eecb802.

* add a test case and allow virtual non-abstract method in ilasm

* allow non-abstract methods in the loader

* fixup dispatch map

* support for simple default interface method scenario

* fix a bug with incorrect usage of MethodIterator skpping the first method. add a test case for overriding but it may not be what we want

* add another simple test case for base class

* allow private/internal methods in ilasm and add a explict impl test

* update reference to mscorlib in il test

* add shared generics and variance case

* allow interface dispatch to return instantiating stubs with the right PARAM_TYPE calling conv

* simple factoring and add a valuetype test case

* add a test case for generic virtual methods

* a bit more refactoring by moving the CALLCONV_PARAMTYPE logic inside getMethodSigInternal

* support explicit methodimpl and remove implicit methodimpl test case

* update test cases to give more clear output

* Fix a bug where GetMethodDescForSlot chokes on interface MethodDesc without precode. This is accdentally discovered by methodimpl test case when iterating methods on a default interface method that has already been JITted

* cleanup code after review and add a bit more comments

* update comments

* only use custom ILAsm for default interface methods tests - some tests are choking on CoreCLR ilasm for security related stuff

* address comments and allow instance methods, and add a constraint value type call test scenario

* disable the failing protected method scenario
@karelz karelz modified the milestone: Future Aug 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.