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

Better pretty printing for const raw pointers #65859

Closed
wants to merge 10 commits into from
109 changes: 60 additions & 49 deletions src/librustc/ty/print/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -906,6 +906,52 @@ pub trait PrettyPrinter<'tcx>:

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

if let ty::Ref(_, ref_ty, _) = ty.kind {
// &str is a special case of references, and should override the general case, so we
// check for and handle that first.

let byte_str = match (ct, &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, &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, &ty.kind) {
(ConstValue::Scalar(Scalar::Raw { data, .. }), ty::Bool) =>
p!(write("{}", if data == 0 { "false" } else { "true" })),
Expand Down Expand Up @@ -943,7 +989,18 @@ 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
// &str should already have been handled, so this is a general representation of a
// reference.
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(ty));
}
(ConstValue::Scalar(Scalar::Ptr(ptr)), ty::FnPtr(_)) => {
let instance = {
let alloc_map = self.tcx().alloc_map.lock();
Expand All @@ -952,54 +1009,8 @@ pub trait PrettyPrinter<'tcx>:
p!(print_value_path(instance.def_id(), instance.substs));
},
_ => {
let printed = if let ty::Ref(_, ref_ty, _) = ty.kind {
let byte_str = match (ct, &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, &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), print(ty))
}
// fallback
p!(write("{:?} : ", ct), print(ty))
}
};
Ok(self)
Expand Down
4 changes: 2 additions & 2 deletions src/test/mir-opt/const_prop/const_prop_fails_gracefully.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ fn main() {
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _4 = const Scalar(AllocId(1).0x0) : &i32;
// _3 = const Scalar(AllocId(1).0x0) : &i32;
// _4 = const {pointer} : &i32;
// _3 = const {pointer} : &i32;
// _2 = const Value(Scalar(AllocId(1).0x0)) : *const i32;
Comment on lines +27 to 28
Copy link
Member

@RalfJung RalfJung Nov 16, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@iwikal iwikal Nov 16, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Good idea, thanks!

// ...
// _1 = move _2 as usize (Misc);
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/const_prop/ref_deref.rs
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
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
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
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
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
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`.