Fix how structs are handled by the generated thunks.
- Struct values need to be `std::move`d when calling thunks.
- `#![allow(improper_ctypes_definitions)]` is needed.
PiperOrigin-RevId: 490384826
diff --git a/cc_bindings_from_rs/bindings.rs b/cc_bindings_from_rs/bindings.rs
index 38096b6..eb9d59e 100644
--- a/cc_bindings_from_rs/bindings.rs
+++ b/cc_bindings_from_rs/bindings.rs
@@ -58,6 +58,27 @@
let rs_body = quote! {
#top_comment
+
+ // Rust warns about non-`#[repr(C)]` structs being used as parameter types or return
+ // type of `extern "C"` functions (such as thunks that might be present in `rs_body`).
+ // This warning makes sense, because in absence of a guaranteed / well-defined ABI
+ // for this structs, one can't author C/C++ definitions compatible with that ABI.
+ // Unless... the author is `cc_bindings_from_rs` invoked with exactly the same version
+ // and cmdline flags as `rustc`. Given this, we just disable warnings like the one
+ // in the example below:
+ //
+ // warning: `extern` fn uses type `DefaultReprPoint`, which is not FFI-safe
+ // --> .../cc_bindings_from_rs/test/structs/structs_cc_api_impl.rs:25:6
+ // |
+ // 25 | ) -> structs::DefaultReprPoint {
+ // | ^^^^^^^^^^^^^^^^^^^^^^^^^ not FFI-safe
+ // |
+ // = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute...
+ // = note: this struct has unspecified layout
+ // = note: `#[warn(improper_ctypes_definitions)]` on by default
+ #![allow(improper_ctypes_definitions)] __NEWLINE__
+ __NEWLINE__
+
#rs_body
};
@@ -107,6 +128,23 @@
}
}
+/// Formats an argument of a thunk. For example:
+/// - most primitive types are passed as-is - e.g. `123`
+/// - structs need to be moved: `std::move(value)`
+/// - in the future additional processing may be needed for other types (this is
+/// speculative so please take these examples with a grain of salt):
+/// - str: utf-8 verification
+/// - &T: calling into `crubit::MutRef::unsafe_get_ptr`
+fn format_cc_thunk_arg<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, value: TokenStream) -> CcSnippet {
+ let mut includes = BTreeSet::new();
+ if ty.is_copy_modulo_regions(tcx, ty::ParamEnv::empty()) {
+ CcSnippet { includes, snippet: value }
+ } else {
+ includes.insert(CcInclude::utility());
+ CcSnippet { includes, snippet: quote! { std::move(#value) } }
+ }
+}
+
/// Formats `ty` into a `CcSnippet` that represents how the type should be
/// spelled in a C++ declaration of an `extern "C"` function.
fn format_ty_for_cc(tcx: TyCtxt, ty: Ty) -> Result<CcSnippet> {
@@ -445,6 +483,12 @@
} else {
let exported_name =
format_cc_ident(symbol_name.name).context("Error formatting exported name")?;
+ let thunk_args = arg_names
+ .clone()
+ .into_iter()
+ .zip(sig.inputs().iter())
+ .map(|(arg, &ty)| format_cc_thunk_arg(tcx, ty, arg).into_tokens(&mut includes))
+ .collect_vec();
quote! {
namespace __crubit_internal {
extern "C" #ret_type #exported_name (
@@ -453,10 +497,7 @@
} // namespace __crubit_internal
inline #ret_type #fn_name (
#( #arg_types #arg_names ),* ) {
- // TODO(b/258232820): Support using `struct`s as an argument of a thunk. The
- // code below won't compile as-is because it unnecessarily invokes a copy
- // constructor. The generated code should go through `std::move(...)`.
- return __crubit_internal :: #exported_name( #( #arg_names ),* );
+ return __crubit_internal :: #exported_name( #( #thunk_args ),* );
}
}
}
@@ -802,8 +843,8 @@
#[cfg(test)]
pub mod tests {
use super::{
- format_def, format_ret_ty_for_cc, format_ty_for_cc, format_ty_for_rs, GeneratedBindings,
- MixedSnippet,
+ format_cc_thunk_arg, format_def, format_ret_ty_for_cc, format_ty_for_cc, format_ty_for_rs,
+ GeneratedBindings, MixedSnippet,
};
use anyhow::Result;
@@ -1646,7 +1687,7 @@
extern "C" std::int32_t ...(S __param_0);
}
inline std::int32_t func(S __param_0) {
- return __crubit_internal::...(__param_0);
+ return __crubit_internal::...(std::move(__param_0));
}
}
);
@@ -2386,12 +2427,51 @@
});
}
+ #[test]
+ fn test_format_cc_thunk_arg() {
+ let testcases = [
+ // ( <Rust type>, (<expected C++ type>, <expected #include>) )
+ ("i32", ("value", "")),
+ ("SomeStruct", ("std::move(value)", "utility")),
+ ];
+ let preamble = quote! {
+ pub struct SomeStruct {
+ pub x: i32,
+ pub y: i32,
+ }
+ };
+ test_ty(&testcases, preamble, |desc, tcx, ty, (expected_snippet, expected_include)| {
+ let (actual_snippet, actual_includes) = {
+ let cc_snippet = format_cc_thunk_arg(tcx, ty, quote! { value });
+ (cc_snippet.snippet.to_string(), cc_snippet.includes)
+ };
+
+ let expected_snippet = expected_snippet.parse::<TokenStream>().unwrap().to_string();
+ assert_eq!(actual_snippet, expected_snippet, "{desc}");
+
+ if expected_include.is_empty() {
+ assert!(actual_includes.is_empty());
+ } else {
+ let expected_header = format_cc_ident(expected_include).unwrap();
+ assert_cc_matches!(
+ format_cc_includes(&actual_includes),
+ quote! { include <#expected_header> }
+ );
+ }
+ });
+ }
+
fn test_ty<TestFn, Expectation>(
testcases: &[(&str, Expectation)],
preamble: TokenStream,
test_fn: TestFn,
) where
- TestFn: Fn(/* testcase_description: */ &str, TyCtxt, Ty, &Expectation) + Sync,
+ TestFn: for<'tcx> Fn(
+ /* testcase_description: */ &str,
+ TyCtxt<'tcx>,
+ Ty<'tcx>,
+ &Expectation,
+ ) + Sync,
Expectation: Sync,
{
for (index, (input, expected)) in testcases.iter().enumerate() {
diff --git a/cc_bindings_from_rs/cc_bindings_from_rs.rs b/cc_bindings_from_rs/cc_bindings_from_rs.rs
index 91b8c86..9fc4a88 100644
--- a/cc_bindings_from_rs/cc_bindings_from_rs.rs
+++ b/cc_bindings_from_rs/cc_bindings_from_rs.rs
@@ -377,6 +377,8 @@
r#"// Automatically @generated C++ bindings for the following Rust crate:
// test_crate
+#![allow(improper_ctypes_definitions)]
+
#[no_mangle]
extern "C" fn __crubit_thunk__RNvCslKXfKXPWofF_10test_crate15public_function() -> () {
test_crate::public_function()
diff --git a/cc_bindings_from_rs/test/structs/structs.rs b/cc_bindings_from_rs/test/structs/structs.rs
index acdf3ca..f7b2b7e 100644
--- a/cc_bindings_from_rs/test/structs/structs.rs
+++ b/cc_bindings_from_rs/test/structs/structs.rs
@@ -5,22 +5,29 @@
//! This crate is used as a test input for `cc_bindings_from_rs` and the
//! generated C++ bindings are then tested via `structs_test.cc`.
-// TODO(b/258232820): Add test coverage for non-`#[repr(C)]` struct. See
-// cl/489571653.
#[repr(C)]
pub struct ReprCPoint {
pub x: i32,
pub y: i32,
}
-// TODO(b/258232820): Remove `#[no_mangle]` and `extern "C"` to extend test
-// coverage to thunks. See cl/489571653.
-#[no_mangle]
-pub extern "C" fn create_repr_c_point_via_free_function(x: i32, y: i32) -> ReprCPoint {
+pub fn create_repr_c_point_via_free_function(x: i32, y: i32) -> ReprCPoint {
ReprCPoint { x, y }
}
-#[no_mangle]
-pub extern "C" fn get_x_of_repr_c_point_via_free_function(p: ReprCPoint) -> i32 {
+pub fn get_x_of_repr_c_point_via_free_function(p: ReprCPoint) -> i32 {
+ p.x
+}
+
+pub struct DefaultReprPoint {
+ pub x: i32,
+ pub y: i32,
+}
+
+pub fn create_default_repr_point_via_free_function(x: i32, y: i32) -> DefaultReprPoint {
+ DefaultReprPoint { x, y }
+}
+
+pub fn get_x_of_default_repr_point_via_free_function(p: DefaultReprPoint) -> i32 {
p.x
}
diff --git a/cc_bindings_from_rs/test/structs/structs_test.cc b/cc_bindings_from_rs/test/structs/structs_test.cc
index 746f891..a7aa0cf 100644
--- a/cc_bindings_from_rs/test/structs/structs_test.cc
+++ b/cc_bindings_from_rs/test/structs/structs_test.cc
@@ -19,5 +19,10 @@
EXPECT_EQ(123, get_x_of_repr_c_point_via_free_function(std::move(p)));
}
+TEST(StructsTest, DefaultReprPointReturnedOrTakenByValue) {
+ DefaultReprPoint p = create_default_repr_point_via_free_function(123, 456);
+ EXPECT_EQ(123, get_x_of_default_repr_point_via_free_function(std::move(p)));
+}
+
} // namespace
} // namespace crubit
diff --git a/common/code_gen_utils.rs b/common/code_gen_utils.rs
index 2c9d878..3405af6 100644
--- a/common/code_gen_utils.rs
+++ b/common/code_gen_utils.rs
@@ -79,6 +79,13 @@
Self::SystemHeader("memory")
}
+ /// Creates a `CcInclude` that represents `#include <utility>` and provides
+ /// C++ functions like `std::move` and C++ types like `std::tuple`.
+ /// See also https://en.cppreference.com/w/cpp/header/utility
+ pub fn utility() -> Self {
+ Self::SystemHeader("utility")
+ }
+
/// Creates a user include: `#include "some/path/to/header.h"`.
pub fn user_header(path: Rc<str>) -> Self {
Self::UserHeader(path)
