diff --git a/ffi/MIGRATE.md b/ffi/MIGRATE.md index 8146fda9b..e5f5d89a2 100644 --- a/ffi/MIGRATE.md +++ b/ffi/MIGRATE.md @@ -10,7 +10,9 @@ such as moq's `path`, `timestamp`, and `url`, shares the handle rather than cloning in Rust, and a copy of one a closure lends, kept past the call, aborts. So do `Arc`- and `Rc`-backed types, such as moq's stats `registry`: an `Arc::strong_count` of what one - holds counts one for all its C++ copies, not one per copy. + holds counts one for all its C++ copies, not one per copy. A type whose copies share + has `_borrowed` in the C header, which C++ asks before it moves out of the last + copy, so a method named `borrowed` on one takes a `rename`. - Swift's and Python's objects have no `close()`, and Python's are no context managers: an object's handle is freed once its last reference goes, in Swift's `deinit` or CPython's deallocation, so no reference can outlive it. Drop the reference instead of diff --git a/ffi/README.md b/ffi/README.md index c8b824af8..ba5f63499 100644 --- a/ffi/README.md +++ b/ffi/README.md @@ -126,7 +126,9 @@ refused, and the census counts why. refuse at compile time aborts, naming the function, as does a null or moved-out handle: there is no Rust caller to return an error to. A setter borrows as `&mut T` and takes its value as `T`, so it writes the value the handle holds, or the one Rust lent it, - while a getter's copy is C's own: a nested field is got, set, and set back. + while a getter's copy is C's own: a nested field is got, set, and set back. A type + whose C++ copies share one value has `_borrowed`, true while a call or future borrows + it, so moving it out would abort. - **Arrays** of a `Vec` are a pointer, a length, and an owner, as a buffer is: passed in, Rust copies each element out of C's array; returned, `_vec__free` frees the array and its first `len` elements, so a binding that takes the elements sets @@ -202,10 +204,10 @@ exceptions and RTTI on or off: it throws nothing and reads no type information. into Rust, and the last copy frees it, on whichever thread drops it. `&self` methods are `const`, and `self` methods are `&&`, so a call that moves the value out reads `std::move(c).into_label()`. A handle taken by value is moved into, by a parameter taken - by value; moving out of a shared copy another copy holds moves out a clone, leaving that - copy whole, as Rust leaves a clone. A call on a moved-from object, or one a call moved - the value out of, aborts; a dangling reference to an object is C++'s own undefined - behavior. + by value; moving out of a shared copy another copy holds, or a pending future borrows, + moves out a clone, leaving that copy whole, as Rust leaves a clone. A call on a + moved-from object, or one a call moved the value out of, aborts; a dangling reference + to an object is C++'s own undefined behavior. - **Sharing** stands in for `clone` where no call can tell the two apart but by counting an `Arc`'s or `Rc`'s clones, which the exporter decides at compile time. A type's copies share when it is `Clone`; `Sync`, since one that is not is borrowed on one thread at a time, which copies on two threads diff --git a/ffi/fixtures/api/src/lib.rs b/ffi/fixtures/api/src/lib.rs index d264cfd1c..fd595406b 100644 --- a/ffi/fixtures/api/src/lib.rs +++ b/ffi/fixtures/api/src/lib.rs @@ -739,6 +739,12 @@ impl Gauge { pub fn into_level(self) -> u64 { self.level() } + + /// The level, read once [`yielded`] finishes: a future borrowing this clone meanwhile. + pub async fn later(&self) -> u64 { + yielded(0).await; + self.level() + } } /// The gauge lent shared to `watch` for the call. diff --git a/ffi/runtime/rt.hpp b/ffi/runtime/rt.hpp index 25925ab6b..7a3e478d3 100644 --- a/ffi/runtime/rt.hpp +++ b/ffi/runtime/rt.hpp @@ -152,7 +152,7 @@ class unique { // The handle of an object whose copies share it, since its clone is a value no call // tells apart from the original but by an `Arc`'s or `Rc`'s count, which counts one for // every copy. The last copy frees it, on whichever thread drops it. -template +template class shared { ::std::shared_ptr ptr_; @@ -166,8 +166,10 @@ class shared { } // The handle for a call that moves the value out, held by this copy alone: a clone's // while another copy shares it, as Rust would move a clone out and leave the copy be. + // A pending future made from a copy holds no copy here, but borrows the value in Rust, + // as though it held one. A moved-from copy is left for the call to refuse, naming it. R *own() { - if (ptr_.use_count() > 1) { + if (ptr_.use_count() > 1 || (ptr_ != nullptr && Borrowed(ptr_.get()))) { *this = shared(Clone(ptr_.get())); } return ptr_.get(); diff --git a/ffi/runtime/rt.rs b/ffi/runtime/rt.rs index 1d4c55041..78ea8377d 100644 --- a/ffi/runtime/rt.rs +++ b/ffi/runtime/rt.rs @@ -595,6 +595,17 @@ impl Object { result } + /// Whether a call or a future borrows the value, so moving it out would abort. A value + /// moved out is not, so the move refuses it, naming itself. + /// + /// # Safety + /// As [`Self::borrow`]. + pub unsafe fn borrowed(handle: *const c_void, what: &str) -> bool { + // SAFETY: the caller's contract. + let object = unsafe { Self::get(handle, what) }; + !matches!(object.state.load(Ordering::Acquire), 0 | TAKEN) + } + /// Move the value out, leaving the handle to be freed. /// /// # Safety diff --git a/ffi/src/abi.rs b/ffi/src/abi.rs index f1f89cef7..3f9122445 100644 --- a/ffi/src/abi.rs +++ b/ffi/src/abi.rs @@ -673,6 +673,9 @@ impl<'a> Abi<'a> { let name = self.object(i); let what = format!("type `{}`", api.rust(&object.path, &object.args)); self.claim(format!("{name}_free"), what.clone(), object.span); + if object.shared { + self.claim(format!("{name}_borrowed"), what.clone(), object.span); + } self.claim_traits(&name, &what, object.span, object.traits, false); self.claim(name.clone(), what, object.span); let owner = Ty::Handle { name, constant: false }; diff --git a/ffi/src/c.rs b/ffi/src/c.rs index 0f16e0fd5..104559e20 100644 --- a/ffi/src/c.rs +++ b/ffi/src/c.rs @@ -212,6 +212,9 @@ impl Header<'_> { let docs = format!("{}\n\n{}", abi.api.rust(&object.path, &object.args), object.docs); self.line(comment(docs.trim(), "").trim_end()); self.line(format!("void {name}_free({name} *self);")); + if object.shared { + self.line(format!("bool {name}_borrowed(const {name} *self);")); + } let t = object.traits; if t.clone { self.line(format!("{name} *{name}_clone(const {name} *self);")); diff --git a/ffi/src/cpp.rs b/ffi/src/cpp.rs index ed66afa84..cdc1fa723 100644 --- a/ffi/src/cpp.rs +++ b/ffi/src/cpp.rs @@ -604,7 +604,7 @@ impl<'a> Header<'a> { // Copies share a handle only where no call tells that apart from a clone, but by an // `Arc`'s or `Rc`'s count, which counts one for every copy. let handle = match object.shared { - true => format!("{d}::shared<::{c}, &::{c}_free, &::{c}_clone>"), + true => format!("{d}::shared<::{c}, &::{c}_free, &::{c}_clone, &::{c}_borrowed>"), false => format!("{d}::unique<::{c}, &::{c}_free>"), }; let mut body = vec![ diff --git a/ffi/src/glue.rs b/ffi/src/glue.rs index 8cacd0826..66fdc6734 100644 --- a/ffi/src/glue.rs +++ b/ffi/src/glue.rs @@ -563,6 +563,11 @@ impl Gen<'_> { self.line(format!( "#[unsafe(no_mangle)]\npub unsafe extern \"C\" fn {name}_free(this: *const c_void) {{\n\tunsafe {{ rt::Object::<{path}>::free(this) }}\n}}\n" )); + if object.shared { + self.line(format!( + "#[unsafe(no_mangle)]\npub unsafe extern \"C\" fn {name}_borrowed(this: *const c_void) -> bool {{\n\tunsafe {{ rt::Object::<{path}>::borrowed(this, \"{name}_borrowed\") }}\n}}\n" + )); + } let borrow = self.shared(i); self.traits(&name, &path, object.traits, |e| { format!("unsafe {{ rt::Object::<{path}>::{borrow}({e}, w) }}") diff --git a/ffi/tests/api.cpp b/ffi/tests/api.cpp index 8b0751064..a01aa9932 100644 --- a/ffi/tests/api.cpp +++ b/ffi/tests/api.cpp @@ -960,6 +960,21 @@ int lifetimes() { check(std::move(a).into_level() == 2 && api::dropped() == before + 1, "the last copy moves out the value"); } + // A pending future borrows the value as though it held a copy: once the copy that made + // it goes, moving out of the last copy moves out a clone, as Rust moves out of a clone + // while a future borrows another. + { + std::uint32_t before = api::dropped(); + api::gauge a = api::gauge::new_(4); + std::optional made(a); + api::gauge_later_future later = made->later(); + api::waker waker([]() {}); + check(!later.poll(waker), "a future pends"); + made.reset(); + check(std::move(a).into_level() == 4 && api::dropped() == before, "the last copy moves out a clone while a future borrows it"); + check(std::move(later).get() == 4 && api::dropped() == before + 1, "a future reads the value it borrows, and frees it once it finishes"); + } + // Copies of a type whose clone is a value of its own, such as a lock behind a `Box`, // keep apart: copying is `clone()`, which crosses into Rust, and a write through one // clone never shows in the other. @@ -1058,6 +1073,14 @@ int broken(const char *which) { api::counter c = api::counter::new_("c", 0); api::counter d = std::move(c); c.get(); + } else if (std::strcmp(which, "shared moved from") == 0) { + api::gauge g = api::gauge::new_(0); + api::gauge h = std::move(g); + std::move(g).into_level(); + } else if (std::strcmp(which, "shared moved out") == 0) { + api::gauge g = api::gauge::new_(0); + std::move(g).into_level(); + std::move(g).into_level(); } else if (std::strcmp(which, "lent copied") == 0) { static std::optional kept; api::watch(api::gauge::new_(0), [](const api::gauge &lent) { diff --git a/ffi/tests/fixtures.rs b/ffi/tests/fixtures.rs index 8bd5fcc82..8c86fa630 100644 --- a/ffi/tests/fixtures.rs +++ b/ffi/tests/fixtures.rs @@ -209,6 +209,11 @@ fn api_cpp() { for (case, message) in [ ("moved", "api_counter_get: a handle's value was moved out"), ("moved from", "api_counter_get: a handle is null"), + ("shared moved from", "api_gauge_into_level: a handle is null"), + ( + "shared moved out", + "api_gauge_into_level: a handle's value was moved out", + ), ("lent copied", "a lent object was copied past its call"), ("value", "value() of a result that holds an error"), ( diff --git a/quest/m3/README.md b/quest/m3/README.md index 852b7296e..9c5583c69 100644 --- a/quest/m3/README.md +++ b/quest/m3/README.md @@ -47,7 +47,6 @@ generic std gap is planned here even when moq never calls it. - [Python's wake on the loop's own thread](/quest/m3/ffi-python-wake.md) - a wake on the event loop's thread resumes the future with no socket pair - [Python's call cost](/quest/m3/ffi-python-calls.md) - a call on an object costs what BoltFFI's does, or the gap is explained - [No unused import in an export's generated crate](/quest/m3/ffi-unused-c-void.md) - the crate rust2-ffi generates builds without warnings when no export uses `void *` -- [C++: a move out of a shared copy while its future pends](/quest/m3/ffi-cpp-shared-future.md) - the last shared copy moves out by a clone even while a destroyed copy's future pends - [A box's clone_from, and a vector's AsRef and AsMut](/quest/m3/ts-box-vec-conversions.md) - `clone_from` of a `Box` and `AsRef<[T]>` of a `Vec` translate - [A std map with a crate's own BuildHasher](/quest/m3/ts-map-own-hasher.md) - std's and hashbrown's maps built with a crate's own hasher translate for keys that are JS objects - [A crate's own hasher that JS copies](/quest/m3/ts-copied-hasher.md) - an enum `Hasher`, whose `&mut` is a place, hashes as a struct one does diff --git a/quest/m3/ffi-cpp-shared-future.md b/quest/m3/ffi-cpp-shared-future.md deleted file mode 100644 index b3b77f4dc..000000000 --- a/quest/m3/ffi-cpp-shared-future.md +++ /dev/null @@ -1,20 +0,0 @@ -# [S] C++: a move out of a shared copy while its future pends - -## Goal - -In the generated C++, a move out of the last copy of a shared object succeeds, as Rust's -`clone` then move would, even while a future made by a `&self` call on a copy since -destroyed still pends, instead of aborting with "a handle is borrowed". - -## Plan - -Found by [#430](https://github.com/moq-dev/rust2/pull/430), whose design this fixes: -types passing its clone guard hold their handle through `std::shared_ptr`, and a move -out of a copy that others share moves out a clone instead. A pending future borrows -Rust's handle without holding a `shared_ptr`, so `use_count` reads 1 and the move takes -the borrowed value, which Rust refuses. - -Options to weigh: a future holds a `shared_ptr` copy of the handle it borrows, so -`use_count` sees it; or the move asks Rust whether the handle is borrowed and clones if so. -Prove it with a test that destroys the copy a pending future came from, then moves out -of the last copy.