native-lib: allow passing structs as arguments #4466

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

tests/native-lib/fail/struct_not_extern_c.rs

+
+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

src/shims/native_lib/mod.rs

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

src/shims/native_lib/ffi.rs

+    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

src/shims/native_lib/ffi.rs

+    }
+}
+
+/// 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

src/shims/native_lib/ffi.rs

+    /// 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

tests/native-lib/fail/struct_not_extern_c.rs

+
+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

tests/native-lib/pass/scalar_arguments.rs

+/// 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.

src/shims/native_lib/mod.rs

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

src/shims/native_lib/mod.rs

+        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.

src/shims/native_lib/mod.rs

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

src/shims/native_lib/mod.rs

+                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.

RalfJung · 2025-07-21T14:09:18Z

tests/native-lib/pass/scalar_arguments.rs

+        part_2: Part2 { bits: 0xabcdef01 },
+        part_3: 0xabcdef01,
+    };
+    assert_eq!(unsafe { pass_struct_complex(complex) }, 0);


Can you add a fail test that looks roughly like this, to ensure we do check initialization properly?

let arg = MaybeUninit::<ComplexStruct>::uninit(); pass_struct_complex(*arg.as_ptr());

This needs derive(Copy, Clone) on the struct. (Don't use .read(), that might defeat the test.)

Co-authored-by: Ralf Jung <[email protected]>

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

src/shims/native_lib/mod.rs Show resolved Hide resolved

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 the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jul 21, 2025

RalfJung reviewed Jul 21, 2025

View reviewed changes

native-lib: pass structs to native code

c7edf0a

nia-e force-pushed the native-structs branch from a5c0d97 to c7edf0a Compare July 22, 2025 19:09

Apply suggestions from code review

83728e0

Co-authored-by: Ralf Jung <[email protected]>

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

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

native-lib: allow passing structs as arguments #4466

Are you sure you want to change the base?

native-lib: allow passing structs as arguments #4466

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nia-e commented Jul 14, 2025 •

edited

Loading

nia-e commented Jul 17, 2025 •

edited

Loading