From c1e8c1a02864d015cf4026e1bfb5eb1abdba5bc4 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 12 May 2025 10:51:04 -0400 Subject: [PATCH 1/3] clang-tidy: Fix bugprone-move-forwarding-reference errors Reported by Sjors Provoost https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2858241771 /ci_container_base/include/mp/proxy-io.h:252:16: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors] /ci_container_base/include/mp/proxy-io.h:336:18: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors] /ci_container_base/include/mp/util.h:186:12: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors] https://cirrus-ci.com/task/6187773452877824?logs=ci#L4712 --- include/mp/proxy-io.h | 4 ++-- include/mp/util.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/mp/proxy-io.h b/include/mp/proxy-io.h index be0d4f87..ac750636 100644 --- a/include/mp/proxy-io.h +++ b/include/mp/proxy-io.h @@ -249,7 +249,7 @@ struct Waiter { const std::unique_lock lock(m_mutex); assert(!m_fn); - m_fn = std::move(fn); + m_fn = std::forward(fn); m_cv.notify_all(); } @@ -333,7 +333,7 @@ class Connection // to the EventLoop TaskSet to avoid "Promise callback destroyed itself" // error in cases where f deletes this Connection object. m_on_disconnect.add(m_network.onDisconnect().then( - [f = std::move(f), this]() mutable { m_loop.m_task_set->add(kj::evalLater(kj::mv(f))); })); + [f = std::forward(f), this]() mutable { m_loop.m_task_set->add(kj::evalLater(kj::mv(f))); })); } EventLoop& m_loop; diff --git a/include/mp/util.h b/include/mp/util.h index ebfc3b5e..8b802abc 100644 --- a/include/mp/util.h +++ b/include/mp/util.h @@ -183,7 +183,7 @@ struct AsyncCallable template AsyncCallable> MakeAsyncCallable(Callable&& callable) { - return std::move(callable); + return std::forward(callable); } //! Format current thread name as "{exe_name}-{$pid}/{thread_name}-{$tid}". From 3a96cdc18f2d1ca202fbc91551f27097fd7ec7f6 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 12 May 2025 10:51:04 -0400 Subject: [PATCH 2/3] clang-tidy: Fix bugprone-move-forwarding-reference error /ci_container_base/include/mp/type-interface.h:47:75: error: forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead [bugprone-move-forwarding-reference,-warnings-as-errors] https://cirrus-ci.com/task/6187773452877824?logs=ci#L4712 Error was caused by mp/type-interface.h CustomBuildField field using std::move instead of std::forward. This commit: - Adds std::forward and std::move several places to preserve lvalue/rvalue status. - Defines a FunctionTraits::Fwd<...>(...) helper which does same thing as std::forward except it takes parameter number instead of a parameter type so generated code doesn't need as verbose as std::forward calls would be. - Changes C++ code generator to pass arguments as `M0::Fwd<1>(arg1)` instead of `arg1` in generated code. This change can be verified by diffing a generated file like build/src/ipc/capnp/mining.capnp.proxy-client.c++ in the bitcoin build before and after this change. Unlike the previous commit which resolved bugprone-move-forwarding-reference errors by just switching std::move to std::forward, this commit required more changes because just switching from std::move to std::forward in the type-interface.h CustomBuildField function would lead to another error in the CustomMakeProxyServer function there which is expecting an rvalue, not an lvalue. That error could have alternately been fixed by changing CustomMakeProxyServer to accept lvalues and copy the shared_ptr. But this would be slightly less efficient and it is better to resolve the problem at the root and just use perfect forwarding everywhere, all the way up the call stack. This required the changes to the code generator and clientInvoke described above. --- include/mp/proxy-types.h | 13 ++++++++++--- include/mp/proxy.h | 13 ++++++++++++- include/mp/type-interface.h | 2 +- src/mp/gen.cpp | 17 ++++++++++++----- 4 files changed, 35 insertions(+), 10 deletions(-) diff --git a/include/mp/proxy-types.h b/include/mp/proxy-types.h index b35264d2..a74c6de0 100644 --- a/include/mp/proxy-types.h +++ b/include/mp/proxy-types.h @@ -385,7 +385,7 @@ struct ClientException template struct ClientParam { - ClientParam(Types&&... values) : m_values(values...) {} + ClientParam(Types&&... values) : m_values{std::forward(values)...} {} struct BuildParams : IterateFieldsHelper { @@ -399,7 +399,14 @@ struct ClientParam ParamList(), Priority<1>(), std::forward(values)..., Make(params)); }; - std::apply(fun, m_client_param->m_values); + // Note: The m_values tuple just consists of lvalue and rvalue + // references, so calling std::move doesn't change the tuple, it + // just causes std::apply to call the std::get overload that returns + // && instead of &, so rvalue references are preserved and not + // turned into lvalue references. This allows the BuildField call to + // move from the argument if it is an rvalue reference or was passed + // by value. + std::apply(fun, std::move(m_client_param->m_values)); } BuildParams(ClientParam* client_param) : m_client_param(client_param) {} @@ -577,7 +584,7 @@ void serverDestroy(Server& server) //! //! ProxyClient::M0::Result ProxyClient::methodName(M0::Param<0> arg0, M0::Param<1> arg1) { //! typename M0::Result result; -//! clientInvoke(*this, &InterfaceName::Client::methodNameRequest, MakeClientParam<...>(arg0), MakeClientParam<...>(arg1), MakeClientParam<...>(result)); +//! clientInvoke(*this, &InterfaceName::Client::methodNameRequest, MakeClientParam<...>(M0::Fwd<0>(arg0)), MakeClientParam<...>(M0::Fwd<1>(arg1)), MakeClientParam<...>(result)); //! return result; //! } //! diff --git a/include/mp/proxy.h b/include/mp/proxy.h index 76be0992..e7faad9a 100644 --- a/include/mp/proxy.h +++ b/include/mp/proxy.h @@ -181,7 +181,8 @@ struct ProxyServerCustom : public ProxyServerBase //! //! Params - TypeList of C++ ClassName::methodName parameter types //! Result - Return type of ClassName::method -//! Param - helper to access individual parameters by index number. +//! Param - helper to access individual parameter types by index number. +//! Fwd - helper to forward arguments by index number. //! Fields - helper alias that appends Result type to the Params typelist if //! it not void. template @@ -199,6 +200,16 @@ struct FunctionTraits<_Result (_Class::*const)(_Params...)> using Param = typename std::tuple_element>::type; using Fields = std::conditional_t, Params, TypeList<_Params..., _Result>>; + + //! Enable perfect forwarding for clientInvoke calls. If parameter is a + //! value type or rvalue reference type, pass it as an rvalue-reference to + //! MakeClientParam and BuildField calls so it can be moved from, and if it + //! is an lvalue reference, pass it an lvalue reference so it won't be moved + //! from. This method does the same thing as std::forward except it takes a + //! parameter number instead of a type as a template argument, so generated + //! code calling this can be less repetitive and verbose. + template + static decltype(auto) Fwd(Param& arg) { return static_cast&&>(arg); } }; //! Traits class for a proxy method, providing the same diff --git a/include/mp/type-interface.h b/include/mp/type-interface.h index 99adf2ab..8a89ac24 100644 --- a/include/mp/type-interface.h +++ b/include/mp/type-interface.h @@ -44,7 +44,7 @@ void CustomBuildField(TypeList>, { if (value) { using Interface = typename decltype(output.get())::Calls; - output.set(CustomMakeProxyServer(invoke_context, std::move(value))); + output.set(CustomMakeProxyServer(invoke_context, std::forward(value))); } } diff --git a/src/mp/gen.cpp b/src/mp/gen.cpp index a50fdbdf..3d841a3f 100644 --- a/src/mp/gen.cpp +++ b/src/mp/gen.cpp @@ -512,10 +512,20 @@ static void Generate(kj::StringPtr src_prefix, add_accessor(field_name); + std::ostringstream fwd_args; for (int i = 0; i < field.args; ++i) { if (argc > 0) client_args << ","; + + // Add to client method parameter list. client_args << "M" << method_ordinal << "::Param<" << argc << "> " << field_name; if (field.args > 1) client_args << i; + + // Add to MakeClientParam argument list using Fwd helper for perfect forwarding. + if (i > 0) fwd_args << ", "; + fwd_args << "M" << method_ordinal << "::Fwd<" << argc << ">(" << field_name; + if (field.args > 1) fwd_args << i; + fwd_args << ")"; + ++argc; } client_invoke << ", "; @@ -529,13 +539,10 @@ static void Generate(kj::StringPtr src_prefix, client_invoke << "Accessor<" << base_name << "_fields::" << Cap(field_name) << ", " << field_flags.str() << ">>("; - if (field.retval || field.args == 1) { + if (field.retval) { client_invoke << field_name; } else { - for (int i = 0; i < field.args; ++i) { - if (i > 0) client_invoke << ", "; - client_invoke << field_name << i; - } + client_invoke << fwd_args.str(); } client_invoke << ")"; From 57a65b854664c13c9801788eeaf71bdb3e597c81 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Mon, 12 May 2025 10:51:04 -0400 Subject: [PATCH 3/3] clang-tidy: Suppress bitcoin-nontrivial-threadlocal error /ci_container_base/include/mp/proxy-io.h:637:1: error: Variable with non-trivial destructor cannot be thread_local. [bitcoin-nontrivial-threadlocal,-warnings-as-errors] https://cirrus-ci.com/task/6187773452877824?logs=ci#L4720 --- include/mp/proxy-io.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/include/mp/proxy-io.h b/include/mp/proxy-io.h index ac750636..dff8c2a6 100644 --- a/include/mp/proxy-io.h +++ b/include/mp/proxy-io.h @@ -634,7 +634,10 @@ void ListenConnections(EventLoop& loop, int fd, InitImpl& init) }); } -extern thread_local ThreadContext g_thread_context; +extern thread_local ThreadContext g_thread_context; // NOLINT(bitcoin-nontrivial-threadlocal) +// Silence nonstandard bitcoin tidy error "Variable with non-trivial destructor +// cannot be thread_local" which should not be a problem on modern platforms, and +// could lead to a small memory leak at worst on older ones. } // namespace mp