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

[ryanofsky]

Warnings are described in commit messages and were reported by Sjors in bitcoin/bitcoin#31802 (comment)

[hebasto]

Concept ACK.

[ryanofsky]

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

[Sjors]

I added the latest version of this to bitcoin/bitcoin#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 e922bb8 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 a5bff37 that suppresses bitcoin-nontrivial-threadlocal, but it has a comment with rationale.

[ryanofsky]

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]

Maybe I should add some of these explanations in code comments

This might help more people understand the code.

[ryanofsky]

re: #172 (comment)

This might help more people understand the code.

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

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

[Sjors]

ACK 57a65b8

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 a5bff37 that suppresses bitcoin-nontrivial-threadlocal, but it has a comment with rationale.

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

[Sjors]

cc @TheCharlatan

[TheCharlatan]

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

[maflcko]

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

[TheCharlatan]

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

[ryanofsky]

re: #172 (comment)

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]

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]

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]

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

[ryanofsky]

re: #172 (comment)

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 (comment)

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]

ACK 57a65b8

[ryanofsky]

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

[ryanofsky]

re: #172 (comment)

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.