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.

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

Jump to bottom

Better pretty printing for const raw pointers #65859

Closed
wants to merge 10 commits into from
Closed

Better pretty printing for const raw pointers #65859

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
22586d7
Better pretty printing for const raw pointers
iwikal Oct 27, 2019
6a6ed12
Add the two missing hex digits
iwikal Oct 27, 2019
d322044
Fix raw const ptr test on 32 bit platforms
iwikal Oct 31, 2019
3364c46
Nicer fallback for const pointers to allocations
iwikal Nov 4, 2019
f2cb4d7
Print const refs like const raw pointers
iwikal Nov 4, 2019
6c26bd6
Try to print as slice before ref
iwikal Nov 11, 2019
175c608
Update expected output in mir-opt tests
iwikal Nov 11, 2019
c2dbe44
Bless mir-opt test
iwikal Nov 16, 2019
27d89ec
Merge branch 'master' into pretty-const-pointers
iwikal Nov 16, 2019
1407cdd
Comment which kinds of refs get printed where
iwikal Nov 16, 2019
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
104 changes: 55 additions & 49 deletions src/librustc/ty/print/pretty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -864,6 +864,49 @@ pub trait PrettyPrinter<'tcx>:

let u8 = self.tcx().types.u8;

if let ty::Ref(_, ref_ty, _) = ct.ty.kind {
let byte_str = match (ct.val, &ref_ty.kind) {
(ConstValue::Scalar(Scalar::Ptr(ptr)), ty::Array(t, n)) if *t == u8 => {
let n = n.eval_usize(self.tcx(), ty::ParamEnv::empty());
Some(self.tcx()
.alloc_map.lock()
.unwrap_memory(ptr.alloc_id)
.get_bytes(&self.tcx(), ptr, Size::from_bytes(n)).unwrap())
},
(ConstValue::Slice { data, start, end }, ty::Slice(t)) if *t == u8 => {
// The `inspect` here is okay since we checked the bounds, and there are
// no relocations (we have an active slice reference here). We don't use
// this result to affect interpreter execution.
Some(data.inspect_with_undef_and_ptr_outside_interpreter(start..end))
},
_ => None,
};

if let Some(byte_str) = byte_str {
p!(write("b\""));
for &c in byte_str {
for e in std::ascii::escape_default(c) {
self.write_char(e as char)?;
}
}
p!(write("\""));
return Ok(self);
}

if let (ConstValue::Slice { data, start, end }, ty::Str) =
(ct.val, &ref_ty.kind)
{
// The `inspect` here is okay since we checked the bounds, and there are no
// relocations (we have an active `str` reference here). We don't use this
// result to affect interpreter execution.
let slice = data.inspect_with_undef_and_ptr_outside_interpreter(start..end);
let s = ::std::str::from_utf8(slice)
.expect("non utf8 str from miri");
p!(write("{:?}", s));
return Ok(self);
}
};

match (ct.val, &ct.ty.kind) {
(_, ty::FnDef(did, substs)) => p!(print_value_path(*did, substs)),
(ConstValue::Unevaluated(did, substs), _) => {
Expand Down Expand Up @@ -919,7 +962,16 @@ pub trait PrettyPrinter<'tcx>:
},
(ConstValue::Scalar(Scalar::Raw { data, .. }), ty::Char) =>
p!(write("{:?}", ::std::char::from_u32(data as u32).unwrap())),
(ConstValue::Scalar(_), ty::RawPtr(_)) => p!(write("{{pointer}}")),
(ConstValue::Scalar(value), ty::Ref(..)) |
(ConstValue::Scalar(value), ty::RawPtr(_)) => {
iwikal marked this conversation as resolved.
Show resolved Hide resolved
match value {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Scalar::Raw { data, size } => {
p!(write("0x{:01$x} : ", data, size as usize * 2));
}
_ => p!(write("{{pointer}} : "))
iwikal marked this conversation as resolved.
Show resolved Hide resolved
};
p!(print(ct.ty));
}
(ConstValue::Scalar(Scalar::Ptr(ptr)), ty::FnPtr(_)) => {
let instance = {
let alloc_map = self.tcx().alloc_map.lock();
Expand All @@ -928,54 +980,8 @@ pub trait PrettyPrinter<'tcx>:
p!(print_value_path(instance.def_id(), instance.substs));
},
_ => {
let printed = if let ty::Ref(_, ref_ty, _) = ct.ty.kind {
let byte_str = match (ct.val, &ref_ty.kind) {
(ConstValue::Scalar(Scalar::Ptr(ptr)), ty::Array(t, n)) if *t == u8 => {
let n = n.eval_usize(self.tcx(), ty::ParamEnv::empty());
Some(self.tcx()
.alloc_map.lock()
.unwrap_memory(ptr.alloc_id)
.get_bytes(&self.tcx(), ptr, Size::from_bytes(n)).unwrap())
},
(ConstValue::Slice { data, start, end }, ty::Slice(t)) if *t == u8 => {
// The `inspect` here is okay since we checked the bounds, and there are
// no relocations (we have an active slice reference here). We don't use
// this result to affect interpreter execution.
Some(data.inspect_with_undef_and_ptr_outside_interpreter(start..end))
},
_ => None,
};

if let Some(byte_str) = byte_str {
p!(write("b\""));
for &c in byte_str {
for e in std::ascii::escape_default(c) {
self.write_char(e as char)?;
}
}
p!(write("\""));
true
} else if let (ConstValue::Slice { data, start, end }, ty::Str) =
(ct.val, &ref_ty.kind)
{
// The `inspect` here is okay since we checked the bounds, and there are no
// relocations (we have an active `str` reference here). We don't use this
// result to affect interpreter execution.
let slice = data.inspect_with_undef_and_ptr_outside_interpreter(start..end);
let s = ::std::str::from_utf8(slice)
.expect("non utf8 str from miri");
p!(write("{:?}", s));
true
} else {
false
}
} else {
false
};
if !printed {
// fallback
p!(write("{:?} : ", ct.val), print(ct.ty))
}
// fallback
p!(write("{:?} : ", ct.val), print(ct.ty))
Copy link
Member

Choose a reason for hiding this comment

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

Ah, here you are using the Debug implementation of ConstValue. That's likely not what we want, it is called Debug for a reason. ;)

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@iwikal iwikal Nov 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

@RalfJung RalfJung Nov 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

_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.

}
};
Ok(self)
Expand Down
6 changes: 3 additions & 3 deletions src/test/mir-opt/const_prop/const_prop_fails_gracefully.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ fn main() {
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _4 = const Scalar(AllocId(1).0x0) : &i32;
// _3 = const Scalar(AllocId(1).0x0) : &i32;
// _2 = const Scalar(AllocId(1).0x0) : *const i32;
// _4 = const {pointer} : &i32;
// _3 = const {pointer} : &i32;
// _2 = const {pointer} : *const i32;
// ...
// _1 = move _2 as usize (Misc);
// ...
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/const_prop/ref_deref.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ fn main() {
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _2 = const Scalar(AllocId(0).0x0) : &i32;
// _2 = const {pointer} : &i32;
// _1 = const 4i32;
// ...
// }
Expand Down
4 changes: 2 additions & 2 deletions src/test/mir-opt/const_prop/slice_len.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ fn main() {
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _4 = const Scalar(AllocId(0).0x0) : &[u32; 3];
// _3 = const Scalar(AllocId(0).0x0) : &[u32; 3];
// _4 = const {pointer} : &[u32; 3];
// _3 = const {pointer} : &[u32; 3];
// _2 = move _3 as &[u32] (Pointer(Unsize));
// ...
// _6 = const 1usize;
Expand Down
26 changes: 26 additions & 0 deletions src/test/ui/const-generics/int-as-ref-const-param.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// normalize-stderr-32bit: "0x" -> "$$PREFIX"
// normalize-stderr-64bit: "0x00000000" -> "$$PREFIX"

#![feature(const_generics, const_compare_raw_pointers)]
//~^ WARN the feature `const_generics` is incomplete and may cause the compiler to crash

struct Const<const P: &'static ()>;

#[repr(C)]
union Transmuter {
pointer: *const (),
reference: &'static (),
}

fn main() {
const A: &'static () = {
unsafe { Transmuter { pointer: 10 as *const () }.reference }
};
const B: &'static () = {
unsafe { Transmuter { pointer: 11 as *const () }.reference }
};

let _: Const<{A}> = Const::<{B}>; //~ mismatched types
let _: Const<{A}> = Const::<{&()}>; //~ mismatched types
let _: Const<{A}> = Const::<{A}>;
}
29 changes: 29 additions & 0 deletions src/test/ui/const-generics/int-as-ref-const-param.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
warning: the feature `const_generics` is incomplete and may cause the compiler to crash
--> $DIR/int-as-ref-const-param.rs:4:12
|
LL | #![feature(const_generics, const_compare_raw_pointers)]
| ^^^^^^^^^^^^^^
|
= note: `#[warn(incomplete_features)]` on by default

error[E0308]: mismatched types
--> $DIR/int-as-ref-const-param.rs:23:25
|
LL | let _: Const<{A}> = Const::<{B}>;
| ^^^^^^^^^^^^ expected `$PREFIX0000000a : &()`, found `$PREFIX0000000b : &()`
|
= note: expected type `Const<$PREFIX0000000a : &()>`
found type `Const<$PREFIX0000000b : &()>`

error[E0308]: mismatched types
--> $DIR/int-as-ref-const-param.rs:24:25
|
LL | let _: Const<{A}> = Const::<{&()}>;
| ^^^^^^^^^^^^^^ expected `$PREFIX0000000a : &()`, found `{pointer} : &()`
|
= note: expected type `Const<$PREFIX0000000a : &()>`
found type `Const<{pointer} : &()>`
Copy link
Member

Choose a reason for hiding this comment

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

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?


error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0308`.
5 changes: 5 additions & 0 deletions src/test/ui/const-generics/raw-ptr-const-param.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
// normalize-stderr-32bit: "0x" -> "$$PREFIX"
// normalize-stderr-64bit: "0x00000000" -> "$$PREFIX"

#![feature(const_generics, const_compare_raw_pointers)]
//~^ WARN the feature `const_generics` is incomplete and may cause the compiler to crash

Expand All @@ -6,4 +9,6 @@ struct Const<const P: *const u32>;
fn main() {
let _: Const<{15 as *const _}> = Const::<{10 as *const _}>; //~ mismatched types
let _: Const<{10 as *const _}> = Const::<{10 as *const _}>;

let _: Const<{10 as *const _}> = Const::<{&8_u32 as *const _}>; //~ mismatched types
}
21 changes: 15 additions & 6 deletions src/test/ui/const-generics/raw-ptr-const-param.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,29 @@
warning: the feature `const_generics` is incomplete and may cause the compiler to crash
--> $DIR/raw-ptr-const-param.rs:1:12
--> $DIR/raw-ptr-const-param.rs:4:12
|
LL | #![feature(const_generics, const_compare_raw_pointers)]
| ^^^^^^^^^^^^^^
|
= note: `#[warn(incomplete_features)]` on by default

error[E0308]: mismatched types
--> $DIR/raw-ptr-const-param.rs:7:38
--> $DIR/raw-ptr-const-param.rs:10:38
|
LL | let _: Const<{15 as *const _}> = Const::<{10 as *const _}>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^ expected `{pointer}`, found `{pointer}`
| ^^^^^^^^^^^^^^^^^^^^^^^^^ expected `$PREFIX0000000f : *const u32`, found `$PREFIX0000000a : *const u32`
|
= note: expected type `Const<{pointer}>`
found type `Const<{pointer}>`
= note: expected type `Const<$PREFIX0000000f : *const u32>`
found type `Const<$PREFIX0000000a : *const u32>`

error: aborting due to previous error
error[E0308]: mismatched types
--> $DIR/raw-ptr-const-param.rs:13:38
|
LL | let _: Const<{10 as *const _}> = Const::<{&8_u32 as *const _}>;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `$PREFIX0000000a : *const u32`, found `{pointer} : *const u32`
|
= note: expected type `Const<$PREFIX0000000a : *const u32>`
found type `Const<{pointer} : *const u32>`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0308`.