Merge dev/defaultintf to master by MichalStrehovsky · Pull Request #15370 · dotnet/coreclr

MichalStrehovsky · 2017-12-05T12:14:50Z

Marking as WIP because I would like to:

Run all the tests on this
Review the changes coming in with people who know about the affected parts of the codebase
See if we need to put more code under FEATURE_DEFAULT_INTERFACES
Figure out how we want to merge this (are we okay with the individual commits or do we want to do some rebasing?)

* 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

* Support protected methods * add a test for diamond shape scenario and tentatively throw exception when that happens

* Support non-virtual calls on interface private members correctly * Support protected methods * Properly handle precode * Throw (tentative) exception when seeing conflict overrides and add a test case (This updates CoreCLR dev/defaultintf the same with the build we are showing at //build)

Updating the PreReleaseLabel name so we get official Default Interface's builds.

This will fix errors like "E:\A\_work\0\a\MicroBuild\Plugins\MicroBuild.Plugins.Signing.1.0.321\build\MicroBuild.Plugins.Signing.targets(0,0): error : File E:\A\_work\0\b\pipelineRepository\bin\obj\SymbolsCatalog\9\signatures.cat is not in one of the expected output locations and cannot be signed. [E:\A\_work\0\b\pipelineRepository\build.proj]" being tracked by https://github.com/dotnet/core-eng/issues/1173

This value was changed back after a rebase to master was done

* Update dependencies and use live/packaged ilasm * Fix ilasm excessive stack usage when deleting lots of generic parameters

…n CoreCLR (dotnet#13172)

* first cut of multiple candidates * new positive case for diamond shape * Add proper missing implementation detection and positive diamond shape scenario support. Untested * fix bug and get test to pass * Add more diamond shape pos/neg tests * Fix bad I8 * Add GVM tests to diamond shape * GVM working * Change test case to better match diamondshape scenario * Fix MethodImpl::Iterator::GetMethodDesc and optimize no-method impl scenario * Update methodimpl test to include multiple slots and fix a bug * Temporarily disable incremental build in this branch * Update methodimpl to include multiple methodipml overriding scenario. This is triggering some bugs and need to investigate * Fix a buffer overrrun bug with interface methodimpl * Update after self-review * Add proper methodImpl validation for methods * Address feedback. Refactoring pending

Update to latest changes from master branch

…ster Actually merge to completely up to date with master

Merge master

Add PB_SignType as explicit variable

Merge master

Merge master to dev/defaultintf

MichalStrehovsky · 2017-12-05T16:44:25Z

@dotnet-bot test Windows_NT x64 corefx_baseline
@dotnet-bot test Ubuntu x64 corefx_baseline

MichalStrehovsky · 2017-12-05T16:45:07Z

Cc @sergiy-k

jkotas · 2017-12-05T19:09:48Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

MichalStrehovsky · 2017-12-06T12:21:43Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline

MichalStrehovsky · 2017-12-06T13:06:12Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline

We ran out of disk space. Cc @dotnet/dnceng

jcagme · 2017-12-06T17:10:43Z

/cc: @mmitche

mmitche · 2017-12-06T17:27:08Z

@MattGal This was that machine I was referring to. We should check to see whether we can figure out what is taking up so much space, and whether the machine has been around a long time.

MichalStrehovsky · 2017-12-06T17:50:20Z

OK, the Windows_NT x64 Checked Build and Test (Jit - CoreFx) failure is also present in the baseline (see #15391). The tests can be considered green.

fadimounir · 2017-12-12T01:04:51Z


+# If set, indicates that this is not an officially supported release
+# Keep in sync with IsPrerelease in dir.props
+set(PRERELEASE 1)


It seems like this is hard coded to 1. Shouldn't this be controlled by an environment variable or something?

We want this to be always enabled in development branches and always disabled in release branches. Unfortunately, we couldn't find any other way. Environment variables wouldn't satisfy that requirement.

davidwrighton

In general, this looks ok, although I have a few comments regarding making the disabling of the the feature more comprehensive. Also, as the change is large and potentially impactful, its difficult to review for correctness, and I'm assuming we're running as much of a test suite as we can manage.

davidwrighton · 2017-12-12T02:25:21Z

+    inline BOOL IsDefaultInterfaceMethod()
+    {
+        LIMITED_METHOD_CONTRACT;
+


if the #if for default interfaces isn't enabled, this should just return false.

davidwrighton · 2017-12-12T02:27:01Z

+    MethodDesc **ppDefaultMethod
+)
+{
+    CONTRACT(BOOL) {


If the default interface #if isn't enabled, this function should return false, and fail to find anything.

davidwrighton · 2017-12-12T02:27:48Z

-        MethodTable::InterfaceMapIterator it = pCanonMT->IterateInterfaceMap();
-        DWORD cPotentialMatchingInterfaces = 0;
-        while (it.Next())
+// chance of this happening by checking for all interfaces which might possibly


What happened to indenting here?

davidwrighton · 2017-12-12T02:28:23Z

-                }
-            }
+        else if(fIsClassInterface)		
+        {		


This code has a lot of trailing spaces, please fix.

fadimounir · 2017-12-12T22:27:35Z


+// Returns true if there is at least one default implementation for this interface method
+// We don't care about conflicts at this stage in order to avoid impact type load performance
+BOOL MethodTableBuilder::HasDefaultInterfaceImplementation(MethodDesc *pDeclMD)


HasDefaultInterfaceImplementation [](start = 25, length = 33)

This should also return false if the default interface #if is not enabled

fadimounir

🕐

MichalStrehovsky · 2017-12-13T13:14:40Z

@jkotas Do you have an opinion on whether we should just squash and merge or make a merge commit, or try to rebase something?

(I'm not too keen on the rebasing because there was a ton of merge conflicts in the auto-update dependencies files that I would rather not have to resolve again.)

jkotas · 2017-12-13T13:17:33Z

I would make a merge commit.

fadimounir

…intf-merge Merge dev/defaultintf to master Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

BruceForstall · 2017-12-20T20:11:10Z

@MichalStrehovsky It looks like this change has broken all release builds with these test failures:

Loader_classloader._DefaultInterfaceMethods_sharedgenerics_sharedgenerics_sharedgenerics_._DefaultInterfaceMethods_sharedgenerics_sharedgenerics_sharedgenerics_cmd
Loader_classloader._DefaultInterfaceMethods_diamondshape_diamondshape_diamondshape_._DefaultInterfaceMethods_diamondshape_diamondshape_diamondshape_cmd

e.g.:
https://ci.dot.net/job/dotnet_coreclr/job/master/job/release_windows_nt/
https://ci.dot.net/job/dotnet_coreclr/job/master/job/x86_release_windows_nt/

MichalStrehovsky · 2017-12-20T20:15:33Z

@BruceForstall Sigh, thanks for bringing it to my attention! Those are new tests that came with the feature, so not a huge concern - it means there will be more bugs to deal with, I guess. I'll file an issue and block them on it.

…intf-merge Merge dev/defaultintf to master Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

yizhang82 and others added 30 commits July 19, 2017 11:16

More update for default interface methods (dotnet#10818)

9b8471b

* Support protected methods * add a test for diamond shape scenario and tentatively throw exception when that happens

Add RuntimeFeature detection for default interface method (dotnet#11940)

2acd613

Enable official builds from dev/defaultintf

688565c

Updating the PreReleaseLabel name so we get official Default Interface's builds.

Fix build break after rebase

cdc5a35

Update to dev-di after rebase

e1fbebf

This value was changed back after a rebase to master was done

Use Live built ILAsm for building tests (dotnet#13148)

65d13c3

* Update dependencies and use live/packaged ilasm * Fix ilasm excessive stack usage when deleting lots of generic parameters

Remove unnecessary SkipVerification attribute that is not supported i…

d019aca

…n CoreCLR (dotnet#13172)

Fix linux build break (dotnet#13441)

503466d

Switch to https. (dotnet#13463)

96fa985

Merge branch 'master' into update_from_master

d68f091

Update dependencies

05bf4a8

Merge pull request dotnet#13987 from davidwrighton/update_from_master

a6c824e

Update to latest changes from master branch

Merge more changes from master

fa337ac

Merge branch 'master' into merge_more_from_master

0f44d11

Update auto-dependencies

c40dcf0

Merge pull request dotnet#14000 from davidwrighton/merge_more_from_ma…

e653765

…ster Actually merge to completely up to date with master

Use an unsigned variable to avoid comparison problems (dotnet#14001)

be5e1a1

Merge branch 'master' into merge_master

09d436a

Merge pull request dotnet#14258 from davidwrighton/merge_master

aace6b1

Merge master

Add PB_SignType as explicit variable

f79bf79

Merge pull request dotnet#14266 from MattGal/Fix_catalog_signing

9399ced

Add PB_SignType as explicit variable

Merge branch 'master' into merge_master

93e64dd

Merge pull request dotnet#14269 from davidwrighton/merge_master

24f5c55

Merge master

Merge branch 'master' into master-merge

fc72257

Merge pull request dotnet#14832 from MichalStrehovsky/master-merge

698db4c

Merge master to dev/defaultintf

Merge branch 'master' into master-merge

ed11dd2

fadimounir reviewed Dec 12, 2017

View reviewed changes

davidwrighton suggested changes Dec 12, 2017

View reviewed changes

fadimounir reviewed Dec 12, 2017

View reviewed changes

fadimounir suggested changes Dec 12, 2017

View reviewed changes

Fix formatting and put more things under #if

7333bb1

MichalStrehovsky force-pushed the defaultintf-merge branch from 01b86cc to 7333bb1 Compare December 13, 2017 13:19

Add a note the linker bug has been fixed

adc5be0

fadimounir approved these changes Dec 14, 2017

View reviewed changes

davidwrighton approved these changes Dec 14, 2017

View reviewed changes

MichalStrehovsky changed the title ~~[WIP] Merge dev/defaultintf to master~~ Merge dev/defaultintf to master Dec 15, 2017

MichalStrehovsky merged commit 1a4a2d5 into dotnet:master Dec 15, 2017

MichalStrehovsky deleted the defaultintf-merge branch December 15, 2017 10:52

dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Dec 15, 2017

Merge pull request dotnet/coreclr#15370 from MichalStrehovsky/default…

6b146de

…intf-merge Merge dev/defaultintf to master Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

jkotas pushed a commit to dotnet/corert that referenced this pull request Dec 15, 2017

Merge pull request dotnet/coreclr#15370 from MichalStrehovsky/default…

db571b1

…intf-merge Merge dev/defaultintf to master Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Jan 13, 2018

Merge pull request dotnet/coreclr#15370 from MichalStrehovsky/default…

a62fc06

…intf-merge Merge dev/defaultintf to master Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Jan 13, 2018

Merge pull request dotnet/coreclr#15370 from MichalStrehovsky/default…

77ad812

…intf-merge Merge dev/defaultintf to master Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

safern pushed a commit to dotnet/corefx that referenced this pull request Jan 16, 2018

Merge pull request dotnet/coreclr#15370 from MichalStrehovsky/default…

04ff565

…intf-merge Merge dev/defaultintf to master Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

safern pushed a commit to dotnet/corefx that referenced this pull request Jan 16, 2018

Merge pull request dotnet/coreclr#15370 from MichalStrehovsky/default…

e3568e0

…intf-merge Merge dev/defaultintf to master Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>

Conversation

MichalStrehovsky commented Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichalStrehovsky commented Dec 5, 2017

Uh oh!

MichalStrehovsky commented Dec 5, 2017

Uh oh!

jkotas commented Dec 5, 2017

Uh oh!

MichalStrehovsky commented Dec 6, 2017

Uh oh!

MichalStrehovsky commented Dec 6, 2017

Uh oh!

jcagme commented Dec 6, 2017

Uh oh!

mmitche commented Dec 6, 2017

Uh oh!

MichalStrehovsky commented Dec 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwrighton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir left a comment

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky commented Dec 13, 2017

Uh oh!

jkotas commented Dec 13, 2017

Uh oh!

fadimounir left a comment

Choose a reason for hiding this comment

Uh oh!

BruceForstall commented Dec 20, 2017

Uh oh!

MichalStrehovsky commented Dec 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

MichalStrehovsky commented Dec 5, 2017 •

edited

Loading