Skip to content

Serialize negative constants according to the operand type #118814

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.

Sign up for GitHub

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

Closed
ashay wants to merge 4 commits into from
Closed

Serialize negative constants according to the operand type #118814

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a24051
Serialize negative constants according to the operand type
ashay Aug 16, 2025
3831af7
Unify logic for printing both positive and negative values
ashay Aug 18, 2025
4686534
Fixed catch-all case to use `size_t` instead of `ssize_t`
ashay Aug 18, 2025
98ce373
Retain existing behavior when printing values between -1000 and +1000
ashay Aug 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions src/coreclr/jit/emitxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12646,16 +12646,32 @@ void emitter::emitDispConstant(const instrDesc* id, bool skipComma) const

if ((val > -1000) && (val < 1000))
{
printf("%d", (int)val);
Copy link
Contributor

@xtqqczze xtqqczze Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we were printing decimal here not hex

Copy link
Author

@ashay ashay Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes! Good catch. Fixed.

}
else if ((val > 0) || (val < -0xFFFFFF))
{
printf("0x%zX", (ssize_t)val);
printf("%d", static_cast<int>(val));
}
else
{
// (val < 0)
printf("-0x%zX", (ssize_t)-val);
Copy link
Member

@jakobbotsch jakobbotsch Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

switch (id->idOpSize())
{
case EA_1BYTE:
printf("0x%X", static_cast<uint8_t>(val));
break;

case EA_2BYTE:
printf("0x%X", static_cast<uint16_t>(val));
break;

case EA_4BYTE:
printf("0x%X", static_cast<uint32_t>(val));
break;

case EA_8BYTE:
printf("0x%X", static_cast<uint64_t>(val));
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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));
Copy link

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
printf("0x%zX", static_cast<size_t>(val));
printf("0x%zX", static_cast<ssize_t>(val));

Copilot uses AI. Check for mistakes.
break;
}
}

emitDispCommentForHandle(cnsVal.cnsVal, id->idDebugOnlyInfo()->idMemCookie, id->idDebugOnlyInfo()->idFlags);
Expand Down
Loading