-
Notifications
You must be signed in to change notification settings - Fork 38.7k
build: Use clang-cl to build on Windows natively #31507
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31507. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
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. |
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? |
|
From #31456 (comment):
Added two commits that enable compiling |
| endif() | ||
| endfunction() | ||
|
|
||
| if(MSVC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't the if(MSVC) go away and we just rely on the CMAKE_CXX_COMPILER_ID test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I think we do want to check MSVC, as it separates MSVC specific syntax /WX from GCC syntax -Werror.
|
At a high level.. seems to me we want to avoid doing |
|
Hmm, actually, it seems like maybe CMAKE_CXX_COMPILER_FRONTEND_VARIANT is what we should be testing against? |
78c3a73 to
caa61b5
Compare
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. |
It doesn't seem reliable, as it might not be set at all for CMake versions < 3.26. |
caa61b5 to
d607109
Compare
|
Rebased on #32028. |
d607109 to
4941ea8
Compare
I'll refresh my benchmarks and post them shortly. |
D:\a\bitcoin\bitcoin\src\script\interpreter.cpp(1531,33): warning : variable 'ext_flag' may be uninitialized when used here [-Wconditional-uninitialized] [D:\a\bitcoin\bitcoin\build\src\bitcoin_consensus.vcxproj]
D:\a\bitcoin\bitcoin\src\script\interpreter.cpp(1733,10): note: in instantiation of function template specialization 'SignatureHashSchnorr<CTransaction>' requested here
D:\a\bitcoin\bitcoin\src\script\interpreter.cpp(1481,21): note: initialize the variable 'ext_flag' to silence this warning
D:\a\bitcoin\bitcoin\src\script\interpreter.cpp(1531,33): warning : variable 'ext_flag' may be uninitialized when used here [-Wconditional-uninitialized] [D:\a\bitcoin\bitcoin\build\src\bitcoin_consensus.vcxproj]
D:\a\bitcoin\bitcoin\src\script\interpreter.cpp(1733,10): note: in instantiation of function template specialization 'SignatureHashSchnorr<CMutableTransaction>' requested here
D:\a\bitcoin\bitcoin\src\script\interpreter.cpp(1481,21): note: initialize the variable 'ext_flag' to silence this warning
bitcoin_clientversion.vcxproj -> D:\a\bitcoin\bitcoin\build\lib\Release\bitcoin_clientversion.lib D:\a\bitcoin\bitcoin\src\util/subprocess.h(759,10): error : private field 'parent_' is not used [-Werror,-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\bitcoin_common.vcxproj]
D:\a\bitcoin\bitcoin\src\util/subprocess.h(760,7): error : private field 'err_wr_pipe_' is not used [-Werror,-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\bitcoin_common.vcxproj]
D:\a\bitcoin\bitcoin\src\util/subprocess.h(1038,7): error : private field 'child_pid_' is not used [-Werror,-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\bitcoin_common.vcxproj] |
a4cc681 to
39b091a
Compare
Below are benchmarks on my Windows machine: Compiled with MSVCCompiled with clang-cl |
39b091a to
a1469e6
Compare
|
Rebased to resolve the conflict with the merged #30595. |
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.
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.
a1469e6 to
c8feaee
Compare
|
Rebased to resolve conflicts. |
| $<TARGET_NAME_IF_EXISTS:QRencode::QRencode> | ||
| $<$<PLATFORM_ID:Darwin>:-framework\ AppKit> | ||
| $<$<CXX_COMPILER_ID:MSVC>:shlwapi> | ||
| $<$<PLATFORM_ID:Windows>:shlwapi> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to be overlinking now?
| /Zc:preprocessor | ||
| /Zc:__cplusplus | ||
| /sdl | ||
| # TODO: __int128 division when using clang-cl requires the builtins library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use the builtins? Not sure I understand the TODO.
.github/workflows/ci.yml
Outdated
| job-name: 'Windows native, VS 2022' | ||
| job-name: 'Windows native, MSVC' | ||
| - job-type: clang-cl | ||
| env_vars: { CXXFLAGS: '-Wno-return-type -Wno-unused-private-field -Wno-error=conditional-uninitialized -Wno-error=missing-braces' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-Wno-error=missing-braces
-Wno-unused-private-field
Why are these needed/are they being fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-Wno-error=missing-braces
From https://github.com/bitcoin/bitcoin/actions/runs/19820405203/job/56781245720:
D:\a\bitcoin\bitcoin\src\test\i2p_tests.cpp(116,34): warning : suggest braces around initialization of subobject [-Wmissing-braces] [D:\a\bitcoin\bitcoin\build\src\test\test_bitcoin.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.26100.0\shared\ws2ipdef.h(251,33): note: expanded from macro 'IN6ADDR_LOOPBACK_INIT'
D:\a\bitcoin\bitcoin\src\test\i2p_tests.cpp(159,38): warning : suggest braces around initialization of subobject [-Wmissing-braces] [D:\a\bitcoin\bitcoin\build\src\test\test_bitcoin.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.26100.0\shared\ws2ipdef.h(251,33): note: expanded from macro 'IN6ADDR_LOOPBACK_INIT'
D:\a\bitcoin\bitcoin\src\test\netbase_tests.cpp(502,36): warning : suggest braces around initialization of subobject [-Wmissing-braces] [D:\a\bitcoin\bitcoin\build\src\test\test_bitcoin.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.26100.0\shared\ws2ipdef.h(251,33): note: expanded from macro 'IN6ADDR_LOOPBACK_INIT'
D:\a\bitcoin\bitcoin\src\test\netbase_tests.cpp(507,36): warning : suggest braces around initialization of subobject [-Wmissing-braces] [D:\a\bitcoin\bitcoin\build\src\test\test_bitcoin.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.26100.0\shared\ws2ipdef.h(251,33): note: expanded from macro 'IN6ADDR_LOOPBACK_INIT'
D:\a\bitcoin\bitcoin\src\test\netbase_tests.cpp(512,36): warning : suggest braces around initialization of subobject [-Wmissing-braces] [D:\a\bitcoin\bitcoin\build\src\test\test_bitcoin.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.26100.0\shared\ws2ipdef.h(251,33): note: expanded from macro 'IN6ADDR_LOOPBACK_INIT'
-Wno-unused-private-field
D:\a\bitcoin\bitcoin\src\util/subprocess.h(759,10): warning : private field 'parent_' is not used [-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\util\bitcoin_util.vcxproj]
D:\a\bitcoin\bitcoin\src\util/subprocess.h(760,7): warning : private field 'err_wr_pipe_' is not used [-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\util\bitcoin_util.vcxproj]
D:\a\bitcoin\bitcoin\src\util/subprocess.h(1038,7): warning : private field 'child_pid_' is not used [-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\util\bitcoin_util.vcxproj]
Why are these needed/are they being fixed?
I'm going to work on the fixes.
751f3b9 to
bf15fb8
Compare
Building on Windows using Clang instead of the MS compiler may be beneficial, considering the issues with the latter, as outlined in #31456:
Additionally, MSVC does not support inline assembly on the ARM and x64 processors.
So this PR:
Additionally, clang-cl:
ConnectBlockAllEcdsaConnectBlockAllSchnorrConnectBlockMixedEcdsaSchnorrDeserializeAndCheckBlockTestDeserializeBlockTest