refactor: fix warnings from clang-tidy-20 and bitcoin-tidy

ryanofsky commented at 5:55 pm on May 12, 2025: collaborator

Warnings are described in commit messages and were reported by Sjors in https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2858241771

hebasto commented at 6:56 pm on May 12, 2025: member

Concept ACK.

in include/mp/proxy-io.h:639 in 8d8e9f65f1 outdated

633@@ -634,7 +634,10 @@ void ListenConnections(EventLoop& loop, int fd, InitImpl& init)
634     });
635 }
636 
637-extern thread_local ThreadContext g_thread_context;
638+extern thread_local ThreadContext g_thread_context; // NOLINT(bitcoin-nontrivial-threadlocal)
639+// Silence nonstandard bitcoin tidy error "Variable with non-trivial destructor
640+// cannot be thread_local" which should not a problem on modern platforms, and

maflcko commented at 9:19 am on May 13, 2025:

0// cannot be thread_local" which should not be a problem on modern platforms, and

in include/mp/proxy-types.h:382 in 8d8e9f65f1 outdated

376@@ -377,7 +377,12 @@ struct ClientParam
377         template <size_t I, typename... Args>
378         auto callBuild(Args&&... args) -> std::enable_if_t<(I < sizeof...(Types))>
379         {
380-            callBuild<I + 1>(std::forward<Args>(args)..., std::get<I>(m_client_param->m_values));
381+            // Note: The m_values tuple just consists of lvalue and rvalue
382+            // references, so calling std::move doesn't change the tuple, it
383+            // just choses the std::get overload that returns && instead of &,

maflcko commented at 9:20 am on May 13, 2025:

0            // just chooses the std::get overload that returns && instead of &,

clang-tidy: Fix bugprone-move-forwarding-reference errors

Reported by Sjors Provoost <sjors@sprovoost.nl>
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

c1e8c1a028

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.

3a96cdc18f

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

57a65b8546

ryanofsky force-pushed on May 15, 2025

ryanofsky commented at 7:37 pm on May 15, 2025: collaborator

Rebased 8d8e9f65f1d9faaafe91ca90910c1f96da9d094d -> e6a71ae1e2ca2186fd464ddee19eaefac6fd567c (pr/tidy.1 -> pr/tidy.2, compare) fixing typos and conflict with #165 Updated e6a71ae1e2ca2186fd464ddee19eaefac6fd567c -> a5bff37ad8f5630a738eb1b0bfa60cf01bedbcdf (pr/tidy.2 -> pr/tidy.3, compare) expanding a comment

ryanofsky force-pushed on May 15, 2025

Sjors commented at 7:32 am on May 16, 2025: member

I added the latest version of this to https://github.com/bitcoin/bitcoin/pull/31802, the tidy job is still happy.

Some background for the first two commits: https://clang.llvm.org/extra/clang-tidy/checks/bugprone/move-forwarding-reference.html

With that in mind the first commit seems fine, the second one e922bb8f6a78d496d409272bcbc829c46cb15fd3 is a bit over my head due to the changes in src/mpgen.cpp which I’m not familiar with.

I don’t have an informed opinion on the last commit a5bff37ad8f5630a738eb1b0bfa60cf01bedbcdf that suppresses bitcoin-nontrivial-threadlocal, but it has a comment with rationale.

ryanofsky commented at 12:30 pm on May 16, 2025: collaborator

Thanks @Sjors. Now that this git repo is a subtree and a bitcoin-core project, I would like to (1) stop my previous practice of merging PRs like this that do not have any ACKs and (2) try to encourage more review to happen earlier in the upstream PR’s in this repository rather than later in the downstream PRs in the bitcoin repository.

So would you feel comfortable reviewing more and maybe giving an ACK along the lines of…

Testing this PR and confirming it fixes the warnings
Looking at the code and confirming it only disables one bitcoin-tidy warning and resolves several bugprone-move-forwarding-reference warnings by changing std::move to std::forward in some places and changing lvalue references to rvalue references other places

… or however you might actually review it? I am happy to answer any questions that could help clarify things. The second commit should be the only complicated one here, and I can break it down. It is:

Adding std::forward and std::move several places to preserve lvalue/rvalue status.
Defining 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.
Changing C++ code generator to pass arguments as M0::Fwd<1>(arg1) instead of arg1 in generated code. You could verify this by diffing a generated file like build/src/ipc/capnp/mining.capnp.proxy-client.c++ in the bitcoin build before and after this change.

Maybe I should add some of these explanations in code comments or commit messages too. Would welcome any feedback.

Sjors commented at 3:42 pm on May 16, 2025: member

Maybe I should add some of these explanations in code comments

This might help more people understand the code.

ryanofsky force-pushed on May 20, 2025

ryanofsky commented at 7:46 pm on May 20, 2025: collaborator

re: #172 (comment)

This might help more people understand the code.

Thanks, added new information above to code comments and commit messages.

Updated a5bff37ad8f5630a738eb1b0bfa60cf01bedbcdf -> 57a65b854664c13c9801788eeaf71bdb3e597c81 (pr/tidy.3 -> pr/tidy.4, compare) updating these comments

Sjors commented at 10:11 am on May 23, 2025: member

ACK 57a65b854664c13c9801788eeaf71bdb3e597c81

I studied the second commit more, along with some of mp/gen.cpp, thanks for the extra comments.

I wrote:

I don’t have an informed opinion on the last commit https://github.com/bitcoin-core/libmultiprocess/commit/a5bff37ad8f5630a738eb1b0bfa60cf01bedbcdf that suppresses bitcoin-nontrivial-threadlocal, but it has a comment with rationale.

The original nontrivial-threadlocal check was added in https://github.com/bitcoin/bitcoin/pull/30146 and reviewed by @maflcko and @hebasto, both already on this thread.

Bambibrown approved

Sjors commented at 8:02 am on May 28, 2025: member

cc @TheCharlatan

in include/mp/proxy-io.h:640 in 57a65b8546

633@@ -634,7 +634,10 @@ void ListenConnections(EventLoop& loop, int fd, InitImpl& init)
634     });
635 }
636 
637-extern thread_local ThreadContext g_thread_context;
638+extern thread_local ThreadContext g_thread_context; // NOLINT(bitcoin-nontrivial-threadlocal)
639+// Silence nonstandard bitcoin tidy error "Variable with non-trivial destructor
640+// cannot be thread_local" which should not be a problem on modern platforms, and
641+// could lead to a small memory leak at worst on older ones.

TheCharlatan commented at 12:59 pm on May 28, 2025:

Mmh, the original PR that triggered the introduction of this check (https://github.com/bitcoin/bitcoin/pull/30095), links to a freebsd bug that seems to have been fixed by now. @vasild is this check even still relevant at all?

maflcko commented at 2:17 pm on May 28, 2025:

My understanding is that we try to avoid it purely out of style/preference: https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1608289449. But no strong opinion.

TheCharlatan commented at 8:14 am on May 29, 2025:

I’ve read through the history again of it again and came to the same conclusion.

ryanofsky commented at 5:16 pm on May 29, 2025:

re: #172 (review)

My understanding is that we try to avoid it purely out of style/preference: bitcoin/bitcoin#30095 (comment). But no strong opinion.

Agree in general it’s good to avoid thread_local destructors. Code will be more clear and explicit if it joins threads and deletes any state associated with them after they are joined, instead adding isolated thread_local variables that get initialized and cleaned up less deterministically.

But in this case, the libmultiprocess library needs to track state associated with threads that it doesn’t have any control of.

In the bitcoin codebase, node, wallet, gui, and RPC threads are all able to make IPC calls, and the IPC implementation needs to track information about threads that make IPC calls so it can guarantee calls from the same local thread always run on the same remote thread, and guarantee that if the remote thread makes a callback it will run on the same local thread (otherwise things like recursive locks will not work). The IPC implementation also should be able to free state associated with threads when they are destroyed, so it uses a thread_local struct to do all these things.

I think keeping the bitcoin-tidy check but allowing use of a thread_local destructor here to detect when unmanaged external threads are destroyed is a reasonable fix. Other possible fixes not relying on thread_local might be possible too, but would involve other tradeoffs: they might rely on OS specific calls instead, or change the memory model to delete state when connections are closed not when threads are destroyed, or change the threading model to not always run calls from same IPC client thread on same IPC server thread. These fixes would add complexity other places, so I don’t think would be preferable.

Sjors commented at 5:03 am on May 30, 2025:

guarantee calls from the same local thread always run on the same remote thread, and guarantee that if the remote thread makes a callback it will run on the same local thread

I assume you mean that local threads correspond one to one with remote threads, perhaps even sharing the same name, but they’re not actually the same thread given that they’re in a different process?

Sjors commented at 5:07 am on May 30, 2025:

I’m curious to learn how non-multiprocess clients like #174 experience this one-to-one mapping.

Even in my own Sv2 sidecar application I haven’t really paid attention to this. I guess it just works(tm).

vasild commented at 11:53 am on June 2, 2025:

Yeah, my understanding is that modern FreeBSD versions should be fine with this.

ryanofsky commented at 2:41 pm on June 3, 2025:

re: #172 (review)

I’m curious to learn how non-multiprocess clients like #174 experience this one-to-one mapping.

When you are using the libmultiprocess library as the IPC client, it forces you into the 1:1 threading model. The first time you make an IPC call from a local thread, the library will automatically make a remote ThreadMap.makeThread call to execute the current IPC request and any future requests on.

But if you are using the Cap’n Proto interface directly from rust or c++ without libmultiprocess, you don’t need to have a 1:1 threading model. You can create remote threads freely and make any IPC method call which takes a Context argument run on whichever thread you specify in the Context.thread field.

re: #172 (review)

I assume you mean that local threads correspond one to one with remote threads, perhaps even sharing the same name, but they’re not actually the same thread given that they’re in a different process?

Yes that’s right. The sentence is also trying to say if a local thread makes an IPC call, and during that IPC call the remote thread needs to call back into the local process, the callback will be executed on literally the same thread that made the first IPC call and is currently waiting for the response.

It’s important for this to happen, for example, if the thread making the first IPC call locks a recursive mutex before the call. A callback from the server to the client will need to run on literally the same client thread to avoid deadlocking if it locks the mutex recursively.

TheCharlatan commented at 8:15 am on May 29, 2025: collaborator

ACK 57a65b854664c13c9801788eeaf71bdb3e597c81

ryanofsky commented at 5:50 pm on May 29, 2025: collaborator

Thanks for the reviews! Will try to merge this and update the subtree

ryanofsky merged this on May 29, 2025

ryanofsky closed this on May 29, 2025

ryanofsky referenced this in commit 154af1eea1 on May 29, 2025

fanquake referenced this in commit 9393aeeca4 on May 30, 2025

ryanofsky referenced this in commit 8b96229da5 on Jun 3, 2025

ryanofsky referenced this in commit a1ca07692b on Jun 5, 2025

ryanofsky referenced this in commit 59030c68cb on Jun 11, 2025

saikiran57 referenced this in commit 8066d9c3d2 on Jul 28, 2025

janus referenced this in commit fcaf9ff8af on Sep 14, 2025

refactor: fix warnings from clang-tidy-20 and bitcoin-tidy #172