From e8f72cfa5f5e7de6cc69d8be4876b1bde39ca19f Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 1 Jul 2026 19:57:23 -0400 Subject: [PATCH 1/4] proxy: eliminate default-constructor and move requirements for IPC return types Previously the generated client declared a default-constructed result variable and passed a reference to it into clientInvoke, requiring the return type to be default-constructible. Restructure the client return path so the value is constructed directly in place: - clientInvoke takes the return type and result Accessor as explicit template arguments (defaulting to void). It deserializes the result with ReadField into a ReadDestTemp, so the value is built from constructor arguments without a default constructor. - Propagate the value as a prvalue: IterateFieldsHelper::handleChain now returns decltype(auto), and the movable path uses C++17 guaranteed copy elision to construct the result in AlignedStorage. - For non-movable return types (no move or copy constructor), copy the capnp response to a flat word buffer on the event-loop thread and deserialize it on the client thread, returning a prvalue via guaranteed copy elision so no move constructor is ever needed. Add AlignedStorage, a typed wrapper around an aligned byte buffer with a ptr() accessor, used immediately by clientInvoke and reused later by TryFinally. Extend FunctionTraits to all four PMF cv-qualifier combinations so clientInvoke can derive the capnp Results type from the request method pointer. The code generator emits clientInvoke(...) for non-void methods and plain clientInvoke(...) for void methods. Co-Authored-By: Claude Opus 4.8 --- include/mp/proxy-types.h | 98 ++++++++++++++++++++++++++++++++++------ include/mp/proxy.h | 15 +++++- include/mp/util.h | 9 ++++ src/mp/gen.cpp | 32 ++++++------- 4 files changed, 119 insertions(+), 35 deletions(-) diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index b790dd39..04f1acc2 100644 --- a/include/mp/proxy-types.h +++ b/include/mp/proxy-types.h @@ -7,6 +7,7 @@ #include +#include #include #include #include @@ -402,18 +403,19 @@ template struct IterateFieldsHelper { template - void handleChain(Arg1& arg1, Arg2& arg2, ParamList, NextFn&& next_fn, NextFnArgs&&... next_fn_args) + decltype(auto) handleChain(Arg1& arg1, Arg2& arg2, ParamList, NextFn&& next_fn, NextFnArgs&&... next_fn_args) { using S = Split; handleChain(arg1, arg2, typename S::First()); - next_fn.handleChain(arg1, arg2, typename S::Second(), + return next_fn.handleChain(arg1, arg2, typename S::Second(), std::forward(next_fn_args)...); } template - void handleChain(Arg1& arg1, Arg2& arg2, ParamList) + decltype(auto) handleChain(Arg1& arg1, Arg2& arg2, ParamList) { - static_cast(this)->handleField(arg1, arg2, ParamList()); + using S = Split; + return static_cast(this)->handleField(arg1, arg2, typename S::First()); } private: IterateFieldsHelper() = default; @@ -679,17 +681,22 @@ void serverDestroy(Server& server) MP_LOG(*server.m_context.loop, Log::Debug) << "IPC server destroy " << CxxTypeName(server); } -//! Entry point called by generated client code that looks like: +//! Entry point called by generated client code. ReturnType and ReturnAccessor +//! both default to void for void methods. For non-void methods the code +//! generator supplies them as explicit template arguments: //! -//! ProxyClient::M0::Result ProxyClient::methodName(M0::Param<0> arg0, M0::Param<1> arg1) { -//! typename M0::Result result; -//! clientInvoke(*this, &InterfaceName::Client::methodNameRequest, MakeClientParam<...>(M0::Fwd<0>(arg0)), MakeClientParam<...>(M0::Fwd<1>(arg1)), MakeClientParam<...>(result)); -//! return result; -//! } +//! Void return: +//! clientInvoke(*this, &InterfaceName::Client::methodRequest, +//! MakeClientParam<...>(M0::Fwd<0>(arg0)), ...); //! -//! Ellipses above are where generated Accessor<> type declarations are inserted. -template -void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, FieldObjs&&... fields) +//! Non-void return: +//! return clientInvoke( +//! *this, &InterfaceName::Client::methodRequest, +//! MakeClientParam<...>(M0::Fwd<0>(arg0)), ...); +//! +//! Ellipses are where Accessor<> type declarations are inserted by the code generator. +template +ReturnType clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, FieldObjs&&... fields) { if (!CurrentThread().waiter) { assert(CurrentThread().thread_name.empty()); @@ -714,6 +721,25 @@ void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, Fiel std::string kj_exception; bool done = false; const char* disconnected = nullptr; + + // Storage for the return value when it is move-constructible; + // std::true_type is a trivial placeholder for the void and non-movable + // cases (where response_copy is used instead). AlignedStorage is used + // instead of std::optional so that placement new can construct the value + // directly from a ReadField return value via C++17 guaranteed copy elision. + // optional::emplace would not work because it would treat the ReadField + // return value as an rvalue to be moved from instead of a copy required to + // be elided. + using ResultStorageT = std::conditional_t< + !std::is_void_v && std::is_move_constructible_v, + ReturnType, std::true_type>; + AlignedStorage result_storage; + bool result_constructed{false}; + // capnp Results type for the non-movable return path. + using InvokeResults [[maybe_unused]] = typename CapRequestTraits< + typename FunctionTraits>::Result>::Results; + kj::Array response_copy; + proxy_client.m_context.loop->sync([&]() { if (!proxy_client.m_context.connection) { const Lock lock(thread_context.waiter->m_mutex); @@ -744,6 +770,30 @@ void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, Fiel try { IterateFields().handleChain( *invoke_context, response, FieldList(), typename FieldObjs::ReadResults{&fields}...); + if constexpr (!std::is_void_v) { + if constexpr (std::is_move_constructible_v) { + new (result_storage.ptr()) ReturnType(ReadField(TypeList(), *invoke_context, + Make(response), + ReadDestTemp())); + result_constructed = true; + } else { + // Non-movable return type: the value cannot be moved + // from the event-loop thread to the client thread, so + // copy the entire capnp response to a flat word buffer + // and deserialize it after wait() on the client thread, + // returning a prvalue with no move constructor needed. + // Copying the whole response is inefficient, but + // non-movable return types are rare and their responses + // should be small. If large responses ever need this + // path, an alternative would be to keep the response + // alive across threads and release it afterward, at the + // cost of more complexity and possible thread-switch + // overhead. + capnp::MallocMessageBuilder builder; + builder.setRoot(static_cast(response)); + response_copy = capnp::messageToFlatArray(builder); + } + } } catch (...) { exception = std::current_exception(); } @@ -767,9 +817,29 @@ void clientInvoke(ProxyClient& proxy_client, const GetRequest& get_request, Fiel Lock lock(thread_context.waiter->m_mutex); thread_context.waiter->wait(lock, [&done]() { return done; }); - if (exception) std::rethrow_exception(exception); + if (exception) { + if constexpr (!std::is_void_v && std::is_move_constructible_v) { + if (result_constructed) result_storage.ptr()->~ReturnType(); + } + std::rethrow_exception(exception); + } if (!kj_exception.empty()) MP_LOGPLAIN(*proxy_client.m_context.loop, Log::Raise) << kj_exception; if (disconnected) MP_LOGPLAIN(*proxy_client.m_context.loop, Log::Raise) << disconnected; + if constexpr (!std::is_void_v) { + if constexpr (std::is_move_constructible_v) { + ReturnType* ptr = result_storage.ptr(); + struct Guard { ReturnType* p; ~Guard() { p->~ReturnType(); } } guard{ptr}; + return std::move(*ptr); + } else { + // Non-movable: deserialize return value from copied response on the client thread. + // ReadField returns a prvalue; returning it triggers C++17 guaranteed copy elision. + capnp::FlatArrayMessageReader msg(response_copy.asPtr()); + auto results_reader = msg.getRoot(); + return ReadField(TypeList(), *invoke_context, + Make(results_reader), + ReadDestTemp()); + } + } } //! Invoke callable `fn()` that may return void. If it does return void, replace diff --git a/include/mp/proxy.h b/include/mp/proxy.h index 2144b571..005e7546 100644 --- a/include/mp/proxy.h +++ b/include/mp/proxy.h @@ -219,8 +219,8 @@ template struct FunctionTraits; //! Specialization of above extracting result and params types assuming the -//! template argument is a pointer-to-method type, -//! decltype(&ClassName::methodName) +//! template argument is a pointer-to-method type. Handles all four PMF +//! cv-qualifier combinations by forwarding to this canonical form. template struct FunctionTraits<_Result (_Class::*const)(_Params...)> { @@ -241,6 +241,17 @@ struct FunctionTraits<_Result (_Class::*const)(_Params...)> template static decltype(auto) Fwd(Param& arg) { return static_cast&&>(arg); } }; +// non-const pointer to non-const method +template +struct FunctionTraits<_Result (_Class::*)(_Params...)> + : FunctionTraits<_Result (_Class::*const)(_Params...)> {}; +// (either pointer kind) to const method — collapses const-method distinction +template +struct FunctionTraits<_Result (_Class::*const)(_Params...) const> + : FunctionTraits<_Result (_Class::*const)(_Params...)> {}; +template +struct FunctionTraits<_Result (_Class::*)(_Params...) const> + : FunctionTraits<_Result (_Class::*const)(_Params...)> {}; //! Traits class for a proxy method, providing the same //! Params/Result/Param/Fields described in the FunctionTraits class above, plus diff --git a/include/mp/util.h b/include/mp/util.h index eac655be..11d7632a 100644 --- a/include/mp/util.h +++ b/include/mp/util.h @@ -247,6 +247,15 @@ void Unlock(Lock& lock, Callback&& callback) callback(); } +//! Uninitialized aligned storage for a single T value. Provides a ptr() +//! accessor to avoid repeated reinterpret_cast/std::launder boilerplate. +template +struct AlignedStorage { + alignas(T) std::byte data[sizeof(T)]; + T* ptr() { return std::launder(reinterpret_cast(data)); } + const T* ptr() const { return std::launder(reinterpret_cast(data)); } +}; + //! Invoke a function and run a follow-up action before returning the original //! result. //! diff --git a/src/mp/gen.cpp b/src/mp/gen.cpp index 7733eba6..0d5118f3 100644 --- a/src/mp/gen.cpp +++ b/src/mp/gen.cpp @@ -552,6 +552,7 @@ static void Generate(kj::StringPtr src_prefix, std::ostringstream client_args; std::ostringstream client_invoke; + std::string client_invoke_return; std::ostringstream server_invoke_start; std::ostringstream server_invoke_end; int argc = 0; @@ -578,22 +579,16 @@ static void Generate(kj::StringPtr src_prefix, ++argc; } - client_invoke << ", "; - if (field.exception.size()) { - client_invoke << "ClientException<" << field.exception << ", "; - } else { - client_invoke << "MakeClientParam<"; - } - - client_invoke << AccessorType(base_name, field) << ">("; - - if (field.retval) { - client_invoke << field_name; + client_invoke << ", ClientException<" << field.exception << ", " + << AccessorType(base_name, field) << ">()"; + } else if (field.retval) { + client_invoke_return = ""; } else { - client_invoke << fwd_args.str(); + client_invoke << ", MakeClientParam<" << AccessorType(base_name, field) << ">(" + << fwd_args.str() << ")"; } - client_invoke << ")"; if (field.exception.size()) { server_invoke_start << "Make::M" << method_ordinal << "::Result ProxyClient<" << message_namespace << "::" << node_name << ">::" << method_name << "(" << super_str << client_args.str() << ") {\n"; - if (fields.has_result) { - def_client << " typename M" << method_ordinal << "::Result result;\n"; - } - def_client << " clientInvoke(" << self_str << ", &" << message_namespace << "::" << node_name - << "::Client::" << method_name << "Request" << client_invoke.str() << ");\n"; - if (fields.has_result) def_client << " return result;\n"; + if (fields.has_result) def_client << " return "; + else def_client << " "; + def_client << "clientInvoke" << client_invoke_return << "(" << self_str << ", &" << message_namespace + << "::" << node_name << "::Client::" << method_name << "Request" + << client_invoke.str() << ");\n"; def_client << "}\n"; server << " kj::Promise " << method_name << "(" << Cap(method_name) From 935dc4915c8b53db490956ecf5ce21ea311d2349 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 1 Jul 2026 21:42:17 -0400 Subject: [PATCH 2/4] proxy: eliminate move-constructor requirements for IPC return types Restructure result handling on the server side to eliminate move-constructor requirements for IPC return types. (Client side move-constructor requirements were removed in the previous commit.) - Add 3-arg TryFinally(fn, after, consume) overload in util.h that stores fn()'s return value via placement new from prvalue (C++17 guaranteed copy elision) and passes it by reference to consume(), then destroys it. This avoids ever needing to move the result. - Merge ServerRet into ServerCall by making ServerCall a template parameterized on the result Accessor (void for void methods). The result is now serialized inside the TryFinally after() callback, forwarding it with the invoked method's value category so move-only results (e.g. vector>) are moved rather than copied. - Update code generator to emit ServerCall() or ServerCall() instead of Make(ServerCall()). Behavior-preserving: results are serialized exactly as before. These changes are combined because they are tightly coupled: redesigning TryFinally to return void forces ServerCall::invoke() to also return void, which breaks ServerRet's existing `auto&& result = Parent::invoke(...)` binding. The only clean intermediate would have ServerRet duplicate the same logic that ServerCall ends up with, so combining produces a simpler result. Co-Authored-By: Claude Opus 4.8 --- include/mp/proxy-types.h | 54 +++++++++++++++++++------------ include/mp/util.h | 68 +++++++++++++++++++++++++++------------- src/mp/gen.cpp | 17 ++++++---- 3 files changed, 92 insertions(+), 47 deletions(-) diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index 04f1acc2..fb2090ea 100644 --- a/include/mp/proxy-types.h +++ b/include/mp/proxy-types.h @@ -517,11 +517,16 @@ ClientParam MakeClientParam(Types&&... values) return {std::forward(values)...}; } +//! Terminal node in the server-side invoke chain. +//! ReturnAccessor = void for void methods; for non-void methods it is the +//! capnp Accessor for the result field. The code generator emits either +//! ServerCall() [void] or ServerCall>() [non-void]. +template struct ServerCall { // FIXME: maybe call call_context.releaseParams() template - decltype(auto) invoke(ServerContext& server_context, TypeList<>, Args&&... args) const + void invoke(ServerContext& server_context, TypeList<>, Args&&... args) const { // If cancel_lock is set, release it while executing the method, and // reacquire it afterwards. The lock is needed to prevent params and @@ -531,13 +536,21 @@ struct ServerCall // because the method can take arbitrarily long to return and the event // loop will need the lock itself in on_cancel if the call is canceled. if (server_context.cancel_lock) server_context.cancel_lock->m_lock.unlock(); - return TryFinally( + InvokeContext& invoke_context = server_context; + // Return type of the invoked method, used below to forward the result + // to BuildField with its original value category: by-value/rvalue + // results are moved from the temporary stored by TryFinally, while + // lvalue-reference results are passed through as lvalues. + using MethodResult = decltype(ProxyServerMethodTraits< + typename decltype(server_context.call_context.getParams())::Reads + >::invoke(server_context, std::forward(args)...)); + TryFinally( [&]() -> decltype(auto) { return ProxyServerMethodTraits< typename decltype(server_context.call_context.getParams())::Reads >::invoke(server_context, std::forward(args)...); }, - [&] { + [&](auto* result) { if (server_context.cancel_lock) server_context.cancel_lock->m_lock.lock(); // If the IPC request was canceled, throw InterruptException // because there is no point continuing and trying to fill the @@ -552,6 +565,20 @@ struct ServerCall // returned to the caller, so it needs to be discarded like // other result values. if (server_context.request_canceled) throw InterruptException{"canceled"}; + // result is null if the method threw; skip serialization in that case. + if constexpr (!std::is_void_v) { + if (result) { + // getResults() is safe to call here since the cancel check above + // ensures the connection is still alive. + auto&& results = server_context.call_context.getResults(); + // Forward *result with MethodResult's value category so + // move-only results (e.g. vector>) + // can be moved into the response rather than copied. + BuildField(TypeList>(), + invoke_context, Make(results), + static_cast(*result)); + } + } }); } }; @@ -565,22 +592,6 @@ struct ServerDestroy } }; -template -struct ServerRet : Parent -{ - ServerRet(Parent parent) : Parent(parent) {} - - template - void invoke(ServerContext& server_context, TypeList<>, Args&&... args) const - { - auto&& result = Parent::invoke(server_context, TypeList<>(), std::forward(args)...); - auto&& results = server_context.call_context.getResults(); - InvokeContext& invoke_context = server_context; - BuildField(TypeList(), invoke_context, Make(results), - std::forward(result)); - } -}; - template struct ServerExcept : Parent { @@ -861,7 +872,10 @@ extern std::atomic server_reqs; //! Entry point called by generated server code that looks like: //! //! kj::Promise ProxyServer::methodName(CallContext call_context) { -//! return serverInvoke(*this, call_context, MakeServerField<0, ...>(MakeServerField<1, ...>(Make(ServerCall())))); +//! return serverInvoke(*this, call_context, +//! MakeServerField<1, ...>(ServerCall<...>())); // non-void +//! return serverInvoke(*this, call_context, +//! MakeServerField<0, ...>(ServerCall())); // void //! } //! //! Ellipses above are where generated Accessor<> type declarations are inserted. diff --git a/include/mp/util.h b/include/mp/util.h index 11d7632a..4a9fe608 100644 --- a/include/mp/util.h +++ b/include/mp/util.h @@ -256,35 +256,61 @@ struct AlignedStorage { const T* ptr() const { return std::launder(reinterpret_cast(data)); } }; -//! Invoke a function and run a follow-up action before returning the original -//! result. +//! Invoke fn() then run an after() callback. Like KJ_DEFER but works better +//! when after() can throw: avoids clang bug +//! https://github.com/llvm/llvm-project/issues/12658 which skips destructors, +//! and when both functions throw lets one exception take precedence. //! -//! This can be used similarly to KJ_DEFER to run cleanup code, but works better -//! if the cleanup function can throw because it avoids clang bug -//! https://github.com/llvm/llvm-project/issues/12658 which skips calling -//! destructors in that case and can lead to memory leaks. Also, if both -//! functions throw, this lets one exception take precedence instead of -//! terminating due to having two active exceptions. +//! after() always receives a pointer whose nullness indicates fn()'s outcome: +//! - Non-null if fn() returned normally. Pointer type and value depend on R: +//! - void: std::true_type* (points to AlignedStorage) +//! - lvalue reference T&: T* pointing to the referenced object +//! - value type T: T* pointing to result in AlignedStorage; C++17 +//! guaranteed copy elision constructs it in place from the prvalue so no +//! move or copy constructor is required (handles non-movable types) +//! - Null if fn() threw; after() is still called so it can do cleanup. +//! If after() throws, its exception takes precedence over any fn() exception. template -decltype(auto) TryFinally(Fn&& fn, After&& after) +void TryFinally(Fn&& fn, After&& after) { bool success{false}; using R = std::invoke_result_t; - try { - if constexpr (std::is_void_v) { - std::forward(fn)(); + if constexpr (std::is_lvalue_reference_v) { + // Lvalue reference return: pass pointer to the referenced object; no + // storage needed since the object's lifetime is managed by the caller. + using StorageT = std::remove_reference_t; + try { + StorageT* ptr = &std::forward(fn)(); success = true; - std::forward(after)(); - return; - } else { - decltype(auto) result = std::forward(fn)(); + std::forward(after)(ptr); + } catch (...) { + if (!success) std::forward(after)(static_cast(nullptr)); + throw; + } + } else { + // Value (or rvalue-reference or void) return. void is mapped to + // std::true_type so the same AlignedStorage pattern covers all cases. + // remove_reference_t handles the rvalue-reference sub-case (move-constructed). + using StorageT = std::conditional_t, std::true_type, std::remove_reference_t>; + AlignedStorage storage; + bool constructed{false}; + try { + if constexpr (std::is_void_v) { + std::forward(fn)(); + new (storage.ptr()) StorageT{}; + } else { + new (storage.ptr()) StorageT(std::forward(fn)()); // C++17 guaranteed copy elision + } + constructed = true; success = true; - std::forward(after)(); - return result; + std::forward(after)(storage.ptr()); + storage.ptr()->~StorageT(); + constructed = false; + } catch (...) { + if (constructed) storage.ptr()->~StorageT(); + if (!success) std::forward(after)(static_cast(nullptr)); + throw; } - } catch (...) { - if (!success) std::forward(after)(); - throw; } } diff --git a/src/mp/gen.cpp b/src/mp/gen.cpp index 0d5118f3..7fe440df 100644 --- a/src/mp/gen.cpp +++ b/src/mp/gen.cpp @@ -555,6 +555,7 @@ static void Generate(kj::StringPtr src_prefix, std::string client_invoke_return; std::ostringstream server_invoke_start; std::ostringstream server_invoke_end; + std::string server_ret_accessor; int argc = 0; for (const auto& field : fields.fields) { if (field.skip) continue; @@ -591,14 +592,16 @@ static void Generate(kj::StringPtr src_prefix, } if (field.exception.size()) { - server_invoke_start << "Make("; + server_invoke_end << ")"; } else if (field.retval) { - server_invoke_start << "Make("; + server_invoke_end << ")"; } - server_invoke_start << ", " << AccessorType(base_name, field) << ">("; - server_invoke_end << ")"; } const std::string static_str{is_construct || is_destroy ? "static " : ""}; @@ -630,8 +633,10 @@ static void Generate(kj::StringPtr src_prefix, << server_invoke_start.str(); if (is_destroy) { def_server << "ServerDestroy()"; + } else if (server_ret_accessor.empty()) { + def_server << "ServerCall()"; } else { - def_server << "ServerCall()"; + def_server << "ServerCall<" << server_ret_accessor << ">()"; } def_server << server_invoke_end.str() << ");\n}\n"; ++method_ordinal; From 09e20bf27206860332a4a691c43a7f5b2f21e8d6 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 1 Jul 2026 19:13:51 -0400 Subject: [PATCH 3/4] test: add Pinned non-movable IPC return and exception tests Add Pinned, a type with no default constructor and no copy or move operations, to exercise the non-movable IPC return path end-to-end. Its CustomReadField uses read_dest.construct() with a ReadDestTemp argument, which is the only way to deserialize a type that can neither be default-constructed (for update()) nor moved (for a std::optional staging variable). Add two FooInterface methods: - returnPinned returns Pinned> by value, verifying the client can retrieve a non-movable return value. - throwPinned throws Pinned> via $Proxy.exception, verifying the exception path constructs a non-movable value as a prvalue. Co-Authored-By: Claude Opus 4.8 --- test/mp/test/foo-types.h | 16 ++++++++++++++++ test/mp/test/foo.capnp | 2 ++ test/mp/test/foo.h | 15 +++++++++++++++ test/mp/test/test.cpp | 11 +++++++++++ 4 files changed, 44 insertions(+) diff --git a/test/mp/test/foo-types.h b/test/mp/test/foo-types.h index b96eabfc..d592e74b 100644 --- a/test/mp/test/foo-types.h +++ b/test/mp/test/foo-types.h @@ -41,6 +41,22 @@ struct FooFn; // IWYU pragma: export struct FooInterface; // IWYU pragma: export } // namespace messages +template +void CustomBuildField(TypeList>, Priority<1>, InvokeContext& invoke_context, Value&& value, Output&& output) +{ + BuildField(TypeList(), invoke_context, output, value.value); +} + +template +decltype(auto) CustomReadField(TypeList>, Priority<1>, InvokeContext& invoke_context, Input&& input, ReadDest&& read_dest) +{ + // read_dest.construct() is used instead of read_dest.update() because Pinned + // has no default constructor, so update()'s default-construct-then-fill path fails. + // ReadDestTemp is required here: without a pre-existing T to pass to + // ReadDestUpdate, we need ReadField to return a T value directly. + return read_dest.construct(ReadField(TypeList(), invoke_context, input, ReadDestTemp())); +} + template void CustomBuildField(TypeList, Priority<1>, InvokeContext& invoke_context, const FooCustom& value, Output&& output) { diff --git a/test/mp/test/foo.capnp b/test/mp/test/foo.capnp index 9e6213fd..2d9d6892 100644 --- a/test/mp/test/foo.capnp +++ b/test/mp/test/foo.capnp @@ -26,6 +26,8 @@ interface FooInterface $Proxy.wrap("mp::test::FooImplementation") { callbackSaved @9 (context :Proxy.Context, arg: Int32) -> (result :Int32); callbackExtended @10 (context :Proxy.Context, callback :ExtendedCallback, arg: Int32) -> (result :Int32); passCustom @11 (arg :FooCustom) -> (result :FooCustom); + returnPinned @26 (vec :List(Int32)) -> (result :List(Int32)); + throwPinned @27 (vec :List(Int32)) -> (error :List(Int32) $Proxy.exception("mp::test::Pinned>")); passEmpty @12 (arg :FooEmpty) -> (result :FooEmpty); passData @24 (arg :Data) -> (result :Data); passMessage @13 (arg :FooMessage) -> (result :FooMessage); diff --git a/test/mp/test/foo.h b/test/mp/test/foo.h index 779c8db1..5f001e7d 100644 --- a/test/mp/test/foo.h +++ b/test/mp/test/foo.h @@ -40,6 +40,19 @@ struct FooEmpty { }; +// Test type that has no default constructor and cannot be copied or moved. +// Used to stress-test the serialization framework's ReadDestTemp path. +template +struct Pinned { + T value; + explicit Pinned(T v) : value(std::move(v)) {} + Pinned() = delete; + Pinned(const Pinned&) = delete; + Pinned& operator=(const Pinned&) = delete; + Pinned(Pinned&&) = delete; + Pinned& operator=(Pinned&&) = delete; +}; + struct FooMessage { std::string message; @@ -83,6 +96,8 @@ class FooImplementation int callbackSaved(int arg) { return m_callback->call(arg); } int callbackExtended(ExtendedCallback& callback, int arg) { return callback.callExtended(arg); } FooCustom passCustom(FooCustom foo) { return foo; } + Pinned> returnPinned(std::vector vec) { return Pinned>{std::move(vec)}; } + void throwPinned(std::vector vec) { throw Pinned>{std::move(vec)}; } FooEmpty passEmpty(FooEmpty foo) { return foo; } FooData passData(FooData foo) { return foo; } FooMessage passMessage(FooMessage foo) { foo.message += " call"; return foo; } diff --git a/test/mp/test/test.cpp b/test/mp/test/test.cpp index f5f35437..4b6bc984 100644 --- a/test/mp/test/test.cpp +++ b/test/mp/test/test.cpp @@ -237,6 +237,17 @@ KJ_TEST("Call FooInterface methods") KJ_EXPECT(custom_in.v1 == custom_out.v1); KJ_EXPECT(custom_in.v2 == custom_out.v2); + std::vector pinned_in = {1, 2, 3}; + Pinned> pinned_out = foo->returnPinned(pinned_in); + KJ_EXPECT(pinned_out.value == pinned_in); + + try { + foo->throwPinned(pinned_in); + KJ_FAIL_EXPECT("throwPinned should have thrown"); + } catch (const Pinned>& e) { + KJ_EXPECT(e.value == pinned_in); + } + foo->passEmpty(FooEmpty{}); FooData empty_data_out = foo->passData(FooData{}); From aa55cddad43fe6931833fdc703b9818a65137414 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 3 Jul 2026 08:03:14 -0400 Subject: [PATCH 4/4] doc: document ReadField destination types and ReadDestTemp Replace the terse ReadDestEmplace comment with structured documentation of the three ReadField destination types (ReadDestEmplace, ReadDestUpdate, and the ReadDestTemp() helper), covering: - The contract for CustomReadField implementors: return decltype(auto) and forward the construct()/update() return value, which is easy to miss because most callers ignore it but is load-bearing when ReadDestTemp() is used. - The contract for emplace callbacks and the three return-type cases (container emplace, ReadDestTemp() prvalue, and the vector reference-like proxy). - When ReadDestTemp() is merely convenient versus strictly necessary, with the Pinned return-value and nested-CustomReadField cases as examples. - Return-value notes on each construct()/update() method and an inline note flagging the placement-new exception-safety gap in ReadDestUpdate::construct. Comment-only change. Co-Authored-By: Claude Opus 4.8 --- include/mp/proxy-types.h | 158 +++++++++++++++++++++++++++++++++------ 1 file changed, 137 insertions(+), 21 deletions(-) diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index fb2090ea..56c64d87 100644 --- a/include/mp/proxy-types.h +++ b/include/mp/proxy-types.h @@ -81,33 +81,98 @@ struct StructField }; - -// Destination parameter type that can be passed to ReadField function as an -// alternative to ReadDestUpdate. It allows the ReadField implementation to call -// the provided emplace_fn function with constructor arguments, so it only needs -// to determine the arguments, and can let the emplace function decide how to -// actually construct the read destination object. For example, if a std::string -// is being read, the ReadField call will call the custom emplace_fn with char* -// and size_t arguments, and the emplace function can decide whether to call the -// constructor via the operator, make_shared, emplace or just return a -// temporary string that is moved from. +//! @par ReadField destination types — ReadDestEmplace, ReadDestUpdate, ReadDestTemp() +//! +//! ReadField and CustomReadField accept a destination argument controlling how +//! the deserialized C++ value is created or updated. Callers choose from: +//! +//! - ReadDestEmplace: provide an emplace callback that constructs the new +//! object. The callback decides where the object lives — directly in a +//! container via emplace_back, inside a std::optional via emplace, as a +//! local temporary, as a thrown exception, or anywhere else. +//! - ReadDestUpdate: provide a reference to an existing object to update in place. +//! - ReadDestTemp(): a helper function (not a class) returning a ReadDestEmplace +//! that constructs a local temporary, allowing ReadField to return the new +//! value directly without a separately declared variable. +//! +//! **Contract for CustomReadField implementors:** Every CustomReadField overload +//! must declare `decltype(auto)` as its return type and return the result of +//! `read_dest.construct(...)` or `read_dest.update(...)` without discarding it. +//! Most ReadField callers ignore the return value, so this requirement is easy +//! to miss. It matters when ReadDestTemp() is passed: in that case, +//! construct() and update() return the newly constructed value as a prvalue, +//! and C++17 guaranteed copy elision propagates it through ReadField to the +//! caller. Discarding the return value or declaring a concrete return type +//! prevents the caller from receiving the value. +//! +//! **Contract for emplace callbacks:** The required return type depends on +//! how the callback is used: +//! +//! - **Typical case (container emplace) — return `auto&`.** The callback +//! should return a reference to the newly created slot. This is needed +//! because ReadDestEmplace::update() calls construct() with no arguments +//! to obtain a mutable reference to the slot, then passes it to the update +//! function. The requirement is easy to miss: for `std::vector`, +//! int's CustomReadField calls construct(value) and discards the return, so +//! a void return appears to work. But for +//! `std::vector>`, shared_ptr's CustomReadField calls +//! update(), which needs the reference to assign a make_shared result to the +//! emplaced slot. Emplace callbacks for generic container types must return +//! `auto&` regardless of the contained type, because which path +//! (construct() vs update()) the contained type's CustomReadField takes is +//! not visible at the point the emplace callback is written. +//! +//! - **ReadDestTemp() case — return a prvalue.** ReadDestTemp()'s emplace +//! callback returns `LocalType{args...}` as a prvalue rather than a +//! reference. Mandatory copy elision propagates this prvalue through +//! construct(), CustomReadField, and ReadField back to the caller, allowing +//! non-movable types to be returned without any move constructor (see +//! ReadDestTemp()). +//! +//! - **Exception — reference-like proxy.** If the contained type's +//! CustomReadField is known to always call construct() and never update(), +//! the emplace callback may safely return a non-reference proxy object +//! rather than a real reference. The only known case is +//! `std::vector`: its emplace callback calls emplace_back() + +//! back(), and back() returns std::vector::reference, a proxy by +//! value rather than a real reference. This is safe because bool's +//! CustomReadField always calls construct(), never update(), so the proxy +//! return value is discarded. Whether there are other legitimate uses for +//! reference-like proxy returns is unclear. + +//! Destination parameter passed to ReadField as an alternative to +//! ReadDestUpdate. Allows ReadField to call the provided emplace_fn with +//! constructor arguments, so ReadField only determines those arguments and +//! leaves the emplace function to decide how to actually create the destination +//! object. For example, when reading a std::string, ReadField calls emplace_fn +//! with char* and size_t arguments, and emplace_fn can choose to call the +//! constructor directly, call make_shared, emplace into a container, or return +//! a temporary to move from. template struct ReadDestEmplace { ReadDestEmplace(TypeList, EmplaceFn emplace_fn) : m_emplace_fn(std::move(emplace_fn)) {} - //! Simple case. If ReadField implementation calls this construct() method - //! with constructor arguments, just pass them on to the emplace function. + //! Simple case. If ReadField calls construct() with constructor arguments, + //! forward them to the emplace function and return its result. + //! + //! The return value is forwarded through the calling CustomReadField (see + //! the group contract section above) and is used when ReadDestTemp() is passed. template decltype(auto) construct(Args&&... args) { return m_emplace_fn(std::forward(args)...); } - //! More complicated case. If ReadField implementation works by calling this - //! update() method, adapt it call construct() instead. This requires - //! LocalType to have a default constructor to create new object that can be - //! passed to update() + //! More complicated case. If ReadField works by calling update(), adapt it + //! to call construct() instead. Calls construct() with no arguments to + //! default-construct an object via the emplace callback (obtaining a + //! reference to it), then passes that reference to update_fn. Requires the + //! emplace callback to be callable with no arguments. Returns the result of + //! construct(). + //! + //! The return value is forwarded through the calling CustomReadField (see + //! the group contract section above) and is used when ReadDestTemp() is passed. template decltype(auto) update(UpdateFn&& update_fn) { @@ -128,8 +193,48 @@ struct ReadDestEmplace EmplaceFn m_emplace_fn; }; -//! Helper function to create a ReadDestEmplace object that constructs a -//! temporary, ReadField can return. +//! Returns a ReadDestEmplace that constructs a local temporary, so ReadField +//! can be called and its result used directly in an expression without +//! declaring a separate variable or container. +//! +//! ReadDestTemp() is mostly a convenience: any type that is +//! default-constructible or movable can be handled without it using +//! ReadDestUpdate or ReadDestEmplace into a std::optional. For example, +//! clientInvoke uses ReadDestTemp() for method return values as a convenience +//! — the alternative would be ReadDestEmplace into a std::optional followed by +//! a move, which is more verbose. +//! +//! For types that are neither default-constructible, copyable, nor movable, +//! ReadDestTemp() can become strictly necessary. Two known cases: +//! +//! **Case 1 — non-movable type as an IPC method return value.** clientInvoke +//! calls ReadField to deserialize the proxy method return value. If the return +//! type (e.g., Pinned, see test/mp/test/foo.h) has no default constructor +//! and is neither copyable nor movable, it cannot be stored in an intermediate +//! std::optional or variable between deserialization and return. ReadDestTemp() +//! allows the value to be constructed directly in the return slot via C++17 +//! guaranteed copy elision. +//! +//! **Case 2 — non-movable type needed inside a CustomReadField +//! implementation.** CustomReadField implementations may call ReadField +//! internally to deserialize sub-values, then pass those values to +//! constructors, functions, or other expressions. If a sub-value type is +//! neither default-constructible nor movable, ReadDestTemp() is the only way +//! to obtain it as a prvalue for immediate use without storing it first. For +//! example, CustomReadField for Pinned (see test/mp/test/foo-types.h) +//! calls read_dest.construct(ReadField(TypeList(), ..., ReadDestTemp())). +//! Using ReadDestTemp() is necessary when T is itself non-movable (e.g., +//! T = Pinned), since neither ReadDestUpdate (requires a pre-existing T) +//! nor ReadDestEmplace into std::optional (requires T to be movable) would +//! work. +//! +//! There may be other cases where ReadDestTemp() is strictly necessary beyond +//! these two. +//! +//! Examples of Bitcoin Core types that lack default constructors and are used +//! in IPC: util::Result, PartiallySignedTransaction, CreatedTransactionResult, +//! WalletAddress. ReadDestTemp() can be useful when constructing or returning +//! values of these types. template auto ReadDestTemp() { @@ -147,7 +252,11 @@ struct ReadDestUpdate { ReadDestUpdate(Value& value) : m_value(value) {} - //! Simple case. If ReadField works by calling update() just forward arguments to update_fn. + //! Simple case. If ReadField works by calling update(), forward arguments + //! to update_fn and return a reference to m_value. + //! + //! The return value is forwarded through the calling CustomReadField (see + //! the group contract section above) and is used when ReadDestTemp() is passed. template Value& update(UpdateFn&& update_fn) { @@ -155,11 +264,18 @@ struct ReadDestUpdate return m_value; } - //! More complicated case. If ReadField works by calling construct(), need - //! to reconstruct m_value in place. + //! More complicated case. If ReadField works by calling construct(), + //! reconstruct m_value in place via explicit destructor call and placement + //! new, and return a reference to m_value. + //! + //! The return value is forwarded through the calling CustomReadField (see + //! the group contract section above) and is used when ReadDestTemp() is passed. template Value& construct(Args&&... args) { + // Exception-unsafe: if ~Value() or the Value constructor throws, + // m_value is left in a destroyed state with no clear recovery path + // (aborting may be the right behavior, but this is unresolved). m_value.~Value(); new (&m_value) Value(std::forward(args)...); return m_value;