Skip to content

disable type validity assertions for better error messages #4434

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

Closed
wants to merge 1 commit into from
Closed
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
15 changes: 14 additions & 1 deletion src/intrinsics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
unwind: mir::UnwindAction,
) -> InterpResult<'tcx, Option<ty::Instance<'tcx>>> {
let this = self.eval_context_mut();
let intrinsic_name = this.tcx.item_name(instance.def_id());

// Force use of fallback body, if available.
if this.machine.force_intrinsic_fallback
Expand All @@ -41,11 +42,23 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
}));
}

// Overwrite some core engine intrinsics that are used to catch UB -- we have better
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we remove them from the core engine and add them to the ctfe intrinsic handler if miri doesn't want them?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's also an option, yeah. Maybe that's more clear.

So what should it be:

  • do this in the library?
  • or do it by neutering the intrinsic in Miri?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this variant as it keeps the checks in miri const eval as you said

Copy link
Member

Choose a reason for hiding this comment

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

Oh, because we don't do validation in normal const eval? So if we removed these checks for all interpreters, we'd miss even more UB in const eval?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, because we don't do validation in normal const eval? So if we removed these checks for all interpreters, we'd miss even more UB in const eval?

Exactly.

// errors if we detect this UB ourselves.
match intrinsic_name {
sym::assert_inhabited
| sym::assert_zero_valid
| sym::assert_mem_uninitialized_valid => {
// Make these a NOP.
this.return_to_block(ret)?;
return interp_ok(None);
}
_ => {}
}

// See if the core engine can handle this intrinsic.
if this.eval_intrinsic(instance, args, dest, ret)? {
return interp_ok(None);
}
let intrinsic_name = this.tcx.item_name(instance.def_id());
let intrinsic_name = intrinsic_name.as_str();

// FIXME: avoid allocating memory
Expand Down
7 changes: 1 addition & 6 deletions tests/fail/intrinsics/uninit_uninhabited_type.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
//@normalize-stderr-test: "\|.*::abort\(\).*" -> "| ABORT()"
//@normalize-stderr-test: "\| +\^+" -> "| ^"
//@normalize-stderr-test: "\n +[0-9]+:[^\n]+" -> ""
//@normalize-stderr-test: "\n +at [^\n]+" -> ""
//@error-in-other-file: aborted execution
#![feature(never_type)]

#[allow(deprecated, invalid_value)]
fn main() {
let _ = unsafe { std::mem::uninitialized::<!>() };
let _ = unsafe { std::mem::uninitialized::<!>() }; //~ERROR: constructing invalid value
}
28 changes: 7 additions & 21 deletions tests/fail/intrinsics/uninit_uninhabited_type.stderr
Original file line number Diff line number Diff line change
@@ -1,27 +1,13 @@

thread 'main' panicked at RUSTLIB/core/src/panicking.rs:LL:CC:
aborted execution: attempted to instantiate uninhabited type `!`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
note: in Miri, you may have to set `MIRIFLAGS=-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect
thread caused non-unwinding panic. aborting.
error: abnormal termination: the program aborted execution
--> RUSTLIB/std/src/sys/pal/PLATFORM/mod.rs:LL:CC
|
LL | ABORT()
| ^ abnormal termination occurred here
|
= note: BACKTRACE:
= note: inside `std::sys::pal::PLATFORM::abort_internal` at RUSTLIB/std/src/sys/pal/PLATFORM/mod.rs:LL:CC
= note: inside `std::panicking::rust_panic_with_hook` at RUSTLIB/std/src/panicking.rs:LL:CC
= note: inside closure at RUSTLIB/std/src/panicking.rs:LL:CC
= note: inside `std::sys::backtrace::__rust_end_short_backtrace::<{closure@std::panicking::begin_panic_handler::{closure#0}}, !>` at RUSTLIB/std/src/sys/backtrace.rs:LL:CC
= note: inside `std::panicking::begin_panic_handler` at RUSTLIB/std/src/panicking.rs:LL:CC
= note: inside `core::panicking::panic_nounwind` at RUSTLIB/core/src/panicking.rs:LL:CC
note: inside `main`
error: Undefined Behavior: constructing invalid value at .value: encountered a value of the never type `!`
--> tests/fail/intrinsics/uninit_uninhabited_type.rs:LL:CC
|
LL | let _ = unsafe { std::mem::uninitialized::<!>() };
| ^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: BACKTRACE:
= note: inside `main` at tests/fail/intrinsics/uninit_uninhabited_type.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

Expand Down
8 changes: 1 addition & 7 deletions tests/fail/intrinsics/zero_fn_ptr.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,4 @@
//@normalize-stderr-test: "\|.*::abort\(\).*" -> "| ABORT()"
//@normalize-stderr-test: "\| +\^+" -> "| ^"
//@normalize-stderr-test: "\n +[0-9]+:[^\n]+" -> ""
//@normalize-stderr-test: "\n +at [^\n]+" -> ""
//@error-in-other-file: aborted execution

#[allow(deprecated, invalid_value)]
fn main() {
let _ = unsafe { std::mem::zeroed::<fn()>() };
let _ = unsafe { std::mem::zeroed::<fn()>() }; //~ERROR: constructing invalid value
}
28 changes: 7 additions & 21 deletions tests/fail/intrinsics/zero_fn_ptr.stderr
Original file line number Diff line number Diff line change
@@ -1,27 +1,13 @@

thread 'main' panicked at RUSTLIB/core/src/panicking.rs:LL:CC:
aborted execution: attempted to zero-initialize type `fn()`, which is invalid
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
note: in Miri, you may have to set `MIRIFLAGS=-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect
thread caused non-unwinding panic. aborting.
error: abnormal termination: the program aborted execution
--> RUSTLIB/std/src/sys/pal/PLATFORM/mod.rs:LL:CC
|
LL | ABORT()
| ^ abnormal termination occurred here
|
= note: BACKTRACE:
= note: inside `std::sys::pal::PLATFORM::abort_internal` at RUSTLIB/std/src/sys/pal/PLATFORM/mod.rs:LL:CC
= note: inside `std::panicking::rust_panic_with_hook` at RUSTLIB/std/src/panicking.rs:LL:CC
= note: inside closure at RUSTLIB/std/src/panicking.rs:LL:CC
= note: inside `std::sys::backtrace::__rust_end_short_backtrace::<{closure@std::panicking::begin_panic_handler::{closure#0}}, !>` at RUSTLIB/std/src/sys/backtrace.rs:LL:CC
= note: inside `std::panicking::begin_panic_handler` at RUSTLIB/std/src/panicking.rs:LL:CC
= note: inside `core::panicking::panic_nounwind` at RUSTLIB/core/src/panicking.rs:LL:CC
note: inside `main`
error: Undefined Behavior: constructing invalid value at .value: encountered a null function pointer
--> tests/fail/intrinsics/zero_fn_ptr.rs:LL:CC
|
LL | let _ = unsafe { std::mem::zeroed::<fn()>() };
| ^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: BACKTRACE:
= note: inside `main` at tests/fail/intrinsics/zero_fn_ptr.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

Expand Down