UB: edition 2015/18 with -Zmir-opt-level=2: destructing tuple inside closure

[matthiaskrgr]

I tried this code:

fn main() {
    let t = (String::from("Hello"), String::from("World"));

    let c = ||  {
        let (_, _) = t;
    };

    c();
}

You need edition 2015 or 2018 to reproduce this.

RUSTFLAGS="-Zmir-opt-level=2" ~/.cargo/bin/cargo miri run

error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
 --> src/main.rs:5:22
  |
5 |         let (_, _) = t;
  |                      ^ using uninitialized data, but this operation requires initialized memory
  |
  = 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 closure at src/main.rs:5:22: 5:23
note: inside `main`
 --> src/main.rs:8:5
  |
8 |     c();
  |     ^^^

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

error: aborting due to previous error; 1 warning emitted

  = 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:134:18: 134: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:490:40: 490: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:454:19: 454: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:140:14: 140: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:490:40: 490: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:454:19: 454: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:140:14: 140: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: aborting due to previous error; 1 warning emitted

[saethlin]

Looks to me like the problem is that DeadStoreElimination doesn't realize that a PlaceMention makes the place not dead.

--- a/mir_dump/scratch.main-{closure#0}.005-015.DeadStoreElimination.before.mir
+++ b/mir_dump/scratch.main-{closure#0}.005-015.DeadStoreElimination.after.mir
@@ -1,4 +1,4 @@
-// MIR for `main::{closure#0}` before DeadStoreElimination
+// MIR for `main::{closure#0}` after DeadStoreElimination
 
 fn main::{closure#0}(_1: &[closure@src/main.rs:4:13: 4:15]) -> () {
     debug t => (*((*_1).0: &(std::string::String, std::string::String))); // in scope 0 at src/main.rs:2:9: 2:10
@@ -9,7 +9,6 @@ fn main::{closure#0}(_1: &[closure@src/main.rs:4:13: 4:15]) -> () {
 
     bb0: {
         Retag([fn entry] _1);            // scope 0 at src/main.rs:4:13: 6:6
-        _2 = deref_copy ((*_1).0: &(std::string::String, std::string::String)); // scope 0 at src/main.rs:5:22: 5:23
         PlaceMention((*_2));             // scope 0 at src/main.rs:5:22: 5:23
         return;                          // scope 0 at src/main.rs:6:6: 6:6
     }

[JakobDegen]

The bug is here. PlaceMention is a nonuse, but should get the same treatment as a NonMutatingUse.

I feel obligated to mention that I have repeatedly warned that these bugs are going to happen. We should really stop using PlaceContext in optimizations. It was written for borrowck. (To be clear: I am as much at fault for this as anyone, I originally wrote this code).

[saethlin]

I was wondering why PlaceContext always has strangely unhelpful information for optimizations...

[RalfJung]

I feel obligated to mention that I have repeatedly warned that these bugs are going to happen. We should really stop using PlaceContext in optimizations. It was written for borrowck. (To be clear: I am as much at fault for this as anyone, I originally wrote this code).

Why is this not just a bug in PlaceContext assignment?

[JakobDegen]

See my comment in the PR - in general, my understanding is that the definition of placecontext is what it is because that's what makes sense in some other part of the compiler. If we start changing placecontext now to have the information that's useful to optimizations, I worry we'll just end up making those parts of the compiler buggy