Better pretty printing for const raw pointers #65859
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (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
added
the
S-waiting-on-review
|
I don't know who would be a great reviewer for this, but I don't know that it's me. I personally find the output with this change pretty intimidating in terms of length-- I don't know how often the full leading-zeros pointer value will be helpful. I guess I don't have a particularly better suggestion to offer, though. cc @estebank in case they have any ideas, and @RalfJung who might have thoughts about direct display of pointers coming from const contexts. @bors r+ |
|
📌 Commit 6a6ed12 has been approved by |
bors
added
S-waiting-on-bors
|
There are some hacks we could pursue to only highlight the parts of the hex value that differ, but it'd be a big hack for a fairly uncommonly used feature. |
…mertj Better pretty printing for const raw pointers closes rust-lang#65349, hopefully.
…mertj Better pretty printing for const raw pointers closes rust-lang#65349, hopefully.
|
Failed in #65917 (comment), @bors r- |
bors
added
S-waiting-on-author
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I'm stuck. I can't figure out how to debug the mir-opt tests. This is the part that has me mystified: That |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The output itself is generated by rust/src/librustc/mir/interpret/value.rs Lines 117 to 134 in 374ad1b which calls rust/src/librustc/mir/interpret/pointer.rs Lines 107 to 111 in 374ad1b However, these should never end up being user-visible, they should be used only for Miri debugging (Miri has lots of |
src/librustc/ty/print/pretty.rs
Outdated
| p!(write("{:?} : ", ct.val), print(ct.ty)) | ||
| } | ||
| // fallback | ||
| p!(write("{:?} : ", ct.val), print(ct.ty)) |
There was a problem hiding this comment.
Ah, here you are using the Debug implementation of ConstValue. That's likely not what we want, it is called Debug for a reason. ;)
There was a problem hiding this comment.
This is no different from the old fallback. I simply moved the block that tries to print slices and strings to happen before the big match statement, so that the new const ref arm we just added doesn't override strings and slices.
There was a problem hiding this comment.
True. This was already bad before. I pointed this out because it might be related to your comment at #65859 (comment).
if you don't want to fix this in this PR, feel free to open an issue. We shouldn't use Debug for regular output such as error messages, IMO.
There was a problem hiding this comment.
I pointed this out because it might be related to your comment at #65859 (comment).
That's what I thought as well, but this is not the code path that causes that particular line in the mir-opt output. It comes from somewhere else, perhaps within miri itself. I don't feel confident making a decision about the fallback case, since I don't know what it affects, so I'll open an issue instead.
And since it won't be user facing, should I just bless this output and be done with it?
There was a problem hiding this comment.
Why is it not user-facing? Isn't this here used for the type mismatch errors that the PR updates?
I think I am confused about which change you are worried about. If this PR makes mir-opt fail, then please either fix that or update the mir-opt tests so that we can see in the diff what the changes are.
There was a problem hiding this comment.
_4 = const {pointer} : &i32;
_3 = const {pointer} : &i32;
_2 = const Scalar(AllocId(1).0x0) : *const i32;I saw this in the actual output from src/tests/mir-opt/const_prop/const_prop_fails_gracefully.rs, and thought I had introduced a bug, because a *const i32 should be covered by my changes, and be represented as {pointer} : *const i32.
So I updated the expected output to be this:
_4 = const {pointer} : &i32;
_3 = const {pointer} : &i32;
_2 = const {pointer} : *const i32;And started trying to get that to happen. But if it's not critical that mir dumps are pretty, I will happily abandon that work. I've solved the merge conflicts and pushed a blessing.
|
☔ The latest upstream changes (presumably #66233) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I tried to add the or_patterns flag, but it says it's an incomplete feature and refuses to compile. |
| (ConstValue::Scalar(_), ty::RawPtr(_)) => p!(write("{{pointer}}")), | ||
| (ConstValue::Scalar(value), ty::Ref(..)) | | ||
| (ConstValue::Scalar(value), ty::RawPtr(_)) => { | ||
| match value { |
There was a problem hiding this comment.
So this is a "fallback" case now, right? We already have a big case for references above. Unfortunately it is not immediately clear what distinguishes these cases.
Could you add some comments both at the if let ty::Ref above and at the match arm here describing why we do the matches we do in the order they are, and which various cases of references each conditional covers?
There was a problem hiding this comment.
I don't know the history of that big if let ty::Ref statement or why it looks the way it does internally, so I don't feel comfortable documenting it. But I added a comment specifying that it needs to happen before the general case.
There was a problem hiding this comment.
From what I can see, the point of that is to print the content of the slice/array when possible for types &[u8], [u8; N] and &str. Do you agree? If yes, please write that. Right now you only mention &str but that is not the only type handled here.
The fallback then is that for all other references and raw pointers, we just print the pointer value, not the thing it points to.
| // _3 = const {pointer} : &i32; | ||
| // _2 = const Value(Scalar(AllocId(1).0x0)) : *const i32; |
There was a problem hiding this comment.
This is weirdly inconsistent between references and raw pointers. To be fair, it was also inconsistent before, but now the inconsistency is stronger.
Also @oli-obk @wesleywiser could you check these -Zdump-mir changes. This is a regression in terms of information content but I am not sure if anyone cares.
There was a problem hiding this comment.
Mir dumps should probably dump the content of referenced allocations, like it dumps promoteds. Without showing alloc IDs this would be impossible to figure out though. I think mir dumps should keep dumping alloc ids
There was a problem hiding this comment.
This is where I got stuck. I still haven't been able to chase down where this Debug print comes from. Maybe we should open another issue?
There was a problem hiding this comment.
You are stuck trying to find out where the old printing for references came from, or the new one, or the one for raw pointers?
There was a problem hiding this comment.
The one for raw pointers. But with @oli-obk's comment in mind, maybe the opposite fix is more appropriate, replacing the new pretty printed references with a more verbose format? Either way it seems out of scope of this PR to me.
There was a problem hiding this comment.
Well the PR changes behavior in an undesirable way when printing references, that part is in scope.
What is being debug-printed here is a ConstKind, I think. You could try removing the Debug impl for that and see where compilation fails.
| | ^^^^^^^^^^^^^^ expected `$PREFIX0000000a : &()`, found `{pointer} : &()` | ||
| | | ||
| = note: expected type `Const<$PREFIX0000000a : &()>` | ||
| found type `Const<{pointer} : &()>` |
There was a problem hiding this comment.
FWIW, on current nightly this prints
= note: expected type `Const<Scalar(0x000000000000000a) : &()>`
found type `Const<Scalar(AllocId(9).0x0) : &()>`
So, yeah, the new output is much better. :D
It seems like right now the code is shared between printing this error and printing the mir-dump, and that is likely not what we want?
|
Sadly I have to admit this task is rising above my level of skill and familiarity with this codebase quicker than I can keep up with. Maybe I'll revisit this once I'm ready, if noone else has picked it up by then. |
|
☔ The latest upstream changes (presumably #66389) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Triage: |
|
@iwikal: if you wanted to pick this back up, and wanted more detailed advice about the parts you're finding tricky, feel free to ping me on Discord or Zulip! |
jyn514
added
S-blocked
closes #65349, hopefully.