node
node copied to clipboard
nodejs
build: add option to enable clang-cl on Windows
The goal here is to do the right changes with lessons learned from https://github.com/nodejs/node/pull/35433.
Stopping for now. Locally, current error is:
..\..\out\Release\\obj\global_intermediate\icudt75l_dat.obj: unknown machine: 0
C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Microsoft\VC\v170\Microsoft.CppCommon.targets(1599,5): error MSB6006: Arrêt de "llvm-lib.exe" avec le code 1. [D:\Git\nodejs\node\tools\icu\icudata.vcxproj]
Which is https://github.com/nodejs/node/issues/34201 (hack: https://github.com/nodejs/node/pull/35433/commits/711a822f831f8c46f5935280257d3b7c8769a56a)
Rebased on https://github.com/nodejs/node/pull/52873 so I can use what it does. I added a better version of the ICU hack based on the suggestion from https://github.com/nodejs/node/issues/34201#issuecomment-660868415. I say we wait for https://github.com/nodejs/build/issues/3709 and test what happens with the cross-compiler in CI.
Rebased on #52873 so I can use what it does. I added a better version of the ICU hack based on the suggestion from #34201 (comment). I say we wait for nodejs/build#3709 and test what happens with the cross-compiler in CI.
First of all, I'm very glad to see this new PR that aims to remove hardcoded stuff. Second, about the ICU hack, please see my comment from the original PR. Since that project is compiled for host architecture, for cross-compilation we cannot use _M_AMD64/_M_ARM64
I have no simple idea to fix it then. If we look at Chromium, I think they just prebuild the data files and add them to source control: https://chromium.googlesource.com/chromium/deps/icu/+/refs/heads/main/README.chromium https://chromium.googlesource.com/chromium/deps/icu/+/refs/heads/main/common/
This feature interests me. Looking at the code, this looks like a PoC, which is completely fine for this stage, but I was just wondering what you see as the end result here @targos?
The way I see it, clang and MSVC would both have to be supported in parallel for some time, before potentially moving to Clang completely. In that time, we'd need a way to choose either of them, eg.
vcbuild.batwould still use MSVC, whilevcbuild.bat clangwould use clang.
Luckily we could leverage
__clang__to have separate code for the 2 compilers, but we'd probably need either to push somedepschanges upstream, or leave them as floating patches. Overall, it would be good to know the scope of changes and affected dependencies once PoC is done so productization can be planned well.
Originally posted by @StefanStojanovic in https://github.com/nodejs/node/pull/35433#pullrequestreview-2025106112
When building locally, I noticed a lot of warnings on the OpenSSL side. Should we care about them?
@nodejs/crypto
Unknown escape sequence:
openssl\crypto\ct\ct_log.c(173,15): warning : unknown escape sequence '\ ' [-Wunknown-escape-sequence] [D:\a\node\node\deps\openssl\openssl.vcxproj]
openssl\include\internal/cryptlib.h(71,35): message : expanded from macro 'CTLOG_FILE' [D:\a\node\node\deps\openssl\openssl.vcxproj]
<command line>(48,32): message : expanded from macro 'OPENSSLDIR' [D:\a\node\node\deps\openssl\openssl.vcxproj]
openssl\crypto\ct\ct_log.c(173,15): warning : unknown escape sequence '\ ' [-Wunknown-escape-sequence] [D:\a\node\node\deps\openssl\openssl.vcxproj]
openssl\include\internal/cryptlib.h(71,35): message : expanded from macro 'CTLOG_FILE' [D:\a\node\node\deps\openssl\openssl.vcxproj]
<command line>(48,47): message : expanded from macro 'OPENSSLDIR' [D:\a\node\node\deps\openssl\openssl.vcxproj]
Unused function:
openssl\ssl\ssl_sess.c(27,1): warning : unused function 'sk_SSL_SESSION_value' [-Wunused-function] [D:\a\node\node\deps\openssl\openssl.vcxproj]
openssl\include\openssl/../../../config/././archs/VC-WIN64A/asm/include/openssl/safestack.h(175,29): message : expanded from macro 'DEFINE_STACK_OF' [D:\a\node\node\deps\openssl\openssl.vcxproj]
openssl\include\openssl/../../../config/././archs/VC-WIN64A/asm/include/openssl/safestack.h(73,40): message : expanded from macro 'SKM_DEFINE_STACK_OF' [D:\a\node\node\deps\openssl\openssl.vcxproj]
You can see them all here: https://github.com/nodejs/node/actions/runs/8989345480/job/24692243725
This switches the builds to ClangCL, correct? Meaning it is not an option, it just does it... right?
(Not an objection, I just want to clarify the intention.)
It's what it does for now, but my intention is to refactor to add an option.
I have no simple idea to fix it then. If we look at Chromium, I think they just prebuild the data files and add them to source control: https://chromium.googlesource.com/chromium/deps/icu/+/refs/heads/main/README.chromium https://chromium.googlesource.com/chromium/deps/icu/+/refs/heads/main/common/
I'll look into these links and the ICU issue generally in much more detail, but for us .dat files are not an issue. We have a problem in the next step when making a .obj file out of it. I'll get back to you once I have a better understanding of this.
It's what it does for now, but my intention is to refactor to add an option.
This should be the way to go. Eventually, we can move to using Clang in the CI, but this should be done in phases and while maintaining MSVC.
Added an option in https://github.com/nodejs/node/pull/52870/commits/58a7bf7446206c2f67730ec6119056a3b634b918
Rebased, squashed into a single commit, removed the ICU hack. Now MSVC should be the default and I think we can land this PR.
once this lands we'll have an option to use clang, but the default will not change
That's the intention.
ICU hack is completely removed so Clang will only work on x64 for now. Is this correct?
No, clang won't work at all. The hack is necessary for x64.
The question there is what will the next steps be
As I see it:
- Find a way to fix ICU build
- Add something in the CI matrix that compiles and tests with
.\vcbuild.bat clang-cl(only vs2022) - For a future Node.js major, enable
clangby default
As I see it:
- Find a way to fix ICU build
- Add something in the CI matrix that compiles and tests with
.\vcbuild.bat clang-cl(only vs2022)- For a future Node.js major, enable
clangby default
Just to add a few more topics on top of this:
- As we discussed here we should make clang version detection dynamic
- Based on this comment it is suggested that x86 should be dropped in Node.js v23. When I was testing, I had issues compiling x86 with Clang, should we even try fixing it if x86 is getting dropped? I guess TSC should make a final decision on dropping x86.
- One more thing we should consider is how will changing from MSVC to clang affect other parts of the ecosystem eg. node-gyp, some packages, etc.. From, what I know, Python tried moving to Clang but the ecosystem was relaying on MSVC so they had to stop that.
One more thing we should consider is how will changing from MSVC to clang affect other parts of the ecosystem eg. node-gyp, some packages, etc.. From, what I know, Python tried moving to Clang but the ecosystem was relaying on MSVC so they had to stop that.
Hopefully, us moving to clang doesn't mean the ecosystem packages have to do it as well.
@StefanStojanovic @targos
A nice option would also be to release secretly a version of Node built with clangcl... not part of the official release, but something advanced users can easily install and test. Serious Windows users could take the release and test it and see if they encounter issues. This can be built and just posted online somewhere random.
Further...
What I would like Node to do before switching over to clangcl, is to run benchmarks. The v8 performance should be fine because Google uses clangcl themselves for building v8. And the JIT output is likely compiler agnostic. But other components of Node.js could have major regressions.
Doing it properly will be much easier once it becomes a compile-time switch. The reason I have not investigated performance more carefully is that I am concerned with methodological biases. You want, as much as possible, to build the same code.
Python can be built with ClangCL: https://github.com/python/cpython/pull/101352
Note that though it makes many things faster, there are some strong performance regressions: https://github.com/python/cpython/issues/77532#issuecomment-1093781943
@targos I tried this branch locally and found that neither vcbuild.bat nor vcbuild.bat clang-cl worked. After some investigation, I saw that C++20 was not being added to project files just as you suspected here.
Before reading the rest of my comment, just keep in mind that I'm probably one of the furthest things from a .gypi expert. I just applied something that seemed logical to me and it worked...
Anyway, I applied the patch I'll send next, and both commands started generating project files with c++20 option (each one in the way it is supposed to), and non-clang compilation even passes. I assume this doesn't fix the other part of the problem you mentioned here, but I hope it'll be helpful to you nonetheless.
From fea38b393a54f3632efec924b01f1df27e378df4 Mon Sep 17 00:00:00 2001
From: StefanStojanovic <[email protected]>
Date: Fri, 10 May 2024 21:12:15 +0200
Subject: [PATCH 1/1] test
---
common.gypi | 42 ++++++++++++++++++++----------------------
1 file changed, 20 insertions(+), 22 deletions(-)
diff --git a/common.gypi b/common.gypi
index b9957070684..88480f85432 100644
--- a/common.gypi
+++ b/common.gypi
@@ -285,32 +285,30 @@
'_target_name!="<(node_core_target_name)")', {
'cflags!': ['-Werror'],
}],
- # TODO(targos): Remove condition and always use LanguageStandard options
- # once node-gyp supports them.
- ['clang==1', {
- 'msvs_settings': {
- 'VCCLCompilerTool': {
+ ],
+ 'msvs_settings': {
+ 'VCCLCompilerTool': {
+ # TODO(targos): Remove condition and always use LanguageStandard options
+ # once node-gyp supports them.
+ 'conditions': [
+ ['clang==1', {
'LanguageStandard': 'stdcpp20',
'LanguageStandard_C': 'stdc11',
- },
- },
- }, {
- 'msvs_settings': {
- 'VCCLCompilerTool': {
'AdditionalOptions': [
+ '/Zc:__cplusplus',
+ # The following option reduces the "error C1060: compiler is out of heap space"
+ '/Zm2000',
+ ]
+ }, {
+ 'AdditionalOptions': [
+ '/Zc:__cplusplus',
+ # The following option enables c++20 on Windows. This is needed for V8 v12.4+
'-std:c++20',
- ],
- },
- },
- }],
- ],
- 'msvs_settings': {
- 'VCCLCompilerTool': {
- 'AdditionalOptions': [
- '/Zc:__cplusplus',
- # The following option reduces the "error C1060: compiler is out of heap space"
- '/Zm2000',
- ],
+ # The following option reduces the "error C1060: compiler is out of heap space"
+ '/Zm2000',
+ ]
+ }]
+ ],
'BufferSecurityCheck': 'true',
'DebugInformationFormat': 1, # /Z7 embed info in .obj files
'ExceptionHandling': 0, # /EHsc
--
2.45.0.windows.1
I'm probably one of the furthest things from a .gypi expert.
I'd really want to meet one of those. 😆
Just a note on this, Most likely, after landing V8 v12.4 (last change to V8) there is an error cross-compiling to ARM64 with ClangCL. I'm certain that previously I could build it if I just changed *pCPU value.
I'm currently working on that ICU issue, so I'll also investigate this as part of that. The error I'm getting btw is this:
In file included from ..\..\deps\v8\src\objects\elements.cc:28:
In file included from ..\..\deps\v8\third_party/fp16/src/include/fp16.h:5:
In file included from ..\..\deps\v8\third_party\fp16\src\include\fp16/fp16.h:17:
..\..\deps\v8\third_party\fp16\src\include\fp16/bitcasts.h(28,9): error : use of undeclared identifier '_CopyFloatFromInt32' [E:\work\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]
..\..\deps\v8\third_party\fp16\src\include\fp16/bitcasts.h(46,20): error : use of undeclared identifier '_CopyInt32FromFloat' [E:\work\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]
..\..\deps\v8\third_party\fp16\src\include\fp16/bitcasts.h(64,9): error : use of undeclared identifier '_CopyDoubleFromInt64' [E:\work\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]
..\..\deps\v8\third_party\fp16\src\include\fp16/bitcasts.h(82,20): error : use of undeclared identifier '_CopyInt64FromDouble' [E:\work\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]
Just a note on this, Most likely, after landing V8 v12.4 (last change to V8) there is an error cross-compiling to ARM64 with ClangCL. I'm certain that previously I could build it if I just changed
*pCPUvalue.I'm currently working on that ICU issue, so I'll also investigate this as part of that. The error I'm getting btw is this:
In file included from ..\..\deps\v8\src\objects\elements.cc:28: In file included from ..\..\deps\v8\third_party/fp16/src/include/fp16.h:5: In file included from ..\..\deps\v8\third_party\fp16\src\include\fp16/fp16.h:17: ..\..\deps\v8\third_party\fp16\src\include\fp16/bitcasts.h(28,9): error : use of undeclared identifier '_CopyFloatFromInt32' [E:\work\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj] ..\..\deps\v8\third_party\fp16\src\include\fp16/bitcasts.h(46,20): error : use of undeclared identifier '_CopyInt32FromFloat' [E:\work\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj] ..\..\deps\v8\third_party\fp16\src\include\fp16/bitcasts.h(64,9): error : use of undeclared identifier '_CopyDoubleFromInt64' [E:\work\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj] ..\..\deps\v8\third_party\fp16\src\include\fp16/bitcasts.h(82,20): error : use of undeclared identifier '_CopyInt64FromDouble' [E:\work\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]
FWIW I found that support for _Copy* intrinsics was added to LLVM 18.1.0 (https://github.com/llvm/llvm-project/commit/03c698a431b4dc23c2b9de3e9befd1860fef6e80)
@targos
Why is this not using std::bit_cast?
https://en.cppreference.com/w/cpp/numeric/bit_cast