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
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