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

Show address offsets in hex #73816

Merged
merged 3 commits into from
Aug 15, 2022
Merged

Conversation

kunalspathak
Copy link
Member

Make the address offset displayed in hex which is in sync with how we display in local variables table. I often find a need to search for given address offset displayed in local variables tables by I have to convert the hex to decimal. This PR converts displaying the address offset in same format. Here is the diff:

image

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 11, 2022
@ghost ghost assigned kunalspathak Aug 11, 2022
@ghost
Copy link

ghost commented Aug 11, 2022

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

Issue Details

Make the address offset displayed in hex which is in sync with how we display in local variables table. I often find a need to search for given address offset displayed in local variables tables by I have to convert the hex to decimal. This PR converts displaying the address offset in same format. Here is the diff:

image
Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@JulieLeeMSFT
Copy link
Member

@kunalspathak, are you trying to merge before RC1 snap? Otherwise, please set this to 8.0.0.

@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib can someone approve this? It doesn't harm if it goes in .NET 7.

@kunalspathak kunalspathak merged commit 0a2b4eb into dotnet:main Aug 15, 2022
@kunalspathak kunalspathak deleted the hex_addroffset branch August 15, 2022 18:01
@BruceForstall
Copy link
Member

I'm opposed to this change.

ARM64 assembly output should match the ISA manual, which uses "#12345" format. It does support "#0x12345", but the "12345H" format is, I believe, for Intel assembly, so shouldn't be used for ARM.

A possibly better solution would be to add a comment with the hex value of any displayed decimal number.

@kunalspathak
Copy link
Member Author

A possibly better solution would be to add a comment with the hex value of any displayed decimal number.

Do you want me to do it for .NET 7 or can it wait for few weeks?

@BruceForstall
Copy link
Member

Do you want me to do it for .NET 7 or can it wait for few weeks?

My suggestion for "a better solution" with a trailing comment could be deferred. Actually, it's possibly not worth doing if we just convert all numbers to "#0x12345" form instead (though, IMO, all numbers as hex is sometimes annoying, it's useful to be able to search for all occurrences as you've found).

However, I really don't like the dropping of "#" and the use of trailing "H" (instead of leading "0x") so I think that should be fixed for .NET 7.

@kunalspathak
Copy link
Member Author

Actually, it's possibly not worth doing if we just convert all numbers to "#0x12345" form instead

I agree to that include in "final local variables assignment table" and that way this problem will be solved. To do minimal changes, I will just change the format of arm/arm64 from 1234ABCH to #0x1234ABC and in .NET 8, make things consistent with respect to "final local variables assignment" table.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 15, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants