Repository navigation
miri function argument passing should use FnAbi #56166
Description
Activity
It seems this issue has been open for a while, is it still relevant? If so, I would like to take a shot at it 😄.
I have started with the refactoring by moving the
pointee_info_atfunction toTyLayoutMethods. For now I still need to figure out how to translate some of the calls that were previously using the CodegenCx reference to work with the different context type available inTyLayoutMethods.
I'm quite confident I can get that to work, the only thing I'm struggling with at the moment are theMaybeResult<TyLayout>s I get when callinglayout_ofon the context. It's the first time I encounter this trait, and I am not quite sure how to work with it, given that I eventually want to end up with anOption<PointeeInfo>instead of aMaybeResult<PointeeInfo>.@eddyb do you have some pointers on this? I might also ask around on IRC. Perhaps I should change the signature to be
MaybeResult<PointeeInfo>?Figured out the situation with the
MaybeResult, will post another update whenpointee_info_athas been movedI have created a duplicate implementation of
pointee_info_atinTyLayoutMethodsas suggested by @eddyb (I will remove the original in a later commit). I think the method by itself is okay now, except that I want to move thetype_is_freezemethod insrc/librustc_codegen_ssa/common.rsto somewhere in therustccrate, right now I have simply copied to implementation intopointee_info_at, which is far from ideal. Any ideas on where to put that function @eddyb or @RalfJung?Current changes are in my fork, should I already turn it into a WIP pull request?
should I already turn it into a WIP pull request?
That's a good idea, because it makes commenting on the impl much easier
Reacted by Daan de Graaf@RalfJung
type_is_freezeactually calls out tois_freeze, providing theparam_envandspanarguments. The following two are equivalent:cx.type_is_freeze(ty) ty.is_freeze(cx, ParamEnv::reveal_all(), DUMMY_SPAN)
Instead of moving
type_is_freezeI could also change the occurrences to useis_freeze, how do you feel about that?should I already turn it into a WIP pull request?
That's a good idea, because it makes commenting on the impl much easier
Done!
Instead of moving type_is_freeze I could also change the occurrences to use is_freeze, how do you feel about that?
Depends on how often it gets called. But really I have very little experience in rustc outside miri, so I'll leave this to @eddyb.
- added a commit that references this issue
on May 5, 2019 @eddyb Regarding moving
FnType stufffromlibrustc_codegen_llvm/abi.rstolibrustc_target/abi/call/mod.rs:This is defined in
librustc_codegen_llvm/abi.rspub trait FnTypeExt<'tcx> { fn of_instance(cx: &CodegenCx<'ll, 'tcx>, instance: &ty::Instance<'tcx>) -> Self; fn new(cx: &CodegenCx<'ll, 'tcx>, sig: ty::FnSig<'tcx>, extra_args: &[Ty<'tcx>]) -> Self; fn new_vtable(cx: &CodegenCx<'ll, 'tcx>, sig: ty::FnSig<'tcx>, extra_args: &[Ty<'tcx>]) -> Self; fn new_internal( cx: &CodegenCx<'ll, 'tcx>, sig: ty::FnSig<'tcx>, extra_args: &[Ty<'tcx>], mk_arg_type: impl Fn(Ty<'tcx>, Option<usize>) -> ArgType<'tcx, Ty<'tcx>>, ) -> Self; fn adjust_for_abi(&mut self, cx: &CodegenCx<'ll, 'tcx>, abi: Abi); fn llvm_type(&self, cx: &CodegenCx<'ll, 'tcx>) -> &'ll Type; fn ptr_to_llvm_type(&self, cx: &CodegenCx<'ll, 'tcx>) -> &'ll Type; fn llvm_cconv(&self) -> llvm::CallConv; fn apply_attrs_llfn(&self, llfn: &'ll Value); fn apply_attrs_callsite(&self, bx: &mut Builder<'a, 'll, 'tcx>, callsite: &'ll Value); }
Looks like we need to move everything except
fn llvm_type(&self, cx: &CodegenCx<'ll, 'tcx>) -> &'ll Type; fn ptr_to_llvm_type(&self, cx: &CodegenCx<'ll, 'tcx>) -> &'ll Type; fn llvm_cconv(&self) -> llvm::CallConv; fn apply_attrs_llfn(&self, llfn: &'ll Value);
These methods seem to so some conversion from
FnTypeinto whatLLVMneeds.- added a commit that references this issue
on May 16, 2019 Also see what @eddyb wrote at rust-lang/miri#1038 (comment)
- changed the title
[-]miri function argument passing should use FnType[/-][+]miri function argument passing should use FnAbi[/+]on Jul 16, 2021 So... what would it take to use this in Miri? @eddyb @oli-obk and maybe @bjorn3
I assume I have to somehow get hold of anFnAbiaround hereand adjust the
eval_fn_callfunction to take such aFnAbiand use it -- though I am not entirely clear on how to use it either. Would this replace one of the existing arguments, or would this be a new argument? But if it's a new argument then nothing forceseval_fn_callto even use it.^^Is there code in our codegen backends that uses
FnAbithat Miri should basically follow?
In the miri engine, when evaluating a function call, it can happen that caller and callee do not agree on the type of an argument. In this case, the argument effectively gets transmuted from the caller type to the callee type. However, this is not legal for all types, and whether it is legal depends on horrible details of the ABI. This logic is implemented for codegen, where it is called
FnType. miri should probably use that same infrastructure.The relevant code in miri that would need changing is here. Currently, we only allow argument type punning for the Rust ABI, and we only allow the valid range of a
Scalar/ScalarPairto change -- effectively, we allow one side to have&Twhile the other side has*const T.For the
FnTypeside, I do not know much, but @eddyb offered to mentor someone trying to do this cleanup. :) He also left the following hints:Cc @oli-obk