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

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

   < to be added >

   Closes #31456.

   Based on https://github.com/bitcoin-core/secp256k1/pull/1647 (the CI lint job failure is expected) and #31503.

   TODO: the build system is configured without -DWERROR=ON at this mooment.

2. hebasto added the label Windows on Dec 16, 2024
3. hebasto added the label Build system on Dec 16, 2024
4. 
   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)
   • #31158 (build, ci: Fix linking bitcoin-chainstate.exe to bitcoinkernel.dll on Windows by hebasto)
   • #28710 (Remove the legacy wallet and BDB dependency by achow101)
   • #21590 (Safegcd-based modular inverses in MuHash3072 by sipa)

   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.

5. DrahtBot added the label CI failed on Dec 16, 2024
6. 
   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.

7. 
   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.

8. 
   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?

9. 
   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.

10. 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.
11. 
    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.
12. 
    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?
13. [UPSTREAM, secp256k1] cmake: Adjust diagnostic flags for clang-cl deb9cd6449
14. cmake: Adjust diagnostic flags for clang-cl 6f55b3d653
15. ci: Test building with clang-cl adc86c30c5
16. cmake: Restrict MSVC-specific workaround to MSVC compiler only

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

    4022d5ecca
17. ci: Switch "Win64 native, fuzz" job to clang-cl caa61b5c7a
18. hebasto force-pushed on Dec 17, 2024
19. 
    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.

20. 
    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.

Contributors
hebasto DrahtBot maflcko fanquake theuni

Labels
Windows Build system CI failed