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.
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
| { | ||
| // In the rare case when array is indexed by nint instead of int, we emit an extra check | ||
| // to ensure the nint value fits in int32_t | ||
| AddIns(INTOP_CONV_NI); |
There was a problem hiding this comment.
If the array is itself NULL, we need to be throwing NullReferenceException instead of IndexOutOfRangeException. This needs both the array and the index to pull that off.
There was a problem hiding this comment.
I assumed this would be handled by the actual array access, wouldn't it?
There was a problem hiding this comment.
Ah, got it, the NullReferenceException would need to have precedence.
| m_pStackPointer -= 2; | ||
| } | ||
|
|
||
| void InterpCompiler::EmitNintIndexCheck(StackInfo *spArray, StackInfo *spIndex) |
There was a problem hiding this comment.
newarr has similar problem (it can take nint). Do we need to fix it as well?
Related comment #120981 (comment)
There was a problem hiding this comment.
I think we should fix it too, but for that one, I think we can create new versions of the INTOP_NEWARR / INTOP_NEWARR_GENERIC, there is no opcode explosion issue like with the array accesses. I'll create an issue for it.
|
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
added
the
NO-MERGE
4 participants
This change contains the following: