build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows

hebasto commented at 8:07 pm on October 25, 2024: member

On the master branch @ 9a7206a34e3179d7280de98a818a13374178ee7b, linking bitcoin-chainstate.exe to bitcoinkernel.dll fails:

0> cmake -B build --preset vs2022-static -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_KERNEL_LIB=ON -DBUILD_SHARED_LIBS=ON
1> cmake --build build --config Release -t bitcoin-chainstate
2...
3LINK : fatal error LNK1181: cannot open input file 'kernel\Release\bitcoinkernel.lib' [C:\Users\hebasto\source\repos\bitcoin\build-static\src\bitcoin-chainstate.vcxproj]

This PR fixes this issue and enables the same scenario in the “Win64 native” CI job.

hebasto added the label Windows on Oct 25, 2024

hebasto added the label Build system on Oct 25, 2024

DrahtBot commented at 8:07 pm on October 25, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31158.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31665 (build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON by davidgumberg)
#31176 (ci: Test cross-built Windows executables on Windows natively by hebasto)
#30595 (kernel: Introduce initial C header API by TheCharlatan)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto renamed this:
~~build, ci: Fix linking `bitcoin-chainstate.exe` to `bticoinkernel.dll` on Windows~~
build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows
on Oct 25, 2024

hebasto force-pushed on Oct 25, 2024

in .github/workflows/ci.yml:185 in ca9554f037 outdated

181@@ -182,7 +182,7 @@ jobs:
182 
183       - name: Generate build system
184         run: |
185-          cmake -B build --preset vs2022-static -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_INSTALLATION_ROOT\scripts\buildsystems\vcpkg.cmake" -DBUILD_GUI=ON -DWITH_BDB=ON -DWITH_MINIUPNPC=ON -DWITH_ZMQ=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWERROR=ON
186+          cmake -B build --preset vs2022-static -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_INSTALLATION_ROOT\scripts\buildsystems\vcpkg.cmake" -DBUILD_GUI=ON -DWITH_BDB=ON -DWITH_MINIUPNPC=ON -DWITH_ZMQ=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_KERNEL_LIB=ON -DBUILD_SHARED_LIBS=ON -DWERROR=ON

TheCharlatan commented at 8:21 pm on October 25, 2024:

Do you want to take this opportunity to also add a better description to the test name? Maybe something like: Win64 native, VS 2022, no depends, libbitcoinkernel?

hebasto commented at 8:29 pm on October 25, 2024:

Thanks! “libbitcoinkernel” taken; it seems awkward to mention “depends”, which is a non-Windows thing, on Windows :)

TheCharlatan commented at 8:34 pm on October 25, 2024:

Ah, I was thinking of mentioning it because of the macOS build, but I guess we can have native macOS and depends, so it makes sense there, but as you say, does not make sense for msvc.

hebasto force-pushed on Oct 25, 2024

in src/kernel/cs_main.h:20 in bc8efd1998 outdated

16@@ -17,6 +17,6 @@
17  * The transaction pool has a separate lock to allow reading from it and the
18  * chainstate at the same time.
19  */
20-extern RecursiveMutex cs_main;
21+inline RecursiveMutex cs_main;

TheCharlatan commented at 1:43 pm on October 26, 2024:

I checked if this could also be a problem for other extern symbols that are currently not used in bitcoin-chainstate, but still part of the kernel library. For example in src/validation.h we have extern const std::vector<std::string> CHECKLEVEL_DOC;, and it looks like using it is indeed problematic too if it were used externally: https://github.com/TheCharlatan/bitcoin/actions/runs/11531552497/job/32102377337#step:10:425. There are a bunch of other such cases, where I’m not entirely sure how to handle them. Could just ignore them for now, but it is a bit annoying that the library might contain symbols that you couldn’t just use.

EDIT: I guess this is another argument for having a clear list of exports from the lib.

DrahtBot added the label Needs rebase on Oct 28, 2024

hebasto force-pushed on Oct 28, 2024

hebasto commented at 12:24 pm on October 28, 2024: member

Rebased due to the conflict with the merged #31130.

DrahtBot removed the label Needs rebase on Oct 28, 2024

theuni commented at 7:18 pm on October 28, 2024: member

Concept ACK and code review ACK. This needs a more useful commit message though.

I wasn’t familiar with this c++17 functionality, and at first glance it looked scary to me (as though we might get multiple cs_mains instantiated). According to cppreference though, this is a reasonable (if not more correct/modern) approach.

This stackoverflow question/answers also covers our use-case exactly.

Agree with @TheCharlatan that we should track down other cases as well.

theuni commented at 7:26 pm on October 28, 2024: member

Wait, I take that back. It seems like we could still get 2 separate cs_mains here: one in the lib and the other in the binary. ACK retracted while I investigate.

fanquake commented at 10:05 am on October 29, 2024: member

Generally I would prefer if we weren’t making source code changes to fix (currently edge-case) Windows linking errors; especially if there are more complete fixes we could be making (even if later-on).

It’d be good if all commits explained the problem, and fix. i.e the first commit 4a1accd1df31cba778167b6a3ff9783ffedb46ab refactor: Inline cs_main definition has no explanation of what the problem/fix is, and doesn’t mention that the change is only needed to fix a build issue when using MSVC, other than “linking fails” in the PR description. I also don’t think this should be a labeled “refactor”.

DrahtBot added the label Needs rebase on Nov 26, 2024

hebasto force-pushed on Dec 1, 2024

DrahtBot removed the label Needs rebase on Dec 1, 2024

hebasto commented at 5:44 pm on December 1, 2024: member

Rebased. The commit messages have been amended. @theuni

Wait, I take that back. It seems like we could still get 2 separate cs_mains here: one in the lib and the other in the binary. ACK retracted while I investigate.

That’s an interesting observation. Could you elaborate on how you think the two separate cs_main instances might arise?

If there’s a relevant section in the documentation or standard that you believe applies, pointing it out would be helpful for clarity.

theuni commented at 10:36 pm on February 4, 2025: member

Rebased. The commit messages have been amended.

@theuni

Wait, I take that back. It seems like we could still get 2 separate cs_mains here: one in the lib and the other in the binary. ACK retracted while I investigate.

That’s an interesting observation. Could you elaborate on how you think the two separate cs_main instances might arise?

If there’s a relevant section in the documentation or standard that you believe applies, pointing it out would be helpful for clarity.

clang 21 (git) has introduced a new warning for this!

And turns out, yeah, it is problematic for us with this PR:

/home/cory/dev/bitcoin/src/kernel/cs_main.h:20:23: warning: 'cs_main' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage [-Wunique-object-duplication] 20 | inline RecursiveMutex cs_main;

master currently does not have this issue.

also (unrelated to this PR, but of interest)

/home/cory/dev/bitcoin/src/support/lockedpool.h:224:31: warning: 'init_flag' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage [-Wunique-object-duplication] 224 | static std::once_flag init_flag;

/home/cory/dev/bitcoin/src/node/miner.h:175:42: warning: 'm_last_block_num_txs' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage [-Wunique-object-duplication] 175 | inline static std::optional<int64_t> m_last_block_num_txs{};

/home/cory/dev/bitcoin/src/node/miner.h:176:42: warning: 'm_last_block_weight' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage [-Wunique-object-duplication] 176 | inline static std::optional<int64_t> m_last_block_weight{};

(maybe more)

This warning shows for bitcoind when REDUCE_EXPORTS is on, but not for the kernel because it currently overrides that.

I have verified that this does work though:

0__attribute__ ((visibility ("default"))) inline RecursiveMutex cs_main;

But I’m not sure if that’s what we want to be doing.

Need to think this through some more.

hebasto commented at 11:20 am on February 5, 2025: member

@theuni

Wait, I take that back. It seems like we could still get 2 separate cs_mains here: one in the lib and the other in the binary. ACK retracted while I investigate.

To clarify this discussion, which binary you are referring to—bitcoind or bitcoin-chainstate? I assume the latter, as the former uses the static kernel library.

theuni commented at 4:43 pm on February 5, 2025: member

@theuni

Wait, I take that back. It seems like we could still get 2 separate cs_mains here: one in the lib and the other in the binary. ACK retracted while I investigate.

To clarify this discussion, which binary you are referring to—bitcoind or bitcoin-chainstate? I assume the latter, as the former uses the static kernel library.

What I’m building isn’t really relevant… the combo of the inline var with hidden visibility when built as a shared lib is a potential problem. Even if we have the CXX_VISIBILITY_PRESET default hack for now, we’ll be removing it in the future.

I’m still trying to understand exactly what the desired outcome is. I suspect that we’re going to need default visibility on cs_main anyway. So maybe it’s not an issue. But I’m still trying to wrap my head around what the inline does here when building BOTH the shared lib and the downstream app that way.

I also want to understand if this is just a distraction, and if we can just keep things the way they currently are, but fixup the visibilities.

hebasto commented at 5:17 pm on February 5, 2025: member

What I’m building isn’t really relevant… the combo of the inline var with hidden visibility when built as a shared lib is a potential problem.

I think it could be problematic when an inline variable with hidden visibility is part of a static library, which is then linked both to a binary and to a shared library. If the binary does not share dependencies with the shared library, it should be safe.

theuni commented at 5:26 pm on February 5, 2025: member

From the docs

The module definition file will be passed to the linker causing all symbols to be exported from the .dll. For global data symbols, __declspec(dllimport) must still be used when compiling against the code in the .dll. All other function symbols will be automatically exported and imported by callers

See bold text. I suspect that’s the real issue here, and that inlining is the opposite of what we want.

theuni commented at 5:27 pm on February 5, 2025: member

If the binary does not share dependencies with the shared library, it should be safe.

The binary has to find cs_main somewhere though. And we don’t want it adding its own.

I think I understand what’s happening now and I’m testing an alternative.

hebasto commented at 5:29 pm on February 5, 2025: member

If the binary does not share dependencies with the shared library, it should be safe.

The binary has to find cs_main somewhere though.

Right. bitcoin-chainstate finds it in libbitcoinkernel.so.

hebasto commented at 5:31 pm on February 5, 2025: member

From the docs

The module definition file will be passed to the linker causing all symbols to be exported from the .dll. For global data symbols, __declspec(dllimport) must still be used when compiling against the code in the .dll. All other function symbols will be automatically exported and imported by callers

See bold text. I suspect that’s the real issue here, and that inlining is the opposite of what we want.

Mind sharing a link?

theuni commented at 5:34 pm on February 5, 2025: member

If the binary does not share dependencies with the shared library, it should be safe.

The binary has to find cs_main somewhere though.

Right. bitcoin-chainstate finds it in libbitcoinkernel.so.

Yes. But when inlining, it’s free to generate its own as opposed to using the kernel’s. Which is the opposite of what we want.

Mind sharing a link?

Sorry, I meant to link there. Updated.

Give me a few min, I’m testing over here :)

theuni commented at 5:59 pm on February 5, 2025: member

Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It’s not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).

hebasto commented at 7:46 pm on February 5, 2025: member

Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It’s not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).

CI fails: https://github.com/theuni/bitcoin/actions/runs/13163570761/job/36738155052

theuni commented at 8:17 pm on February 5, 2025: member

Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It’s not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).

CI fails: https://github.com/theuni/bitcoin/actions/runs/13163570761/job/36738155052

Yeah, the above is only enough to test bitcoin-chainstate.exe. In reality, the condition in the header would need to be something like: #elif defined(MSC_VER) && !defined(STATIC_LIBBITCOINKERNEL) && !defined(BITCOIN_INTERNAL_BUILD), which would get defined for everything that’s not bitcoin-chainstate.

hebasto commented at 9:56 pm on February 6, 2025: member

Converting to a draft after today’s offline discussion with @theuni.

hebasto marked this as a draft on Feb 6, 2025

hebasto force-pushed on Feb 7, 2025

hebasto marked this as ready for review on Feb 7, 2025

hebasto commented at 3:58 pm on February 7, 2025: member

@theuni

Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It’s not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).

Thank you for your insight!

This PR has been reworked based on your branch. The inline variables are avoided.

theuni commented at 5:05 pm on February 7, 2025: member

Thank you @hebasto! We had a pretty long discussion about this. I think that hebasto’s approach is probably fine and would never be an issue in the real world. I’ll admit that I’m being overly paranoid and probably wrong about the guarantees that linkers provide…

But still, I’m not alone in my paranoia. For example, Google has banned non-constexpr inline header variables for Chromium out of similar concerns.

This reddit thread also contains a discussion from which illustrates some of my concerns and made me nervous.

For reference, the original proposal was:

0-extern RecursiveMutex cs_main;
1+inline RecursiveMutex cs_main;

And here’s how the binaries changed in Linux:

extern (as-in current master):

0 $ nm -C src/kernel/libbitcoinkernel.so | grep cs_main
100000000003b3028 D cs_main
2
3 $ nm -C  src/bitcoin-chainstate | grep cs_main
4                 U cs_main

header-inline (this PR’s original proposal):

0 $ nm -C src/kernel/libbitcoinkernel.so | grep cs_main
100000000003b3010 V cs_main
200000000003b35f0 V guard variable for cs_main
3
4 $ nm -C  src/bitcoin-chainstate | grep cs_main
5000000000000e010 V cs_main
6000000000000e048 V guard variable for cs_main

Notice that the symbols, now weak, now exist in the shared lib as well as the binary. The runtime loader should pick one to use on both sides of the binary boundary, but technically the link/load behaviors here are platform-specific.

The root cause of the link failure being addressed here is that CMake’s visibility machinery handles functions but not variables. So there are basically 3 potential solutions:

A c++17-style inline variable as hebasto originally proposed here. I admit I’m being overly paranoid, but I’d rather not do that.
Give cs_main hidden visiblility and add a get_csmain() getter function for bitcon-chainstate to use. That would work but would mean that bitcoin-chainstate’s code would diverge from the rest of the codebase. (assuming we didn’t do a global replacement, which would be a HUGE hammer)
Add our own visibility header for exported variables, as hebasto has done here in the updated impl of this PR. This means no changes to how the symbols look, other than a strong guarantee that MSVC is importing as opposed to duplicating.

I pretty strongly prefer the 3rd as it’s minimally invasive and leaves no potential for regressions in bitcoind/kernel.

Will review in detail.

cmake, kernel: Enable `WINDOWS_EXPORT_ALL_SYMBOLS` property

To enable linking to `bitcoinkernel.dll`, temporarily enable the CMake's
`WINDOWS_EXPORT_ALL_SYMBOLS` property. Global data symbols, such as
`cs_main`, must still be properly attributed with
`__declspec(dllexport)` and `__declspec(dllimport)`.

This change can be reconsidered once the kernel API headers become
available.

Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>

ab6616bc67

ci, windows: Build shared and static `bitcoinkernel` libraries

This change expands the matrix jobs for the native Windows builds to
test shared and static `bitcoinkernel` libraries.
To conserve resources, the functional tests are enabled for a single job
only.

afa2ebeccb

ci, windows: Treat linker warnings as errors

This change prevents issues related to the `__declspec(dllimport)`
attribute, such as LNK4217 or LNK4286.

939e8e9008

in src/kernel/symbol_visibility.h:10 in fe79ea0824 outdated

 5+  #if defined(_WIN32)
 6+    #define BITCOINKERNEL_EXPORT __declspec(dllexport)
 7+  #else
 8+    #define BITCOINKERNEL_EXPORT __attribute__ ((visibility ("default")))
 9+  #endif
10+#elif defined(_WIN32) && !defined(BITCOINKERNEL_STATIC)

theuni commented at 5:50 pm on February 7, 2025:

The meaning of BITCOINKERNEL_STATIC here is overloaded. Just to differentiate the use-cases, I’d prefer to have a 3rd define here:

#elif defined(_WIN32) && !defined(BITCOIN_BUILD_INTERNAL) && !defined(BITCOINKERNEL_STATIC)

BITCOINKERNEL_STATIC: I’m a user of the kernel building against a static kernel. BITCOIN_BUILD_INTERNAL: I’m a component of Core who’s not building against the kernel at all, I just happen to be seeing this header.

The former should stay as-is for bitcoin-chainstate, the latter is what we should be defining for the other internal libs.

hebasto commented at 9:07 pm on February 7, 2025:

BITCOIN_BUILD_INTERNAL: I’m a component of Core who’s not building against the kernel at all, I just happen to be seeing this header.

BITCOIN_INTERNAL_BUILD was in my draft of the updated branch this morning :)

Will rework the PR shortly.

hebasto commented at 9:20 pm on February 7, 2025:

Thanks! Reworked.

hebasto force-pushed on Feb 7, 2025

maflcko commented at 3:11 pm on February 8, 2025: member

Give cs_main hidden visiblility and add a get_csmain() getter function for bitcon-chainstate to use. That would work but would mean that bitcoin-chainstate’s code would diverge from the rest of the codebase. (assuming we didn’t do a global replacement, which would be a HUGE hammer)

(Probably out-of-scope for this pull, but there is already a getter for cs_main: chainman.GetMutex(), which can be used today, and can make it easier to turn cs_main from a global variable into a member field of chainman in the future. Though, that doesn’t work for all places today and a global getter get_csmain would still be needed for now.)

build, ci: Fix linking bitcoin-chainstate.exe to bitcoinkernel.dll on Windows #31158

Code Coverage & Benchmarks

Reviews

Conflicts

build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows #31158