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

Move promoted MIR out of mir::Body #63580

Merged
merged 10 commits into from
Aug 26, 2019
Merged

Conversation

wesleywiser
Copy link
Member

@wesleywiser wesleywiser commented Aug 15, 2019

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2019
@rust-highfive

This comment has been minimized.

src/librustc_mir/borrow_check/nll/renumber.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/mod.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/mod.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/promote_consts.rs Show resolved Hide resolved
src/librustc_codegen_ssa/mir/place.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/inline.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/promote_consts.rs Outdated Show resolved Hide resolved
src/librustc_metadata/decoder.rs Outdated Show resolved Hide resolved
src/librustc_metadata/encoder.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2019

curious, why didn't the failing tests error before? They should have emitted the const_err warning (at least the code looks to me like it should).

Can you adjust the test accordingly?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2019

Awesome. This is ready now, right?

If so, please adjust the PR message

@wesleywiser
Copy link
Member Author

I believe so. I'm just waiting for the tests to pass.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2019

r=me once they do

@rust-highfive

This comment has been minimized.

@wesleywiser
Copy link
Member Author

wesleywiser commented Aug 17, 2019

I ran --bless to fix an off-by-one line number

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 17, 2019

Is that off by one swapping all the time? Maybe there's some ordering that is randomized via a hashmap iteration?

@wesleywiser
Copy link
Member Author

Is that off by one swapping all the time? Maybe there's some ordering that is randomized via a hashmap iteration?

I don't think so. I ran the test a bunch of times locally and it consistently failed. I think I just missed adding this change before committing.

@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 26, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Aug 26, 2019

📌 Commit d6bf776 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 26, 2019
@bors
Copy link
Contributor

bors commented Aug 26, 2019

⌛ Testing commit d6bf776 with merge 555d7a2...

bors added a commit that referenced this pull request Aug 26, 2019
@bors
Copy link
Contributor

bors commented Aug 26, 2019

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 555d7a2 to master...

@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #63580!

Tested on commit 555d7a2.
Direct link to PR: #63580

💔 miri on windows: test-fail → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-fail → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Aug 26, 2019
Tested on commit rust-lang/rust@555d7a2.
Direct link to PR: <rust-lang/rust#63580>

💔 miri on windows: test-fail → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-fail → build-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
src/librustc_metadata/decoder.rs Show resolved Hide resolved
src/librustc/mir/mod.rs Show resolved Hide resolved
src/librustc/mir/mod.rs Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Aug 27, 2019

Looks like this PR causes ICEs when running the Miri test suite with MIR optimizations. To reproduce, build Miri with a rustc including this PR, and run

RUST_BACKTRACE=1 ./miri run-debug tests/run-pass/hashmap.rs -Zmir-opt-level=2
thread 'rustc' panicked at 'index out of bounds: the len is 0 but the index is 0', /rustc/9b91b9c10e3c87ed333a1e34c4f46ed68f1eee06/src/libcore/slice/mod.rs:2715:10
Stacktrace
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.35/src/backtrace/libunwind.rs:88
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.35/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:47
   3: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:36
   4: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:200
   5: std::panicking::default_hook
             at src/libstd/panicking.rs:214
   6: rustc::util::common::panic_hook
   7: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:481
   8: std::panicking::continue_panic_fmt
             at src/libstd/panicking.rs:384
   9: rust_begin_unwind
             at src/libstd/panicking.rs:311
  10: core::panicking::panic_fmt
             at src/libcore/panicking.rs:85
  11: core::panicking::panic_bounds_check
             at src/libcore/panicking.rs:61
  12: rustc_mir::interpret::eval_context::InterpCx<M>::load_mir
  13: rustc_mir::const_eval::const_eval_raw_provider
  14: rustc::ty::query::__query_compute::const_eval_raw
  15: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors for rustc::ty::query::queries::const_eval_raw>::compute
  16: rustc::dep_graph::graph::DepGraph::with_task_impl
  17: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::get_query
  18: rustc_mir::const_eval::const_eval_raw_provider
  19: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors for rustc::ty::query::queries::const_eval_raw>::compute::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/query/plumbing.rs:999
  20: rustc::ty::query::__query_compute::const_eval_raw
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/query/plumbing.rs:950
  21: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors for rustc::ty::query::queries::const_eval_raw>::compute
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/query/plumbing.rs:991
  22: rustc::dep_graph::graph::DepGraph::with_task_impl
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/dep_graph/graph.rs:334
  23: rustc::dep_graph::graph::DepGraph::with_task
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/dep_graph/graph.rs:202
  24: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::force_query_with_job::{{closure}}::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/query/plumbing.rs:558
  25: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::start_query::{{closure}}::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/query/plumbing.rs:277
  26: rustc::ty::context::tls::enter_context::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1847
  27: rustc::ty::context::tls::set_tlv
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1780
  28: rustc::ty::context::tls::enter_context
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1846
  29: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::start_query::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/query/plumbing.rs:276
  30: rustc::ty::context::tls::with_related_context::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1953
  31: rustc::ty::context::tls::with_context::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1936
  32: rustc::ty::context::tls::with_context_opt
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1925
  33: rustc::ty::context::tls::with_context
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1936
  34: rustc::ty::context::tls::with_related_context
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1949
  35: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::start_query
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/query/plumbing.rs:265
  36: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::force_query_with_job::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/query/plumbing.rs:550
  37: rustc::ty::query::plumbing::with_diagnostics
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/query/plumbing.rs:210
  38: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::force_query_with_job
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/query/plumbing.rs:549
  39: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::get_query
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/query/plumbing.rs:378
  40: rustc::ty::query::TyCtxtAt::const_eval_raw
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/query/plumbing.rs:1076
  41: rustc_mir::interpret::eval_context::InterpCx<M>::const_eval_raw
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc_mir/interpret/eval_context.rs:672
  42: rustc_mir::interpret::place::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_static_to_mplace
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc_mir/interpret/place.rs:590
  43: rustc_mir::interpret::place::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_place::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc_mir/interpret/place.rs:660
  44: rustc::mir::Place::iterate_over
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/mir/mod.rs:1925
  45: rustc::mir::Place::iterate
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/mir/mod.rs:1892
  46: rustc_mir::interpret::place::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_place
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc_mir/interpret/place.rs:635
  47: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::eval_rvalue_into_place
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc_mir/interpret/step.rs:243
  48: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::statement
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc_mir/interpret/step.rs:85
  49: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::step
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc_mir/interpret/step.rs:61
  50: rustc_mir::interpret::step::<impl rustc_mir::interpret::eval_context::InterpCx<M>>::run
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc_mir/interpret/step.rs:40
  51: miri::eval::eval_main::{{closure}}
             at src/eval.rs:188
  52: miri::eval::eval_main
             at src/eval.rs:187
  53: <miri::MiriCompilerCalls as rustc_driver::Callbacks>::after_analysis::{{closure}}
             at src/bin/miri.rs:52
  54: rustc_interface::passes::BoxedGlobalCtxt::enter::{{closure}}::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc_interface/passes.rs:817
  55: rustc::ty::context::tls::enter_global::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1879
  56: rustc::ty::context::tls::enter_context::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1847
  57: rustc::ty::context::tls::set_tlv
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1780
  58: rustc::ty::context::tls::enter_context
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1846
  59: rustc::ty::context::tls::enter_global
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc/ty/context.rs:1878
  60: rustc_interface::passes::BoxedGlobalCtxt::enter::{{closure}}
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc_interface/passes.rs:817
  61: rustc_interface::passes::BoxedGlobalCtxt::access::{{closure}}
             at ./<::rustc_data_structures::box_region::declare_box_region_type macros>:21
  62: rustc_interface::passes::create_global_ctxt::{{closure}}
  63: alloc::boxed::<impl core::ops::generator::Generator for core::pin::Pin<alloc::boxed::Box<G>>>::resume
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/liballoc/boxed.rs:1073
  64: rustc_data_structures::box_region::PinnedGenerator<I,A,R>::access
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc_data_structures/box_region.rs:52
  65: rustc_interface::passes::BoxedGlobalCtxt::access
             at ./<::rustc_data_structures::box_region::declare_box_region_type macros>:24
  66: rustc_interface::passes::BoxedGlobalCtxt::enter
             at /rustc/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/src/librustc_interface/passes.rs:817
  67: <miri::MiriCompilerCalls as rustc_driver::Callbacks>::after_analysis
             at src/bin/miri.rs:45
  68: rustc_interface::interface::run_compiler_in_existing_thread_pool
  69: std::thread::local::LocalKey<T>::with
  70: scoped_tls::ScopedKey<T>::set
  71: syntax::with_globals
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
query stack during panic:
#0 [const_eval_raw] const-evaluating `main`
#1 [const_eval_raw] const-evaluating `main`
    --> /home/r/.rustup/toolchains/0444b9f66acb5da23dc816e0d8eb59623ba9ea50/lib/rustlib/src/rust/src/libstd/collections/hash/map.rs:2452:9
     |
2452 |         KEYS.with(|keys| {
     |         ^^^^
end of query stack

Centril added a commit to Centril/rust that referenced this pull request Aug 29, 2019
…ted, r=oli-obk

Resolve some small issues related to rust-lang#63580

This resolves some feedback left on rust-lang#63580 after it was merged:

- Adds documentation to `mir::Static` and `mir::StaticKind`
- Simplifies `maybe_get_optimized_mir()` and `maybe_get_promoted_mir()`

cc @bjorn3 @RalfJung
bors added a commit that referenced this pull request Aug 29, 2019
Rollup of 11 pull requests

Successful merges:

 - #63811 (Correctly suggest adding bounds to `impl Trait` argument)
 - #63933 (Resolve some small issues related to #63580)
 - #63938 (or-pattern: fix typo in error message)
 - #63945 (Recover `mut $pat` and other improvements)
 - #63958 (const_prop: only call error_to_const_error if we are actually showing something)
 - #63961 (Add Option<Span> to `require_lang_item`)
 - #63963 (remove the reference to __cxa_thread_atexit_impl)
 - #63965 (Prevent syntax error in LD linker version script)
 - #63968 (rustc_apfloat: make the crate #![no_std] explicitly.)
 - #63970 (Notify me (flip1995) when Clippy toolstate changes)
 - #63980 (add missing `#[repr(C)]` on the Slices union)

Failed merges:

 - #63989 (Add Yaah to clippy toolstain notification list)

r? @ghost
wesleywiser added a commit to wesleywiser/rust that referenced this pull request Sep 2, 2019
PR rust-lang#63580 broke miri's ability to run the run-pass test suite with MIR
optimizations enabled. The issue was that we weren't properly handling
the substs and DefId associated with a Promoted value. This didn't break
anything in rustc because in rustc this code runs before the Inliner
pass which is where the DefId and substs can diverge from their initial
values. It broke Miri though because it ran this code again after
running the optimization pass.
Centril added a commit to Centril/rust that referenced this pull request Sep 5, 2019
…oli-obk

Fix const eval bug breaking run-pass tests in Miri

PR rust-lang#63580 broke miri's ability to run the run-pass test suite with MIR
optimizations enabled. The issue was that we weren't properly handling
the substs and DefId associated with a Promoted value. This didn't break
anything in rustc because in rustc this code runs before the Inliner
pass which is where the DefId and substs can diverge from their initial
values. It broke Miri though because it ran this code again after
running the optimization pass.

r? @oli-obk
cc @RalfJung
Centril added a commit to Centril/rust that referenced this pull request Sep 5, 2019
…oli-obk

Fix const eval bug breaking run-pass tests in Miri

PR rust-lang#63580 broke miri's ability to run the run-pass test suite with MIR
optimizations enabled. The issue was that we weren't properly handling
the substs and DefId associated with a Promoted value. This didn't break
anything in rustc because in rustc this code runs before the Inliner
pass which is where the DefId and substs can diverge from their initial
values. It broke Miri though because it ran this code again after
running the optimization pass.

r? @oli-obk
cc @RalfJung
Centril added a commit to Centril/rust that referenced this pull request Sep 5, 2019
…oli-obk

Fix const eval bug breaking run-pass tests in Miri

PR rust-lang#63580 broke miri's ability to run the run-pass test suite with MIR
optimizations enabled. The issue was that we weren't properly handling
the substs and DefId associated with a Promoted value. This didn't break
anything in rustc because in rustc this code runs before the Inliner
pass which is where the DefId and substs can diverge from their initial
values. It broke Miri though because it ran this code again after
running the optimization pass.

r? @oli-obk
cc @RalfJung

//FIXME(oli-obk): having a `maybe_push()` method on `IndexVec` might be nice
if let Some(promoted) = promoter.promote_candidate(def_id, candidate, promotions.len()) {
promotions.push(promoted);
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure you can just use extend in such situations.

promoter.promote_candidate(candidate);

//FIXME(oli-obk): having a `maybe_push()` method on `IndexVec` might be nice
if let Some(promoted) = promoter.promote_candidate(def_id, candidate, promotions.len()) {
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to pass the IndexVec key and not an usize here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants