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

Add leading 0x to offset in Debug fmt of Pointer #71459

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

divergentdave
Copy link
Contributor

Currently the Debug format for Pointer prints its offset in hexadecimal, for example, alloc38657819+e2 or alloc35122748+64. This PR adds a leading 0x to the offset, in order to make it apparent that it is indeed a hexadecimal number. This came up during discussion of rust-lang/miri#1354. r? @RalfJung

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @RalfJung (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2020
@RalfJung
Copy link
Member

Thanks!

I think this is much better, when I see alloc137+13 I would not expect the offset to be hex.
But Cc @oli-obk and @eddyb who reworked a lot of MIR printing code recently; what do you think?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 23, 2020

🤷 I don't have a strong opinion. It'll make the relocation size problem in the hex dump worse though, which I think is the main reason I tried to keep it as short as possible

@RalfJung
Copy link
Member

I wonder why hex dumps do not show up in the diff here?

At least for pointers outside of hex dumps, I think this is a clear improvement. Even within hex dumps, I'd rather shorten alloc to a or so, that's less confusing IMO.

@RalfJung
Copy link
Member

For example, src/test/mir-opt/const_allocation/32bit/rustc.main.ConstProp.after.mir at the bottom still looks like

alloc17 (size: 48, align: 4) {
    0x00 │ 00 00 00 00 __ __ __ __ ╾alloc4+0─╼ 00 00 00 00 │ ....░░░░╾──╼....
    0x10 │ 00 00 00 00 __ __ __ __ ╾alloc8+0─╼ 02 00 00 00 │ ....░░░░╾──╼....
    0x20 │ 01 00 00 00 2a 00 00 00 ╾alloc13+0╼ 03 00 00 00 │ ....*...╾──╼....
}

How can that be?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 23, 2020

Likely because the refactoring for the alloc42+69 change happened in parallel to the hex dump PR, so the hex dump PR repeated the printing logic

@RalfJung
Copy link
Member

Ah, indeed it does:

let mut target = format!("{}+{}", target_id, offset);

So should we take advantage of that and leave the dumps without 0x? Seems oddly inconsistent though. (And btw this needs a FIXME to also print the tag.)

What about landing this PR to change just Pointer printing, and then I can make a follow-up where we play with various options for the hex-dumps?

@oli-obk
Copy link
Contributor

oli-obk commented Apr 23, 2020

Yea, makes sense, let's merge this PR

@RalfJung
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 23, 2020

📌 Commit c16b6e0 has been approved by RalfJung

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Apr 23, 2020
…RalfJung

Add leading 0x to offset in Debug fmt of Pointer

Currently the `Debug` format for `Pointer` prints its offset in hexadecimal, for example, `alloc38657819+e2` or `alloc35122748+64`. This PR adds a leading `0x` to the offset, in order to make it apparent that it is indeed a hexadecimal number. This came up during discussion of rust-lang/miri#1354. r? @RalfJung
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 24, 2020
Rollup of 6 pull requests

Successful merges:

 - rust-lang#70845 (Make the `structural_match` error diagnostic for const generics clearer)
 - rust-lang#71063 (Document unsafety in core::{option, hash})
 - rust-lang#71068 (Stabilize UNICODE_VERSION (feature unicode_version))
 - rust-lang#71426 (fix error code in E0751.md)
 - rust-lang#71459 (Add leading 0x to offset in Debug fmt of Pointer)
 - rust-lang#71492 (Document unsafety in core::{panicking, alloc::layout, hint, iter::adapters::zip})

Failed merges:

r? @ghost
@bors bors merged commit ed25ca0 into rust-lang:master Apr 24, 2020
@RalfJung
Copy link
Member

Turns out MIR allocation dumping was actually using decimal, while normal Pointer printing was hex, but both looked exactly the same... ouch.

@eddyb
Copy link
Member

eddyb commented Apr 27, 2020

@RalfJung I'm surprised you didn't notice #71459 (comment) had no :x, heh. It seemed to be the point of linking that snippet

I never expected hex offsets, since I had only seen decimal ones thus far.

@RalfJung
Copy link
Member

RalfJung commented Apr 27, 2020

Pointer printing was using hex offsets since at least since d01ef7d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants