MIR inlining doesn't work together with MIR validation

[arielb1]

STR

tested on commit 1410d56 on x86_64-unknown-linux-gnu

// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn main() { main2::<()>(); }

fn main2<T>() {
    let c = |_x| {};
    c(&mut 1);
}

$ rustc validate-fail.rs -Z mir-emit-validate=1 -Z mir-opt-level=2

Expected result

Code compiles

Actual result

ICE - I think the problem is that Validate is restricted to taking a local as an argument, which we can't expand to a closure upvar.

error: internal compiler error: rust://src/librustc_mir/transform/inline.rs:715: Arg operand `1` is (_4.0: &mut i32), not local

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.23.0-dev running on x86_64-unknown-linux-gnu

note: run with `RUST_BACKTRACE=1` for a backtrace

thread 'rustc' panicked at 'Box<Any>', rust://src/librustc_errors/lib.rs:485:8
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
             at rust://src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print
             at rust://src/libstd/sys_common/backtrace.rs:68
   2: std::panicking::default_hook::{{closure}}
             at rust://src/libstd/sys_common/backtrace.rs:57
             at rust://src/libstd/panicking.rs:381
   3: std::panicking::default_hook
             at rust://src/libstd/panicking.rs:391
   4: std::panicking::rust_panic_with_hook
             at rust://src/libstd/panicking.rs:577
   5: std::panicking::begin_panic
             at rust://src/libstd/panicking.rs:538
   6: rustc_errors::Handler::bug
             at rust://src/librustc_errors/lib.rs:485
   7: rustc::session::opt_span_bug_fmt::{{closure}}
             at rust://src/librustc/session/mod.rs:992
   8: rustc::session::opt_span_bug_fmt
             at rust://src/librustc/ty/context.rs:0
   9: rustc::session::bug_fmt
             at rust://src/librustc/session/mod.rs:972
  10: <rustc_mir::transform::inline::Integrator<'a, 'tcx> as rustc::mir::visit::MutVisitor<'tcx>>::visit_local
             at rust://src/librustc_mir/transform/inline.rs:715
  11: <rustc_mir::transform::inline::Integrator<'a, 'tcx> as rustc::mir::visit::MutVisitor<'tcx>>::visit_lvalue
             at rust://src/librustc/mir/visit.rs:0
  12: <rustc_mir::transform::inline::Integrator<'a, 'tcx> as rustc::mir::visit::MutVisitor<'tcx>>::visit_basic_block_data
             at rust://src/librustc/mir/visit.rs:361
             at rust://src/librustc/mir/visit.rs:105
             at rust://src/librustc/mir/visit.rs:320
             at rust://src/librustc_mir/transform/inline.rs:746
  13: rustc_mir::transform::inline::Inliner::run_pass
             at rust://src/librustc_mir/transform/inline.rs:486
             at rust://src/librustc_mir/transform/inline.rs:144
  14: <rustc_mir::transform::inline::Inline as rustc_mir::transform::MirPass>::run_pass
             at rust://src/librustc_mir/transform/inline.rs:55
  15: rustc_mir::transform::optimized_mir::{{closure}}::{{closure}}
             at rust://src/librustc_mir/transform/mod.rs:167
  16: rustc_mir::transform::optimized_mir::{{closure}}
             at rust://src/librustc_mir/transform/mod.rs:172
  17: rustc_mir::transform::optimized_mir
             at rust://src/librustc_mir/transform/mod.rs:226
  18: rustc::dep_graph::graph::DepGraph::with_task_impl
             at rust://src/librustc/ty/maps/plumbing.rs:370
             at rust://src/librustc/dep_graph/graph.rs:273
  19: rustc::ty::maps::<impl rustc::ty::maps::queries::optimized_mir<'tcx>>::force
             at rust://src/librustc/dep_graph/graph.rs:189
             at rust://src/librustc/ty/maps/plumbing.rs:452
             at rust://src/librustc_errors/lib.rs:548
             at rust://src/librustc/ty/maps/plumbing.rs:445
             at rust://src/librustc/ty/maps/plumbing.rs:115
             at rust://src/librustc/ty/maps/plumbing.rs:444
  20: rustc::ty::maps::<impl rustc::ty::maps::queries::optimized_mir<'tcx>>::try_get
             at rust://src/librustc/ty/maps/plumbing.rs:288
             at rust://src/librustc/ty/maps/plumbing.rs:486
  21: rustc_mir::transform::inline::Inliner::run_pass
             at rust://src/librustc_mir/transform/inline.rs:127
  22: <rustc_mir::transform::inline::Inline as rustc_mir::transform::MirPass>::run_pass
             at rust://src/librustc_mir/transform/inline.rs:55
  23: rustc_mir::transform::optimized_mir::{{closure}}::{{closure}}
             at rust://src/librustc_mir/transform/mod.rs:167
  24: rustc_mir::transform::optimized_mir::{{closure}}
             at rust://src/librustc_mir/transform/mod.rs:172
  25: rustc_mir::transform::optimized_mir
             at rust://src/librustc_mir/transform/mod.rs:226
  26: rustc::dep_graph::graph::DepGraph::with_task_impl
             at rust://src/librustc/ty/maps/plumbing.rs:370
             at rust://src/librustc/dep_graph/graph.rs:273
  27: rustc::ty::maps::<impl rustc::ty::maps::queries::optimized_mir<'tcx>>::force
             at rust://src/librustc/dep_graph/graph.rs:189
             at rust://src/librustc/ty/maps/plumbing.rs:452
             at rust://src/librustc_errors/lib.rs:548
             at rust://src/librustc/ty/maps/plumbing.rs:445
             at rust://src/librustc/ty/maps/plumbing.rs:115
             at rust://src/librustc/ty/maps/plumbing.rs:444
  28: rustc::ty::maps::<impl rustc::ty::maps::queries::optimized_mir<'tcx>>::try_get
             at rust://src/librustc/ty/maps/plumbing.rs:288
             at rust://src/librustc/ty/maps/plumbing.rs:486
  29: rustc::ty::maps::TyCtxtAt::optimized_mir
             at rust://src/librustc/ty/maps/plumbing.rs:525
  30: rustc::ty::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::instance_mir
             at rust://src/librustc/ty/maps/plumbing.rs:518
             at rust://src/librustc/ty/mod.rs:2288
  31: rustc_trans_utils::collector::collect_items_rec
             at rust://src/librustc_trans_utils/collector.rs:1057
             at rust://src/librustc_trans_utils/collector.rs:391
  32: rustc_trans_utils::collector::collect_crate_translation_items
             at rust://src/librustc_trans_utils/collector.rs:310
  33: rustc_trans::base::collect_and_partition_translation_items
             at rust://src/librustc_trans/base.rs:1147
             at rust://src/librustc/util/common.rs:120
             at rust://src/librustc_trans/base.rs:1146
  34: rustc::dep_graph::graph::DepGraph::with_task_impl
             at rust://src/librustc/ty/maps/plumbing.rs:370
             at rust://src/librustc/dep_graph/graph.rs:273
  35: rustc::ty::maps::<impl rustc::ty::maps::queries::collect_and_partition_translation_items<'tcx>>::force
             at rust://src/librustc/dep_graph/graph.rs:189
             at rust://src/librustc/ty/maps/plumbing.rs:452
             at rust://src/librustc_errors/lib.rs:548
             at rust://src/librustc/ty/maps/plumbing.rs:445
             at rust://src/librustc/ty/maps/plumbing.rs:115
             at rust://src/librustc/ty/maps/plumbing.rs:444
  36: rustc::ty::maps::<impl rustc::ty::maps::queries::collect_and_partition_translation_items<'tcx>>::try_get
             at rust://src/librustc/ty/maps/plumbing.rs:288
             at rust://src/librustc/ty/maps/plumbing.rs:486
  37: rustc::ty::maps::TyCtxtAt::collect_and_partition_translation_items
             at rust://src/librustc/ty/maps/plumbing.rs:525
  38: rustc::ty::maps::<impl rustc::ty::context::TyCtxt<'a, 'tcx, 'lcx>>::collect_and_partition_translation_items
             at rust://src/librustc/ty/maps/plumbing.rs:518
  39: rustc_trans::base::trans_crate
             at rust://src/librustc_trans/base.rs:885
  40: <rustc_trans::LlvmTransCrate as rustc_trans_utils::trans_crate::TransCrate>::trans_crate
             at rust://src/librustc_trans/lib.rs:183
  41: rustc_driver::driver::compile_input::{{closure}}
             at rust://src/librustc_driver/driver.rs:1106
             at rust://src/librustc/util/common.rs:120
             at rust://src/librustc_driver/driver.rs:1105
             at rust://src/librustc_driver/driver.rs:247
  42: rustc::ty::context::TyCtxt::create_and_enter
             at rust://src/librustc_driver/driver.rs:1089
             at rust://src/librustc/ty/context.rs:1522
             at rust://src/libstd/thread/local.rs:377
             at rust://src/libstd/thread/local.rs:288
             at rust://src/librustc/ty/context.rs:1519
             at rust://src/librustc/ty/context.rs:1506
             at rust://src/libstd/thread/local.rs:377
             at rust://src/libstd/thread/local.rs:288
             at rust://src/librustc/ty/context.rs:1503
             at rust://src/librustc/ty/context.rs:1138
  43: rustc_driver::driver::compile_input
             at rust://src/librustc_driver/driver.rs:1010
             at rust://src/librustc_driver/driver.rs:209
  44: rustc_driver::run_compiler
             at rust://src/librustc_driver/lib.rs:253

I suppose for now a solution could be to remove the offending Validate statements - I suppose MIRI always runs with MIR optimizations off. cc @RalfJung

[nikomatsakis]

This is just a bug in the inliner, as far as I can tell. It was mapping arguments to lvalues like foo.0 in the case of closures, but sometimes that's inconvenient (e.g., around StorageDead), where we really want to map a local to another local. I've got a fix that spills foo.0 into a new temporary.

[nikomatsakis]

Hmm, StorageDead is not very convincing. It may be that we could actually handle those paths without spilling. Let me poke a bit more.

[nikomatsakis]

OK, so, I'm a bit torn. On the one hand, I think we could get away without "unpacking" the tuple into locals. On the other hand, this code could be much cleaner if just do the unpacking (and then do some other simplifications).

[nikomatsakis]

Ah, ok, I now at least know how to characterize the failure. It occurs when an argument is referenced but not through an operand (such as in a borrow). In that case, the "rewriter" would fall through to visit_local, which demanded that the arg local maps to another local -- which was not the case for cosures, anyway, as we are mapping to tuple.0 etc.

[nikomatsakis]

Well, I ran out of time, so I pushed an "adequate" fix to #45879. This is neither the "better fix" (which I also have locally, actually) that avoids spilling locals and directly references tuple.0, nor is it the cleaner code (given that we require every local in the callee to a local in the caller, we could make a simple map and use it from visit_local and remove most of the methods in the Integrator, I believe). I'm sort of inclined to the simpler, obviously correct code -- let some after-the-fact optimizations cleanup the mess.

[oli-obk]

The relevant miri issue is rust-lang/miri#331

[RalfJung]

@oli-obk that's a very different issue.

If I understand this one here, the problem is that StorageDead takes a Local, not an Operand or an Lvalue. Since StorageDead predates MIR validation, TBH I feel this bug is incorrectly titled -- the validation statement takes Lvalues, so it should not be a problem, right?

AFAIK StorageDead does not make any sense for non-Locals, so I agree with @nikomatsakis that this is "just" a bug in the inliner.

What @oli-obk mentions should not lead to ICEs, but it means that miri cannot meaningfully run with both validation and inlining turned on.

[arielb1]

@RalfJung

The problem is that Validate take a local, not that StorageDead takes a local. Nope, this is a straight bug in inlining. It performs mapping in visit_operand rather than visit_lvalue.

[RalfJung]

Ah, so the problem is that it takes an Lvalue rather than an Operand, which makes it unlike all other statements.

[zmt00]

This appears to be fixed as of at least the most recent nightly.

[-@- rust]$ cat validate-fail.rs
fn main() { main2::<()>(); }
fn main2<T>() {
    let c = |_x| {};
    c(&mut 1);
}
[-@- rust]$ rustup run nightly rustc validate-fail.rs -Z mir-emit-validate=1 -Z mir-opt-level=2
[-@- rust]$ rustup run nightly rustc --version --verbose
rustc 1.28.0-nightly (524ad9b9e 2018-05-29)
binary: rustc
commit-hash: 524ad9b9e03656f3fdeb03ed82fe78db3916e566
commit-date: 2018-05-29
host: x86_64-unknown-linux-gnu
release: 1.28.0-nightly
LLVM version: 6.0

[oli-obk]

@RalfJung do we even care about validation being correct atm?

The crash is gone, but that might just mean validation statements got removed

[RalfJung]

Well... I somewhat care. But validation is indeed broken currently.

Also, AFAIK @eddyb (or someone else) changed the StorageDead type such that the original panic is now impossible -- StorageDead takes a Local now. So this bug can probably be closed.