Implement instantiating and unboxing through portable stublinker code… #106
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
… - Handle only the cases with register to register moves - Followon work to remove the old instantiating/unboxing stub infra (except for x86, x86 will remain special) - Shares abi processing logic with delegate shuffle thunk creation - Architecture specific logic is relatively simple - Do not permit use of HELPERREG in computed instantiating stubs - Fix GetArgLoc such that it works on all architectures and OS combinations Add a JIT stress test case for testing all of the various combinations - Use the same calling convention test architecture that was used as part of tail call work Rename secure delegates to wrapper delegates - Secure delegates are no longer a feature of the runtime - But the wrapper delegate lives on as a workaround for a weird detail of the ARM32 abi
jkotas
reviewed
Nov 19, 2019
| @@ -62,6 +67,7 @@ | |||
| <DefineConstants Condition="'$(FeatureDefaultInterfaces)' == 'true'">$(DefineConstants);FEATURE_DEFAULT_INTERFACES</DefineConstants> | |||
| <DefineConstants Condition="'$(FeatureTypeEquivalence)' == 'true'">$(DefineConstants);FEATURE_TYPEEQUIVALENCE</DefineConstants> | |||
| <DefineConstants Condition="'$(FeatureBasicFreeze)' == 'true'">$(DefineConstants);FEATURE_BASICFREEZE</DefineConstants> | |||
| <DefineConstants Condition="'$(FeaturePortableShuffleThunks)' == 'true'">$(DefineConstants);FEATURE_PORTABLE_SHUFFLE_THUNKS</DefineConstants> | |||
There was a problem hiding this comment.
Nit: This is not really needed here. FEATURE_PORTABLE_SHUFFLE_THUNKS has no impact on anything implemented in C# and chances that it will ever have are pretty low. There are number of other FEATURE_XXX that are defined for cmake only.
jkotas
reviewed
Nov 19, 2019
| @@ -562,7 +561,7 @@ protected override MethodInfo GetMethodImpl() | |||
| return (MethodInfo)_methodBase; | |||
| } | |||
|
|
|||
| // Otherwise, must be an inner delegate of a SecureDelegate of an open virtual method. In that case, call base implementation | |||
| // Otherwise, must be an inner delegate of a WrapperDelegate of an open virtual method. In that case, call base implementation | |||
There was a problem hiding this comment.
Suggested change
| // Otherwise, must be an inner delegate of a WrapperDelegate of an open virtual method. In that case, call base implementation | |
| // Otherwise, must be an inner delegate of a wrapper delegate of an open virtual method. In that case, call base implementation |
jkotas
reviewed
Nov 19, 2019
src/coreclr/src/vm/frames.h
Outdated
| @@ -1790,7 +1765,11 @@ class SecureDelegateFrame : public TransitionFrame | |||
| PromoteCallerStack(fn, sc); | |||
| } | |||
|
|
|||
| virtual Assembly *GetAssembly(); | |||
| virtual Assembly *GetAssembly() | |||
There was a problem hiding this comment.
Not needed anymore. And the base implementation does not need to be virtual anymore.
jkotas
reviewed
Nov 19, 2019
src/coreclr/src/vm/i386/cgencpu.h
Outdated
| @@ -99,6 +99,7 @@ EXTERN_C void SinglecastDelegateInvokeStub(); | |||
| #define ENREGISTERED_RETURNTYPE_INTEGER_MAXSIZE 4 | |||
| #define CALLDESCR_ARGREGS 1 // CallDescrWorker has ArgumentRegister parameter | |||
|
|
|||
| #define NO_SHUFFLE_INSTANTIATINGSTUB 1 | |||
There was a problem hiding this comment.
Ah, no, that's a legacy of an old factoring of the FEATURE flags that was terrible. Let me delete that.
|
@davidwrighton is this worth running outerloop testing on? |
|
@jashook I've run outerloop before in the coreclr repo, and it found nothing, so I don't really want to run it more unless I have to. I have a small set of changes from Jan to incorporate, and then I'll run the normal CI again, and then merge the code. |
|
/azp list |
|
/azp run runtime-coreclr |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@davidwrighton I have started collecting data on the merged commit. |
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.
Add a JIT stress test case for testing all of the various combinations
Rename secure delegates to wrapper delegates