Merge libmultiprocess Windows support and MinGW cleanup fix

Rebase bitcoin-core/libmultiprocess#231 onto upstream master
2dba130478 and include the MinGW ThreadContext workaround from
bitcoin/bitcoin#32387. Preserve current upstream race fixes and CI.

The rebased upstream history is retained locally on
codex/libmultiprocess-windows.

Author: Sjors

src/ipc/libmultiprocess/.github/workflows/ci.yml

@@ -148,7 +148,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        config: [default, llvm, gnu32, sanitize, olddeps, newdeps]
+        config: [default, llvm, gnu32, sanitize, olddeps, newdeps, windows]
 
     name: build • ${{ matrix.config }}
 

src/ipc/libmultiprocess/CMakeLists.txt

@@ -193,6 +193,7 @@ target_link_libraries(multiprocess PUBLIC CapnProto::capnp)
 target_link_libraries(multiprocess PUBLIC CapnProto::capnp-rpc)
 target_link_libraries(multiprocess PUBLIC CapnProto::kj)
 target_link_libraries(multiprocess PUBLIC CapnProto::kj-async)
+target_link_libraries(multiprocess PUBLIC Threads::Threads)
 set_target_properties(multiprocess PROPERTIES
     PUBLIC_HEADER "${MP_PUBLIC_HEADERS}")
 install(TARGETS multiprocess EXPORT LibTargets

src/ipc/libmultiprocess/ci/README.md

@@ -22,6 +22,7 @@ CI_CONFIG=ci/configs/gnu32.bash    ci/scripts/run.sh
 CI_CONFIG=ci/configs/sanitize.bash ci/scripts/run.sh
 CI_CONFIG=ci/configs/olddeps.bash  ci/scripts/run.sh
 CI_CONFIG=ci/configs/newdeps.bash  ci/scripts/run.sh
+CI_CONFIG=ci/configs/windows.bash  ci/scripts/run.sh
 ```
 
 By default CI jobs will reuse their build directories. `CI_CLEAN=1` can be specified to delete them before running instead.
@@ -50,4 +51,3 @@ macOS or Linux:
     --matrix config:sanitize
   ```
 
-

src/ipc/libmultiprocess/ci/configs/windows.bash

@@ -0,0 +1,21 @@
+CI_DESC="CI job cross-compiling to Windows with MinGW, tested with Wine"
+CI_DIR=build-windows
+CI_CACHE_NIX_STORE=true
+NIX_ARGS=(
+  --arg windows true
+  --arg minimal true
+)
+CMAKE_ARGS=(
+  -G Ninja
+  -DCMAKE_SYSTEM_NAME=Windows
+  -DCMAKE_FIND_ROOT_PATH_MODE_PROGRAM=NEVER
+  -DCMAKE_CROSSCOMPILING_EMULATOR=wine
+  # -Wa,-mbig-obj: template-heavy C++ generates >32K COFF sections per .obj;
+  # BigCOFF format raises the limit.  Must be passed to the assembler via -Wa.
+  "-DCMAKE_CXX_FLAGS=-Wa,-mbig-obj"
+)
+# CXX is set by the nix cross shell to the mingw g++ wrapper; cmake picks
+# it up from the environment, so no CMAKE_CXX_COMPILER override needed.
+BUILD_ARGS=(-k 0)
+# Build native mpgen first (as a code generator for cross-compiled test/example targets).
+MPGEN_PRE_BUILD=1

src/ipc/libmultiprocess/ci/scripts/ci.sh

@@ -38,6 +38,49 @@ if [ -n "${CAPNP_CHECKOUT-}" ]; then
 fi
 
 src_dir=$PWD
+
+# If cross-compiling, build native mpgen first so it can be used as a code
+# generator when cmake invokes it from add_custom_command (which does not go
+# through CMAKE_CROSSCOMPILING_EMULATOR, unlike add_test executables).
+if [ -n "${MPGEN_PRE_BUILD-}" ]; then
+  native_dir="${src_dir}/${CI_DIR}-native"
+  [ -n "${CI_CLEAN-}" ] && rm -rf "$native_dir"
+  mkdir -p "$native_dir"
+  # Unset cross-compilation env vars so cmake uses the native compiler and
+  # does a native (not cross) build.  Key vars to clear:
+  #   CXX/CC/AR/RANLIB/LD - set to cross-compiler by the cross shell
+  #   cmakeFlags - nix cross shell injects -DCMAKE_SYSTEM_NAME=Windows etc.
+  # Pass NATIVE_CAPNPROTO_PREFIX as CMAKE_PREFIX_PATH so cmake finds the
+  # native Cap'n Proto rather than the cross-compiled one.
+  native_cmake_args=()
+  if [ -n "${NATIVE_CAPNPROTO_PREFIX-}" ]; then
+    # Build a cmake prefix path with native capnproto and its dependencies
+    # (openssl, zlib) so find_package and find_dependency succeed.
+    native_prefix="${NATIVE_CAPNPROTO_PREFIX}"
+    [ -n "${NATIVE_OPENSSL_DEV-}" ] && native_prefix="${native_prefix};${NATIVE_OPENSSL_DEV}"
+    [ -n "${NATIVE_OPENSSL_LIB-}" ] && native_prefix="${native_prefix};${NATIVE_OPENSSL_LIB}"
+    [ -n "${NATIVE_ZLIB_DEV-}" ]    && native_prefix="${native_prefix};${NATIVE_ZLIB_DEV}"
+    [ -n "${NATIVE_ZLIB_LIB-}" ]    && native_prefix="${native_prefix};${NATIVE_ZLIB_LIB}"
+    native_cmake_args+=(
+      "-DCMAKE_PREFIX_PATH=${native_prefix}"
+      "-DCapnProto_DIR=${NATIVE_CAPNPROTO_PREFIX}/lib/cmake/CapnProto"
+    )
+  fi
+  # -static-libstdc++ / -static-libgcc: the native mpgen must run inside the
+  # cross nix shell where the native libstdc++.so may not be in LD_LIBRARY_PATH.
+  native_cmake_args+=("-DCMAKE_EXE_LINKER_FLAGS=-static-libstdc++ -static-libgcc")
+  (cd "$native_dir" && env -u CXX -u CC -u AR -u RANLIB -u LD -u cmakeFlags cmake "$src_dir" "${native_cmake_args[@]+${native_cmake_args[@]}}" && cmake --build . -t mpgen)
+  CMAKE_ARGS+=("-DMPGEN_EXECUTABLE=${native_dir}/mpgen")
+
+  # Override capnp tool executables: the cross capnproto cmake config sets
+  # CAPNP_EXECUTABLE to capnp.exe (Windows binary), which can't run on Linux.
+  # Use the native capnp/capnpc-c++ binaries from pkgs.capnproto in nativeBuildInputs.
+  _capnp=$(command -v capnp 2>/dev/null || true)
+  _capnpc=$(command -v capnpc-c++ 2>/dev/null || true)
+  [ -n "$_capnp" ]   && CMAKE_ARGS+=("-DCAPNP_EXECUTABLE=$_capnp")
+  [ -n "$_capnpc" ]  && CMAKE_ARGS+=("-DCAPNPC_CXX_EXECUTABLE=$_capnpc")
+fi
+
 mkdir -p "$CI_DIR"
 cd "$CI_DIR"
 export CMAKE_BUILD_PARALLEL_LEVEL="$(nproc)"
@@ -67,4 +110,13 @@ else
     cmake --build . --target "$t" -- "${BUILD_ARGS[@]+"${BUILD_ARGS[@]}"}"
   done
 fi
+# When cross-compiling for Windows, copy GCC and MCF thread runtime DLLs
+# alongside the test executables so wine can find them (wine DLL loading
+# checks the executable's directory first, before any search-path logic).
+if [ -n "${WIN_RUNTIME_DLLS-}" ]; then
+  IFS=: read -ra _dll_dirs <<< "$WIN_RUNTIME_DLLS"
+  for _dir in "${_dll_dirs[@]}"; do
+    find "$_dir" -maxdepth 1 -name "*.dll" -exec cp -n {} test/ \;
+  done
+fi
 ctest --output-on-failure

src/ipc/libmultiprocess/example/example.cpp

@@ -8,6 +8,7 @@
 #include <cstring> // IWYU pragma: keep
 #include <filesystem>
 #include <fstream>
+#include <functional>
 #include <future>
 #include <iostream>
 #include <kj/common.h>
@@ -26,11 +27,11 @@ namespace fs = std::filesystem;
 
 static auto Spawn(mp::EventLoop& loop, const std::string& process_argv0, const std::string& new_exe_name)
 {
-    const auto [pid, socket] = mp::SpawnProcess([&](mp::SpawnConnectInfo info) -> std::vector<std::string> {
+    const auto [pid, socket] = mp::SpawnProcess([&](std::string connect_info) -> std::vector<std::string> {
         fs::path path = process_argv0;
         path.remove_filename();
         path.append(new_exe_name);
-        return {path.string(), std::move(info)};
+        return {path.string(), std::move(connect_info)};
     });
     return std::make_tuple(mp::ConnectStream<InitInterface>(loop, mp::MakeStream(loop, socket)), pid);
 }

src/ipc/libmultiprocess/include/mp/proxy-io.h

@@ -947,12 +947,7 @@ void ListenConnections(EventLoop& loop, SocketId fd, InitImpl& init, std::option
     });
 }
 
-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.
-
-//! Return the current thread's ThreadContext.
+//! Return the current thread's ThreadContext, creating it on first use.
 //!
 //! Why per-thread state is needed at all: libmultiprocess has no control over
 //! which threads the C++ application uses to call ProxyClient methods after
@@ -969,7 +964,60 @@ extern thread_local ThreadContext g_thread_context; // NOLINT(bitcoin-nontrivial
 //! are freed. A thread_local object is the C++ mechanism that provides both
 //! of these: per-thread storage plus a destructor that runs at thread exit
 //! (the C equivalent would be a pthread key destructor). This is why
-//! ThreadContext is thread_local and why its destructor is nontrivial.
+//! ThreadContext is thread_local and why its destructor is nontrivial —
+//! which is what makes the MinGW bug below bite here.
+//!
+//! It is normally an ordinary function-local thread_local object destroyed at
+//! thread exit, except on MinGW, where it is a lazily-created heap object
+//! that is deliberately leaked (held by a trivially-destructible thread_local
+//! pointer, so no destructor is registered at thread exit at all). Note that
+//! on MinGW this skips more than freeing memory: releasing the remote Thread
+//! capabilities at client-thread exit is what lets the server destroy that
+//! thread's dedicated server threads, so with the workaround, server threads
+//! belonging to exited client threads are only freed when the connection
+//! closes (via the disconnect cleanup callbacks registered in SetThread /
+//! ProxyClient<Thread>::m_disconnect_cb).
+//!
+//! The MinGW workaround exists because MinGW-w64's emutls implementation can
+//! free the heap storage backing thread_local variables before the C++
+//! destructors registered via __cxa_thread_atexit run at thread exit. Both
+//! cleanups are implemented as winpthreads pthread key destructors (run by
+//! _pthread_cleanup_dest in winpthreads src/thread.c), and pthread key
+//! destructor order is unspecified, so gcc's emutls key destructor
+//! (emutls_destroy in libgcc/emutls.c, which frees each thread_local
+//! object's storage and the per-thread array) can run before the key
+//! destructor that invokes the C++ thread_local destructors. When that
+//! happens, any thread_local object with a nontrivial destructor is
+//! destroyed after its own memory was already freed. For this ThreadContext
+//! struct that meant ~ThreadContext walking the request_threads /
+//! callback_threads std::map trees through freed memory and double-freeing
+//! their nodes, corrupting the heap.
+//!
+//! This was observed in Bitcoin Core Windows CI as intermittent
+//! STATUS_HEAP_CORRUPTION (0xC0000374) exit code 3221226356 crashes of both
+//! bitcoin-cli (its EventLoop::loop thread exiting at IPC teardown) and
+//! bitcoin-node (per-request server threads created by
+//! ProxyServer<ThreadMap>::makeThread() exiting after client disconnects) in
+//! x86_64-w64-mingw32 (msvcrt) cross builds, but not in
+//! x86_64-w64-mingw32ucrt builds — presumably pthread-key creation-order
+//! luck, since the underlying bug is CRT-independent and unsafe on all
+//! mingw-posix toolchains. Under gdb the crash appears as SIGSEGV in
+//! ~ThreadContext inside _pthread_cleanup_dest with the object memory
+//! containing the 0xfeeefeee "freed heap" debug fill pattern, matching the
+//! signature described in the upstream reports below. (Without a debugger
+//! there is no fill pattern, so the destructor walks stale-but-intact
+//! pointers and double-frees them, which is why the corruption was
+//! intermittent and often detected later, e.g. in kj::AsyncIoContext
+//! teardown, rather than at the faulting site.)
+//!
+//! Known upstream reports of this mingw-w64 bug:
+//! - https://sourceforge.net/p/mingw-w64/bugs/527/ "thread_local stl object's destructor causing crash"
+//! - https://sourceforge.net/p/mingw-w64/bugs/727/ "Crash during destruction of thread_local objects"
+//! - https://sourceforge.net/p/mingw-w64/bugs/445/ "thread_local destructors broken with posix threading"
+//! - https://sourceforge.net/p/mingw-w64/bugs/859/ "thread_local destructors not called at thread exit"
+//! - https://github.com/msys2/MINGW-packages/issues/2519 "gcc thread_local destructor cause use after free"
+//! - https://www.mail-archive.com/mingw-w64-public@lists.sourceforge.net/msg17975.html
+//!   (proposed fix discussion: "fix __cxa_thread_atexit destructors on GCC")
 ThreadContext& CurrentThread();
 
 } // namespace mp

src/ipc/libmultiprocess/include/mp/util.h

@@ -29,6 +29,17 @@
 #include <memory>
 #endif
 
+#ifdef WIN32
+// WIN32_LEAN_AND_MEAN excludes commdlg.h which defines `#define INTERFACE
+// IPrintDialogServices` — this conflicts with capnp::Kind::INTERFACE used
+// in CAPNP_DECLARE_INTERFACE_HEADER. Must be defined before winsock2.h.
+#ifndef WIN32_LEAN_AND_MEAN
+#define WIN32_LEAN_AND_MEAN
+#endif
+#include <windows.h>
+#include <winsock2.h>
+#endif
+
 namespace mp {
 
 //! Generic utility functions used by capnp code.
@@ -289,33 +300,53 @@ std::string ThreadName(const char* exe_name);
 //! errors in python unit tests.
 std::string LogEscape(const kj::StringTree& string, size_t max_size);
 
+//! Convert an argument vector into a single command line string suitable for
+//! CreateProcess, following the quoting rules of CommandLineToArgvW, which
+//! executables use to split the command line back into arguments. Declared
+//! unconditionally (not just on windows) so it can be unit tested on any
+//! platform.
+std::string CommandLineFromArgv(const std::vector<std::string>& argv);
+
 using Stream = kj::Own<kj::AsyncIoStream>;
 
+#ifdef WIN32
+// On Windows, ProcessId is defined to be the local process HANDLE rather than
+// global process ID, because handles are more useful for controlling and
+// waiting for processes. It it possible to obtain the actual process ID from
+// handles by calling the GetProcessId API.
+using ProcessId = HANDLE;
+using SocketId = SOCKET;
+constexpr SocketId SocketError{INVALID_SOCKET};
+#else
 using ProcessId = int;
 using SocketId = int;
 constexpr SocketId SocketError{-1};
-
-//! Information about parent process passed to child process as a command-line
-//! argument. On unix this is the child socket fd number formatted as a string.
-using SpawnConnectInfo = std::string;
-
-//! Callback type used by SpawnProcess below.
-using SpawnConnectInfoToArgsFn = std::function<std::vector<std::string>(const SpawnConnectInfo&)>;
+#endif
 
 //! Spawn a new process that communicates with the current process over a socket
-//! pair. Calls connect_info_to_args callback with a connection string that
-//! needs to be passed to the child process, and executes the argv command line
-//! it returns. Returns child process id and socket id.
-std::tuple<ProcessId, SocketId> SpawnProcess(SpawnConnectInfoToArgsFn&& connect_info_to_args);
-
-//! Initialize spawned child process using the SpawnConnectInfo string passed to it,
-//! returning a socket id for communicating with the parent process.
-SocketId StartSpawned(const SpawnConnectInfo& connect_info);
+//! pair. Calls spawn_argv callback with a connection string that needs to be
+//! passed to the child process, and executes the argv command line it returns.
+//! Returns child process id and socket id.
+//!
+//! The connection string is just a file descriptor number on unix. On windows,
+//! it is a path to a named pipe the parent process will write
+//! WSADuplicateSocket info to. In both cases, the child process can call
+//! StartSpawned to get a socket handle from the connection string.
+std::tuple<ProcessId, SocketId> SpawnProcess(const std::function<std::vector<std::string>(std::string)>& spawn_argv);
+
+//! Initialize spawned child process. The connect_info argument is the
+//! connection string SpawnProcess generated in the parent process and passed
+//! to the child on its command line. Returns socket id for communicating with
+//! the parent process.
+SocketId StartSpawned(const std::string& connect_info);
 
 //! Create a socket pair that can be used to communicate within a process or
 //! between parent and child processes.
 std::array<SocketId, 2> SocketPair();
 
+//! Close a socket, throwing a KJ exception on failure.
+void CloseSocket(SocketId fd);
+
 //! Start a process and return its process id. Caller should call WaitProcess
 //! on the returned id.
 ProcessId StartProcess(const std::vector<std::string>& args);