Skip to content

Add internal intrinsics for instance-based calli - #114495

Merged
steveharter merged 10 commits into
dotnet:mainfrom
steveharter:ReflectionCalli
Apr 16, 2025
Merged

Add internal intrinsics for instance-based calli#114495
steveharter merged 10 commits into
dotnet:mainfrom
steveharter:ReflectionCalli

Conversation

@steveharter

@stevehartersteveharter commented Apr 10, 2025

Copy link
Copy Markdown
Contributor

Contributes to #112994. The next step in this process will use these intrinsics.

Adds support for both CoreClr JIT and R2R to modify the function pointer signature to add HasThis | ExplicitThis so specific function pointer signatures can be called on an instance method.

Local testing verified both:

  • R2R\crossgen2 created the native code with the HasThis | ExplicitThis signature and thus prevented JIT.
  • The non-R2R runtime cases replaced the IL of the method and added HasThis | ExplicitThis to the signature.

The tests added here currently don't verify the intrinsics, but an earlier version did (see the first commit). Once the next step of hooking into reflection is done, these tests will cover the new intrinsics added here. In particular, the new tests address missing test cases of calling an instance-based method with non-primitive return types such as Guid, DateTime and TimeSpan that would fail if reflection called these methods with function pointers \ Calli without HasThis | ExplicitThis.

@stevehartersteveharter added this to the 10.0.0 milestone Apr 10, 2025
@stevehartersteveharter self-assigned this Apr 10, 2025
CopilotAI review requested due to automatic review settings April 10, 2025 16:28
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 12 out of 14 changed files in this pull request and generated 3 comments.

Files not reviewed (2)
  • src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj: Language not supported
  • src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csproj: Language not supported

Comment threadsrc/coreclr/vm/stubgen.h Outdated
Comment threadsrc/coreclr/vm/stubgen.cpp Outdated
@MichalPetryka

MichalPetryka commented Apr 10, 2025

Copy link
Copy Markdown
Contributor

Wouldn't it be simpler to just write those stubs in IL instead of having to implement them in every VM?
Like this.

Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated
Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated
Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated
Comment threadsrc/coreclr/vm/corelib.h Outdated
@jkotas

Copy link
Copy Markdown
Member

Wouldn't it be simpler to just write those stubs in IL instead of having to implement them in every VM?

We would need to invent build infrastructure to merge this IL into CoreLib....

Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated
Comment threadsrc/coreclr/vm/stubgen.cpp Outdated
Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGMT otherwise. Thank you!

Comment threadsrc/coreclr/vm/siginfo.cpp Outdated
Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated
Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated
Comment threadsrc/coreclr/vm/jitinterface.cpp Outdated
Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated
Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated
Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated
Comment threadsrc/coreclr/vm/siginfo.hpp Outdated
Comment threadsrc/coreclr/vm/siginfo.cpp Outdated
Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated
Comment threadsrc/coreclr/vm/siginfo.cpp Outdated
Comment threadsrc/coreclr/vm/siginfo.cpp Outdated

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated
Comment threadsrc/coreclr/vm/instancecalli.cpp Outdated

@AaronRobinsonMSFTAaronRobinsonMSFT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@steveharter

Copy link
Copy Markdown
ContributorAuthor

/ba-g infra timeout on building Libraries Test Run release coreclr linux_musl x64 Debug

@steveharter
steveharter merged commit 24d8707 into dotnet:mainApr 16, 2025
@steveharter
steveharter deleted the ReflectionCalli branch April 16, 2025 14:14
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 17, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@steveharter@MichalPetryka@jkotas@AaronRobinsonMSFT