CoreCLR support for default interface implementation of static virtual methods

[trylek]

This change introduces a "static virtual" version of the diamondshape test and makes
runtime changes to make it functional. As default interface implementations can cause
ambiguities in the presence of diamond pattern (when a class implements two interfaces
and each of them contains a default interface implementation of a static virtual method
defined in an interface they both inherit from), the gist of the runtime / JIT change deals
with this ambiguity that wasn't seen previously in static virtual methods without default
interface implementations.

When the static virtual method resolution in TryResolveConstraintMethodApprox called
from getCallInfo fail due to ambiguity, we take further action based on whether the
SVM call involves shared generics. In the presence of shared generics we cannot always
determine the SVM call target at compile time so we use the ConstrainedMethodEntrySlot
dictionary entry to represent the runtime resolution. In the absence of shared generics,
we instruct runtime to throw the ambiguous resolution exception by means of disallowing
access to the target method and calling out to a throwing JIT helper.

I have expanded the original diamondshape test suite of positive and negative tests
for non-generic and generic variants to also include interfaces on struct and execution
of the SVM via ldftn and calli as well as the shared generic test requiring runtime lookup.

Thanks

Tomas

P.S. At this moment this change only targets CoreCLR. Mono and NativeAOT will need
additional work. Adding specific Crossgen2 support would be a possible future perf optimization.

[davidwrighton]

I've identified some more work...

[davidwrighton]

Why do these functions have the final flag set?

[trylek]

Unhandled exception. System.TypeLoadException: Method implementation on an interface 'IFoo2' from assembly 'svm_diamondshape, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null' must be a final method.

Am I right to understand this is an artifact of the pre-existing limitation that there can always be just one "actual" override? Should I strive to remove this limitation as it seems applicable no more?

[davidwrighton]

This seems odd. These are static methods, and I don't believe final makes sense on static methods. I don't believe we need the static flag on implementation on the class either.

[AlekseyTs]

These are static methods, and I don't believe final makes sense on static methods.

I agree with this.

I don't believe we need the static flag on implementation on the class either.

I am not sure I agree with this. The implemented method is static, so I think the implementing method should be marked static too. BTW, we do mark implementations in classes like that. Is there a reason. Here is what the spec says

II.23.1.10 Flags for methods [MethodAttributes]
Static 0x0010 Defined on type, else per instance  

[AlekseyTs]

BTW, I think final should be used for re-abstraction. Such instance methods are marked "abstract virtual final" and static methods will be "static abstract virtual final". Do we have any examples for re-abstraction for static methods?

[trylek]

@AlekseyTs - am I right to understand that by re-abstraction you mean something like a "newslot" in the sense of introducing a place in the type hierarchy where you'd suppress the SVM implementations defined "at a lower level" and require new ones to be supplied higher up the type hierarchy? If my interpretation is correct, I don't recall any targeted testing for this in our original SVM implementation but it shouldn't be rocket science to expand on the SVM diamond pattern test to include this case. I apologize if I misunderstood you, please clarify in such case.

[AlekseyTs]

I'll respond with a code example because we are using many "previously undefined terms":

interface I1
{
    static abstract void M();
}

interface I2 : I1
{
    static void I1.M(){}
}

interface I3 : I2
{
    static abstract void I1.M(); // re-abstraction
}

class C2 : I2 {} // All is good

class C3: I3 {} // error CS0535: 'C3' does not implement interface member 'I1.M()'

It works the same way for instance methods today in the compiler. And it is supposed to work this way for instance methods in the runtime too.

[trylek]

Thanks Aleksey, I believe your code example matches what I was trying to describe. I assume that the "diamond pattern" here may mean that in one edge of the type hierarchy graph the SVM has been "re-abstracted" while in another "the original SVM is implemented" thus forming another type of resolution ambiguity. We should certainly include some of these corner cases in unit testing for the feature.

[AlekseyTs]

I believe your code example matches what I was trying to describe.

Great! It still looks like we need to get to a firm agreement around flags used in metadata for method declarations in each scenario.

[trylek]

Thanks @AlekseyTs for your comment. Do you plan to provide additional PR feedback on this change or are you fine with me merging it in? We can follow up on the ECMA 335 augment PR by clarifying the exact metadata flags used to represent SVMs in the various scenarios.

[ghost]

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

[trylek]

Hello @davidwrighton and @AndyAyersMS! I have managed to put together a batch of changes that make both the positive and negative diamond shape tests pass. I would very much welcome an initial review as much of the work is in JIT where I don't yet consider myself an expert. For now I plan that as the next step I'll work on the additional test scenarios per David's PR feedback (structs, ldftn).

Thanks a lot

Tomas

[trylek]

C:\git\runtime>C:\git\runtime\artifacts\tests\coreclr\windows.x64.Debug\Loader\classloader\StaticVirtualMethods\DiamondShape\svm_diamondshape_d\svm_diamondshape_d.cmd
BEGIN EXECUTION
 "C:\git\runtime\artifacts\tests\coreclr\windows.x64.Debug\Tests\Core_Root\corerun.exe"   svm_diamondshape_d.dll
Calling IFoo.Foo on Foo - expecting exception.
Exception caught: System.Runtime.AmbiguousImplementationException: Ambiguous implementation found.
   at Program.Negative() in C:\git\runtime\src\tests\Loader\classloader\StaticVirtualMethods\DiamondShape\svm_diamondshape.il:line 566
Calling I1.Func on I47Class - expecting exception
Exception caught: System.Runtime.AmbiguousImplementationException: Ambiguous implementation found.
   at Program.Negative() in C:\git\runtime\src\tests\Loader\classloader\StaticVirtualMethods\DiamondShape\svm_diamondshape.il:line 600
Calling GI1<T>.Func on GI23Class<S> - expecting exception
Exception caught: System.Runtime.AmbiguousImplementationException: Ambiguous implementation found.
   at Program.Negative() in C:\git\runtime\src\tests\Loader\classloader\StaticVirtualMethods\DiamondShape\svm_diamondshape.il:line 635
Calling I1.Func on I4Class - expecting I4.Func
At I4.Func
PASS
Calling I1.Func on I8Class - expecting I8.Func
At I8.Func
PASS
Calling GI1.Func on GI4Class<object> - expecting GI4.Func<S>
System.Object, System.String, GI4
PASS
PASS
PASS
Expected: 100
Actual: 100
END EXECUTION - PASSED
PASSED

[AndyAyersMS]

I guess I would rather need a way to either make JIT emit a throw instead of the call into the caller

Not sure this is the best suggestion, but something similar already exists which keys off the accessAllowed field on CORINFO_CALL_INFO. You can set this to CORINFO_ACCESS_ILLEGAL and then specify via callsiteCalloutHelper which helper the JIT should call before making the call in question. If necessary you can add a new helper that throws the proper exception.

If you then teach the jit that this new helper will always throw the JIT will (eventually) remove the rest of what follows (that is, the actual call). It may be more challenging to try and suppress emitting the actual call as you'd need to mimic the effect of that call properly.

[AndyAyersMS]

Also, jit changes look reasonable so far (modulo resolution of the invalid case).

[trylek]

Thanks Andy and JanK for your initial feedback. I have addressed the three "organizational" suggestions by JanK. For Andy's suggestion to somehow base the SVM lookup on accessAllowed field, I'd love to hear from @davidwrighton before I churn the code further as I suspect that this refactoring would amount to completely rewriting the JIT changes.

[trylek]

For the callsiteCalloutHelper, I also don't see how we can propagate the typesystem objects (MethodTable, MethodDesc) to the error throw helpers to let us include them in the error message. This is not implemented today in the initial version of the JIT helper but I plan to add it as part of cleanup when removing the WIP tag and putting up a final version of the change.

[jkotas]

For the callsiteCalloutHelper, I also don't see how we can propagate the typesystem objects (MethodTable, MethodDesc) to the error throw helpers to let us include them in the error message.

It is what CORINFO_HELPER_DESC::args are meant for.

[trylek]

I believe that by now I have addressed most of PR feedback. I have added JIT changes to cater for the negative tests per David's suggestion, I'm now trying to solicit feedback towards Andy's suggestion regarding simplification of some of these by leveraging accessAllowed / callsiteCalloutHelper. In accordance with David's suggestion I expanded the testing matrix by adding struct and ldftn variants to each of the negative and positive cases. I'm not yet clear on expanding the FCThrow statements in the new JIT helper to include the method and type in question as I don't yet fully understand where's the kAmbiguousImplementationException coming from, whether it expects particular arguments and how should I reflect that in the FCThrow statements.

[SamMonoRT]

cc @BrzVlad

[trylek]

I believe I finally have a working implementation that passes all the negative and positive tests. I have also speculatively updated the C# test source code albeit it's not yet really used. I have removed the WIP tag and I believe the change is ready for a final round of reviews. Thanks everyone for your insightful comments and suggestions that turned out to be invaluable in implementing this change!

[trylek]

/cc @BrzVlad

[trylek]

/azp run runtime-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[trylek]

OK, I now believe the change to be fully functional - in my latest round of testing I only hit the two tailcall crashes on Windows arm in outerloop (#70042) and Mono crashes in the new SVM diamond shape test. I have created the issue #70040 to track the Mono implementation and temporarily blocked the test out in issues.targets. As next step we need to follow up regarding the Mono and NativeAOT implementations.

[trylek]

I have removed the Mono-specific issues.targets entry as it seems to be working now, I'll follow up based on test results.

[AndyAyersMS]

Happy to see the existing bail out logic for the jit worked out. I approve the (now empty) set of jit changes.

[jakobbotsch]

Sorry, just noticed this during a conflict resolution -- but this should get a matching change in CorInfoHelpFunc.cs, and I think it should also get a JIT-EE GUID update.

[jakobbotsch]

If you plan to submit the JIT-EE GUID update, then can you also please take care of #69900 at the same time?

[jakobbotsch]

In fact, it would be nice to name this CORINFO_HELP_THROW_AMBIGUOUS_RESOLUTION and move it up together with the other CORINFO_HELP_THROW_* helpers at the same time.