From c5b7a9c4b5b06cab36b0e3914854d6ee4a600d46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Tue, 4 Mar 2025 20:30:42 +0100 Subject: [PATCH 1/3] Factor out edge breaking code --- .../src/add_call_guards.rs | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/compiler/rustc_mir_transform/src/add_call_guards.rs b/compiler/rustc_mir_transform/src/add_call_guards.rs index 55694cacd92e0..c64bb30fa2157 100644 --- a/compiler/rustc_mir_transform/src/add_call_guards.rs +++ b/compiler/rustc_mir_transform/src/add_call_guards.rs @@ -40,6 +40,16 @@ impl<'tcx> crate::MirPass<'tcx> for AddCallGuards { let mut new_blocks = Vec::new(); let cur_len = body.basic_blocks.len(); + let mut new_block = |source_info: SourceInfo, is_cleanup: bool, target: BasicBlock| { + let block = BasicBlockData { + statements: vec![], + is_cleanup, + terminator: Some(Terminator { source_info, kind: TerminatorKind::Goto { target } }), + }; + let idx = cur_len + new_blocks.len(); + new_blocks.push(block); + BasicBlock::new(idx) + }; for block in body.basic_blocks_mut() { match block.terminator { @@ -53,19 +63,7 @@ impl<'tcx> crate::MirPass<'tcx> for AddCallGuards { ) || self == &AllCallEdges) => { // It's a critical edge, break it - let call_guard = BasicBlockData { - statements: vec![], - is_cleanup: block.is_cleanup, - terminator: Some(Terminator { - source_info, - kind: TerminatorKind::Goto { target: *destination }, - }), - }; - - // Get the index it will be when inserted into the MIR - let idx = cur_len + new_blocks.len(); - new_blocks.push(call_guard); - *destination = BasicBlock::new(idx); + *destination = new_block(source_info, block.is_cleanup, *destination); } _ => {} } From 5c1733e4f4d51d7887a117a06ac4c465d55c3b8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Wed, 5 Mar 2025 13:14:08 +0100 Subject: [PATCH 2/3] Break critical edges in inline asm before code generation An inline asm terminator defines outputs along its target edges -- a fallthrough target and labeled targets. Code generation implements this by inserting code directly into the target blocks. This approach works only if the target blocks don't have other predecessors. Establish required invariant by extending existing code that breaks critical edges before code generation. --- .../src/add_call_guards.rs | 26 +++++++++++++ tests/codegen/asm/critical.rs | 37 +++++++++++++++++++ 2 files changed, 63 insertions(+) create mode 100644 tests/codegen/asm/critical.rs diff --git a/compiler/rustc_mir_transform/src/add_call_guards.rs b/compiler/rustc_mir_transform/src/add_call_guards.rs index c64bb30fa2157..ce7d7a435c844 100644 --- a/compiler/rustc_mir_transform/src/add_call_guards.rs +++ b/compiler/rustc_mir_transform/src/add_call_guards.rs @@ -65,6 +65,32 @@ impl<'tcx> crate::MirPass<'tcx> for AddCallGuards { // It's a critical edge, break it *destination = new_block(source_info, block.is_cleanup, *destination); } + Some(Terminator { + kind: + TerminatorKind::InlineAsm { + asm_macro: InlineAsmMacro::Asm, + ref mut targets, + ref operands, + unwind, + .. + }, + source_info, + }) if self == &CriticalCallEdges => { + let has_outputs = operands.iter().any(|op| { + matches!(op, InlineAsmOperand::InOut { .. } | InlineAsmOperand::Out { .. }) + }); + let has_labels = + operands.iter().any(|op| matches!(op, InlineAsmOperand::Label { .. })); + let invoke = + matches!(unwind, UnwindAction::Cleanup(_) | UnwindAction::Terminate(_)); + if has_outputs && (has_labels || invoke) { + for target in targets.iter_mut() { + if pred_count[*target] > 1 { + *target = new_block(source_info, block.is_cleanup, *target); + } + } + } + } _ => {} } } diff --git a/tests/codegen/asm/critical.rs b/tests/codegen/asm/critical.rs new file mode 100644 index 0000000000000..8c039900cab38 --- /dev/null +++ b/tests/codegen/asm/critical.rs @@ -0,0 +1,37 @@ +//@ only-x86_64 +//@ compile-flags: -C no-prepopulate-passes +#![feature(asm_goto)] +#![feature(asm_goto_with_outputs)] +#![crate_type = "lib"] +use std::arch::asm; + +// Regression test for #137867. Check that critical edges have been split before code generation, +// and so all stores to the asm output occur on disjoint paths without any of them jumping to +// another callbr label. +// +// CHECK-LABEL: @f( +// CHECK: [[OUT:%.*]] = callbr i32 asm +// CHECK-NEXT: to label %[[BB0:.*]] [label %[[BB1:.*]], label %[[BB2:.*]]], +// CHECK: [[BB1]]: +// CHECK-NEXT: store i32 [[OUT]], ptr %a +// CHECK-NEXT: br label %[[BBR:.*]] +// CHECK: [[BB2]]: +// CHECK-NEXT: store i32 [[OUT]], ptr %a +// CHECK-NEXT: br label %[[BBR]] +// CHECK: [[BB0]]: +// CHECK-NEXT: store i32 [[OUT]], ptr %a +// CHECK-NEXT: br label %[[BBR]] +// CHECK: [[BBR]]: +// CHECK-NEXT: [[RET:%.*]] = load i32, ptr %a +// CHECK-NEXT: ret i32 [[RET]] +#[unsafe(no_mangle)] +pub unsafe fn f(mut a: u32) -> u32 { + asm!( + "jmp {} + jmp {}", + label {}, + label {}, + inout("eax") a, + ); + a +} From 02d7fc167fea5013e2300b1c5a3ca75f7a048664 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Thu, 6 Mar 2025 00:00:00 +0000 Subject: [PATCH 3/3] Factor out check whether an unwind action generates invoke --- .../rustc_mir_transform/src/add_call_guards.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_mir_transform/src/add_call_guards.rs b/compiler/rustc_mir_transform/src/add_call_guards.rs index ce7d7a435c844..bc335cee14797 100644 --- a/compiler/rustc_mir_transform/src/add_call_guards.rs +++ b/compiler/rustc_mir_transform/src/add_call_guards.rs @@ -57,10 +57,7 @@ impl<'tcx> crate::MirPass<'tcx> for AddCallGuards { kind: TerminatorKind::Call { target: Some(ref mut destination), unwind, .. }, source_info, }) if pred_count[*destination] > 1 - && (matches!( - unwind, - UnwindAction::Cleanup(_) | UnwindAction::Terminate(_) - ) || self == &AllCallEdges) => + && (generates_invoke(unwind) || self == &AllCallEdges) => { // It's a critical edge, break it *destination = new_block(source_info, block.is_cleanup, *destination); @@ -81,9 +78,7 @@ impl<'tcx> crate::MirPass<'tcx> for AddCallGuards { }); let has_labels = operands.iter().any(|op| matches!(op, InlineAsmOperand::Label { .. })); - let invoke = - matches!(unwind, UnwindAction::Cleanup(_) | UnwindAction::Terminate(_)); - if has_outputs && (has_labels || invoke) { + if has_outputs && (has_labels || generates_invoke(unwind)) { for target in targets.iter_mut() { if pred_count[*target] > 1 { *target = new_block(source_info, block.is_cleanup, *target); @@ -104,3 +99,11 @@ impl<'tcx> crate::MirPass<'tcx> for AddCallGuards { true } } + +/// Returns true if this unwind action is code generated as an invoke as opposed to a call. +fn generates_invoke(unwind: UnwindAction) -> bool { + match unwind { + UnwindAction::Continue | UnwindAction::Unreachable => false, + UnwindAction::Cleanup(_) | UnwindAction::Terminate(_) => true, + } +}