-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Don't compute FnAbi for LLVM intrinsics #160077
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
base: main
Are you sure you want to change the base?
Changes from all commits
35f9fa2
707fca5
fd68ce3
e2873a0
e848978
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,8 @@ struct EvaluatedCalleeAndArgs<'tcx, M: Machine<'tcx>> { | |
| callee: FnVal<'tcx, M::ExtraFnVal>, | ||
| args: Vec<FnArg<'tcx, M::Provenance>>, | ||
| fn_sig: ty::FnSig<'tcx>, | ||
| fn_abi: &'tcx FnAbi<'tcx, Ty<'tcx>>, | ||
| /// None if LLVM intrinsic | ||
| fn_abi: Option<&'tcx FnAbi<'tcx, Ty<'tcx>>>, | ||
| /// True if the function is marked as `#[track_caller]` ([`ty::InstanceKind::requires_caller_location`]) | ||
| with_caller_location: bool, | ||
| } | ||
|
|
@@ -480,13 +481,17 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { | |
| ty::FnPtr(..) => { | ||
| let fn_ptr = self.read_pointer(&func)?; | ||
| let fn_val = self.get_ptr_fn(fn_ptr)?; | ||
| (fn_val, self.fn_abi_of_fn_ptr(fn_sig_binder, extra_args)?, false) | ||
| (fn_val, Some(self.fn_abi_of_fn_ptr(fn_sig_binder, extra_args)?), false) | ||
| } | ||
| ty::FnDef(def_id, args) => { | ||
| let instance = self.resolve(def_id, args.no_bound_vars().unwrap())?; | ||
| // Don't compute FnAbi for LLVM intrinsics. Trying to that would panic. | ||
| let has_fn_abi = !matches!(instance.def, ty::InstanceKind::LlvmIntrinsic(_)); | ||
| ( | ||
| FnVal::Instance(instance), | ||
| self.fn_abi_of_instance_no_deduced_attrs(instance, extra_args)?, | ||
| has_fn_abi | ||
| .then(|| self.fn_abi_of_instance_no_deduced_attrs(instance, extra_args)) | ||
| .transpose()?, | ||
| instance.def.requires_caller_location(*self.tcx), | ||
| ) | ||
| } | ||
|
|
@@ -557,15 +562,34 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { | |
| let EvaluatedCalleeAndArgs { callee, args, fn_sig, fn_abi, with_caller_location } = | ||
| self.eval_callee_and_args(terminator, func, args, &destination)?; | ||
|
|
||
| self.init_fn_call( | ||
| callee, | ||
| (fn_sig.abi(), fn_abi), | ||
| &args, | ||
| with_caller_location, | ||
| &dest_place, | ||
| target, | ||
| if fn_abi.can_unwind { unwind } else { mir::UnwindAction::Unreachable }, | ||
| )?; | ||
| match callee { | ||
| FnVal::Instance( | ||
| instance | ||
| @ ty::Instance { def: ty::InstanceKind::LlvmIntrinsic(_), args: _ }, | ||
| ) => { | ||
| // FIXME: Should `InPlace` arguments be reset to uninit? | ||
| M::call_llvm_intrinsic( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You can make |
||
| self, | ||
| instance, | ||
| &Self::copy_fn_args(&args), | ||
| &dest_place, | ||
| target, | ||
| )?; | ||
| } | ||
| _ => { | ||
| let fn_abi = fn_abi.expect("FnAbi should have been computed for this call"); | ||
| self.init_fn_call( | ||
| callee, | ||
| (fn_sig.abi(), fn_abi), | ||
| &args, | ||
| with_caller_location, | ||
| &dest_place, | ||
| target, | ||
| if fn_abi.can_unwind { unwind } else { mir::UnwindAction::Unreachable }, | ||
| )?; | ||
| } | ||
| }; | ||
|
|
||
| // Sanity-check that `eval_fn_call` either pushed a new frame or | ||
| // did a jump to another block. We disable the sanity check for functions that | ||
| // can't return, since Miri sometimes does have to keep the location the same | ||
|
|
@@ -581,6 +605,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> { | |
|
|
||
| let EvaluatedCalleeAndArgs { callee, args, fn_sig, fn_abi, with_caller_location } = | ||
| self.eval_callee_and_args(terminator, func, args, &mir::Place::return_place())?; | ||
| let fn_abi = fn_abi.expect("FnAbi should have been computed for this call"); | ||
|
|
||
| self.init_fn_tail_call( | ||
| callee, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -222,6 +222,9 @@ fn check_call_site_abi<'tcx>( | |||||||
| args.no_bound_vars().unwrap(), | ||||||||
| DUMMY_SP, | ||||||||
| ); | ||||||||
| if let InstanceKind::LlvmIntrinsic(..) = instance.def { | ||||||||
| return; | ||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
| } | ||||||||
| tcx.fn_abi_of_instance(typing_env.as_query_input((instance, ty::List::empty()))) | ||||||||
| } | ||||||||
| _ => { | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -433,7 +433,6 @@ fn fn_abi_sanity_check<'tcx>( | |
|
|
||
| fn fn_arg_sanity_check<'tcx>( | ||
| cx: &LayoutCx<'tcx>, | ||
| fn_abi: &FnAbi<'tcx, Ty<'tcx>>, | ||
| spec_abi: ExternAbi, | ||
| arg: &ArgAbi<'tcx, Ty<'tcx>>, | ||
| is_ret: bool, | ||
|
|
@@ -466,28 +465,17 @@ fn fn_abi_sanity_check<'tcx>( | |
| PassMode::Direct(attrs) => { | ||
| // Here the Rust type is used to determine the actual ABI, so we have to be very | ||
| // careful. Scalar/Vector is fine, since backends will generally use | ||
| // `layout.backend_repr` and ignore everything else. We should just reject | ||
| //`Aggregate` entirely here, but some targets need to be fixed first. | ||
| // `layout.backend_repr` and ignore everything else. | ||
| match arg.layout.backend_repr { | ||
| BackendRepr::Scalar(_) | ||
| | BackendRepr::SimdVector { .. } | ||
| | BackendRepr::SimdScalableVector { .. } => {} | ||
| BackendRepr::ScalarPair { .. } => { | ||
| panic!("`PassMode::Direct` used for ScalarPair type {}", arg.layout.ty) | ||
| } | ||
| BackendRepr::Memory { sized } => { | ||
| // For an unsized type we'd only pass the sized prefix, so there is no universe | ||
| // in which we ever want to allow this. | ||
| assert!(sized, "`PassMode::Direct` for unsized type in ABI: {:#?}", fn_abi); | ||
|
|
||
| // This really shouldn't happen even for sized aggregates, since | ||
| // `immediate_llvm_type` will use `layout.fields` to turn this Rust type into an | ||
| // LLVM type. This means all sorts of Rust type details leak into the ABI. | ||
| // The unadjusted ABI however uses Direct for all args. It is ill-specified, | ||
| // but unfortunately we need it for calling certain LLVM intrinsics. | ||
| assert!( | ||
| matches!(spec_abi, ExternAbi::Unadjusted), | ||
| "`PassMode::Direct` for aggregates only allowed for \"unadjusted\"\n\ | ||
| BackendRepr::Memory { .. } => { | ||
| panic!( | ||
| "`PassMode::Direct` for aggregates not allowed\n\ | ||
| Problematic type: {:#?}", | ||
| arg.layout, | ||
| ); | ||
|
|
@@ -539,9 +527,9 @@ fn fn_abi_sanity_check<'tcx>( | |
| } | ||
|
|
||
| for arg in fn_abi.args.iter() { | ||
| fn_arg_sanity_check(cx, fn_abi, spec_abi, arg, false); | ||
| fn_arg_sanity_check(cx, spec_abi, arg, false); | ||
| } | ||
| fn_arg_sanity_check(cx, fn_abi, spec_abi, &fn_abi.ret, true); | ||
| fn_arg_sanity_check(cx, spec_abi, &fn_abi.ret, true); | ||
| } | ||
|
|
||
| #[tracing::instrument( | ||
|
|
@@ -636,26 +624,13 @@ fn fn_abi_adjust_for_abi<'tcx>( | |
| fn_abi: &mut FnAbi<'tcx, Ty<'tcx>>, | ||
| abi: ExternAbi, | ||
| ) { | ||
| if abi == ExternAbi::Unadjusted { | ||
| // The "unadjusted" ABI passes aggregates in "direct" mode. That's fragile but needed for | ||
| // some LLVM intrinsics. | ||
| fn unadjust<'tcx>(arg: &mut ArgAbi<'tcx, Ty<'tcx>>) { | ||
| // This still uses `PassMode::Pair` for ScalarPair types. That's unlikely to be intended, | ||
| // but who knows what breaks if we change this now. | ||
| if matches!(arg.layout.backend_repr, BackendRepr::Memory { .. }) { | ||
| assert!( | ||
| arg.layout.backend_repr.is_sized(), | ||
| "'unadjusted' ABI does not support unsized arguments" | ||
| ); | ||
| } | ||
| arg.make_direct_deprecated(); | ||
| } | ||
| assert_ne!( | ||
| abi, | ||
| ExternAbi::Unadjusted, | ||
| "fn_abi_of_instance should not be called on LLVM intrinsics" | ||
| ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's kind of annoying that this panics and hence everyone has to carefully tiptoe around it. Can't we treat this more like Rust intrinsics where we can compute an ABI but it just doesn't matter? |
||
|
|
||
| unadjust(&mut fn_abi.ret); | ||
| for arg in fn_abi.args.iter_mut() { | ||
| unadjust(arg); | ||
| } | ||
| } else if abi.is_rustic_abi() { | ||
| if abi.is_rustic_abi() { | ||
| fn_abi.adjust_for_rust_abi(cx); | ||
| } else { | ||
| fn_abi.adjust_for_foreign_abi(cx, abi); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.