Serialize negative constants according to the operand type #118814
Conversation
Prior to this patch, any negative constants smaller than -0xFFFFFF would get serialized to 64-bit constants, so we would end up with assembly statements like `imul eax, esi, 0xFFFFFFFFFEFFFFFF`, which is invalid since the constant is larger than 32 bits, which is the size of `eax` and `esi` registers. This patch fixes the code so that for known 1-, 2, 4-, and 8-byte types, we first truncate the constant to the correct number of bits before printing it. Fix dotnet#118813
github-actions
bot
added
the
needs-area-label
dotnet-policy-service
bot
added
the
community-contribution
vcsjones
added
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
src/coreclr/jit/emitxarch.cpp
Outdated
| break; | ||
|
|
||
| case EA_8BYTE: | ||
| printf("0x%X", static_cast<int64_t>(val)); |
There was a problem hiding this comment.
This is not correct if int is 32-bit. %X expects an unsigned int argument but int64_t may be wider than that.
I think you can use "0x%zX" with (size_t)(uint64_t)val. I don't see why one would use the signed type ssize_t with %zX.
There was a problem hiding this comment.
Thanks for catching that! I had missed that %X expects an unsigned integer. Instead of printing the integers with the sign, I unified the logic for printing both positive and negative numbers by printing the unsigned equivalent. This has the added benefit of making the code simpler, IMO.
The `%X` format specifier expects an unsigned value, so passing a negaive integer is undefined behavior. Instead of printing a value with the sign, this patch prints the two's complement of the negative value with the appropriate width (as specified in the instruction). Since this logic is the same for both positive and negative integers, this patch unifies the code for printing all integers.
| { | ||
| printf("%d", (int)val); |
There was a problem hiding this comment.
we were printing decimal here not hex
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a code generation issue where negative constants were being serialized incorrectly in x86/x64 assembly output. The problem occurred when negative constants smaller than -0xFFFFFF were printed as 64-bit values even when used with smaller operand sizes, resulting in invalid assembly like imul eax, esi, 0xFFFFFFFFFEFFFFFF.
Key changes:
- Replaced the previous logic that handled positive/negative constants differently with operand size-aware formatting
- Added a switch statement to truncate constants to the appropriate bit width (1, 2, 4, or 8 bytes) before printing
- Ensures generated assembly uses correctly sized constants that match the operand types
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| break; | ||
|
|
||
| case EA_8BYTE: | ||
| printf("0x%X", static_cast<uint64_t>(val)); |
There was a problem hiding this comment.
The format specifier %X is used for uint64_t but should be %llX or %lX depending on the platform. Using %X with a 64-bit value may cause truncation or undefined behavior on some platforms.
| printf("0x%X", static_cast<uint64_t>(val)); | |
| printf("0x%llX", static_cast<unsigned long long>(val)); |
Copilot uses AI. Check for mistakes.
| break; | ||
|
|
||
| default: | ||
| printf("0x%zX", static_cast<size_t>(val)); |
There was a problem hiding this comment.
Casting a potentially negative ssize_t value to size_t (unsigned) may not preserve the intended bit pattern. Consider using the original (ssize_t)val cast to maintain sign information, or ensure the truncation behavior is intentional.
| printf("0x%zX", static_cast<size_t>(val)); | |
| printf("0x%zX", static_cast<ssize_t>(val)); |
Copilot uses AI. Check for mistakes.
| } | ||
| else | ||
| { | ||
| // (val < 0) | ||
| printf("-0x%zX", (ssize_t)-val); |
There was a problem hiding this comment.
The below code will never print negative values anymore. I do not think we want that to change.
FWIW, the jit disasm is a power user scenario for curious users only. It is not meant to be full fidelity/correct assembly output, and in various cases the produced disassembly will not be fully faithful. That is expected.
Prior to this patch, any negative constants smaller than -0xFFFFFF would
get serialized to 64-bit constants, so we would end up with assembly
statements like
imul eax, esi, 0xFFFFFFFFFEFFFFFF, which is invalidsince the constant is larger than 32 bits, which is the size of
eaxand
esiregisters.This patch fixes the code so that for known 1-, 2, 4-, and 8-byte types,
we first truncate the constant to the correct number of bits before
printing it.
Fix #118813