Skip to content

Add constrained support for calls #116702

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 3 commits into from
Jun 17, 2025

Conversation

BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Jun 16, 2025

Plus minor refactor around StackInfo. Remove size field since it doesn't look like we will ever use it.

Currently there is no support for boxing with generic shared types.

Fixes Loader/classloader/regressions/208900

@Copilot Copilot AI review requested due to automatic review settings June 16, 2025 16:52
@BrzVlad BrzVlad requested review from janvorli and kg as code owners June 16, 2025 16:53
Copy link
Contributor

Tagging subscribers to this area: @BrzVlad, @janvorli, @kg
See info in area-owners.md if you want to be subscribed.

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 adds support for constrained calls by introducing a new box.ptr opcode and associated handling, refactors StackInfo to remove its unused size field, and implements a shared EmitBox helper to consolidate boxing logic.

  • Introduce INTOP_BOX_PTR in intops.def and handle it in the interpreter loop (interpexec.cpp).
  • Refactor StackInfo (remove size), update all construction sites, and add EmitBox in compiler.h/compiler.cpp.
  • Update EmitCall to handle thisTransform cases including boxing/dereferencing, and remove <CLRTestPriority> from the test project.

Reviewed Changes

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

Show a summary per file
File Description
src/tests/Loader/classloader/regressions/208900/bug.csproj Removed outdated <CLRTestPriority> property
src/coreclr/vm/interpexec.cpp Added INTOP_BOX_PTR case and updated unboxed/boxed data logic
src/coreclr/interpreter/intops.def Defined new OPDEF(INTOP_BOX_PTR, "box.ptr", ...)
src/coreclr/interpreter/compiler.h Removed size from StackInfo; added EmitBox declaration and renamed EmitCall param
src/coreclr/interpreter/compiler.cpp Updated PushTypeExplicit, EmitConv, clause init, and EmitCall; implemented EmitBox helper
Comments suppressed due to low confidence (2)

src/coreclr/interpreter/compiler.h:499

  • [nitpick] The parameter clsHnd is the unboxed type handle, which may be confusing. Consider renaming it to unboxedClsHnd to make its purpose clearer.
void    EmitBox(StackInfo* pStackInfo, CORINFO_CLASS_HANDLE clsHnd, bool argByRef);

src/coreclr/interpreter/compiler.cpp:2898

  • [nitpick] Add a brief comment above EmitBox explaining that clsHnd refers to the unboxed type being boxed and that argByRef controls whether to use box or box.ptr.
void InterpCompiler::EmitBox(StackInfo* pStackInfo, CORINFO_CLASS_HANDLE clsHnd, bool argByRef)

@BrzVlad
Copy link
Member Author

BrzVlad commented Jun 16, 2025

@davidwrighton When using gshared types that need to be resolved at runtime, we currently add specific opcodes to do the resolve and store the result in a var. I'm not really a big fan of this approach because it requires indirection, bloats the code with instructions and it might require to add a fair amount of new opcodes, for the gshared version that loads the type from an argument var. I'm thinking that a less invasive approach could be to have the same format for all instructions and when an instruction receives a generic handle as a data item, it either represents the actual type, or if the handle pointer is tagged it represents some other data that is used to resolve the type at runtime as part of the instruction execution. What do you think about this alternative approach ? I could give it a go if it is not on your radar.

To prevent having uninitialized fields
@BrzVlad BrzVlad force-pushed the feature-clr-interp-constrained branch from 284db1c to f0623fa Compare June 16, 2025 17:10
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM modulo the test priority change.

BrzVlad added 2 commits June 17, 2025 08:36
Rename constrainedClass to pConstrainedToken for clarity.
Extract box code emit into separate method.
It doesn't seem like we will be needing it. Use constructor in more places
@BrzVlad BrzVlad force-pushed the feature-clr-interp-constrained branch from f0623fa to 220d60e Compare June 17, 2025 05:37
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.

3 participants