build: Use clang-cl to build on Windows natively

hebasto commented at 10:58 am on December 16, 2024: member

Building on Windows using Clang instead of the MS compiler may be beneficial, considering the issues with the latter, as outlined in #31456:

non-optimized codegen: [build: remove need to test for endianness #29852 (comment)](/bitcoin-bitcoin/29852/#issuecomment-2049803970)

compile failure: [MSVC 17.12.0 internal compiler error #31303](https://github.com/bitcoin/bitcoin/issues/31303)

legal, but brittle stdlib: [util: Drop boost posix_time in ParseISO8601DateTime #31391 (comment)](/bitcoin-bitcoin/31391/#issuecomment-2510762011)

unspecified issue: [refactor: Check translatable format strings at compile-time #31061 (comment)](/bitcoin-bitcoin/31061/#issuecomment-2531244463)

Additionally, MSVC does not support inline assembly on the ARM and x64 processors.

So this PR:

Closes #31456.
Could be an alternative to #24773.

Additionally, clang-cl builds faster on the CI:

MSVC - 20m 35s
clang-cl - 12m 43s

hebasto added the label Windows on Dec 16, 2024

hebasto added the label Build system on Dec 16, 2024

DrahtBot commented at 10:58 am on December 16, 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/31507.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31665 (build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON by davidgumberg)
#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.

DrahtBot added the label CI failed on Dec 16, 2024

DrahtBot commented at 12:00 pm on December 16, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34465651629

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

maflcko commented at 12:52 pm on December 16, 2024: member

Seems fine to add this, because it is easy to remove and can increase the build compatibly in the meantime.

Also, it allows to evaluate the codegen, which could be substantially better than msvc, or detect more msvc bugs.

If there are enough benefits to clang-cl, the docs could be updated to use it and msvc could be dropped from the docs and possibly CI.

fanquake commented at 12:54 pm on December 16, 2024: member

Also, it allows to evaluate the codegen, which could be substantially better than msvc, or detect more msvc bugs.

Does it? My read of the docs was that clang just tries to internally work around all the MSVC bugs, so it seems like it will just continue to “hide” issues?

hebasto commented at 1:33 pm on December 16, 2024: member

From #31456#issue-2729797132:

MSVC has many issues, for example:

compile failure: [MSVC 17.12.0 internal compiler error #31303](https://github.com/bitcoin/bitcoin/issues/31303)

Added two commits that enable compiling fuzz/utxo_snapshot.cpp with clang-cl and switch “Win64 native, fuzz” job to clang-cl.

in cmake/module/TryAppendCXXFlags.cmake:122 in 49cfcaf254 outdated

119@@ -120,7 +120,11 @@ function(try_append_cxx_flags flags)
120 endfunction()
121 
122 if(MSVC)

theuni commented at 7:44 pm on December 16, 2024:

Can’t the if(MSVC) go away and we just rely on the CMAKE_CXX_COMPILER_ID test?

hebasto commented at 12:08 pm on December 17, 2024:

In this case, I think we do want to check MSVC, as it separates MSVC specific syntax /WX from GCC syntax -Werror.

theuni commented at 7:46 pm on December 16, 2024: member

At a high level.. seems to me we want to avoid doing if(MSVC) as much as possible and instead check against the compiler id, no? Except for the cases where we REALLY mean MSVC.

theuni commented at 8:02 pm on December 16, 2024: member

Hmm, actually, it seems like maybe CMAKE_CXX_COMPILER_FRONTEND_VARIANT is what we should be testing against?

hebasto force-pushed on Dec 17, 2024

fanquake commented at 11:49 am on December 17, 2024: member

< to be added >

It’d be good if this could actually be filled in, so it’s clear what the goals are / what’s trying to be acheived here.

hebasto commented at 12:11 pm on December 17, 2024: member

Hmm, actually, it seems like maybe CMAKE_CXX_COMPILER_FRONTEND_VARIANT is what we should be testing against?

It doesn’t seem reliable, as it might not be set at all for CMake versions < 3.26.

DrahtBot added the label Needs rebase on Jan 27, 2025

hebasto force-pushed on Mar 10, 2025

hebasto commented at 5:47 pm on March 10, 2025: member

Rebased on #32028.

hebasto force-pushed on Mar 10, 2025

DrahtBot removed the label CI failed on Mar 10, 2025

DrahtBot removed the label Needs rebase on Mar 10, 2025

hebasto commented at 3:28 pm on March 11, 2025: member

< to be added >

It’d be good if this could actually be filled in, so it’s clear what the goals are / what’s trying to be acheived here.

Done.

hebasto force-pushed on Mar 11, 2025

in cmake/leveldb.cmake:98 in 80e7e80a6d outdated

 96     IF_CHECK_PASSED "-Wno-conditional-uninitialized"
 97   )
 98   try_append_cxx_flags("-Wsuggest-override" TARGET nowarn_leveldb_interface SKIP_LINK
 99     IF_CHECK_PASSED "-Wno-suggest-override"
100   )
101+  try_append_cxx_flags("-Wunused-member-function" TARGET nowarn_leveldb_interface SKIP_LINK

fanquake commented at 3:39 am on March 12, 2025:

In 76659d92f1925f561736ffe0f070ed0b9fef8d23: It’d be good if commits like this could contain actual details, rather than just “Adjust diagnostic flags”; especially given this is touching non-Windows code. Also, any reason to not fix the issues in leveldb, rather than adding more suppressions?

hebasto commented at 11:58 am on May 17, 2025:

More details have been added to the commit message.

Also, any reason to not fix the issues in leveldb, rather than adding more suppressions?

I’ll consider this option.

hebasto commented at 10:10 pm on May 17, 2025:

Also, any reason to not fix the issues in leveldb, rather than adding more suppressions?

I’ll consider this option.

Please see https://github.com/bitcoin-core/leveldb-subtree/pull/53.

DrahtBot added the label Needs rebase on Mar 14, 2025

hebasto force-pushed on Mar 17, 2025

DrahtBot removed the label Needs rebase on Mar 17, 2025

DrahtBot added the label Needs rebase on Mar 31, 2025

hebasto force-pushed on May 13, 2025

fanquake commented at 11:27 am on May 13, 2025: member

0   test_bitcoin.vcxproj -> D:\a\bitcoin\bitcoin\build\bin\Release\test_bitcoin.exe
1lld-link : warning : found both wWinMain and WinMain; using latter [D:\a\bitcoin\bitcoin\build\src\qt\bitcoin-qt.vcxproj]
2lld-link : error : undefined symbol: __declspec(dllimport) PathRemoveFileSpecW [D:\a\bitcoin\bitcoin\build\src\qt\bitcoin-qt.vcxproj]
3  >>> referenced by bitcoinqt.lib(guiutil.obj):(bool __cdecl GUIUtil::SetStartOnSystemStartup(bool))
4lld-link : error : undefined symbol: __declspec(dllimport) PathRemoveFileSpecW [D:\a\bitcoin\bitcoin\build\src\qt\test\test_bitcoin-qt.vcxproj]
5  >>> referenced by bitcoinqt.lib(guiutil.obj):(bool __cdecl GUIUtil::SetStartOnSystemStartup(bool))

0  C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\shared\ws2ipdef.h(251,33): note: expanded from macro 'IN6ADDR_LOOPBACK_INIT'
1lld-link : error : undefined symbol: __divti3 [D:\a\bitcoin\bitcoin\build\src\test\fuzz\fuzz.vcxproj]
2  >>> referenced by fuzz.dir\Release\feefrac.obj:(void __cdecl feefrac_mul_div_fuzz_target(class std::span<unsigned char const, -1>))
3  >>> referenced by fuzz.dir\Release\feefrac.obj:(?Div@FeeFrac@@SA_J_LH_N@Z)

DrahtBot removed the label Needs rebase on May 13, 2025

fanquake referenced this in commit 8309a9747a on May 13, 2025

hebasto force-pushed on May 13, 2025

hebasto force-pushed on May 15, 2025

hebasto renamed this:
~~[POC] build: Use clang-cl to build on Windows natively~~
build: Use clang-cl to build on Windows natively
on May 15, 2025

DrahtBot added the label Needs rebase on May 16, 2025

hebasto force-pushed on May 16, 2025

DrahtBot removed the label Needs rebase on May 16, 2025

DrahtBot added the label Needs rebase on May 17, 2025

cmake: Always link `bitcoinqt` to `shlwapi` when building for Windows a4d504f97a

cmake: Explicitly disable `__int128` when building on Windows

MSVC does not support `__int128` by design, while clang-cl offers only
limited support out of the box: `__int128` division requires the
builtins library.

For now, disable the use of `__int128` when building on Windows.

dcc881558e

hebasto force-pushed on May 17, 2025

hebasto marked this as ready for review on May 17, 2025

DrahtBot removed the label Needs rebase on May 17, 2025

cmake: Adjust diagnostic flags for clang-cl

Being compatible with cl.exe, clang-cl supports most of the same
command-line options. These options are typically guarded by the `MSVC`
CMake variable.

Options not supported by clang-cl, such as `/wdNNN`, are now guarded by
`CMAKE_CXX_COMPILER_ID STREQUAL "MSVC"`.

Additionally, `-Wno-unused-member-function` is now used when compiling
leveldb with clang-cl.

bffafda3db

ci: Test building with clang-cl 336ca00df3

hebasto force-pushed on May 18, 2025

fanquake commented at 9:11 am on May 21, 2025: member

Reading the description, I’m still not really clear on the goals here. It seems like this is just adding even more ways to compile for Windows, which we have to add more CI / and more work arounds for, but doesn’t actually improve the Windows binaries we are shipping to end users in any way. This might make more sense if we were actually going to switch to shipping Windows binaries using Clang.

build: Use clang-cl to build on Windows natively #31507

Code Coverage & Benchmarks

Reviews

Conflicts