diff --git a/cranelift/codegen/src/inst_predicates.rs b/cranelift/codegen/src/inst_predicates.rs index 7be7af565780..cdaa5ebc2582 100644 --- a/cranelift/codegen/src/inst_predicates.rs +++ b/cranelift/codegen/src/inst_predicates.rs @@ -148,7 +148,7 @@ pub fn has_memory_fence_semantics(op: Opcode) -> bool { | Opcode::AtomicStore | Opcode::Fence | Opcode::Debugtrap => true, - Opcode::Call | Opcode::CallIndirect => true, + Opcode::Call | Opcode::CallIndirect | Opcode::TryCall | Opcode::TryCallIndirect => true, op if op.can_trap() => true, _ => false, } diff --git a/cranelift/codegen/src/ir/exception_table.rs b/cranelift/codegen/src/ir/exception_table.rs index a5a23c5fde5c..1e55f1d563e0 100644 --- a/cranelift/codegen/src/ir/exception_table.rs +++ b/cranelift/codegen/src/ir/exception_table.rs @@ -155,6 +155,12 @@ impl ExceptionTableData { pub fn signature(&self) -> SigRef { self.sig } + + /// Clears all entries in this exception table, but leaves the function signature. + pub fn clear(&mut self) { + self.tags.clear(); + self.targets.clear(); + } } /// A wrapper for the context required to display a diff --git a/cranelift/codegen/src/isa/aarch64/abi.rs b/cranelift/codegen/src/isa/aarch64/abi.rs index 178e44cd0fc9..2e0d29a1ce71 100644 --- a/cranelift/codegen/src/isa/aarch64/abi.rs +++ b/cranelift/codegen/src/isa/aarch64/abi.rs @@ -1127,7 +1127,7 @@ impl ABIMachineSpec for AArch64MachineDeps { fn get_regs_clobbered_by_call(call_conv: isa::CallConv, is_exception: bool) -> PRegSet { match call_conv { isa::CallConv::Winch => WINCH_CLOBBERS, - _ if is_exception => ALL_CLOBBERS, + isa::CallConv::Tail if is_exception => ALL_CLOBBERS, _ => DEFAULT_AAPCS_CLOBBERS, } } diff --git a/cranelift/codegen/src/isa/call_conv.rs b/cranelift/codegen/src/isa/call_conv.rs index cb869c7ca4f9..bbb681b9b92b 100644 --- a/cranelift/codegen/src/isa/call_conv.rs +++ b/cranelift/codegen/src/isa/call_conv.rs @@ -84,7 +84,7 @@ impl CallConv { /// Does this calling convention support exceptions? pub fn supports_exceptions(&self) -> bool { match self { - CallConv::Tail => true, + CallConv::Tail | CallConv::SystemV => true, _ => false, } } @@ -92,7 +92,7 @@ impl CallConv { /// What types do the exception payload value(s) have? pub fn exception_payload_types(&self, pointer_ty: Type) -> &[Type] { match self { - CallConv::Tail => match pointer_ty { + CallConv::Tail | CallConv::SystemV => match pointer_ty { types::I32 => &[types::I32, types::I32], types::I64 => &[types::I64, types::I64], _ => unreachable!(), diff --git a/cranelift/codegen/src/isa/riscv64/abi.rs b/cranelift/codegen/src/isa/riscv64/abi.rs index 5df160884887..d8362abc3cc0 100644 --- a/cranelift/codegen/src/isa/riscv64/abi.rs +++ b/cranelift/codegen/src/isa/riscv64/abi.rs @@ -639,13 +639,12 @@ impl ABIMachineSpec for Riscv64MachineDeps { } fn get_regs_clobbered_by_call( - _call_conv_of_callee: isa::CallConv, + call_conv_of_callee: isa::CallConv, is_exception: bool, ) -> PRegSet { - if is_exception { - ALL_CLOBBERS - } else { - DEFAULT_CLOBBERS + match call_conv_of_callee { + isa::CallConv::Tail if is_exception => ALL_CLOBBERS, + _ => DEFAULT_CLOBBERS, } } diff --git a/cranelift/codegen/src/isa/s390x/abi.rs b/cranelift/codegen/src/isa/s390x/abi.rs index 27076655608e..d761f1bbe1b8 100644 --- a/cranelift/codegen/src/isa/s390x/abi.rs +++ b/cranelift/codegen/src/isa/s390x/abi.rs @@ -907,7 +907,7 @@ impl ABIMachineSpec for S390xMachineDeps { is_exception: bool, ) -> PRegSet { match call_conv_of_callee { - _ if is_exception => ALL_CLOBBERS, + isa::CallConv::Tail if is_exception => ALL_CLOBBERS, isa::CallConv::Tail => TAIL_CLOBBERS, _ => SYSV_CLOBBERS, } diff --git a/cranelift/codegen/src/isa/x64/abi.rs b/cranelift/codegen/src/isa/x64/abi.rs index c6ed91cc4f86..7f657bc16ba3 100644 --- a/cranelift/codegen/src/isa/x64/abi.rs +++ b/cranelift/codegen/src/isa/x64/abi.rs @@ -914,7 +914,7 @@ impl ABIMachineSpec for X64ABIMachineSpec { match call_conv_of_callee { CallConv::Winch => ALL_CLOBBERS, CallConv::WindowsFastcall => WINDOWS_CLOBBERS, - _ if is_exception => ALL_CLOBBERS, + CallConv::Tail if is_exception => ALL_CLOBBERS, _ => SYSV_CLOBBERS, } } diff --git a/cranelift/codegen/src/unreachable_code.rs b/cranelift/codegen/src/unreachable_code.rs index 71827702201a..68d7bd33fdeb 100644 --- a/cranelift/codegen/src/unreachable_code.rs +++ b/cranelift/codegen/src/unreachable_code.rs @@ -22,11 +22,20 @@ pub fn eliminate_unreachable_code( let _tt = timing::unreachable_code(); let mut pos = FuncCursor::new(func); let mut used_tables = EntitySet::with_capacity(pos.func.stencil.dfg.jump_tables.len()); + let mut used_exception_tables = + EntitySet::with_capacity(pos.func.stencil.dfg.exception_tables.len()); while let Some(block) = pos.next_block() { if domtree.is_reachable(block) { let inst = pos.func.layout.last_inst(block).unwrap(); - if let ir::InstructionData::BranchTable { table, .. } = pos.func.dfg.insts[inst] { - used_tables.insert(table); + match pos.func.dfg.insts[inst] { + ir::InstructionData::BranchTable { table, .. } => { + used_tables.insert(table); + } + ir::InstructionData::TryCall { exception, .. } + | ir::InstructionData::TryCallIndirect { exception, .. } => { + used_exception_tables.insert(exception); + } + _ => (), } continue; } @@ -55,4 +64,10 @@ pub fn eliminate_unreachable_code( jt_data.clear(); } } + + for (exception, exception_data) in func.stencil.dfg.exception_tables.iter_mut() { + if !used_exception_tables.contains(exception) { + exception_data.clear(); + } + } } diff --git a/cranelift/codegen/src/verifier/mod.rs b/cranelift/codegen/src/verifier/mod.rs index 1e82d727bb1b..2450e802239d 100644 --- a/cranelift/codegen/src/verifier/mod.rs +++ b/cranelift/codegen/src/verifier/mod.rs @@ -1464,23 +1464,6 @@ impl<'a> Verifier<'a> { Ok(()) } - fn pointer_type_or_error(&self, inst: Inst, errors: &mut VerifierErrors) -> Result { - // Ensure we have an ISA so we know what the pointer size is. - if let Some(isa) = self.isa { - Ok(isa.pointer_type()) - } else { - errors - .fatal(( - inst, - self.context(inst), - format!("need an ISA to validate correct pointer type"), - )) - // Will always return an `Err`, but the `Ok` type - // doesn't match, so map it. - .map(|_| Type::default()) - } - } - fn typecheck_block_call( &self, inst: Inst, @@ -1504,7 +1487,9 @@ impl<'a> Verifier<'a> { )); } for (arg, param) in args.zip(block_params.iter()) { - let arg_ty = self.block_call_arg_ty(arg, inst, target_type, errors)?; + let Some(arg_ty) = self.block_call_arg_ty(arg, inst, target_type, errors)? else { + continue; + }; let param_ty = self.func.dfg.value_type(*param); if arg_ty != param_ty { errors.nonfatal(( @@ -1523,9 +1508,9 @@ impl<'a> Verifier<'a> { inst: Inst, target_type: BlockCallTargetType, errors: &mut VerifierErrors, - ) -> Result { + ) -> Result, ()> { match arg { - BlockArg::Value(v) => Ok(self.func.dfg.value_type(v)), + BlockArg::Value(v) => Ok(Some(self.func.dfg.value_type(v))), BlockArg::TryCallRet(_) | BlockArg::TryCallExn(_) => { // Get the invoked signature. let et = match self.func.dfg.insts[inst].exception_table() { @@ -1548,7 +1533,7 @@ impl<'a> Verifier<'a> { (BlockArg::TryCallRet(i), BlockCallTargetType::ExNormalRet) if (i as usize) < sig.returns.len() => { - Ok(sig.returns[i as usize].value_type) + Ok(Some(sig.returns[i as usize].value_type)) } (BlockArg::TryCallRet(_), BlockCallTargetType::ExNormalRet) => { errors.fatal(( @@ -1567,20 +1552,24 @@ impl<'a> Verifier<'a> { unreachable!() } (BlockArg::TryCallExn(i), BlockCallTargetType::Exception) => { - match sig - .call_conv - .exception_payload_types(self.pointer_type_or_error(inst, errors)?) - .get(i as usize) - { - Some(ty) => Ok(*ty), - None => { - errors.fatal(( - inst, - self.context(inst), - format!("out-of-bounds `exnN` block argument"), - ))?; - unreachable!() + if let Some(isa) = self.isa { + match sig + .call_conv + .exception_payload_types(isa.pointer_type()) + .get(i as usize) + { + Some(ty) => Ok(Some(*ty)), + None => { + errors.fatal(( + inst, + self.context(inst), + format!("out-of-bounds `exnN` block argument"), + ))?; + unreachable!() + } } + } else { + Ok(None) } } (BlockArg::TryCallExn(_), _) => { diff --git a/cranelift/filetests/filetests/egraph/try_call-mem-clobber.clif b/cranelift/filetests/filetests/egraph/try_call-mem-clobber.clif new file mode 100644 index 000000000000..91a235aa89a0 --- /dev/null +++ b/cranelift/filetests/filetests/egraph/try_call-mem-clobber.clif @@ -0,0 +1,25 @@ +; Regression test for the alias analysis not considering try_call to be a memory fence + +test optimize +set opt_level=speed_and_size +target x86_64 + +function u0:0(i64 sret, i64) system_v { + ss0 = explicit_slot 8 + sig0 = (i64) system_v + fn0 = u0:1 sig0 + +block0(v0: i64, v1: i64): + v20 = stack_addr.i64 ss0 + ; check: v20 = stack_addr.i64 ss0 + store v1, v20 ; store v1 to ss0 + try_call fn0(v20), sig0, block1, [] + +block1: + v21 = stack_addr.i64 ss0 + v2 = load.i64 v21; load v2 from ss0 after the fn0 call potentially changed it + ; check: v2 = load.i64 v20 + store v2, v0 ; v2 used to be incorrectly replaced by v1 in the egraph pass + ; nextln: store v2, v0 + return +} diff --git a/cranelift/filetests/filetests/verifier/exceptions.clif b/cranelift/filetests/filetests/verifier/exceptions.clif index 8378acda6abb..fb1b21fc1250 100644 --- a/cranelift/filetests/filetests/verifier/exceptions.clif +++ b/cranelift/filetests/filetests/verifier/exceptions.clif @@ -112,12 +112,12 @@ function %f4(i32) -> i32 { ;; Non-supported calling convention. function %f5(i32) -> i32 { - sig0 = (i32) -> f32 system_v - fn0 = %g(i32) -> f32 system_v + sig0 = (i32) -> f32 windows_fastcall + fn0 = %g(i32) -> f32 windows_fastcall block0(v1: i32): v2 = f64const 0x1.0 - try_call fn0(v1), sig0, block1(ret0, v2), [ tag1: block2(), default: block3(v2) ] ; error: calling convention `system_v` of callee does not support exceptions + try_call fn0(v1), sig0, block1(ret0, v2), [ tag1: block2(), default: block3(v2) ] ; error: calling convention `windows_fastcall` of callee does not support exceptions block1(v3: f32, v4: f64): v5 = iconst.i32 1 diff --git a/cranelift/frontend/src/frontend.rs b/cranelift/frontend/src/frontend.rs index bf59233aee29..f9083a162444 100644 --- a/cranelift/frontend/src/frontend.rs +++ b/cranelift/frontend/src/frontend.rs @@ -151,8 +151,8 @@ impl<'short, 'long> InstBuilderBase<'short> for FuncInstBuilder<'short, 'long> { ir::InstructionData::BranchTable { table, .. } => { let pool = &self.builder.func.dfg.value_lists; - // Unlike all other jumps/branches, jump tables are - // capable of having the same successor appear + // Unlike most other jumps/branches and like try_call, + // jump tables are capable of having the same successor appear // multiple times, so we must deduplicate. let mut unique = EntitySet::::new(); for dest_block in self @@ -179,7 +179,39 @@ impl<'short, 'long> InstBuilderBase<'short> for FuncInstBuilder<'short, 'long> { } } - inst => debug_assert!(!inst.opcode().is_branch()), + ir::InstructionData::TryCall { exception, .. } + | ir::InstructionData::TryCallIndirect { exception, .. } => { + let pool = &self.builder.func.dfg.value_lists; + + // Unlike most other jumps/branches and like br_table, + // exception tables are capable of having the same successor + // appear multiple times, so we must deduplicate. + let mut unique = EntitySet::::new(); + for dest_block in self + .builder + .func + .stencil + .dfg + .exception_tables + .get(*exception) + .expect("you are referencing an undeclared exception table") + .all_branches() + { + let block = dest_block.block(pool); + if !unique.insert(block) { + continue; + } + + // Call `declare_block_predecessor` instead of `declare_successor` for + // avoiding the borrow checker. + self.builder + .func_ctx + .ssa + .declare_block_predecessor(block, inst); + } + } + + inst => assert!(!inst.opcode().is_branch()), } if data.opcode().is_terminator() { @@ -1207,8 +1239,10 @@ mod tests { use alloc::string::ToString; use cranelift_codegen::entity::EntityRef; use cranelift_codegen::ir::condcodes::IntCC; - use cranelift_codegen::ir::{types::*, UserFuncName}; - use cranelift_codegen::ir::{AbiParam, Function, InstBuilder, MemFlags, Signature, Value}; + use cranelift_codegen::ir::{ + types::*, AbiParam, BlockCall, ExceptionTableData, ExtFuncData, ExternalName, Function, + InstBuilder, MemFlags, Signature, UserExternalName, UserFuncName, Value, + }; use cranelift_codegen::isa::{CallConv, TargetFrontendConfig, TargetIsa}; use cranelift_codegen::settings; use cranelift_codegen::verifier::verify_function; @@ -1969,6 +2003,96 @@ block0: block0: v0 = iconst.i32 -1 return +}", + ); + } + + #[test] + fn try_call() { + let mut sig = Signature::new(CallConv::SystemV); + sig.params.push(AbiParam::new(I8)); + sig.returns.push(AbiParam::new(I32)); + let mut fn_ctx = FunctionBuilderContext::new(); + let mut func = Function::with_name_signature(UserFuncName::testcase("sample"), sig); + + let sig0 = func.import_signature(Signature::new(CallConv::SystemV)); + let name = func.declare_imported_user_function(UserExternalName::new(0, 0)); + let fn0 = func.import_function(ExtFuncData { + name: ExternalName::User(name), + signature: sig0, + colocated: false, + }); + + let mut builder = FunctionBuilder::new(&mut func, &mut fn_ctx); + + let block0 = builder.create_block(); + let block1 = builder.create_block(); + let block2 = builder.create_block(); + let block3 = builder.create_block(); + + let my_var = Variable::from_u32(0); + builder.declare_var(my_var, I32); + + builder.switch_to_block(block0); + let branch_val = builder.append_block_param(block0, I8); + builder.ins().brif(branch_val, block1, &[], block2, &[]); + + builder.switch_to_block(block1); + let one = builder.ins().iconst(I32, 1); + builder.def_var(my_var, one); + + let normal_return = + BlockCall::new(block3, [].into_iter(), &mut builder.func.dfg.value_lists); + let exception_table = builder + .func + .dfg + .exception_tables + .push(ExceptionTableData::new(sig0, normal_return, [])); + builder.ins().try_call(fn0, &[], exception_table); + + builder.switch_to_block(block2); + let two = builder.ins().iconst(I32, 2); + builder.def_var(my_var, two); + + let normal_return = + BlockCall::new(block3, [].into_iter(), &mut builder.func.dfg.value_lists); + let exception_table = builder + .func + .dfg + .exception_tables + .push(ExceptionTableData::new(sig0, normal_return, [])); + builder.ins().try_call(fn0, &[], exception_table); + + builder.switch_to_block(block3); + let ret_val = builder.use_var(my_var); + builder.ins().return_(&[ret_val]); + + builder.seal_all_blocks(); + builder.finalize(); + + let flags = cranelift_codegen::settings::Flags::new(cranelift_codegen::settings::builder()); + let ctx = cranelift_codegen::Context::for_function(func); + ctx.verify(&flags).expect("should be valid"); + + check( + &ctx.func, + "function %sample(i8) -> i32 system_v { + sig0 = () system_v + fn0 = u0:0 sig0 + +block0(v0: i8): + brif v0, block1, block2 + +block1: + v1 = iconst.i32 1 + try_call fn0(), sig0, block3(v1), [] ; v1 = 1 + +block2: + v2 = iconst.i32 2 + try_call fn0(), sig0, block3(v2), [] ; v2 = 2 + +block3(v3: i32): + return v3 }", ); }