-
Notifications
You must be signed in to change notification settings - Fork 5.2k
[clr-interp] Add various tweaks to allow code patterns around byref usage that are tested by various IL tests #121144
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
[clr-interp] Add various tweaks to allow code patterns around byref usage that are tested by various IL tests #121144
Conversation
…exible about allowing ByRefs and O values on the stack to be stored into locals.
There was a problem hiding this 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 pull request implements support for transient pointers as defined in ECMA-335 section I.12.3.2.1 within the CoreCLR interpreter. The changes introduce a new stack type StackTypeTransientPointer to track when a byref pointer is "transient" (e.g., from ldloca operations), allowing more accurate type tracking and proper handling during operations like arithmetic, conversions, and comparisons.
Key changes:
- Added
StackTypeTransientPointerenum value and infrastructure for tracking transient pointer state - Encapsulated direct access to the
typefield inStackInfobehind aGetStackType()accessor - Added methods to manage transient pointer state (
SetAsTransientPointer,IsTransientPointer,BashStackTypeToI_ForTransientPointerUse,BashStackTypeToIForConvert)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/coreclr/interpreter/compiler.h | Adds StackTypeTransientPointer enum value, makes StackInfo::type private with accessor methods, and adds transient pointer management methods |
| src/coreclr/interpreter/compiler.cpp | Updates all direct accesses to .type field to use GetStackType(), adds transient pointer handling logic in arithmetic/conversion operations, marks ldloca results as transient pointers |
Comments suppressed due to low confidence (1)
src/coreclr/interpreter/compiler.cpp:1818
- The
EmitConvfunction creates a newStackInfowith placement new, which always sets the internaltypefield directly via the constructor, bypassing transient pointer tracking. If a transient pointer undergoes conversion, its transient state will be lost afterEmitConv. This could cause incorrect behavior when a transient pointer is converted and then needs to be treated as a regular byref or integer. Consider preserving transient pointer state where appropriate, or ensuring thatBashStackTypeToIForConvertis called before conversions that should clear this state.
new (sp) StackInfo(type, NULL, var);
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ton/runtime into add_transient_pointer_support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo the nit
Those have been removed in the ECMA auguments. |
…nsient_pointer_support
…as that is no longer a concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
|
/ba-g The various arm queues are currently broken in helix, and the one test failure is a known (but rare) failure already known to build analysis #105124. |
Allow use of the transient pointer concept from ECMA 335 using a "bashing" approach where the StackType is adjusted to I as needed. This is done at the same points that the JIT does this bashing. This was originally justified by I.12.3.2.1 in the ECMA spec, but as that has been defined out of the spec, this is only supported in order to enable various tests to pass and match JIT behavior.
Allow the convert opcode to operate on StackTypeByRef and StackTypeO as if they were of StackTypeI.