Expose `unsafe` functions to C++, as if they were not `unsafe`.
For example (Rust):
```rust
pub unsafe fn foo() {}
```
becomes (C++):
```c++
void foo();
```
And can be called from C++ as such.
Given the initial use-case targeted by Crubit is manually written interop []
### Status Quo
The idea behind not implementing `unsafe` yet was that, well, we do not want to implement something that will prevent us from adopting a better solution later. We should implement it the right way from the start, and in the meantime, do nothing, so that we do not have to deal with technical debt from making a wrong choice at first.
However, because workarounds exist, this doesn't work as well as described. We are only moving the technical debt down to end users. After all, users can wrap them with a safe function, and then expose that to C++ anyway! See b/254095482#comment8.
We might ask: given that users will call unsafe functions, should we allow it by using `unsafe` functions directly in FFI, or should we allow it via user-defined wrapper functions? "Don't allow it" is not an option![0]
If we pick the status quo, people work around this by marking their functions as safe. These workarounds are permanent: once we add `unsafe` FFI, there is no reliable way to go back and find everywhere that people worked around missing `unsafe` FFI, and retroactively add safety support. And so, ironically, by not supporting `unsafe` in FFI, we guarantee a long-term future with more dangerous FFI -- we won't be able to find everywhere we should be adding `unsafe`. In the short term, these workaround functions are more dangerous (if they are callable from Rust, they're not marked unsafe!), and more difficult to review (using `unsafe` in a "weird" way).
In contrast, if we pick a solution -- any complete solution, really -- that allows people to invoke `unsafe` functions over FFI, then people will directly expose their `unsafe` functions into the FFI, and now it becomes **legible**. We can enumerate every single instance of an unsafe function which is callable from C++, and either fix it when we have a better/newer approach, or mark it with an attribute that allowlists it into a fake-safe C++ API. This has larger direct costs: we are forced to actually do this migration when the time comes, or write that ignore attribute. However, these direct costs are just the result of making the invisible hidden costs, visible -- and forcing us to directly come to terms with the existing unsafe FFI.
So: there are concrete benefits to supporting `unsafe`. This does not block us from later adopting a better form of `unsafe` support (though we will need to mark up existing functions, this is not a huge cost). And it will make any kind of long-term technical debt from `unsafe` easier to spot and fix later.
I believe, therefore, that this is a win. We should support `unsafe`, in the short-medium term. In the long-term, it's a research project for how best to expose `unsafe` to C++ code.
### What do other projects do?
We're in good company -- everyone else I tried also does the same thing as this CL proposes doing for Crubit.
#### cbindgen
`unsafe` is ignored, as in this CL:
```rust
#[no_mangle]
pub unsafe extern "C" fn add(left: usize, right: usize) -> usize {
left + right
}
```
```c++
#include <cstdarg>
#include <cstdint>
#include <cstdlib>
#include <ostream>
#include <new>
extern "C" {
uintptr_t add(uintptr_t left, uintptr_t right);
} // extern "C"
```
#### cxx
`unsafe` is ignored, like in this CL:
```rust
#[cxx::bridge]
mod ffi {
extern "Rust" {
unsafe fn add(left: usize, right: usize) -> usize;
}
}
pub unsafe fn add(left: usize, right: usize) -> usize {
left + right
}
```
```c++
#pragma once
#include <cstddef>
::std::size_t add(::std::size_t left, ::std::size_t right) noexcept;
```
### Footnotes
[0]: Or, is it an option? Well, we could absolutely reject workarounds during code review. However, this would be incapacitating. It means we cannot pass pointers/references to Rust without designing either a safety annotation system (so that we can pass pointers and dereference them in Rust), or else design safe aliasing reference wrappers (so that the function doesn't need to be unsafe at all). Both currently lack a solution and agreement, and will for some time still.
PiperOrigin-RevId: 592703897
Change-Id: I3b6ef341f92f995fd9c5aabd250b858b4b4f8ad0
diff --git a/cc_bindings_from_rs/bindings.rs b/cc_bindings_from_rs/bindings.rs
index 045a1c3..bccfcd9 100644
--- a/cc_bindings_from_rs/bindings.rs
+++ b/cc_bindings_from_rs/bindings.rs
@@ -895,6 +895,14 @@
#fully_qualified_fn_name( #( #fn_args ),* )
}
};
+ // Wrap the call in an unsafe block, for the sake of RFC #2585
+ // `unsafe_block_in_unsafe_fn`.
+ if sig.unsafety == Unsafety::Unsafe {
+ // Note: This causes unsafe {... unsafe {...} ...}, which seems not ideal.
+ // Either the inner unsafe should be moved out, or just elided when
+ // there's an outer unsafe.
+ thunk_body = quote! {unsafe {#thunk_body}};
+ }
if !is_c_abi_compatible_by_value(sig.output()) {
thunk_params.push(quote! {
__ret_slot: &mut ::core::mem::MaybeUninit<#thunk_ret_type>
@@ -936,9 +944,16 @@
};
let thunk_name = make_rs_ident(thunk_name);
+ let unsafe_qualifier = if sig.unsafety == Unsafety::Unsafe {
+ quote! {unsafe}
+ } else {
+ quote! {}
+ };
Ok(quote! {
#[no_mangle]
- extern "C" fn #thunk_name #generic_params ( #( #thunk_params ),* ) -> #thunk_ret_type {
+ #unsafe_qualifier extern "C" fn #thunk_name #generic_params (
+ #( #thunk_params ),*
+ ) -> #thunk_ret_type {
#thunk_body
}
})
@@ -950,14 +965,6 @@
bail!("C variadic functions are not supported (b/254097223)");
}
- match sig.unsafety {
- Unsafety::Normal => (),
- Unsafety::Unsafe => {
- // TODO(b/254095482): Figure out how to handle `unsafe` functions.
- bail!("Bindings for `unsafe` functions are not fully designed yet (b/254095482)");
- }
- }
-
Ok(())
}
@@ -3271,17 +3278,56 @@
}
#[test]
- fn test_format_item_unsupported_fn_unsafe() {
+ fn test_format_item_fn_extern_c_unsafe() {
let test_src = r#"
#[no_mangle]
pub unsafe extern "C" fn foo() {}
"#;
test_format_item(test_src, "foo", |result| {
- let err = result.unwrap_err();
- assert_eq!(
- err,
- "Bindings for `unsafe` functions \
- are not fully designed yet (b/254095482)"
+ let result = result.unwrap().unwrap();
+ let main_api = &result.main_api;
+ assert!(main_api.prereqs.is_empty());
+ assert_cc_matches!(
+ main_api.tokens,
+ quote! {
+ void foo();
+ }
+ );
+ assert!(result.rs_details.is_empty());
+ });
+ }
+
+ /// For non-extern "C" unsafe functions, we need a thunk, and it needs some
+ /// `unsafe`.
+ ///
+ /// The thunk itself needs to be unsafe, because it wraps an unsafe function
+ /// and is still in-principle itself directly callable. It also needs to
+ /// have an unsafe block inside of it due to RFC #2585
+ /// `unsafe_block_in_unsafe_fn`.
+ #[test]
+ fn test_format_item_fn_unsafe() {
+ let test_src = r#"
+ #[no_mangle]
+ pub unsafe fn foo() {}
+ "#;
+ test_format_item(test_src, "foo", |result| {
+ let result = result.unwrap().unwrap();
+ let main_api = &result.main_api;
+ assert!(main_api.prereqs.is_empty());
+ assert_cc_matches!(
+ main_api.tokens,
+ quote! {
+ void foo();
+ }
+ );
+ assert_cc_matches!(
+ result.rs_details,
+ quote! {
+ #[no_mangle]
+ unsafe extern "C" fn __crubit_thunk_foo() -> () {
+ unsafe { ::rust_out::foo() }
+ }
+ }
);
});
}
@@ -6450,6 +6496,16 @@
"",
),
),
+ // Unsafe extern "C" function pointers are, to C++, just function pointers.
+ (
+ "unsafe extern \"C\" fn(f32, f32) -> f32",
+ (
+ "crubit :: type_identity_t < float (float , float) > &",
+ "<crubit/support/for/tests/internal/cxx20_backports.h>",
+ "",
+ "",
+ ),
+ ),
(
// Nested function pointer (i.e. `TypeLocation::Other`) means that
// we need to generate a C++ function *pointer*, rather than a C++
@@ -6623,10 +6679,6 @@
"extern \"C\" fn (f32, f32) -> SomeStruct",
"Function pointers can't have a thunk: Return type requires a thunk",
),
- (
- "unsafe fn(i32) -> i32",
- "Bindings for `unsafe` functions are not fully designed yet (b/254095482)",
- ),
// TODO(b/254094650): Consider mapping this to Clang's (and GCC's) `__int128`
// or to `absl::in128`.
("i128", "C++ doesn't have a standard equivalent of `i128` (b/254094650)"),
diff --git a/cc_bindings_from_rs/test/functions/functions.rs b/cc_bindings_from_rs/test/functions/functions.rs
index 1cd10aa..0f44aa2 100644
--- a/cc_bindings_from_rs/test/functions/functions.rs
+++ b/cc_bindings_from_rs/test/functions/functions.rs
@@ -118,3 +118,13 @@
x + y
}
}
+
+pub mod unsafe_fn_tests {
+ /// # Safety
+ ///
+ /// This function has no safety requirements - it is only marked as `unsafe`
+ /// to facilitate minimal testing of bindings generated for such functions.
+ pub unsafe fn unsafe_add(x: i32, y: i32) -> i32 {
+ x + y
+ }
+}
diff --git a/cc_bindings_from_rs/test/functions/functions_test.cc b/cc_bindings_from_rs/test/functions/functions_test.cc
index a57a34b..f2d54f3 100644
--- a/cc_bindings_from_rs/test/functions/functions_test.cc
+++ b/cc_bindings_from_rs/test/functions/functions_test.cc
@@ -18,17 +18,17 @@
namespace fn_abi_tests = functions::fn_abi_tests;
namespace fn_param_ty_tests = functions::fn_param_ty_tests;
-TEST(FnAbiTests, ExternCNoMangle) {
+TEST(FnAbiTest, ExternCNoMangle) {
EXPECT_THAT(fn_abi_tests::get_42_as_f64_via_no_mangle_extern_c(),
DoubleEq(42.0));
}
-TEST(FnAbiTests, ExternCWithExportName) {
+TEST(FnAbiTest, ExternCWithExportName) {
EXPECT_EQ(12 + 34,
fn_abi_tests::add_i32_via_extern_c_with_export_name(12, 34));
}
-TEST(FnAbiTests, ExternCWithMangling) {
+TEST(FnAbiTest, ExternCWithMangling) {
// TODO(b/262904507): Uncomment the test assertion below after ensuring that
// the `genrule` in `test/functions/BUILD` invokes `cc_bindings_from_rs` with
// the same rustc cmdline flags as when `rustc` is used to build
@@ -41,26 +41,26 @@
// fn_abi_tests::add_i32_via_extern_c_with_mangling(12, 34));
}
-TEST(FnAbiTests, Rust) {
+TEST(FnAbiTest, Rust) {
EXPECT_EQ(12 + 34, fn_abi_tests::add_i32_via_rust_abi(12, 34));
}
-TEST(FnParamTyTests, Float64) {
+TEST(FnParamTyTest, Float64) {
EXPECT_THAT(fn_param_ty_tests::add_f64(12.0, 34.0), DoubleEq(12.0 + 34.0));
}
-TEST(FnParamTyTests, Int32) {
+TEST(FnParamTyTest, Int32) {
EXPECT_EQ(12 + 34, fn_param_ty_tests::add_i32(12, 34));
}
-TEST(FnParamTyTests, rs_char) {
+TEST(FnParamTyTest, rs_char) {
std::optional<const rs_std::rs_char> input = rs_std::rs_char::from_u32(U'A');
ASSERT_TRUE(input.has_value());
rs_std::rs_char output = fn_param_ty_tests::char_to_ascii_lowercase(*input);
EXPECT_EQ(std::uint32_t{U'a'}, std::uint32_t{output});
}
-TEST(FnParamTyTests, Int32Ptr) {
+TEST(FnParamTyTest, Int32Ptr) {
std::int32_t x = 12;
std::int32_t y = 34;
std::int32_t sum; // uninitialized
@@ -70,21 +70,21 @@
EXPECT_EQ(12 + 34, sum);
}
-TEST(FnParamTyTests, Int32Ref) {
+TEST(FnParamTyTest, Int32Ref) {
std::int32_t x = 123;
std::int32_t y = 456;
const std::int32_t& result = fn_param_ty_tests::get_ref_to_smaller_int(x, y);
EXPECT_EQ(&result, &x);
}
-TEST(FnParamTyTests, Int32RefWithInferredLifetime) {
+TEST(FnParamTyTest, Int32RefWithInferredLifetime) {
std::int32_t x = 123;
const std::int32_t& result =
fn_param_ty_tests::get_identical_ref_with_inferred_lifetime(x);
EXPECT_EQ(&result, &x);
}
-TEST(FnParamTyTests, Int32MutRef) {
+TEST(FnParamTyTest, Int32MutRef) {
std::int32_t sum = -123;
fn_param_ty_tests::set_mut_ref_to_sum_of_ints(sum, 456, 789);
EXPECT_EQ(sum, 456 + 789);
@@ -94,7 +94,7 @@
std::int32_t MultiplyInt32(std::int32_t x, std::int32_t y) { return x * y; }
-TEST(FnParamTyTests, FnPtr) {
+TEST(FnParamTyTest, FnPtr) {
std::int32_t sum = fn_param_ty_tests::apply_binary_i32_op(12, 34, AddInt32);
EXPECT_EQ(sum, 12 + 34);
@@ -103,7 +103,7 @@
EXPECT_EQ(product, 56 * 78);
}
-TEST(OtherFnTests, VoidReturningFunction) {
+TEST(OtherFnTest, VoidReturningFunction) {
namespace tests = functions::unit_ret_ty_tests;
tests::set_global_i32_via_extern_c_with_export_name(123);
EXPECT_EQ(123, tests::get_global_i32_via_extern_c_with_export_name());
@@ -112,11 +112,16 @@
EXPECT_EQ(456, tests::get_global_i32_via_extern_c_with_export_name());
}
-TEST(OtherFnTests, DuplicatedParamNames) {
+TEST(OtherFnTest, DuplicatedParamNames) {
namespace tests = functions::other_fn_param_tests;
EXPECT_EQ(12 + 34, tests::add_i32_via_rust_abi_with_duplicated_param_names(
12, 34, 56, 78));
}
+TEST(UnsafeFnTest, UnsafeFunction) {
+ namespace tests = functions::unsafe_fn_tests;
+ EXPECT_EQ(12 + 34, tests::unsafe_add(12, 34));
+}
+
} // namespace
} // namespace crubit
