native-lib: allow passing structs as arguments by nia-e · Pull Request #4466 · rust-lang/miri

nia-e · 2025-07-14T01:42:12Z

~~Blocked on #4456 currently to make rebasing easier when that lands.~~ This allows passing arbitrary structs as arguments to native code; receiving a struct as a return value is not yet implemented. Also includes a couple of tests.

I figured the review queue looked a bit too empty :D

nia-e · 2025-07-14T02:07:08Z

+
+fn main() {
+    let pass_me = PassMe { value: 42, other_value: 1337 };
+    unsafe { pass_struct(pass_me) }; //~ ERROR: Undefined Behavior: passing a non-#[repr(C)] struct over FFI


Is this correct? I think it's UB, but maybe if the user intentionally sets #[allow(improper_ctypes)] we might just want to report this as unsupported instead of UB?

Yeah, reporting this as "unsupported" is better.

nia-e · 2025-07-16T09:43:04Z

Think this is ready for review. I split off a bunch of the new code this introduces into a different file to not pollute native_lib/mod.rs too much more than it already is

@rustbot ready

nia-e · 2025-07-17T10:55:08Z

Seems like there's some trouble here, nvm.

@rustbot author

rustbot · 2025-07-17T10:55:12Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

nia-e · 2025-07-17T17:58:21Z

Should be fixed. Also bumped the libffi version since apparently there was a soundness issue that got resolved since the previous version we were on; that wasn't the cause of my test failures, but it's probably smart to include it anyways. There's a couple // TODO:s, but those are either out of scope for this first PR (unions) or things I'm just uncertain of and need advice (the other 2).

@rustbot ready

RalfJung

Thanks! libffi is much cooler than I expected. :)

I have some ideas for how we could unify the scalar and struct handling... but I think that's better done in a follow-up PR, so I'll post some things on Zulip.

@rustbot author

RalfJung · 2025-07-21T13:19:10Z

+    unsafe { cif.call(fun, &arg_ptrs) }
+}
+
+/// A wrapper type for `libffi::middle::Type` which also holds a pointer to the data.


Can you explain what this means in a self-contained way, rather than referencing other concepts the reader is unlikely to know?

Also, it's odd to lead with this being a "type" when it is called "arg".

RalfJung · 2025-07-21T13:25:50Z

+    }
+}
+
+/// An owning form of `FfiArg`.


The prefixes "C" and "Ffi" don't do a great job reflecting "owned" and borrowed.

So what about OwnedArg and BorrowedArg? The "downcast" function (also an odd name, what does it downcast?) can then be called borrow.

CPrimitive could be called ScalarArg as that seems to match what we use it for.

Also, please generally define types bottom-up and with increasing complexity/subtlety, to simplify reading the file in order:

ScalarArg

OwnedArg

BorrowedArg

RalfJung · 2025-07-21T13:27:19Z

+    /// Struct with its computed type layout and bytes.
+    Struct(FfiType, Box<[u8]>),


This doesn't have to be a struct, right? It can be any type that FfiType can represent, which includes basic integers.

I'll change the name, presumably the next PR idea which you brought up on Zulip would make this enum redundant anyway

RalfJung · 2025-07-21T13:27:43Z

+
+fn main() {
+    let pass_me = PassMe { value: 42, other_value: 1337 };
+    unsafe { pass_struct(pass_me) }; //~ ERROR: Undefined Behavior: passing a non-#[repr(C)] struct over FFI


Yeah, reporting this as "unsupported" is better.

RalfJung · 2025-07-21T13:28:27Z

+/// Test passing a basic struct as an argument.
+fn test_pass_struct() {


The file these tests are in is called "scalar_arguments". Do these arguments you are passing here ,ook like scalars to you? :)

Please make a new file for aggregate arguments.

RalfJung · 2025-07-21T13:36:39Z

+        args: &'tcx ty::List<ty::GenericArg<'tcx>>,
+    ) -> InterpResult<'tcx, FfiType> {
+        // TODO: is this correct? Maybe `repr(transparent)` when the inner field
+        // is itself `repr(c)` is ok?


Yes, that would be okay. But let's leave it to a future PR, since a repr(transparent) wrapper around e.g. i32 should also be supported and that'll be a bit more interesting -- it's going to be an Immediate.

RalfJung · 2025-07-21T13:57:14Z

+                if let Some(prov) = ptr.provenance {
+                    // The first time this happens, print a warning.
+                    if !this.machine.native_call_mem_warned.replace(true) {
+                        // Newly set, so first time we get here.
+                        this.emit_diagnostic(NonHaltingDiagnostic::NativeCallSharedMem { tracing });
+                    }
+
+                    this.expose_provenance(prov)?;
+                };


The same snippet will be needed in the struct case, so please make this a helper function.

nia-e · 2025-08-24T17:52:05Z

Hm, I rebased on master to make use of get_range() here as well and it seems like repr(C) structs can be immediates now (or at least, it causes tests to fail with that reason). Should I do the refactor you mentioned on Zulip in this branch? It's already mostly written locally

rustbot · 2025-08-24T17:52:40Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

RalfJung · 2025-08-27T15:46:47Z

Hm, I rebased on master to make use of get_range() here as well and it seems like repr(C) structs can be immediates now (or at least, it causes tests to fail with that reason). Should I do the refactor you mentioned on Zulip in this branch? It's already mostly written locally

Turns out repr(C) types cannot be Scalar, but they can be ScalarPair... so yeah we'll have to include that refactor in this PR I guess.

RalfJung · 2025-08-27T15:53:34Z

+                            .alloc_id_from_addr(
+                                mplace_ptr.addr().bytes(),


Here you are doing the equivalent of casting a pointer to an integer and back. Please don't. You can get the alloc ID from the pointer directly.

Specifically, please use ptr_get_alloc_id. That also gives you the offset inside the allocation.

However, you'll need to first separately handle the zero-sized case. mplace.ptr() might not point to an Allocation if the type has size 0.

Are ZSTs allowed over FFI? I was under the impression that there's no way to represent them in C so I figured just erroring here makes sense

I guess erroring is fine but that still require a separate check to print a non-obscure error.

RalfJung · 2025-08-27T15:54:46Z

+                                alloc_ptr.with_addr(mplace_ptr.addr().bytes_usize()),
+                                mplace.layout.size.bytes_usize(),
+                            )
+                            .to_vec()


Round-tripping through a Vec seems odd?

Fair, I guess it's pretty easily avoidable

nia-e · 2025-08-27T22:38:16Z

Should be good. I couldn't use your suggestion of write_target_uint since that only takes a u128 while a scalar pair could be up to 256 bits, so I had to adjust its logic to account for 2 scalars. Frankly that whole section could belong in a separate function to turn an immediate into bytes, but if one exists I couldn't find it.

I had also added some tests for that code (passing structs that were a: i32, b: i16 and a: i16, b: i32 so that there might be alignment padding between the fields) but removed them from here since test_pass_struct already handles the tricky case (second field with greater align than the first).

RalfJung · 2025-08-28T06:04:33Z

Should be good. I couldn't use your suggestion of write_target_uint since that only takes a u128 while a scalar pair could be up to 256 bits

You should be calling write_target_uint twice then, once for each scalar.

RalfJung · 2025-08-28T06:05:15Z

+                let mplace_ptr = mplace.ptr();
+                let sz = mplace.layout.size.bytes_usize();
+                if sz == 0 {
+                    throw_unsup_format!("Attempting to pass a ZST over FFI");


Suggested change

throw_unsup_format!("Attempting to pass a ZST over FFI");

throw_unsup_format!("attempting to pass a ZST over FFI");

RalfJung · 2025-08-28T06:08:22Z

+                let sz = imm.layout.size.bytes_usize();
+                // TODO: Is this actually ok? Seems like the only way to figure
+                // out how the scalars are laid out relative to each other.
+                let align_second = match imm.layout.backend_repr {
+                    rustc_abi::BackendRepr::Scalar(_) => 1,
+                    rustc_abi::BackendRepr::ScalarPair(_, sc2) => sc2.align(this).bytes_usize(),
+                    _ => unreachable!(),
+                };
+                // How many bytes to skip between scalars if necessary for alignment.
+                let skip = sz_first.next_multiple_of(align_second).strict_sub(sz_first);
+


Have a look at https://github.com/rust-lang/rust/blob/ece1397e3ff40e7f8a411981844a8214659da970/compiler/rustc_const_eval/src/interpret/place.rs#L716-L732 for how to determine the offset of the 2nd field.

RalfJung · 2025-08-28T06:08:57Z

+                match this.data_layout().endian {
+                    rustc_abi::Endian::Little => {


You should not be doing your own endianess handling. write_target_uint can do that for you.

RalfJung · 2025-08-29T12:49:04Z

@rustbot author

nia-e · 2025-08-30T06:45:02Z

@rustbot ready

RalfJung · 2025-08-31T17:45:22Z

+                        // This relies on the `expose_provenance` in the `visit_reachable_allocs` callback
+                        // below to expose the actual interpreter-level allocation.


This comment is entirely in the wrong place now... previously it referred to a std::ptr::with_exposed_provenance_mut, now it stopped making any sense. Please ask before just taking random guesses as to where to put such comments.

Is it wrong? If either scalar is a pointer, the code under the comment exposes provenance only in the AM; then later when visit_reachable_allocs is called it'll do alloc.get_bytes_unchecked_raw().expose_provenance() on the allocation said pointer points to. Unless I misunderstood the meaning of that comment, that's what it referred to in the first place

It's correct that the code below only exposes the ptr in the AM. But the claim that this act somehow relies on the ptr also being exposed in the "host" makes no sense. That referred to the with_exposed_provenance_mut, which as per its documentation requires a prior call to expose_provenance, and the comment was explaining where that expose_provenance happened.

The new code side-steps the with_exposed_provenance_mut, making it less clear where to put the comment. But this here is definitely the wrong spot.

RalfJung · 2025-08-31T18:52:10Z

I've done some refactoring and cleanup, found it easier to just edit the code than write it out.^^ Please take a look at the commit I added and use that to train your model for future Miri PRs. ;)

RalfJung · 2025-08-31T18:54:44Z

-                // bytes are part of this allocation and initialised. They might be marked
-                // as uninit in Miri, but all bytes returned by `MiriAllocBytes` are
-                // initialised.
-                unsafe {


In particular, I got rid of some unsafe code here.

nia-e · 2025-08-31T19:12:02Z

Ty! Glad this is good now ^^

nia-e force-pushed the native-structs branch 2 times, most recently from ca18991 to 4e6a464 Compare July 14, 2025 02:05

nia-e commented Jul 14, 2025

View reviewed changes

nia-e force-pushed the native-structs branch 2 times, most recently from 1faf540 to e9670f0 Compare July 16, 2025 09:42

rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Jul 16, 2025

nia-e commented Jul 17, 2025

View reviewed changes

Comment thread src/shims/native_lib/mod.rs Outdated

nia-e force-pushed the native-structs branch 2 times, most recently from 7cd63db to 532a735 Compare July 17, 2025 10:19

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Jul 17, 2025

nia-e force-pushed the native-structs branch 6 times, most recently from 809f60e to edfa595 Compare July 17, 2025 17:58

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Jul 17, 2025

nia-e force-pushed the native-structs branch 5 times, most recently from 685a17c to a5c0d97 Compare July 17, 2025 18:21

RalfJung reviewed Jul 21, 2025

View reviewed changes

rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label Jul 21, 2025

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Aug 17, 2025

nia-e force-pushed the native-structs branch from 5a68bf8 to a4e189b Compare August 24, 2025 17:52

RalfJung reviewed Aug 27, 2025

View reviewed changes

RalfJung reviewed Aug 28, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Aug 29, 2025

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Aug 30, 2025

RalfJung reviewed Aug 31, 2025

View reviewed changes

native-lib: pass structs to native code

4b82474

RalfJung force-pushed the native-structs branch from d3d20cc to 0b04a2d Compare August 31, 2025 18:50

RalfJung enabled auto-merge August 31, 2025 18:52

RalfJung reviewed Aug 31, 2025

View reviewed changes

some refactoring and cleanup

eff2b42

RalfJung force-pushed the native-structs branch from 0b04a2d to eff2b42 Compare August 31, 2025 19:03

RalfJung added this pull request to the merge queue Aug 31, 2025

Merged via the queue into rust-lang:master with commit bcda3b3 Aug 31, 2025
15 checks passed

rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label Aug 31, 2025

		/// Struct with its computed type layout and bytes.
		Struct(FfiType, Box<[u8]>),

		/// Test passing a basic struct as an argument.
		fn test_pass_struct() {

	throw_unsup_format!("Attempting to pass a ZST over FFI");
	throw_unsup_format!("attempting to pass a ZST over FFI");

		match this.data_layout().endian {
		rustc_abi::Endian::Little => {

		// This relies on the `expose_provenance` in the `visit_reachable_allocs` callback
		// below to expose the actual interpreter-level allocation.

Conversation

nia-e commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nia-e commented Jul 16, 2025

Uh oh!

Uh oh!

nia-e commented Jul 17, 2025

Uh oh!

rustbot commented Jul 17, 2025

Uh oh!

nia-e commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RalfJung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nia-e commented Aug 24, 2025

Uh oh!

rustbot commented Aug 24, 2025

Uh oh!

RalfJung commented Aug 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nia-e commented Aug 27, 2025

Uh oh!

RalfJung commented Aug 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Aug 29, 2025

Uh oh!

nia-e commented Aug 30, 2025

Uh oh!

RalfJung Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Aug 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

nia-e commented Jul 14, 2025 •

edited

Loading

nia-e commented Jul 17, 2025 •

edited

Loading

RalfJung Aug 31, 2025 •

edited

Loading

RalfJung Aug 31, 2025 •

edited

Loading

nia-e commented Aug 31, 2025 •

edited

Loading