-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix / disable several coreclr tests with interpreter #121492
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
Fix / disable several coreclr tests with interpreter #121492
Conversation
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
|
Tagging subscribers to this area: @BrzVlad, @janvorli, @kg |
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 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 |
src/tests/JIT/Regression/JitBlue/Runtime_58259/Runtime_58259.cs
Outdated
Show resolved
Hide resolved
|
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. |
|
/ba-g the Android failures in the cryptography tests are unrelated to this change |
|
@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? |
This change contains the following: