Finalize override lookup algorithm #12753

yizhang82 · 2017-07-11T22:34:11Z

properly detect diamond shape positive case (where I4 overrides both I2/I3 which both overrides I1) by keep tracking of a current list of best candidates. I went for the simplest algorithm and didn't build any complex graph / DFS since the majority case the list of interfaces would be small, and interface dispatch cache would ensure majority of cases we don't need to redo the (slow) dispatch. If needed we can revisit this to make it a proper topological sort.
VerifyVirtualMethodsImplemented now properly validates default interface scenarios - it is happy if there is at least one implementation and early returns. It doesn't worry about conflicting overrides, for performance reasons.
add more test case for diamond shape scenarios, both positive and negative
NotSupportedException thrown in conflicting override scenario now has a proper error message
properly supports GVM when detecting method impl overrides
Revisited code that adds method impl for interfaces. added proper methodimpl validation and ensure methodimpl are virtual and final (and throw exception if it is not final)
Added test scenario with method that has multiple method impl. found and fixed a bug where the slot array is not big enough when building method impls for interfaces.
Disabled incremental build in this branch as I'm running into incremental linker bugs

This fixes https://github.com/dotnet/coreclr/issues/12149, https://github.com/dotnet/coreclr/issues/12147

@davidwrighton PTAL.

…e scenario support. Untested

…cenario

… This is triggering some bugs and need to investigate

yizhang82 · 2017-08-04T01:19:39Z

@fadimounir Can you please review this early next week?

fadimounir · 2017-08-10T23:24:06Z

CMakeLists.txt

@@ -344,6 +344,9 @@ if (WIN32)
  set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /DEBUG /PDBCOMPRESS")
  set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /STACK:1572864")

+  # Disable incremental link due to incremental linking CFG bug crashing crossgen


Disable incremental link due to incremental linking CFG bug crashing crossgen [](start = 2, length = 79)

Can you file a tracking bug, so that when the linker gets fixed and updated in LKG, we can restore this back?

Also, if you can add a small comment with a brief description of what the bug was, and the bug ID tracking the undoing of this change, that would be awesome :)

Sure. Logged https://github.com/dotnet/coreclr/issues/13344 and added comment

fadimounir · 2017-08-10T23:25:58Z

src/dlls/mscorrc/mscorrc.rc

@@ -1252,6 +1252,8 @@ BEGIN
    IDS_CLASSLOAD_MI_VIRTUALMISMATCH        "Method '%3' on type '%1' from assembly '%2' tried to implement a method declaration with a different virtual state."
    IDS_CLASSLOAD_MI_MUSTBEVIRTUAL          "Method '%3' on type '%1' from assembly '%2' must be virtual to implement a method on an interface or super type."
    IDS_CLASSLOAD_MI_BAD_SIG                "Type '%1' from assembly '%2' contains an invalid method implementation signature."
+    IDS_CLASSLOAD_MI_FINAL_IMPL             "Method implementation on an interface must be a final method.  Type: '%1'.  Assembly: '%2'."


Method implementation on an interface must be a final method. Type: '%1'. Assembly: '%2'." [](start = 45, length = 92)

Change the text to be:

"Method implementation on interface '%1' from assembly '%2' must be a final method."

To follow the pattern

sounds good. fixed.

fadimounir · 2017-08-10T23:35:19Z

src/vm/methodtable.cpp

+    TypeString::AppendType(strTargetClassName, pTargetClass);
+
+    COMPlusThrow(
+        kNotSupportedException,


kNotSupportedException [](start = 8, length = 22)

I think it would make more sense to make this a TypeLoadException, or do we hit this code path during an actual call instead of during type loading?
If it's type loading, then we should change this to TypeLoadException

This is in a interface dispatch. So not TypeLoadException :)

fadimounir · 2017-08-10T23:39:00Z

src/vm/methodtable.cpp

+                                        continue;
+
+                                    MethodTable *pDeclMT = pDeclMD->GetMethodTable();
+                                    BOOL bMatch = FALSE;


BOOL bMatch = FALSE; [](start = 36, length = 20)

unused variable

fadimounir · 2017-08-10T23:40:42Z

src/vm/methodtable.cpp

+                                        {
+                                            // This is a matching override. We'll instantiate pCurMD later
+                                            pCurMD = pMD;
+                                            break;


break [](start = 44, length = 5)

This will just break from the inner loop. I think you also want to break out of the outer loop

Actually the outter loop also compares each match against all current list of candidates. So this is correct. But I do agree the code is getting complicated and the else statement is too big. I'll refactor it

fadimounir · 2017-08-10T23:40:56Z

src/vm/methodtable.cpp

+                                    }
+                                    else if (pDeclMD == pInterfaceMD)
+                                    {
+                                        // Exact match override 
                                        pCurMD = pMD;
                                        break;


break [](start = 40, length = 5)

Same

fadimounir · 2017-08-10T23:43:24Z

src/vm/methodtable.cpp

+// Find the default interface implementation method for interface dispatch
+// It is either the interface method with default interface method implementation, 
+// or an most specific interface with an explicit methodimpl overriding the method
+BOOL MethodTable::FindDefaultInterfaceImplementation(


FindDefaultInterfaceImplementation [](start = 18, length = 34)

This method is becoming increasingly complicated, with many levels of nested loops. Could you please refactor?

Agreed. I'll break this up.

fadimounir · 2017-08-10T23:48:51Z

src/vm/methodtable.cpp

                        {
-                            if (!pBestCandidateMT->CanCastToInterface(pCurMT))
+                            // pCurMT is a more specific choice
+                            // If pCurMT is more specific than IFoo and IBar, only update first entry IFoo and null out IBar


IFoo and IBar [](start = 63, length = 13)

Please add a comment at the begining of the function with an example of the type hierarchy that you are referring to here, to make it more clear

fadimounir · 2017-08-10T23:50:21Z

src/vm/methodtable.cpp

+                        if (pCandidateMT == NULL)
+                            continue;
+
+                        if (pCandidateMT->HasSameTypeDefAs(pCurMT))


if (pCandidateMT->HasSameTypeDefAs(pCurMT)) [](start = 24, length = 43)

Is there a case where we need to check first if pCandidateMT == pCurMT?

HasSameTypeDefAs will return true even when 2 instantiations are different, if the generic typedef is the same.

In reply to: 132596188 [](ancestors = 132596188)

Yeah, changed to ==

fadimounir · 2017-08-10T23:52:48Z

src/vm/methodtable.cpp

-                            if (!pBestCandidateMT->CanCastToInterface(pCurMT))
+                            // pCurMT is a more specific choice
+                            // If pCurMT is more specific than IFoo and IBar, only update first entry IFoo and null out IBar
+                            if (!seenMoreSpecific)


seenMoreSpecific [](start = 33, length = 16)

Can there be an even more specific choice further in the array?

ex: if array[4] is more specific that array[2], in that case your code is clearing out array[4], and using array[2] as the 'more specific'

The more specific is always the current pCurMT in this case (not array[4] or array[2]). So we effectively NULL-out all less specific entries and assign the first one to pCurMT. There is no point keeping array[4] or array[2] at all - only pCurMT "survives".

fadimounir · 2017-08-10T23:55:23Z

🕐

fadimounir · 2017-08-14T18:15:43Z

There are some code changes specific to the generic cases with variance. Please add some tests to cover the variant case to make sure it works (ex: see my comment on 'pCandidateMT == pCurMT'). There might be some bugs with variance (or not :) )

yizhang82 · 2017-08-15T22:54:33Z

I'll address refactoring in another PR.

dnfclas added the cla-already-signed label Jul 11, 2017

yizhang82 force-pushed the dev/defaultintf branch from e7ee2eb to f38b091 Compare July 19, 2017 18:17

yizhang82 force-pushed the dev/defaultintf branch from dba1190 to d7ab567 Compare July 19, 2017 18:42

yizhang82 force-pushed the dev/defaultintf branch 2 times, most recently from 32e1682 to fb212e6 Compare August 1, 2017 17:24

Yi Zhang (CLR) added 16 commits August 2, 2017 14:42

first cut of multiple candidates

148b238

new positive case for diamond shape

ca74ba5

Add proper missing implementation detection and positive diamond shap…

1ad4ef1

…e scenario support. Untested

fix bug and get test to pass

bb302ca

Add more diamond shape pos/neg tests

2e7eb17

Fix bad I8

aa84878

Add GVM tests to diamond shape

6215131

GVM working

b7eff4a

Change test case to better match diamondshape scenario

e50ac81

Fix MethodImpl::Iterator::GetMethodDesc and optimize no-method impl s…

c470278

…cenario

Update methodimpl test to include multiple slots and fix a bug

e92cf0d

Temporarily disable incremental build in this branch

b70bf87

Update methodimpl to include multiple methodipml overriding scenario.…

11f34e3

… This is triggering some bugs and need to investigate

Fix a buffer overrrun bug with interface methodimpl

b4574a9

Update after self-review

0703aef

Add proper methodImpl validation for methods

86729c8

yizhang82 force-pushed the dev/defaultintf branch from fb212e6 to 86729c8 Compare August 2, 2017 21:43

fadimounir reviewed Aug 10, 2017

View reviewed changes

Address feedback. Refactoring pending

e771bd2

yizhang82 merged commit 3f0dd78 into dotnet:dev/defaultintf Aug 15, 2017

karelz modified the milestone: Future Aug 28, 2017

ghost mentioned this pull request Jan 31, 2020

Build failure: [dev/defaultintf] warning C4018: '<': signed/unsigned mismatch dotnet/runtime#8881

Closed

Finalize override lookup algorithm #12753

Finalize override lookup algorithm #12753

Uh oh!

Conversation

yizhang82 commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yizhang82 commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Disable incremental link due to incremental linking CFG bug crashing crossgen [](start = 2, length = 79)

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!

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!

yizhang82 Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fadimounir commented Aug 10, 2017

Uh oh!

fadimounir commented Aug 14, 2017

Uh oh!

yizhang82 commented Aug 15, 2017

Uh oh!

Uh oh!

yizhang82 commented Jul 11, 2017 •

edited

Loading

yizhang82 Aug 11, 2017 •

edited

Loading