Skip to content

[crossgen2] Prevent inlining of PInvoke marshalling accross version bubbles #115277

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

Merged
merged 2 commits into from
May 23, 2025

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented May 4, 2025

Inlining of PInvoke stubs should be blocked explicitly in crossgen2. It was blocked as side-effect of JIT never inlining methods with EH, but that is not a safe assumption to make now that the JIT is able to inline methods with EH.

Contributes to #113742

@Copilot Copilot AI review requested due to automatic review settings May 4, 2025 03:19
@jkotas jkotas changed the title [crossgen2] Prevent inlining of PInvoke marshalling accross version b… [crossgen2] Prevent inlining of PInvoke marshalling accross version bubbles May 4, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR aims to prevent inlining of PInvoke marshalling across the version bubble. The changes remove a version bubble check from the JIT interface implementation and add a corresponding check in the compilation module group to enforce the desired behavior.

  • Removed version bubble check in pInvokeMarshalingRequired in CorInfoImpl.ReadyToRun.cs.
  • Added a version bubble check in GeneratesPInvoke in ReadyToRunCompilationModuleGroupBase.cs.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs Removed legacy check for version bubble compliance.
src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilationModuleGroupBase.cs Added explicit version bubble check to prevent PInvoke inlining.
Comments suppressed due to low confidence (1)

src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs:3079

  • The removal of this version bubble check in pInvokeMarshalingRequired should be clearly coordinated with the new logic in GeneratesPInvoke to ensure consistent behavior. Please verify that the change does not unintentionally affect related code paths.
// Marshalling behavior isn't modeled as protected by R2R rules, so disable pinvoke inlining for code outside of the version bubble

@jkotas
Copy link
Member Author

jkotas commented May 4, 2025

cc @AndyAyersMS

This is fixing the crossgen2 logic bug. I am not deleting the JIT workaround in this change. I think we want to evaluate code quality and JIT throughput impact of deleting the workaround separately.

@jkotas
Copy link
Member Author

jkotas commented May 23, 2025

/ba-g networking failure, infrastructure timeout

@jkotas jkotas merged commit 8f84f8e into dotnet:main May 23, 2025
90 of 93 checks passed
@jkotas jkotas deleted the issue-113742 branch May 23, 2025 06:30
SimaTian pushed a commit that referenced this pull request May 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants