-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Does the core safety claim need rephrasing? #1364
Comments
Arbitrary unsafe functions are not exposed to Rust. The FFI has a list of the C++ functions that need to be reviewed, like this: Lines 18 to 25 in 1822e22
and I can examine just those 4 C++ functions to confirm that they are indeed safe to call from Rust (i.e. a Rust function with the same signature and behavior would be a safe function, not an unsafe one). After that, it's guaranteed that if any source of undefined behavior appears in the program, it's either from (1) a Rust This is in contrast with traditional C–Rust FFI using bindgen, which always involves unsafe code on the Rust side of the FFI, like this: https://github.com/rust-lang/git2-rs/blob/git2-0.19.0/src/repo.rs#L395-L401. |
OK, sure, but only a subset of C++ functions can be exposed to Rust as safe. Are you saying CXX doesn't allow other C++ functions (e.g. (I don't know Rust very well; I assumed |
Whether a function is safe to call from Rust is determined by whether the extern "C++" {
unsafe fn pop_back(self: Pin<&mut CxxVector<c_int>>);
} See https://cxx.rs/extern-c++.html#functions-and-member-functions. |
Ok thanks. But what about the other questions? |
CXX does allow other C++ functions to be exposed to Rust, by writing In general, if you have some arbitrary C++ function to call whose behavior is not safe, you're not going to call it without writing either // C++
template <typename T>
bool checked_pop_back(std::vector<T>& vec) {
if (vec.empty()) {
return false;
} else {
vec.pop_back();
return true;
}
}
// Rust
unsafe extern "C++" {
#[cxx_name = "checked_pop_back"]
fn checked_pop_back_int(vec: Pin<&mut CxxVector<c_int>>) -> bool;
}
assert!(ffi::checked_pop_back_int(vec)); or the Rust side: // Rust
extern "C++" {
unsafe fn pop_back(vec: Pin<&mut CxxVector<c_int>>);
}
fn checked_pop_back(vec: Pin<&mut CxxVector<c_int>>) {
assert(!vec.is_empty());
unsafe { ffi::pop_back(vec) }
} (From experience with >300 CXX-based libraries at Meta, the bindings tend to consist overwhelmingly of safe functions, like this. This makes them accessible to engineers with limited Rust experience. We've had engineers in their first and second week with Rust successfully write bindings that would have been challenging for me to produce without CXX as a Rust expert and C++ "knowledgeable".)
The assumption is wrong -- |
s/real-world/some real-world/ or even s/real-world/many real-world/ if you want to make that claim.
Yes, any API using pure value semantics in C++ (to the degree C++ can express it) will always be like that. It's great that so much of Meta's C++ code is written that way.
Not AT ALL questioning the usefulness of this work. I think it's absolutely fantastic. However, you do yourself a disservice by not being precise about the safety claims. It would be easy, for example, for one of those beginning engineers to read your README and reach the wrong conclusions about which things are safe. Also, someone more experienced like me but with less patience will look at your claim, realize it's impossible as stated, and dismiss CXX prematurely. Of course some of the Rust code (the code that declares certain things to be safe at the very least!) needs to be vetted. It would be better to make the claims more accurate, and it would be helpful to characterize the properties of a C++ API that can be declared safe in Rust. FWIW. |
From the README:
I really haven't looked at this in depth so I might be missing something, but it seems to me that given a binding system that exposes unsafe functions to Rust code, the code that uses these unsafe functions has to be audited for correctness.
Some ground assumptions I'm making:
The text was updated successfully, but these errors were encountered: