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]

Sorry for the long wait!

[geetanshjuneja]

Also do I need to add a test for return type mismatch?

[RalfJung]

@rustbot author

[geetanshjuneja]

@rustbot ready

[RalfJung]

Also do I need to add a test for return type mismatch?

Yes please do.

[RalfJung]

This looks great, thanks! Please squash the commits, then we can land this. Please use the --keep-base flag when squashing so that the force-push diff is easier to review. Then write @rustbot ready after you pushed the squashed PR.

@rustbot author

[geetanshjuneja]

Hmm on my macOS c_char is i8

[RalfJung]

Just use c_short instead of c_char.

[RalfJung]

Thanks for the PR and the patience. :)

[geetanshjuneja]

So the next step would be to use check_shim_abi for non variadic shims? If yes I can work on that as well

[RalfJung]

Yeah the next step is to use this for all the existing shims. Please don't do them all in one PR. :) Pick one file, e.g. src/shims/foreign_items.rs, and make a PR just for that. In the first pass, skip any that cause some extra complications.