Add MIR validation for unwind out from nounwind functions + fixes to make validation pass #113124

nbdd0121 · 2023-06-28T15:12:28Z

@Nilstrieb This is the MIR validation you asked in #112403 (comment).

Two passes need to be fixed to get the validation to pass:

RemoveNoopLandingPads currently unconditionally introduce a resume block (even there is none to begin with!), changed to not do that
Generator state transform introduces a assert which may unwind, and its drop elaboration also introduces many new UnwindActions, so in this case run the AbortUnwindingCalls after the transformation.

I believe this PR should also fix Rust-for-Linux/linux#1016, cc @ojeda

r? @Nilstrieb

rustbot · 2023-06-28T15:12:35Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

bors · 2023-07-07T09:24:43Z

☔ The latest upstream changes (presumably #113429) made this pull request unmergeable. Please resolve the merge conflicts.

compiler/rustc_const_eval/src/transform/validate.rs

Noratrieb · 2023-07-08T08:53:48Z

I'm not sure about the nop-landing-pads change
r? mir-opt

cjgillot · 2023-07-08T09:01:06Z

compiler/rustc_mir_transform/src/remove_noop_landing_pads.rs

-            resume_block
+        // Find the resume block
+        let Some(resume_block) = body.basic_blocks.iter_enumerated().find(|(_bb, block)| {
+            matches!(block.terminator().kind, TerminatorKind::Resume) && block.statements.is_empty()


Could we ever have a resume terminator with statements?

if yes, we may need to create an empty resume block for this pass;

if no, the block.statements.is_empty() would be better as an assertion + comment.

I think the answer is yes, and the empty resume block is indeed needed for this pass. I'll need to use a different way to skip the empty resume block creation for nounwind functions.

cjgillot · 2023-07-08T09:02:38Z

compiler/rustc_mir_transform/src/shim.rs

+                    &mut body,
+                    &[],
+                    Some(MirPhase::Runtime(RuntimePhase::Optimized)),
+                );


Should this be in generator.rs, where we construct this MIR? Where you run AbortUnwindingCalls.

Validator wants to know about layouts, and the layouts require the lowering of resume, so a cycle can be created if validation is performed in generator.rs.

compiler/rustc_mir_transform/src/generator.rs

nbdd0121 · 2023-07-08T17:38:55Z

compiler/rustc_mir_transform/src/generator.rs

+    // Update the body's def to become the drop glue.
+    let drop_in_place = tcx.require_lang_item(LangItem::DropInPlace, None);
+    body.source.instance = InstanceDef::DropGlue(drop_in_place, Some(gen_ty));


I added these lines so that AbortUnwindingCalls will use the ABI of drop_in_place instead of the resume function, but apparently this breaks the validation.

Currently body.source for the generator drop MIR contains that of the generator, which I don't think is correct, but I don't know what causes the validation to fail so I couldn't fix it.

What's the failure message?

It's failing in the CI.

##[error]error: internal compiler error: broken MIR in DropGlue(DefId(2:2150 ~ core[a05a]::ptr::drop_in_place), Some([async fn body@fake-test-src-base/higher-ranked/trait-bounds/normalize-under-binder/issue-80706.rs:57:5: 61:6])) (after phase change to runtime-optimized) at bb1[2]: Field projection `PlaceRef { local: _1, projection: [Deref, Downcast(None, 3)] }.0` specified type `std::pin::Pin<std::boxed::Box<dyn std::future::Future<Output = std::result::Result<<SaveUser<'_> as StorageRequestReturnType>::Output, <InMemoryStorage as Storage>::Error>>>>`, but actual type is `std::pin::Pin<std::boxed::Box<dyn std::future::Future<Output = std::result::Result<<SaveUser<'_> as StorageRequestReturnType>::Output, <S as Storage>::Error>>>>` --> fake-test-src-base/higher-ranked/trait-bounds/normalize-under-binder/issue-80706.rs:60:18 LL | .await; | ^ | = note: delayed at compiler/rustc_const_eval/src/transform/validate.rs:380:30 = note: delayed at compiler/rustc_const_eval/src/transform/validate.rs:380:30 0: <rustc_errors::HandlerInner>::emit_diagnostic 1: <rustc_errors::Handler>::delay_span_bug::<rustc_span::span_encoding::Span, alloc::string::String> 2: <rustc_const_eval::transform::validate::TypeChecker>::fail::<alloc::string::String> 3: <rustc_const_eval::transform::validate::TypeChecker as rustc_middle::mir::visit::Visitor>::visit_projection_elem 4: <rustc_const_eval::transform::validate::TypeChecker as rustc_middle::mir::visit::Visitor>::visit_place 5: <rustc_const_eval::transform::validate::TypeChecker as rustc_middle::mir::visit::Visitor>::visit_terminator 6: <rustc_const_eval::transform::validate::Validator as rustc_middle::mir::MirPass>::run_pass 8: rustc_mir_transform::shim::make_shim 9: rustc_query_impl::plumbing::__rust_begin_short_backtrace::<rustc_query_impl::query_impl::mir_shims::dynamic_query::{closure#2}::{closure#0}, rustc_middle::query::erase::Erased<[u8; 8]>> 9: rustc_query_impl::plumbing::__rust_begin_short_backtrace::<rustc_query_impl::query_impl::mir_shims::dynamic_query::{closure#2}::{closure#0}, rustc_middle::query::erase::Erased<[u8; 8]>> 10: <rustc_query_impl::query_impl::mir_shims::dynamic_query::{closure#2} as core::ops::function::FnOnce<(rustc_middle::ty::context::TyCtxt, rustc_middle::ty::instance::InstanceDef)>>::call_once 11: rustc_query_system::query::plumbing::try_execute_query::<rustc_query_impl::DynamicConfig<rustc_query_system::query::caches::DefaultCache<rustc_middle::ty::instance::InstanceDef, rustc_middle::query::erase::Erased<[u8; 8]>>, false, false, false>, rustc_query_impl::plumbing::QueryCtxt, false> 12: rustc_query_impl::query_impl::mir_shims::get_query_non_incr::__rust_end_short_backtrace 14: rustc_monomorphize::collector::collect_used_items 15: rustc_monomorphize::collector::collect_items_rec 16: rustc_monomorphize::collector::collect_items_rec 17: rustc_monomorphize::collector::collect_items_rec 17: rustc_monomorphize::collector::collect_items_rec 18: rustc_monomorphize::collector::collect_items_rec 19: <core::panic::unwind_safe::AssertUnwindSafe<rustc_data_structures::sync::par_for_each_in<alloc::vec::Vec<rustc_middle::mir::mono::MonoItem>, rustc_monomorphize::collector::collect_crate_mono_items::{closure#1}::{closure#0}>::{closure#0}::{closure#0}> as core::ops::function::FnOnce<()>>::call_once 20: std::panicking::try::<(), core::panic::unwind_safe::AssertUnwindSafe<rustc_data_structures::sync::par_for_each_in<alloc::vec::Vec<rustc_middle::mir::mono::MonoItem>, rustc_monomorphize::collector::collect_crate_mono_items::{closure#1}::{closure#0}>::{closure#0}::{closure#0}>> 21: rustc_data_structures::sync::par_for_each_in::<alloc::vec::Vec<rustc_middle::mir::mono::MonoItem>, rustc_monomorphize::collector::collect_crate_mono_items::{closure#1}::{closure#0}> 22: <rustc_session::session::Session>::time::<(), rustc_monomorphize::collector::collect_crate_mono_items::{closure#1}> 23: rustc_monomorphize::collector::collect_crate_mono_items 24: rustc_monomorphize::partitioning::collect_and_partition_mono_items 25: rustc_query_impl::plumbing::__rust_begin_short_backtrace::<rustc_query_impl::query_impl::collect_and_partition_mono_items::dynamic_query::{closure#2}::{closure#0}, rustc_middle::query::erase::Erased<[u8; 24]>> 26: <rustc_query_impl::query_impl::collect_and_partition_mono_items::dynamic_query::{closure#2} as core::ops::function::FnOnce<(rustc_middle::ty::context::TyCtxt, ())>>::call_once 27: rustc_query_system::query::plumbing::try_execute_query::<rustc_query_impl::DynamicConfig<rustc_query_system::query::caches::SingleCache<rustc_middle::query::erase::Erased<[u8; 24]>>, false, false, false>, rustc_query_impl::plumbing::QueryCtxt, false> 28: rustc_query_impl::query_impl::collect_and_partition_mono_items::get_query_non_incr::__rust_end_short_backtrace 29: <rustc_codegen_llvm::LlvmCodegenBackend as rustc_codegen_ssa::traits::backend::CodegenBackend>::codegen_crate 30: <rustc_session::session::Session>::time::<alloc::boxed::Box<dyn core::any::Any>, rustc_interface::passes::start_codegen::{closure#0}> 31: rustc_interface::passes::start_codegen 32: <rustc_middle::ty::context::GlobalCtxt>::enter::<<rustc_interface::queries::Queries>::ongoing_codegen::{closure#0}, core::result::Result<alloc::boxed::Box<dyn core::any::Any>, rustc_span::ErrorGuaranteed>> 33: <rustc_interface::queries::Queries>::ongoing_codegen 35: rustc_span::set_source_map::<core::result::Result<(), rustc_span::ErrorGuaranteed>, rustc_interface::interface::run_compiler<core::result::Result<(), rustc_span::ErrorGuaranteed>, rustc_driver_impl::run_compiler::{closure#1}>::{closure#0}::{closure#0}> 36: <scoped_tls::ScopedKey<rustc_span::SessionGlobals>>::set::<rustc_interface::interface::run_compiler<core::result::Result<(), rustc_span::ErrorGuaranteed>, rustc_driver_impl::run_compiler::{closure#1}>::{closure#0}, core::result::Result<(), rustc_span::ErrorGuaranteed>> 37: std::sys_common::backtrace::__rust_begin_short_backtrace::<rustc_interface::util::run_in_thread_pool_with_globals<rustc_interface::interface::run_compiler<core::result::Result<(), rustc_span::ErrorGuaranteed>, rustc_driver_impl::run_compiler::{closure#1}>::{closure#0}, core::result::Result<(), rustc_span::ErrorGuaranteed>>::{closure#0}::{closure#0}, core::result::Result<(), rustc_span::ErrorGuaranteed>> 38: std::panicking::try::<core::result::Result<(), rustc_span::ErrorGuaranteed>, core::panic::unwind_safe::AssertUnwindSafe<<std::thread::Builder>::spawn_unchecked_<rustc_interface::util::run_in_thread_pool_with_globals<rustc_interface::interface::run_compiler<core::result::Result<(), rustc_span::ErrorGuaranteed>, rustc_driver_impl::run_compiler::{closure#1}>::{closure#0}, core::result::Result<(), rustc_span::ErrorGuaranteed>>::{closure#0}::{closure#0}, core::result::Result<(), rustc_span::ErrorGuaranteed>>::{closure#1}::{closure#0}>> 38: std::panicking::try::<core::result::Result<(), rustc_span::ErrorGuaranteed>, core::panic::unwind_safe::AssertUnwindSafe<<std::thread::Builder>::spawn_unchecked_<rustc_interface::util::run_in_thread_pool_with_globals<rustc_interface::interface::run_compiler<core::result::Result<(), rustc_span::ErrorGuaranteed>, rustc_driver_impl::run_compiler::{closure#1}>::{closure#0}, core::result::Result<(), rustc_span::ErrorGuaranteed>>::{closure#0}::{closure#0}, core::result::Result<(), rustc_span::ErrorGuaranteed>>::{closure#1}::{closure#0}>> 39: <<std::thread::Builder>::spawn_unchecked_<rustc_interface::util::run_in_thread_pool_with_globals<rustc_interface::interface::run_compiler<core::result::Result<(), rustc_span::ErrorGuaranteed>, rustc_driver_impl::run_compiler::{closure#1}>::{closure#0}, core::result::Result<(), rustc_span::ErrorGuaranteed>>::{closure#0}::{closure#0}, core::result::Result<(), rustc_span::ErrorGuaranteed>>::{closure#1} as core::ops::function::FnOnce<()>>::call_once::{shim:vtable#0} 40: std::sys::unix::thread::Thread::new::thread_start 41: <unknown> 42: <unknown>

What am I supposed to do with this? remove the line and add a fixme?

This MIR body will exactly become the drop glue, so you can keep that line as-is.

Digged into the ICE and the failure is caused by validator not considering the subst of the generator when determining the type. Adding a ty::EarlyBinder::bind(...).instantiate(self.tcx, args) fix the issue.

bors · 2023-07-14T17:39:19Z

☔ The latest upstream changes (presumably #113591) made this pull request unmergeable. Please resolve the merge conflicts.

cjgillot · 2023-08-09T20:54:46Z

r=me after rebase

Otherwise the file name generated for generator_drop will become core.ptr-drop_in_place.[generator@<FILEPATH>_<NUMBERS>].generator_drop.0.mir instead of main-{closure#0}.generator_drop.0.mir which breaks a mir-opt test.

cjgillot · 2023-08-20T08:41:32Z

@bors r+

bors · 2023-08-20T08:41:34Z

📌 Commit 0a7202d has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-08-20T09:58:55Z

⌛ Testing commit 0a7202d with merge ff55fa3...

bors · 2023-08-20T11:48:31Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing ff55fa3 to master...

rust-timer · 2023-08-20T14:04:06Z

Finished benchmarking commit (ff55fa3): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-1.1%, -0.5%]	6
Improvements ✅ (secondary)	-0.3%	[-0.6%, -0.2%]	3
All ❌✅ (primary)	-0.7%	[-1.1%, -0.5%]	6

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.7%	[2.3%, 3.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	12
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Bootstrap: 637.108s -> 634.223s (-0.45%)
Artifact size: 346.98 MiB -> 346.98 MiB (0.00%)

Validation introduced in rust-lang#113124 allows UnwindAction::Continue and TerminatorKind::Resume to occur only in functions with ABI that can unwind. The function ABI depends on the panic strategy, which can vary across crates. Usually MIR is built and validated in the same crate. The coroutine drop glue thus far was an exception. As a result validation could fail when mixing different panic strategies. Avoid the problem by executing AbortUnwindingCalls along with the validation.

Fix coroutine validation for mixed panic strategy Validation introduced in rust-lang#113124 allows `UnwindAction::Continue` and `TerminatorKind::Resume` to occur only in functions with ABI that can unwind. The function ABI depends on the panic strategy, which can vary across crates. Usually MIR is built and validated in the same crate. The coroutine drop glue thus far was an exception. As a result validation could fail when mixing different panic strategies. Avoid the problem by executing `AbortUnwindingCalls` along with the validation. Fixes rust-lang#116953.

Rollup merge of rust-lang#118422 - tmiasko:mix, r=compiler-errors Fix coroutine validation for mixed panic strategy Validation introduced in rust-lang#113124 allows `UnwindAction::Continue` and `TerminatorKind::Resume` to occur only in functions with ABI that can unwind. The function ABI depends on the panic strategy, which can vary across crates. Usually MIR is built and validated in the same crate. The coroutine drop glue thus far was an exception. As a result validation could fail when mixing different panic strategies. Avoid the problem by executing `AbortUnwindingCalls` along with the validation. Fixes rust-lang#116953.

Validation introduced in rust-lang#113124 allows UnwindAction::Continue and TerminatorKind::Resume to occur only in functions with ABI that can unwind. The function ABI depends on the panic strategy, which can vary across crates. Usually MIR is built and validated in the same crate. The coroutine drop glue thus far was an exception. As a result validation could fail when mixing different panic strategies. Avoid the problem by executing AbortUnwindingCalls along with the validation.

Validation introduced in rust-lang#113124 allows UnwindAction::Continue and TerminatorKind::Resume to occur only in functions with ABI that can unwind. The function ABI depends on the panic strategy, which can vary across crates. Usually MIR is built and validated in the same crate. The coroutine drop glue thus far was an exception. As a result validation could fail when mixing different panic strategies. Avoid the problem by executing AbortUnwindingCalls along with the validation. (cherry picked from commit 5161b22)

rustbot assigned Noratrieb Jun 28, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 28, 2023

ojeda mentioned this pull request Jun 28, 2023

rustc wanted features & bugfixes Rust-for-Linux/linux#355

Open

50 tasks

Noratrieb reviewed Jul 8, 2023

View reviewed changes

compiler/rustc_const_eval/src/transform/validate.rs Outdated Show resolved Hide resolved

rustbot assigned wesleywiser and unassigned Noratrieb Jul 8, 2023

cjgillot reviewed Jul 8, 2023

View reviewed changes

cjgillot self-assigned this Jul 8, 2023

nbdd0121 force-pushed the eh_frame branch from f668179 to 6713544 Compare July 8, 2023 16:56

This comment has been minimized.

Sign in to view

nbdd0121 commented Jul 8, 2023

View reviewed changes

nbdd0121 added 4 commits August 18, 2023 13:51

Do not create new resume block if there isn't one already

cfbf1bf

Run AbortUnwindingCalls after generator transform

cec8e09

Add MIR validation for unwind out from nounwind functions

56b9337

Perform MIR validation on drop glue of generator

907e431

nbdd0121 force-pushed the eh_frame branch from 6713544 to 907e431 Compare August 18, 2023 13:36

This comment has been minimized.

Sign in to view

nbdd0121 added 2 commits August 18, 2023 16:40

Add missing instantiation of generator ty in validator

eb4d6d9

Change generator_drop's instance to that of generator for dump_mir

0a7202d

Otherwise the file name generated for generator_drop will become core.ptr-drop_in_place.[generator@<FILEPATH>_<NUMBERS>].generator_drop.0.mir instead of main-{closure#0}.generator_drop.0.mir which breaks a mir-opt test.

nbdd0121 force-pushed the eh_frame branch from 8647b2e to 0a7202d Compare August 18, 2023 15:40

cjgillot mentioned this pull request Aug 20, 2023

Don't do intra-pass validation on MIR shims #115005

Merged

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 20, 2023

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 20, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 20, 2023

bors merged commit ff55fa3 into rust-lang:master Aug 20, 2023
11 checks passed

rustbot added this to the 1.73.0 milestone Aug 20, 2023

This was referenced Aug 20, 2023

Enable -Zdrop-tracking-mir by default #107421

Merged

interpret/miri: call the panic_nounwind machinery the same way codegen does #114993

Merged

nbdd0121 deleted the eh_frame branch August 25, 2023 12:17

tmiasko mentioned this pull request Nov 28, 2023

Fix coroutine validation for mixed panic strategy #118422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add MIR validation for unwind out from nounwind functions + fixes to make validation pass #113124

Add MIR validation for unwind out from nounwind functions + fixes to make validation pass #113124

nbdd0121 commented Jun 28, 2023

rustbot commented Jun 28, 2023

bors commented Jul 7, 2023

Noratrieb commented Jul 8, 2023

cjgillot Jul 8, 2023

nbdd0121 Jul 8, 2023

cjgillot Jul 8, 2023

nbdd0121 Jul 8, 2023

This comment has been minimized.

nbdd0121 Jul 8, 2023

cjgillot Aug 4, 2023

nbdd0121 Aug 4, 2023

nbdd0121 Aug 10, 2023

cjgillot Aug 12, 2023

nbdd0121 Aug 18, 2023

bors commented Jul 14, 2023

cjgillot commented Aug 9, 2023

This comment has been minimized.

cjgillot commented Aug 20, 2023

bors commented Aug 20, 2023

bors commented Aug 20, 2023

bors commented Aug 20, 2023

rust-timer commented Aug 20, 2023

Add MIR validation for unwind out from nounwind functions + fixes to make validation pass #113124

Add MIR validation for unwind out from nounwind functions + fixes to make validation pass #113124

Conversation

nbdd0121 commented Jun 28, 2023

rustbot commented Jun 28, 2023

bors commented Jul 7, 2023

Noratrieb commented Jul 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jul 14, 2023

cjgillot commented Aug 9, 2023

This comment has been minimized.

cjgillot commented Aug 20, 2023

bors commented Aug 20, 2023

bors commented Aug 20, 2023

bors commented Aug 20, 2023

rust-timer commented Aug 20, 2023

Overall result: ✅ improvements - no action needed