Skip to content

Add support for creating delegates to static virtual methods#66936

Merged
MichalStrehovsky merged 2 commits intodotnet:mainfrom
MichalStrehovsky:delStaticVirt
Mar 29, 2022
Merged

Add support for creating delegates to static virtual methods#66936
MichalStrehovsky merged 2 commits intodotnet:mainfrom
MichalStrehovsky:delStaticVirt

Conversation

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Mar 21, 2022

Requires a JitInterface change because we need to be able to pass information about constraints to getReadyToRunDelegateCtorHelper

@MichalStrehovsky MichalStrehovsky requested a review from jkotas March 21, 2022 09:58
jakobbotsch
jakobbotsch reviewed Mar 21, 2022
View reviewed changes
This was referenced Mar 21, 2022
Closed
Closed
jakobbotsch
jakobbotsch approved these changes Mar 24, 2022
View reviewed changes
Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JIT part looks ok to me.

@jkotas
Copy link
Member

jkotas commented Mar 24, 2022 •
edited
Loading

Ok, done. It's a bit of a misuse but I couldn't find a better spot to transfer this information within RyuJIT.

Can you change typeInfo to store a new MethodPointerInfo structure instead CORINFO_RESOLVED_TOKEN that contains the resolved token and anything else that is convenient to keep around for later? I think it would be much cleaner than attaching the information to CORINFO_RESOLVED_TOKEN.

In general, directly reusing JIT/EE interface structures for bookkeeping in the JIT tends to work poorly. It is best to decouple the two purposes whenever we run into problems.

@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Mar 25, 2022

Can you change typeInfo to store a new MethodPointerInfo structure instead CORINFO_RESOLVED_TOKEN

Looks like that's going to be much cleaner, thanks! Looks like typeInfo tried to be too general purpose in this sense and that made things confusing:

  • typeInfo::IsToken() will only be true for the loading of a function pointer case. typeInfo::m_token is only used for the function pointers as well.
  • typeInfo::m_method is dead code.

I'll try to clean that up as part of this.

jakobbotsch
jakobbotsch reviewed Mar 25, 2022
View reviewed changes
@jkotas
Copy link
Member

jkotas commented Mar 25, 2022

Formatting...

jkotas
jkotas approved these changes Mar 28, 2022
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@MichalStrehovsky MichalStrehovsky merged commit 370f160 into dotnet:main Mar 29, 2022
@MichalStrehovsky MichalStrehovsky deleted the delStaticVirt branch March 29, 2022 03:44
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
@MichalStrehovsky @radekdoulik
Add support for creating delegates to static virtual methods (dotnet#…
7fe76b1 
…66936)

Requires a JitInterface change because we need to be able to pass information about constraints to `getReadyToRunDelegateCtorHelper`
@ghost ghost locked as resolved and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jakobbotsch jakobbotsch jakobbotsch approved these changes

@jkotas jkotas jkotas approved these changes

Assignees

@MichalStrehovsky MichalStrehovsky

Labels

area-crossgen2-coreclr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MichalStrehovsky @jkotas @jakobbotsch