FnAbi Compatability check

[geetanshjuneja]

Links to #3842

[geetanshjuneja]

I have made changes only in a single place to make sure that this what we want.

[RalfJung]

Thanks for the PR!

Judging from the CI failures, this does not seem to be quite right. And (as noted inline) I think this will never be able to do the "proper" check. So I don't think this is the right approach.

The right approach has to be somewhat similar to what happens for regular function calls in Miri. Basically, we need a helper function that takes a list of types (the expected argument types) and a return type, and then somehow checks that against the FnAbi that is passed around in the Miri shim handling. This likely involved constructing our own FnAbi to compare, and I have no idea how to do that...

Also, I would suggest to not touch intrinsics for now. They are somewhat special ABI-wise. Let's start with normal shims, i.e. all the things on foreign_items.

[geetanshjuneja]

The right approach has to be somewhat similar to what happens for regular function calls in Miri. Basically, we need a helper function that takes a list of types (the expected argument types) and a return type, and then somehow checks that against the FnAbi that is passed around in the Miri shim handling. This likely involved constructing our own FnAbi to compare, and I have no idea how to do that...

For non variadic fn's can't we just simply create FnAbi struct using the types available in docs. The issue only comes for variadic fn's as we dont know the types of the extra args.

[RalfJung]

Yes we "just" have to do that but it is not necessarily so easy. :)
But we can probably construct a fn ptr type corresponding to the desired signature, and then use fn_abi_of_fn_ptr.

[geetanshjuneja]

Yes we "just" have to do that but it is not necessarily so easy. :)

Why can't we directly instantiate an object of FnAbi struct?
Also are you talking about this fn_abi_of_fn_ptr ?

[RalfJung]

Why can't we instantiate an object of FnAbi struct?

That involves computing a PassMode, and we most definitely do not want to compute those ourselves. We want to use the logic in the Rust compiler for computing those.

Also are you talking about this fn_abi_of_fn_ptr ?

Yes. It can be invoked in Miri via this.fn_abi_of_fn_ptr (where this is an InterpCx).

[geetanshjuneja]

To resolve I was thinking of implementing the following things:

Create a fn check_fn_abi which would take a list of input types , an output type and Fnabi that is passed in the miri shim handling. This fn would create a FnSig using the args. Now we will create expected FnAbi using this fn sig and compare it with the input Fnsig. If it doesn't matches throw ub.
Now Transmute can be called safely inside shim.

[RalfJung]

That sounds like a good plan. There are methods defined in https://github.com/rust-lang/rust/blob/master/compiler/rustc_const_eval/src/interpret/call.rs that are probably useful but have to be made pub before they can be called from Miri.

[geetanshjuneja]

There are methods defined in https://github.com/rust-lang/rust/blob/master/compiler/rustc_const_eval/src/interpret/call.rs that are probably useful but have to be made pub before they can be called from Miri.

Are you talking about check_argument_compatmethod here? If yes now then I think we can compare the two FnAbi by iterating over the args of the two abis by calling check_argument_compat method.

Also inputs_and_output field of FnSig requires list as RawList<(), T> so I need to convert given input and output arg slice into RawList<(), T> using from_arena method?

[RalfJung]

Are you talking about check_argument_compatmethod here?

Yes.

Also inputs_and_output field of FnSig requires list as RawList<(), T> so I need to convert given input and output arg slice into RawList<(), T> using from_arena method?

You can use these methods: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/?search=mk_type_list.

[geetanshjuneja]

I am mostly done with the code now I just need check_argument_compat to be public.

[RalfJung]

Okay, please make a PR against the rustc repo to make that function public then. :)

[RalfJung]

Please don't do rustup in the middle of a regular PR. I did a regular rustup, so if you rebase your PR should pick that up. :)

[RalfJung]

@rustbot author
Please post @rustbot ready when the PR is ready for the next round of review.

[geetanshjuneja]

@rustbot ready

[RalfJung]

@rustbot author

[RalfJung]

Please also add a test.

[geetanshjuneja]

@rustbot ready

[geetanshjuneja]

@RalfJung plz review I have made the required changes.

[RalfJung]

Hey, have some patience. I do this mostly in my free time. You're in the queue, and I will get to your PR eventually. Other people have been patiently waiting a lot more than 3 days.

[oli-obk]

That's an odd trait bound, afaict the libcore impls are generic and not macro defined for only a limited number of lengths. Is this bound really necessary?

[geetanshjuneja]

I think we can remove this bound. Btw I just copied it from check_shim.

[oli-obk]

Ah, probably from the olden days when we only had impls for arrays of length 32