Skip to content
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.

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

hex number format in .net 7 jit assembly #81329

Closed
kasperk81 opened this issue Jan 29, 2023 · 14 comments · Fixed by #89773
Closed

hex number format in .net 7 jit assembly #81329

kasperk81 opened this issue Jan 29, 2023 · 14 comments · Fixed by #89773
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration Priority:2 Work that is important, but not critical for the release
Milestone

Comments

@kasperk81
Copy link
Contributor

jitdisasm in .net 7 is printing h suffix for hex numbers. h suffix may be allowed by intel assembly spec and normal for microsoft assemblers, but clang/gcc don't support it. it's unfamiliar to users and their disassemblers (which don't print h suffix) and their assemblers (which don't recognize the h suffix). please replace %02XH with 0x%02x or keep it readable decimal as it was before .net 7.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 29, 2023
@kasperk81
Copy link
Contributor Author

.net 6: using decimal recognized by masm, clang, gcc, nasm, ptx etc. and their users

.intel_syntax noprefix

x:
  mov    [rsp+16], rdi
  mov    rdi, [rsp+9]

.net 7: using hex with h suffix (instead of 0x prefix) recognized only by masm and its users

.intel_syntax noprefix

x:
  mov    [rsp+10H], rdi
  mov    rdi, [rsp+09H]

@EgorBo
Copy link
Member

EgorBo commented Jan 29, 2023

Overall JitDisam is not a real asm code, e.g. it contains pseudo instructions like mov gword

cc @BruceForstall on hex formatting

@teo-tsirpanis teo-tsirpanis added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 29, 2023
@ghost
Copy link

ghost commented Jan 29, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

jitdisasm in .net 7 is printing h suffix for hex numbers. h suffix may be allowed by intel assembly spec and normal for microsoft assemblers, but clang/gcc don't support it. it's unfamiliar to users and their disassemblers (which don't print h suffix) and their assemblers (which don't recognize the h suffix). please replace %02XH with 0x%02x or keep it readable decimal as it was before .net 7.

Author: kasperk81
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@kunalspathak
Copy link
Member

Overall JitDisam is not a real asm code, e.g. it contains pseudo instructions like mov gword

Correct. This was changed in .NET 7 in #73816 to ease the search of given address offset displayed in local variables table.

@kasperk81
Copy link
Contributor Author

#73816 (comment)
change to 0x format for readability

@JulieLeeMSFT JulieLeeMSFT added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed untriaged New issue has not been triaged by the area owner labels Jan 30, 2023
@kasperk81
Copy link
Contributor Author

@BruceForstall feedback about reverting to well known 0x prefix was from the user when i enabled dotnet daily build on https://godbolt.org public website (compiler-explorer/compiler-workflows@226373d). this feedback echoes your comment: #73816 (comment). i think it should be changed

@kunalspathak
Copy link
Member

Agree. I will revert the format of disassembly, and instead make changes in local variables table to make search easier.

@JulieLeeMSFT JulieLeeMSFT added this to the 8.0.0 milestone Apr 13, 2023
@kunalspathak
Copy link
Member

kunalspathak commented May 2, 2023

I was looking into this and the original change done in xarch was to make it consistent regardless of how much is the displacement. Prior to my change, it was showing a mixed of decimal and hexadecimal and some of them also included H as seen below.

if (frameRef)
{
printf("%02XH", (unsigned)disp);
}
else if (disp < 1000)
{
printf("%02XH", (unsigned)disp);
}
else if (disp <= 0xFFFF)
{
printf("%04XH", (unsigned)disp);
}
else
{
printf("%08XH", (unsigned)disp);
}

This makes me feel that we should retain the original change or update all the above references to eliminate H. Is there an instruction manual that guides what is the right format to display? @tannergooding - you might know this.

@tannergooding
Copy link
Member

tannergooding commented May 3, 2023

There is no "official" format for x86 assembly and what exactly is supported can differ based on assembler.

That being said, most assemblers support 0x (MASM is a notable exception) while the support for h is definitely more variable. The majority of official samples from the Intel/AMD architecture manuals use 0x as well.

@kunalspathak
Copy link
Member

That being said, most assemblers support 0x (MASM is a notable exception) while the support for h is definitely more variable. The majority of official samples from the Intel/AMD architecture manuals use 0x as well.

Thanks @tannergooding . A follow-up question, do they distinguish between displaying the address that is less than 1000, the way we are doing? I am ok with using 0x or h format as long as they are consistent for various address and not restrict it based on the actual address value.

@tannergooding
Copy link
Member

There isn't really a standard there either and which is "better" depends on context.

If you're doing something like "field offsets" then decimal may be better. If you're doing arbitrary addresses or displacements, then hex may be better.

The same goes for practically any immediate value. For shufps, using hex for the immediate is almost always better even for small constants, as an example.

@kunalspathak kunalspathak added the Priority:2 Work that is important, but not critical for the release label Jun 5, 2023
@kunalspathak
Copy link
Member

@jakobbotsch - can you please take a look?

@jakobbotsch
Copy link
Member

@kunalspathak What is the work item here? To update the places in the xarch backend where we print hex numbers with "H" suffix to instead print with "0x" prefix?

jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Aug 1, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 1, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 2, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration Priority:2 Work that is important, but not critical for the release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants