Skip to content

Conversation

@janvorli
Copy link
Member

@janvorli janvorli commented Nov 10, 2025

This change contains the following:

  • Fix cases when returning smaller than 32 bit int / uint values - introduce new INTOP_RET variants like Mono
  • Fix index out of range check case when native int is used as an index to an array. To avoid adding an explosion of array element access IR opcodes, it adds a range check opcode that is used only when the index is nint.
  • put back zeroing to INTOP_NEWOBJ_VT, few tests were failing because it was missing and we've already fixed the issue we had with the zeroing before by setting the "noCallArgs"
  • Disable a test that uses pinvoke marshalling with calli

This change contains the following:
* Fix cases when returning smaller than 32 bit int / uint values -
  introduce new INTOP_RET variants like Mono
* Fix index out of range check case when native int is used as an index to
  an array. To avoid adding an explosion of array element access IR opcodes,
  it adds a range check opcode that is used only when the index is nint.
* put back zeroing to INTOP_NEWOBJ_VT, few tests were failing because it
  was missing and we've already fixed the issue we had with the zeroing
  before by setting the "noCallArgs"
* Disable a test that uses pinvoke marshalling
@janvorli janvorli added this to the 11.0.0 milestone Nov 10, 2025
@janvorli janvorli self-assigned this Nov 10, 2025
Copilot AI review requested due to automatic review settings November 10, 2025 12:00
@janvorli janvorli requested review from BrzVlad and kg as code owners November 10, 2025 12:00
@dotnet-policy-service
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 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 proper handling of small integer return types (int8, uint8, int16, uint16) and native integer (nint) array indexing in the CoreCLR interpreter.

  • Added new return opcodes (RET_I1, RET_U1, RET_I2, RET_U2) to properly sign/zero extend small integer return values
  • Implemented INTOP_CONV_NI to validate and convert nint values to int32 for array indexing operations
  • Added memset initialization for value types before constructor calls to ensure deterministic behavior

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/JIT/Regression/JitBlue/Runtime_58259/Runtime_58259.cs Added ActiveIssue attribute to skip test on CoreCLR interpreter due to PInvoke marshalling issue
src/coreclr/interpreter/inc/intops.def Added new opcode definitions for specialized return instructions and nint conversion
src/coreclr/vm/interpexec.cpp Implemented execution logic for new return opcodes with proper type extension, INTOP_CONV_NI with range validation, and value type zero-initialization
src/coreclr/interpreter/compiler.h Declared EmitNintIndexCheck helper function
src/coreclr/interpreter/compiler.cpp Implemented GetRetForType, EmitNintIndexCheck, updated array operations to handle nint indices, fixed SUB_I4 condition, and updated validation for new return opcodes

@janvorli
Copy link
Member Author

It seems something may be wrong with the nint index checking stuff based on my local libraries tests. It might be a red herring though. Let's wait with merging the change in until I can verify it.

@janvorli janvorli added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 12, 2025
@janvorli janvorli removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 20, 2025
@janvorli
Copy link
Member Author

/ba-g the Android failures in the cryptography tests are unrelated to this change

@janvorli
Copy link
Member Author

@kg, @davidwrighton, @BrzVlad I've fixed the problem in the nint index handling, now it works as expected based on my local testing. Can one of you take a look at this PR please?

@janvorli janvorli merged commit 5277ab4 into dotnet:main Nov 21, 2025
102 of 105 checks passed
@janvorli janvorli deleted the fix-several-coreclr-tests-interpreted branch November 21, 2025 17:30
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.

5 participants