Runtime support for static virtual interface methods #52173
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
trylek
added
the
NO-MERGE
trylek
force-pushed
the
NoMergeRIForCITesting
branch
3 times, most recently
from
4de82c1 to
1043316
Compare
- Missing interface class flag on interfaces in TypeHierarchy tests - Improper use of the class token when deriving, implementing interfaces or defining a MethodImpl - This caused incorrect behavior where ilasm will generate a typespec to a non-generic method instead of the correct typedef/typeref token - Common C# code should be public - Improved CheckForFailure code in type hierarchy tests
* Minimalistic JIT change to enable the new constrained. opcodes * Runtime changes up to the point of constraint resolution in getCallInfo
… constrained ldftn and call scenarios (dotnet#6) - R2R is not yet handled with this change
- structures derive from System.ValueType not System.Valuetype - Expected strings need to handle generics a little bit more
* Remove covariant tests * Fix type hierarchy tests to override the right method * Remove DebuggerBreak
- Fix incorrect details in interface variance test - Add loop over actually implemented interfaces to try and find possible matches in a way which matches the behavior of normal interface variance matching
* Partial fix for the negative virtual static methods 1) UnimplementedAbstractMethodOnConcreteClass - I have added logic to verify that all SVMs are implemented once a class is fully loaded. That required me to propagate a new flag around limiting the class load level for methods, otherwise we were hitting stack overflows in many of the tests. I have recycled a pre-existing HRESULT ID which is probably not what we want, I just didn't want to churn the codebase before hearing from you what is the right way to proceed here. 2) UnimplementedAbstractMethodOnAbstractClass - the one remaining failing case is 'ConstraintCheckShouldFail' - I guess that corresponds to what you alluded to in mentioning that we're not properly checking class constraints yet. 3) MultipleMethodImplRecordsSameMethodDecl - I added an extra parameter to the SVM resolution logic that traverses all the MethodImpls and throws when hitting a duplicate. The remaining failures involve the following negative tests: InterfaceVariance (pending my offline feedback) VarianceSafety (pending my offline feedback) UnimplementedAbstractMethodOnAbstractClass / ConstraintCheckShouldFail Thanks Tomas * Address David's PR feedback * Add virtual static check to SVM resolution logic
…re strict in what we accept while verifying the type (dotnet#21)
…tnet#24) For now, treat constrained non-virtual calls and ldtfn's as blocking Crossgen1/2 compilation. Mark the RuntimeHandle structures as NonVersionable, as they are tied to the JIT behavior AND may be needed by the ldtoken instruction without an explicit token reference.
trylek
force-pushed
the
NoMergeRIForCITesting
branch
from
1043316 to
dbea3da
Compare
|
@jkotas This is a complete implementation of the static virtuals feature, I'd like you opinion as to whether or not we should wrap all the changes in a #ifdef FEATURE_VIRTUAL_STATIC_INTERFACE_METHODS wrapper or not. |
I do not think we need to wrap these changes under ifdefs. Ifdefs for FEATUREs that are enabled in all shipping configs have no value. Also, we may want to delete similar existing |
|
@jkotas that matches my opinion, but it does differ from past approaches, so I wanted your opinion. |
|
The library test bug seems to be #52710. |
trylek
added
NO-SQUASH
trylek
commented
May 14, 2021
| @@ -14,6 +14,8 @@ | |||
| <DisabledProjects Include="$(TestRoot)GC\Performance\Framework\GCPerfTestFramework.csproj" /> | |||
| <DisabledProjects Include="$(TestRoot)Loader\classloader\generics\regressions\DD117522\Test.csproj" /> | |||
| <DisabledProjects Include="$(TestRoot)Loader\classloader\generics\GenericMethods\VSW491668.csproj" /> <!-- issue 5501 --> | |||
| <DisabledProjects Include="$(TestRoot)Loader\classloader\StaticVirtualMethods\**\generatetest.csproj" /> <!-- test generators --> | |||
| <DisabledProjects Include="$(TestRoot)Performance\Scenario\JitBench\unofficial_dotnet\JitBench.csproj" /> <!-- no official build support for SDK-style netcoreapp2.0 projects --> | |||
There was a problem hiding this comment.
I don't know why JitBench.csproj seems like our addition, I guess I messed up the conflict merge.
trylek
changed the title
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change implements initial CoreCLR runtime support for static virtual interface methods and adds over 1700 test cases covering various aspects of this new runtime feature.
In the JIT, we are relaxing the assumption that ".constrained" must be always followed by a "callvirt" by allowing two more IL instructions to be constrained, "call" and "ldftn".
In the JIT interface, we're adding bits of code to CEEInfo::getCallInfo to properly handle the new static virtual methods. We're extending constrained method resolution to cater for the static virtual method case in a new method "ResolveStaticVirtualMethod".
In our work on the implementation we found a pre-existing JIT interface issue - the interplay between getCallInfo and embedGenericHandle doesn't work well in case of constrained calls as the existing API surface doesn't provide any means for communicating compile-time resolution of the constraint from getCallInfo to embedGenericHandle. In this change we're working around this by deferring to runtime resolution of the constraint in the presence of shared generics.
In the method table builder, we're newly tracking a flag saying whether we're implementing an interface declaring static virtual methods (fHasVirtualStaticMethods) that is used to speed up the runtime checks by skipping irrelevant interfaces.
For Crossgen / Crossgen2, this change only adds minimalistic support in the form of bailing out in getCallInfo whenever we hit a call to a static virtual method, cancelling AOT compilation of the caller and deferring it to runtime JIT.