-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updating the JIT to support marshaling blittable generics. #103
Conversation
…ing in favor of fixing the text in IDS_EE_BADMARSHAL_GENERICS_RESTRICTION
I think the only open question I had was on how to properly test crossgen2 against the appropriate tests: dotnet/coreclr#23899 (comment) |
cc @trylek for the crossgen2 question. |
@tannergooding - right now the only CI testing available for Crossgen2 is a single pipeline I created recently, "runtime crossgen2" which runs checked Pri0 tests on the three OS'es / x64. Sadly the results aren't yet completely clean, there are typically something like 60~70 remaining failures. For local testing, it's easiest to use the tool ReadyToRun.SuperIlc - ping me or Jan Vorlicek on IM if you're interested in guidance how to do that. |
I was able to locally validate that the Crossgen2 changes work as expected. I believe this should be ready for review/merging (provided CI is stable enough for it to pass now). |
Is there test coverage for the unmanaged -> managed direction (delegates)? I am not able to find it, but it may be just a github search not working. |
No, I hadn't written any tests for the reverse case. I'll try to get a few written tomorrow. |
@jkotas, the changes for the reverse case are a bit more complex and require updating the I've started updating the relevant checks, but was wondering if you wanted it as part of this PR or if it would be preferred to have a separate PR handling them? |
Yes, it would be better to have those as separate change. What does the current version of the change does in these cases? Does it fail predictably? |
Yes. These paths all throw an
|
What about cases like |
That looks to work today, even without these changes (as does the reverse). |
It does not work in master today. You will get an exception like:
Note that you need to actually call the delegate. The marshaling stub compilation is done lazily. It should continue to throw exception, same as the forward direction. |
Ah, I see. I had only validated that the calls succeeded. I can reproduce the above failure without these changes. With these changes, the marshaling stub works for blittable generics and fails with the following for blocked types (such as
|
Move the logic to find slot defining method of generic virtual methods to a common location and use it from the scanner. The fact that I touched compilation dependencies in the JitInterface but didn't update the scanner should have been a red flag. Fixes a Release-only issue discovered in dotnet#103. Can't wait to have the CI fully up and running.
Remaining tests are the multimodule ones and static/shared library support.
This resolves https://github.com/dotnet/coreclr/issues/1685.
This is a port of dotnet/coreclr#23899.
CC. @jkoritzinsky, @AaronRobinsonMSFT, @jkotas