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

Miri interning: replace ICEs by proper errors #71665

Merged
merged 6 commits into from
May 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/librustc_mir/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ fn eval_body_using_ecx<'mir, 'tcx>(
intern_kind,
ret,
body.ignore_interior_mut_in_const_validation,
)?;
);

debug!("eval_body_using_ecx done: {:?}", *ret);
Ok(ret)
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/const_eval/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ pub(crate) fn const_caller_location(
let mut ecx = mk_eval_cx(tcx, DUMMY_SP, ty::ParamEnv::reveal_all(), false);

let loc_place = ecx.alloc_caller_location(file, line, col);
intern_const_alloc_recursive(&mut ecx, InternKind::Constant, loc_place, false).unwrap();
intern_const_alloc_recursive(&mut ecx, InternKind::Constant, loc_place, false);
ConstValue::Scalar(loc_place.ptr)
}

Expand Down
3 changes: 3 additions & 0 deletions src/librustc_mir/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -871,6 +871,9 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// Our result will later be validated anyway, and there seems no good reason
// to have to fail early here. This is also more consistent with
// `Memory::get_static_alloc` which has to use `const_eval_raw` to avoid cycles.
// FIXME: We can hit delay_span_bug if this is an invalid const, interning finds
// that problem, but we never run validation to show an error. Can we ensure
// this does not happen?
Copy link
Member Author

Choose a reason for hiding this comment

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

I still need to try and actually case the ICE here -- but my worry is that the delay_span_bug in interning all assume that validation will be run after interning is done, and this code here violates that assumption.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out we can get that to ICE even without Miri:

#![feature(const_transmute)]
#![warn(const_err)]

use std::mem;

const X: &i32 = unsafe {
    let x = 0;
    mem::transmute(&x)
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add a test for this in this PR or fix it in this PR?

Copy link
Member Author

@RalfJung RalfJung May 13, 2020

Choose a reason for hiding this comment

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

The PR does contain a test for the ICE outside Miri -- mostly accidentally, when I added allow(const_err) I did not realize I was testing an ICE. ;)

For whether this can ICE with Miri, I haven't done another attempt at triggering it (this one specifically is what I am worried about).

let val = self.tcx.const_eval_raw(param_env.and(gid))?;
self.raw_const_to_mplace(val)
}
Expand Down
330 changes: 177 additions & 153 deletions src/librustc_mir/interpret/intern.rs

Large diffs are not rendered by default.

3 changes: 1 addition & 2 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -702,8 +702,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
)) => l.is_bits() && r.is_bits(),
interpret::Operand::Indirect(_) if mir_opt_level >= 2 => {
let mplace = op.assert_mem_place(&self.ecx);
intern_const_alloc_recursive(&mut self.ecx, InternKind::ConstProp, mplace, false)
.expect("failed to intern alloc");
intern_const_alloc_recursive(&mut self.ecx, InternKind::ConstProp, mplace, false);
true
}
_ => false,
Expand Down
4 changes: 3 additions & 1 deletion src/test/ui/consts/dangling-alloc-id-ice.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
// https://github.com/rust-lang/rust/issues/55223
#![allow(const_err)]

union Foo<'a> {
y: &'a (),
long_live_the_unit: &'static (),
}

const FOO: &() = { //~ ERROR any use of this value will cause an error
const FOO: &() = { //~ ERROR it is undefined behavior to use this value
//~^ ERROR encountered dangling pointer in final constant
oli-obk marked this conversation as resolved.
Show resolved Hide resolved
let y = ();
unsafe { Foo { y: &y }.long_live_the_unit }
};
Expand Down
22 changes: 17 additions & 5 deletions src/test/ui/consts/dangling-alloc-id-ice.stderr
Original file line number Diff line number Diff line change
@@ -1,13 +1,25 @@
error: any use of this value will cause an error
--> $DIR/dangling-alloc-id-ice.rs:8:1
error: encountered dangling pointer in final constant
--> $DIR/dangling-alloc-id-ice.rs:9:1
|
LL | / const FOO: &() = {
LL | |
LL | | let y = ();
LL | | unsafe { Foo { y: &y }.long_live_the_unit }
LL | | };
| |__^ encountered dangling pointer in final constant
| |__^

error[E0080]: it is undefined behavior to use this value
--> $DIR/dangling-alloc-id-ice.rs:9:1
|
LL | / const FOO: &() = {
LL | |
LL | | let y = ();
LL | | unsafe { Foo { y: &y }.long_live_the_unit }
LL | | };
| |__^ type validation failed: encountered a dangling reference (use-after-free)
|
= note: `#[deny(const_err)]` on by default
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.

error: aborting due to previous error
error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0080`.
2 changes: 1 addition & 1 deletion src/test/ui/consts/dangling_raw_ptr.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const FOO: *const u32 = { //~ ERROR any use of this value will cause an error
const FOO: *const u32 = { //~ ERROR encountered dangling pointer in final constant
let x = 42;
&x
};
Expand Down
6 changes: 2 additions & 4 deletions src/test/ui/consts/dangling_raw_ptr.stderr
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
error: any use of this value will cause an error
error: encountered dangling pointer in final constant
--> $DIR/dangling_raw_ptr.rs:1:1
|
LL | / const FOO: *const u32 = {
LL | | let x = 42;
LL | | &x
LL | | };
| |__^ encountered dangling pointer in final constant
|
= note: `#[deny(const_err)]` on by default
| |__^

error: aborting due to previous error

25 changes: 0 additions & 25 deletions src/test/ui/consts/miri_unleashed/mutable_const.rs

This file was deleted.

39 changes: 0 additions & 39 deletions src/test/ui/consts/miri_unleashed/mutable_const.stderr

This file was deleted.

16 changes: 0 additions & 16 deletions src/test/ui/consts/miri_unleashed/mutable_const2.rs

This file was deleted.

29 changes: 0 additions & 29 deletions src/test/ui/consts/miri_unleashed/mutable_const2.stderr

This file was deleted.

3 changes: 1 addition & 2 deletions src/test/ui/consts/miri_unleashed/mutable_references.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,11 @@ struct Foo<T>(T);
// this is fine for the same reason as `BAR`.
static BOO: &mut Foo<()> = &mut Foo(());

// interior mutability is fine
struct Meh {
x: &'static UnsafeCell<i32>,
}

unsafe impl Sync for Meh {}

static MEH: Meh = Meh {
x: &UnsafeCell::new(42),
};
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/consts/miri_unleashed/mutable_references.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0594]: cannot assign to `*OH_YES`, as `OH_YES` is an immutable static item
--> $DIR/mutable_references.rs:37:5
--> $DIR/mutable_references.rs:36:5
|
LL | *OH_YES = 99;
| ^^^^^^^^^^^^ cannot assign
Expand All @@ -22,12 +22,12 @@ help: skipping check for `const_mut_refs` feature
LL | static BOO: &mut Foo<()> = &mut Foo(());
| ^^^^^^^^^^^^
help: skipping check that does not even have a feature gate
--> $DIR/mutable_references.rs:27:8
--> $DIR/mutable_references.rs:26:8
|
LL | x: &UnsafeCell::new(42),
| ^^^^^^^^^^^^^^^^^^^^
help: skipping check for `const_mut_refs` feature
--> $DIR/mutable_references.rs:31:27
--> $DIR/mutable_references.rs:30:27
|
LL | static OH_YES: &mut i32 = &mut 42;
| ^^^^^^^
Expand Down
37 changes: 37 additions & 0 deletions src/test/ui/consts/miri_unleashed/mutable_references_err.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// compile-flags: -Zunleash-the-miri-inside-of-you

#![allow(const_err)]

use std::cell::UnsafeCell;

// this test ensures that our mutability story is sound

struct Meh {
x: &'static UnsafeCell<i32>,
}
unsafe impl Sync for Meh {}

// the following will never be ok! no interior mut behind consts, because
// all allocs interned here will be marked immutable.
const MUH: Meh = Meh { //~ ERROR: mutable memory (`UnsafeCell`) is not allowed in constant
x: &UnsafeCell::new(42),
};

struct Synced {
x: UnsafeCell<i32>,
}
unsafe impl Sync for Synced {}

// Make sure we also catch this behind a type-erased `dyn Trait` reference.
const SNEAKY: &dyn Sync = &Synced { x: UnsafeCell::new(42) };
//~^ ERROR: mutable memory (`UnsafeCell`) is not allowed in constant

// Make sure we also catch mutable references.
const BLUNT: &mut i32 = &mut 42;
//~^ ERROR: mutable memory (`&mut`) is not allowed in constant

fn main() {
unsafe {
*MUH.x.get() = 99;
}
}
40 changes: 40 additions & 0 deletions src/test/ui/consts/miri_unleashed/mutable_references_err.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
error: mutable memory (`UnsafeCell`) is not allowed in constant
--> $DIR/mutable_references_err.rs:16:1
|
LL | / const MUH: Meh = Meh {
LL | | x: &UnsafeCell::new(42),
LL | | };
| |__^

error: mutable memory (`UnsafeCell`) is not allowed in constant
--> $DIR/mutable_references_err.rs:26:1
|
LL | const SNEAKY: &dyn Sync = &Synced { x: UnsafeCell::new(42) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: mutable memory (`&mut`) is not allowed in constant
--> $DIR/mutable_references_err.rs:30:1
|
LL | const BLUNT: &mut i32 = &mut 42;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: skipping const checks
|
help: skipping check that does not even have a feature gate
--> $DIR/mutable_references_err.rs:17:8
|
LL | x: &UnsafeCell::new(42),
| ^^^^^^^^^^^^^^^^^^^^
help: skipping check that does not even have a feature gate
--> $DIR/mutable_references_err.rs:26:27
|
LL | const SNEAKY: &dyn Sync = &Synced { x: UnsafeCell::new(42) };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: skipping check for `const_mut_refs` feature
--> $DIR/mutable_references_err.rs:30:25
|
LL | const BLUNT: &mut i32 = &mut 42;
| ^^^^^^^

error: aborting due to 3 previous errors; 1 warning emitted

29 changes: 0 additions & 29 deletions src/test/ui/consts/miri_unleashed/mutable_references_ice.rs

This file was deleted.

25 changes: 0 additions & 25 deletions src/test/ui/consts/miri_unleashed/mutable_references_ice.stderr

This file was deleted.

Loading