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 executing code with errors #121508

Closed
matthiaskrgr opened this issue Feb 23, 2024 · 14 comments
Closed

Miri executing code with errors #121508

matthiaskrgr opened this issue Feb 23, 2024 · 14 comments
Assignees
Labels
A-miri Area: The miri tool C-bug Category: This is a bug.

Comments

@matthiaskrgr
Copy link
Member

struct Struct<T>(T);

impl<T> std::ops::Deref for Struct<T> {
    type Target = dyn Fn(T);
    fn deref(&self) -> &assert_mem_uninitialized_valid::Target {
        unimplemented!()
    }
}

fn main() {
    let f = Struct(Default::default());
    f(0);
    f(0);
}

MIRIFLAGS=-Zmiri-backtrace=full RUSTFLAGS="-Zmir-opt-level=0" ~/.cargo/bin/cargo miri run

Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done
WARNING: Ignoring `RUSTC_WRAPPER` environment variable, Miri does not support wrapping.
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.01s
     Running `/home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner target/miri/x86_64-unknown-linux-gnu/debug/mri`
error: post-monomorphization error: the type has an unknown layout
  --> src/main.rs:12:5
   |
12 |     f(0);
   |     ^ the type has an unknown layout
   |
   = note: inside `main` at src/main.rs:12:5: 12:6
   = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5: 250:71
   = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:155:18: 155:21
   = note: inside closure at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:166:18: 166:82
   = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:284:13: 284:31
   = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:555:40: 555:43
   = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:519:19: 519:81
   = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:142:14: 142:33
   = note: inside closure at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:148:48: 148:73
   = note: inside `std::panicking::r#try::do_call::<{closure@std::rt::lang_start_internal::{closure#2}}, isize>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:555:40: 555:43
   = note: inside `std::panicking::r#try::<isize, {closure@std::rt::lang_start_internal::{closure#2}}>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:519:19: 519:81
   = note: inside `std::panic::catch_unwind::<{closure@std::rt::lang_start_internal::{closure#2}}, isize>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:142:14: 142:33
   = note: inside `std::rt::lang_start_internal` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:148:20: 148:98
   = note: inside `std::rt::lang_start::<()>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:165:17: 170:6

error[E0433]: failed to resolve: use of undeclared crate or module `assert_mem_uninitialized_valid`
 --> src/main.rs:5:25
  |
5 |     fn deref(&self) -> &assert_mem_uninitialized_valid::Target {
  |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of undeclared crate or module `assert_mem_uninitialized_valid`

error: aborting due to 2 previous errors

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

with mir-opt-level 3, this reports UB:

MIRIFLAGS=-Zmiri-backtrace=full RUSTFLAGS="-Zmir-opt-level=3" ~/.cargo/bin/cargo miri run

Preparing a sysroot for Miri (target: x86_64-unknown-linux-gnu)... done
WARNING: Ignoring `RUSTC_WRAPPER` environment variable, Miri does not support wrapping.
   Compiling mri v0.1.0 (/tmp/mri)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.07s
     Running `/home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/bin/cargo-miri runner target/miri/x86_64-unknown-linux-gnu/debug/mri`
warning: You have explicitly enabled MIR optimizations, overriding Miri's default which is to completely disable them. Any optimizations may hide UB that Miri would otherwise detect, and it is not necessarily possible to predict what kind of UB will be missed. If you are enabling optimizations to make Miri run faster, we advise using cfg(miri) to shrink your workload instead. The performance benefit of enabling MIR optimizations is usually marginal at best.

error: Undefined Behavior: entering unreachable code
  --> src/main.rs:5:5
   |
5  |     fn deref(&self) -> &assert_mem_uninitialized_valid::Target {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ entering unreachable code
   |
   = 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 `<Struct<i32> as std::ops::Deref>::deref` at src/main.rs:5:5: 5:63
note: inside `main`
  --> src/main.rs:12:5
   |
12 |     f(0);
   |     ^
   = note: inside `<fn() as std::ops::FnOnce<()>>::call_once - shim(fn())` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5: 250:71
   = note: inside `std::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/sys_common/backtrace.rs:155:18: 155:21
   = note: inside closure at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:166:18: 166:82
   = note: inside `std::ops::function::impls::<impl std::ops::FnOnce<()> for &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>::call_once` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:284:13: 284:31
   = note: inside `std::panicking::r#try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:555:40: 555:43
   = note: inside `std::panicking::r#try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:519:19: 519:81
   = note: inside `std::panic::catch_unwind::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:142:14: 142:33
   = note: inside closure at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:148:48: 148:73
   = note: inside `std::panicking::r#try::do_call::<{closure@std::rt::lang_start_internal::{closure#2}}, isize>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:555:40: 555:43
   = note: inside `std::panicking::r#try::<isize, {closure@std::rt::lang_start_internal::{closure#2}}>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:519:19: 519:81
   = note: inside `std::panic::catch_unwind::<{closure@std::rt::lang_start_internal::{closure#2}}, isize>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:142:14: 142:33
   = note: inside `std::rt::lang_start_internal` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:148:20: 148:98
   = note: inside `std::rt::lang_start::<()>` at /home/matthias/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/rt.rs:165:17: 170:6

error[E0433]: failed to resolve: use of undeclared crate or module `assert_mem_uninitialized_valid`
 --> src/main.rs:5:25
  |
5 |     fn deref(&self) -> &assert_mem_uninitialized_valid::Target {
  |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of undeclared crate or module `assert_mem_uninitialized_valid`

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

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

which is interesting given that the code not even passes cargo check 🤔

error[E0433]: failed to resolve: use of undeclared crate or module `assert_mem_uninitialized_valid`
 --> src/main.rs:5:25
  |
5 |     fn deref(&self) -> &assert_mem_uninitialized_valid::Target {
  |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of undeclared crate or module `assert_mem_uninitialized_valid`

For more information about this error, try `rustc --explain E0433`.
error: could not compile `mri` (bin "mri") due to 1 previous error
@matthiaskrgr matthiaskrgr added C-bug Category: This is a bug. A-miri Area: The miri tool A-mir-opt Area: MIR optimizations labels Feb 23, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 23, 2024
@Noratrieb
Copy link
Member

Noratrieb commented Feb 23, 2024

lol, that is some, uh, wild error recovery for sure! @rust-lang/miri Miri should probably not execute code when there are errors :D

@Noratrieb Noratrieb removed A-mir-opt Area: MIR optimizations needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 23, 2024
@RalfJung
Copy link
Member

We're asking rustc if there are any errors, and apparently it says no.^^

@Noratrieb
Copy link
Member

💀

@matthiaskrgr
Copy link
Member Author

could be related to #121129 maybe?
There were a lot of changes to error handling recently 😅

@saethlin saethlin changed the title Undefined Behavior: entering unreachable code mit-opt level >2 Undefined Behavior: entering unreachable code mir-opt level >2 Feb 23, 2024
@Noratrieb Noratrieb changed the title Undefined Behavior: entering unreachable code mir-opt level >2 Miri executing code with errors Feb 23, 2024
@RalfJung
Copy link
Member

I would be surprised if it's that, we do have a test for not executing when there are errors and this passes. So maybe this is a different kind of error that evades that check? Not sure how though.

Still, Cc @nnethercote

@nnethercote
Copy link
Contributor

could be related to #121129 maybe? There were a lot of changes to error handling recently 😅

Unlikely to be #121129, but #121206 is a definite contender. It's Saturday morning here, I will take a look on Monday.

@nnethercote
Copy link
Contributor

We're asking rustc if there are any errors, and apparently it says no.^^

This is almost certainly going to be related to changes I've made in the handling of stashed diagnostics. I'm now at the point where I really hate stashed diagnostics. They used to be eagerly counted as errors, even though they could be cancelled, which was a huge soundness hole in how ErrorGuaranteed worked. So I fixed that in #120828. But now we have the opposite problem, where it's easy to forget about stashed diagnostics, and conclude that no errors have been issued when there is a stashed diagnostic pending. Argh.

@nnethercote nnethercote self-assigned this Feb 26, 2024
@nnethercote
Copy link
Contributor

I'm now heading in a direction where stashed errors are not allowed to be cancelled, but also counted as soon as they are stashed. I think that will be a good middle ground.

@Noratrieb
Copy link
Member

That makes sense to me. You can't cancel an emit()ted diagnostic either, and stash is just emit-but-maybe-modify-later.

@nnethercote
Copy link
Contributor

How can I build and run a local miri? I did x build src/tools/miri --stage 2 and tried to run the resulting binary but hit things involving sysroots and cargo miri and I don't know how to proceed.

@saethlin
Copy link
Member

You should x build miri cargo-miri then use cargo +stageN miri run.

Miri is not usable on its own like rustc is because the rustc sysroot is always bundled, the Miri one is not.

@nnethercote
Copy link
Contributor

Thanks! Is this written down somewhere?

@saethlin
Copy link
Member

Considering how much the dev guide slips here and there I'm pretty sure it's not. Most work on Miri is done out-of-tree and therefore doesn't go through bootstrap. So unfortunately the in-tree workflows don't get the attention they deserve.

@RalfJung
Copy link
Member

RalfJung commented Feb 27, 2024

You should x build miri cargo-miri then use cargo +stageN miri run.

I didn't even know that one. oO

You can run individual files with ./x.py miri --stage 0 run --args <filename>.

Where should this be documented?

nnethercote added a commit to nnethercote/rust that referenced this issue Feb 27, 2024
Stashed errors used to be counted as errors, but could then be
cancelled, leading to `ErrorGuaranteed` soundness holes. rust-lang#120828 changed
that, closing the soundness hole. But it introduced other difficulties
because you sometimes have to account for pending stashed errors when
making decisions about whether errors have occured/will occur and it's
easy to overlook these.

This commit aims for a middle ground.
- Stashed errors (not warnings) are counted immediately as emitted
  errors, avoiding the possibility of forgetting to consider them.
- The ability to cancel (or downgrade) stashed errors is eliminated, by
  disallowing the use of `steal_diagnostic` with errors, and introducing
  the more restrictive methods `try_steal_{modify,replace}_and_emit_err`
  that can be used instead.

Other things:
- `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both
  return `Option<ErrorGuaranteed>`, which enables the removal of two
  `delayed_bug` calls and one `Ty::new_error_with_message` call. This is
  possible because we store error guarantees in
  `DiagCtxt::stashed_diagnostics`.
- Storing the guarantees also saves us having to maintain a counter.
- Calls to the `stashed_err_count` method are no longer necessary
  alongside calls to `has_errors`, which is a nice simplification, and
  eliminates two more `span_delayed_bug` calls and one FIXME comment.
- Tests are added for three of the four fixed PRs mentioned below.
- `issue-121108.rs`'s output improved slightly, omitting a non-useful
  error message.

Fixes rust-lang#121451.
Fixes rust-lang#121477.
Fixes rust-lang#121504.
Fixes rust-lang#121508.
nnethercote added a commit to nnethercote/rust that referenced this issue Feb 27, 2024
Stashed errors used to be counted as errors, but could then be
cancelled, leading to `ErrorGuaranteed` soundness holes. rust-lang#120828 changed
that, closing the soundness hole. But it introduced other difficulties
because you sometimes have to account for pending stashed errors when
making decisions about whether errors have occured/will occur and it's
easy to overlook these.

This commit aims for a middle ground.
- Stashed errors (not warnings) are counted immediately as emitted
  errors, avoiding the possibility of forgetting to consider them.
- The ability to cancel (or downgrade) stashed errors is eliminated, by
  disallowing the use of `steal_diagnostic` with errors, and introducing
  the more restrictive methods `try_steal_{modify,replace}_and_emit_err`
  that can be used instead.

Other things:
- `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both
  return `Option<ErrorGuaranteed>`, which enables the removal of two
  `delayed_bug` calls and one `Ty::new_error_with_message` call. This is
  possible because we store error guarantees in
  `DiagCtxt::stashed_diagnostics`.
- Storing the guarantees also saves us having to maintain a counter.
- Calls to the `stashed_err_count` method are no longer necessary
  alongside calls to `has_errors`, which is a nice simplification, and
  eliminates two more `span_delayed_bug` calls and one FIXME comment.
- Tests are added for three of the four fixed PRs mentioned below.
- `issue-121108.rs`'s output improved slightly, omitting a non-useful
  error message.

Fixes rust-lang#121451.
Fixes rust-lang#121477.
Fixes rust-lang#121504.
Fixes rust-lang#121508.
nnethercote added a commit to nnethercote/rust that referenced this issue Feb 27, 2024
Stashed errors used to be counted as errors, but could then be
cancelled, leading to `ErrorGuaranteed` soundness holes. rust-lang#120828 changed
that, closing the soundness hole. But it introduced other difficulties
because you sometimes have to account for pending stashed errors when
making decisions about whether errors have occured/will occur and it's
easy to overlook these.

This commit aims for a middle ground.
- Stashed errors (not warnings) are counted immediately as emitted
  errors, avoiding the possibility of forgetting to consider them.
- The ability to cancel (or downgrade) stashed errors is eliminated, by
  disallowing the use of `steal_diagnostic` with errors, and introducing
  the more restrictive methods `try_steal_{modify,replace}_and_emit_err`
  that can be used instead.

Other things:
- `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both
  return `Option<ErrorGuaranteed>`, which enables the removal of two
  `delayed_bug` calls and one `Ty::new_error_with_message` call. This is
  possible because we store error guarantees in
  `DiagCtxt::stashed_diagnostics`.
- Storing the guarantees also saves us having to maintain a counter.
- Calls to the `stashed_err_count` method are no longer necessary
  alongside calls to `has_errors`, which is a nice simplification, and
  eliminates two more `span_delayed_bug` calls and one FIXME comment.
- Tests are added for three of the four fixed PRs mentioned below.
- `issue-121108.rs`'s output improved slightly, omitting a non-useful
  error message.

Fixes rust-lang#121451.
Fixes rust-lang#121477.
Fixes rust-lang#121504.
Fixes rust-lang#121508.
@bors bors closed this as completed in 260ae70 Feb 29, 2024
bors added a commit to rust-lang/miri that referenced this issue Feb 29, 2024
nnethercote added a commit to nnethercote/rust that referenced this issue Mar 1, 2024
Stashed errors used to be counted as errors, but could then be
cancelled, leading to `ErrorGuaranteed` soundness holes. rust-lang#120828 changed
that, closing the soundness hole. But it introduced other difficulties
because you sometimes have to account for pending stashed errors when
making decisions about whether errors have occured/will occur and it's
easy to overlook these.

This commit aims for a middle ground.
- Stashed errors (not warnings) are counted immediately as emitted
  errors, avoiding the possibility of forgetting to consider them.
- The ability to cancel (or downgrade) stashed errors is eliminated, by
  disallowing the use of `steal_diagnostic` with errors, and introducing
  the more restrictive methods `try_steal_{modify,replace}_and_emit_err`
  that can be used instead.

Other things:
- `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both
  return `Option<ErrorGuaranteed>`, which enables the removal of two
  `delayed_bug` calls and one `Ty::new_error_with_message` call. This is
  possible because we store error guarantees in
  `DiagCtxt::stashed_diagnostics`.
- Storing the guarantees also saves us having to maintain a counter.
- Calls to the `stashed_err_count` method are no longer necessary
  alongside calls to `has_errors`, which is a nice simplification, and
  eliminates two more `span_delayed_bug` calls and one FIXME comment.
- Tests are added for three of the four fixed PRs mentioned below.
- `issue-121108.rs`'s output improved slightly, omitting a non-useful
  error message.

Fixes rust-lang#121451.
Fixes rust-lang#121477.
Fixes rust-lang#121504.
Fixes rust-lang#121508.
RalfJung pushed a commit to RalfJung/rust that referenced this issue Mar 3, 2024
flip1995 pushed a commit to flip1995/rust that referenced this issue Mar 7, 2024
Stashed errors used to be counted as errors, but could then be
cancelled, leading to `ErrorGuaranteed` soundness holes. rust-lang#120828 changed
that, closing the soundness hole. But it introduced other difficulties
because you sometimes have to account for pending stashed errors when
making decisions about whether errors have occured/will occur and it's
easy to overlook these.

This commit aims for a middle ground.
- Stashed errors (not warnings) are counted immediately as emitted
  errors, avoiding the possibility of forgetting to consider them.
- The ability to cancel (or downgrade) stashed errors is eliminated, by
  disallowing the use of `steal_diagnostic` with errors, and introducing
  the more restrictive methods `try_steal_{modify,replace}_and_emit_err`
  that can be used instead.

Other things:
- `DiagnosticBuilder::stash` and `DiagCtxt::stash_diagnostic` now both
  return `Option<ErrorGuaranteed>`, which enables the removal of two
  `delayed_bug` calls and one `Ty::new_error_with_message` call. This is
  possible because we store error guarantees in
  `DiagCtxt::stashed_diagnostics`.
- Storing the guarantees also saves us having to maintain a counter.
- Calls to the `stashed_err_count` method are no longer necessary
  alongside calls to `has_errors`, which is a nice simplification, and
  eliminates two more `span_delayed_bug` calls and one FIXME comment.
- Tests are added for three of the four fixed PRs mentioned below.
- `issue-121108.rs`'s output improved slightly, omitting a non-useful
  error message.

Fixes rust-lang#121451.
Fixes rust-lang#121477.
Fixes rust-lang#121504.
Fixes rust-lang#121508.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-miri Area: The miri tool C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

6 participants