[POC] 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.

Based on #32028.

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:

#31176 (ci: Test cross-built Windows executables on Windows natively by hebasto)
#30595 (kernel: Introduce initial C header API by TheCharlatan)
#29491 ([EXPERIMENTAL] Schnorr batch verification for blocks by fjahr)

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?

DrahtBot added the label Needs rebase on Mar 14, 2025

Squashed 'src/secp256k1/' changes from 0cdc758a56..70f149b9a1

70f149b9a1 Merge bitcoin-core/secp256k1#1662: bench: add ellswift to bench help output
6b3fe51fb6 bench: add ellswift to bench help output
d84bb83e26 Merge bitcoin-core/secp256k1#1661: configure: Show exhaustive tests in summary
3f54ed8c1b Merge bitcoin-core/secp256k1#1659: include: remove WARN_UNUSED_RESULT for functions always returning 1
20b05c9d3f configure: Show exhaustive tests in summary
e56716a3bc Merge bitcoin-core/secp256k1#1660: ci: Fix exiting from ci.sh on error
d87c3bc58f ci: Fix exiting from ci.sh on error
1b6e081538 include: remove WARN_UNUSED_RESULT for functions always returning 1
2abb35b034 Merge bitcoin-core/secp256k1#1657: tests: remove unused uncounting_illegal_callback_fn
51907fa918 tests: remove unused uncounting_illegal_callback_fn
a7a5117144 Merge bitcoin-core/secp256k1#1359: Fix symbol visibility issues, add test for it
13ed6f65dc Merge bitcoin-core/secp256k1#1593: Remove deprecated `_ec_privkey_{negate,tweak_add,tweak_mul}` aliases from API
d1478763a5 build: Drop no longer needed  `-fvisibility=hidden` compiler option
8ed1d83d92 ci: Run `tools/symbol-check.py`
41d32ab2de test: Add `tools/symbol-check.py`
88548058b3 Introduce `SECP256K1_LOCAL_VAR` macro
03bbe8c615 Merge bitcoin-core/secp256k1#1655: gha: Print all *.log files, in a separate action
59860bcc24 gha: Print all *.log files, in a separate action
4ba1ba2af9 Merge bitcoin-core/secp256k1#1647: cmake: Adjust diagnostic flags for `clang-cl`
abd25054a1 Merge bitcoin-core/secp256k1#1656: musig: Fix clearing of pubnonces
961ec25a83 musig: Fix clearing of pubnonces
3186082387 Merge bitcoin-core/secp256k1#1614: Add _ge_set_all_gej and use it in musig for own public nonces
6c2a39dafb Merge bitcoin-core/secp256k1#1639: Make static context const
37d2c60bec Remove deprecated _ec_privkey_{negate,tweak_add,tweak_mul} aliases
432ac57705 Make static context const
1b1fc09341 Merge bitcoin-core/secp256k1#1642: Verify `compressed` argument in `secp256k1_eckey_pubkey_serialize`
c0d9480fbb Merge bitcoin-core/secp256k1#1654: use `EXIT_` constants over magic numbers for indicating program execution status
13d389629a CONTRIBUTING: mention that `EXIT_` codes should be used
c855581728 test, bench, precompute_ecmult: use `EXIT_...` constants for `main` return values
965393fcea examples: use `EXIT_...` constants for `main` return values
2e3bf13653 Merge bitcoin-core/secp256k1#1646: README: add instructions for verifying GPG signatures
b682dbcf84 README: add instructions for verifying GPG signatures
00774d0723 Merge bitcoin-core/secp256k1#1650: schnorrsig: clear out masked secret key in BIP-340 nonce function
a82287fb85 schnorrsig: clear out masked secret key in BIP-340 nonce function
4c50d73dd9 ci: Add new "Windows (clang-cl)" job
84c0bd1f72 cmake: Adjust diagnostic flags for clang-cl
f79f46c703 Merge bitcoin-core/secp256k1#1641: doc: Improve cmake instructions in README
2ac9f558c4 doc: Improve cmake instructions in README
1823594761 Verify `compressed` argument in `secp256k1_eckey_pubkey_serialize`
8deef00b33 Merge bitcoin-core/secp256k1#1634: Fix some misspellings
39705450eb Fix some misspellings
ec329c2501 Merge bitcoin-core/secp256k1#1633: release cleanup: bump version after 0.6.0
c97059f594 release cleanup: bump version after 0.6.0
64228a648f musig: Use _ge_set_all_gej for own public nonces
300aab1c05 tests: Improve _ge_set_all_gej(_var) tests
365f274ce3 group: Simplify secp256k1_ge_set_all_gej
d3082ddead group: Add constant-time secp256k1_ge_set_all_gej

git-subtree-dir: src/secp256k1
git-subtree-split: 70f149b9a1bf4ed3266f97774d0ae9577534bf40

dd59896431

Update secp256k1 subtree to latest master 0e8254fef9

cmake: Adjust diagnostic flags for clang-cl 10c2c56122

ci: Test building with clang-cl b73ae188aa

cmake: Restrict MSVC-specific workaround to MSVC compiler only

This change enables compiling `fuzz/utxo_snapshot.cpp` with clang-cl.

75d792a482

ci: Switch "Win64 native, fuzz" job to clang-cl 19191706cb

hebasto force-pushed on Mar 17, 2025

DrahtBot removed the label Needs rebase on Mar 17, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts