Fix interpreter double and 64 bit constant processing #116115
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There is a bug in extracting 64 bit and double constants on both the interpreter compiler and execution sides. The problem was that the lower 32 bits were extracted as signed 32 bit int and then casted to signed 64 bit int and or-ed to the other 32 bits shifted up. The problem is that when the lower 32 bits were signed, the sign got extended to the upper 32 bits and the combined result was incorrect. This fixes 24 codegen bringup tests with the interpreter that were previously failing with "Assert.Equal() Failure: Values differ".
github-actions
bot
added
the
needs-area-label
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in extracting 64-bit and double constants by correcting how the lower 32 bits are handled, preventing unintended sign extension issues.
- Fixed the extraction in INTOP_LDC_I8 and INTOP_LDC_R8 operations in interpexec.cpp
- Changed the extraction of the lower 32 bits in intops.h from a signed version to an unsigned version using getU4LittleEndian
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/vm/interpexec.cpp | Adjusted casting for lower 32 bits to avoid sign extension issues. |
| src/coreclr/interpreter/intops.h | Updated lower 32 bits extraction to use getU4LittleEndian for unsigned data. |
Comments suppressed due to low confidence (2)
src/coreclr/vm/interpexec.cpp:146
- Casting ip[2] to a uint32_t correctly prevents sign extension issues when constructing the 64-bit constant. Consider adding tests for edge-case values to ensure the behavior is as expected.
LOCAL_VAR(ip[1], int64_t) = (int64_t)(uint32_t)ip[2] + ((int64_t)ip[3] << 32);
src/coreclr/interpreter/intops.h:101
- Using getU4LittleEndian for the lower 32 bits avoids erroneous sign extension. Verify that all related tests cover scenarios with negative lower-half values.
return (int64_t)getU4LittleEndian(ptr) | ((int64_t)getI4LittleEndian(ptr + 4)) << 32;
kg
approved these changes
May 29, 2025
Open
3 tasks
BrzVlad
approved these changes
May 30, 2025
3 tasks
teo-tsirpanis
added
area-CodeGen-Interpreter-coreclr
and removed
needs-area-label
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is a bug in extracting 64 bit and double constants on both the interpreter compiler and execution sides. The problem was that the lower 32 bits were extracted as signed 32 bit int and then casted to signed 64 bit int and or-ed to the other 32 bits shifted up. The problem is that when the lower 32 bits were signed, the sign got extended to the upper 32 bits and the combined result was incorrect.
This change fixes the issue. It fixes 24 codegen bringup tests with the interpreter that were previously failing with "Assert.Equal() Failure: Values differ".