build: add option to enable clang-cl on Windows #52870

targos · 2024-05-07T05:49:01Z

The goal here is to do the right changes with lessons learned from #35433.

nodejs-github-bot · 2024-05-07T05:49:06Z

Review requested:

@nodejs/actions
@nodejs/gyp

Refs: nodejs/node#52870

tools/gyp/pylib/gyp/MSVSSettings.py

targos · 2024-05-07T06:52:33Z

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 #34201 (hack: 711a822)

targos · 2024-05-07T16:41:09Z

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.

StefanStojanovic · 2024-05-07T16:52:31Z

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

Refs: nodejs/node#52870

targos · 2024-05-08T06:26:55Z

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/

targos · 2024-05-08T07:13:43Z

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.bat would still use MSVC, while vcbuild.bat clang would use clang.

Luckily we could leverage __clang__ to have separate code for the 2 compilers, but we'd probably need either to push some deps changes 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 #35433 (review)

targos · 2024-05-08T07:15:29Z

I ported everything from the previous PR. Build works on my Windows PC.

targos · 2024-05-08T07:30:07Z

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

lemire · 2024-05-08T15:17:43Z

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

targos · 2024-05-08T15:24:36Z

It's what it does for now, but my intention is to refactor to add an option.

StefanStojanovic · 2024-05-08T21:08:47Z

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.

targos · 2024-05-09T11:14:14Z

Added an option in 58a7bf7

targos · 2024-05-10T05:25:43Z

Rebased, squashed into a single commit, removed the ICU hack.
Now MSVC should be the default and I think we can land this PR.

lemire · 2024-05-10T23:35:09Z

I'm probably one of the furthest things from a .gypi expert.

I'd really want to meet one of those. 😆

targos · 2024-05-11T07:13:19Z

@StefanStojanovic Thanks, I added your patch!

nodejs-github-bot · 2024-05-11T07:17:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59087/

targos · 2024-05-11T17:11:15Z

CI is green.

lpinca

RSLGTM

targos · 2024-05-11T20:01:48Z

configure.py

@@ -1042,8 +1049,9 @@ def get_gas_version(cc):
 # quite prepared to go that far yet.
 def check_compiler(o):
  if sys.platform == 'win32':
-    o['variables']['clang'] = 0


I made a mistake here, sorry. I'll keep the lines setting to 0 when the flag isn't passed

Most changes are gated by the `clang==1` condition to avoid breaking MSVC builds. Select C/C++ language standard with ClCompile options. This avoids passing the `-std:c++20` flag while compiling C code. Do it only under clang option to avoid breaking addons until node-gyp supports the new LanguageStandard options. Disable precompiled header configuration for now as it doesn't seem to work with clang-cl. Disable C++20 warnings emitted by the Visual Studio C++ STL. They're very noisy and not our responsibility to fix. Co-authored-by: Daniel Lemire <daniel@lemire.me> Co-authored-by: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>

nodejs-github-bot · 2024-05-12T10:02:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59167/

nodejs-github-bot · 2024-05-13T06:27:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59199/

StefanStojanovic · 2024-05-13T08:33:01Z

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]

targos · 2024-05-13T08:55:20Z

This needs a new approval after my force-push

StefanStojanovic

Let's give it a try, although my approval is gray so I doubt it will help you...

targos · 2024-05-13T18:39:50Z

@lemire Can you give me another green tick?

targos · 2024-05-13T18:48:33Z

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]

FWIW I found that support for _Copy* intrinsics was added to LLVM 18.1.0 (llvm/llvm-project@03c698a)

lemire · 2024-05-13T18:52:22Z

@targos

Why is this not using std::bit_cast?

https://en.cppreference.com/w/cpp/numeric/bit_cast

nodejs-github-bot · 2024-05-13T19:05:07Z

Landed in efa63f3

StefanStojanovic · 2024-05-14T06:09:55Z

FWIW I found that support for _Copy* intrinsics was added to LLVM 18.1.0 (llvm/llvm-project@03c698a)

I already have a PR in FP16 not to use _Copy* for ClangCL. As there is no LLVM 18.0.0 from what I see in their releases, I could potentially use __clang_major__ to restrict it to valid ClangCL versions only. Although I couldn't test it with ClangCL installed from Visual Studio, I'll try downloading LLVM and checking if it works correctly with v18.x

Why is this not using std::bit_cast?

The FP16 is a header-only library and states Compatible with C99 and C++11. Since std::bit_cast is a C++20 feature it cannot be used there.

Most changes are gated by the `clang==1` condition to avoid breaking MSVC builds. Select C/C++ language standard with ClCompile options. This avoids passing the `-std:c++20` flag while compiling C code. Do it only under clang option to avoid breaking addons until node-gyp supports the new LanguageStandard options. Disable precompiled header configuration for now as it doesn't seem to work with clang-cl. Disable C++20 warnings emitted by the Visual Studio C++ STL. They're very noisy and not our responsibility to fix. Co-authored-by: Daniel Lemire <daniel@lemire.me> Co-authored-by: Stefan Stojanovic <stefan.stojanovic@janeasystems.com> PR-URL: #52870 Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. labels May 7, 2024

targos added the wip Issues and PRs that are still a work in progress. label May 7, 2024

targos mentioned this pull request May 7, 2024

build: enable Clang-cl Windows builds #35433

Closed

targos force-pushed the clang-cl-v2 branch from 2fdab6c to 57b44a5 Compare May 7, 2024 06:24

targos added a commit to targos/gyp-next that referenced this pull request May 7, 2024

feat: support language standard keys in msvs_settings

a8e8ac7

Refs: nodejs/node#52870

targos mentioned this pull request May 7, 2024

feat: support language standard keys in msvs_settings nodejs/gyp-next#252

Merged

targos commented May 7, 2024

View reviewed changes

tools/gyp/pylib/gyp/MSVSSettings.py Outdated Show resolved Hide resolved

targos added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label May 7, 2024

targos mentioned this pull request May 7, 2024

build: harmonize Clang checks and update V8 warning cflags #52873

Closed

targos force-pushed the clang-cl-v2 branch from a0ba0e0 to 1106df1 Compare May 7, 2024 13:35

targos mentioned this pull request May 7, 2024

Install ClangCL on Windows hosts nodejs/build#3709

Closed

targos force-pushed the clang-cl-v2 branch from 1106df1 to 3936842 Compare May 7, 2024 16:41

targos added a commit to nodejs/gyp-next that referenced this pull request May 8, 2024

feat: support language standard keys in msvs_settings (#252)

322f6d5

Refs: nodejs/node#52870

targos force-pushed the clang-cl-v2 branch from 58a7bf7 to ed3492d Compare May 10, 2024 05:24

targos removed the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label May 10, 2024

targos force-pushed the clang-cl-v2 branch from ed3492d to 0a7e15b Compare May 11, 2024 07:12

targos force-pushed the clang-cl-v2 branch from 0a7e15b to a8c4ac2 Compare May 11, 2024 07:16

lemire approved these changes May 11, 2024

View reviewed changes

StefanStojanovic approved these changes May 11, 2024

View reviewed changes

lpinca approved these changes May 11, 2024

View reviewed changes

targos commented May 11, 2024

View reviewed changes

targos force-pushed the clang-cl-v2 branch from a8c4ac2 to 26b2f98 Compare May 12, 2024 06:52

StefanStojanovic approved these changes May 13, 2024

View reviewed changes

StefanStojanovic mentioned this pull request May 13, 2024

build node.js using clangcl when under Windows #52809

Open

lemire approved these changes May 13, 2024

View reviewed changes

targos added the commit-queue Add this label to land a pull request using GitHub Actions. label May 13, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 13, 2024

nodejs-github-bot merged commit efa63f3 into nodejs:main May 13, 2024
50 checks passed

targos deleted the clang-cl-v2 branch May 13, 2024 19:07

targos mentioned this pull request May 15, 2024

v22.2.0 release proposal #52971

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add option to enable clang-cl on Windows #52870

build: add option to enable clang-cl on Windows #52870

targos commented May 7, 2024 •

edited

nodejs-github-bot commented May 7, 2024

targos commented May 7, 2024

targos commented May 7, 2024

StefanStojanovic commented May 7, 2024

targos commented May 8, 2024

targos commented May 8, 2024

targos commented May 8, 2024

targos commented May 8, 2024

lemire commented May 8, 2024

targos commented May 8, 2024

StefanStojanovic commented May 8, 2024

targos commented May 9, 2024

targos commented May 10, 2024

lemire commented May 10, 2024

targos commented May 11, 2024

nodejs-github-bot commented May 11, 2024

targos commented May 11, 2024

lpinca left a comment

targos May 11, 2024

targos May 12, 2024

nodejs-github-bot commented May 12, 2024

nodejs-github-bot commented May 13, 2024

StefanStojanovic commented May 13, 2024

targos commented May 13, 2024

StefanStojanovic left a comment

targos commented May 13, 2024

targos commented May 13, 2024

lemire commented May 13, 2024

nodejs-github-bot commented May 13, 2024

StefanStojanovic commented May 14, 2024

build: add option to enable clang-cl on Windows #52870

build: add option to enable clang-cl on Windows #52870

Conversation

targos commented May 7, 2024 • edited

nodejs-github-bot commented May 7, 2024

targos commented May 7, 2024

targos commented May 7, 2024

StefanStojanovic commented May 7, 2024

targos commented May 8, 2024

targos commented May 8, 2024

targos commented May 8, 2024

targos commented May 8, 2024

lemire commented May 8, 2024

targos commented May 8, 2024

StefanStojanovic commented May 8, 2024

targos commented May 9, 2024

targos commented May 10, 2024

lemire commented May 10, 2024

targos commented May 11, 2024

nodejs-github-bot commented May 11, 2024

targos commented May 11, 2024

lpinca left a comment

Choose a reason for hiding this comment

targos May 11, 2024

Choose a reason for hiding this comment

targos May 12, 2024

Choose a reason for hiding this comment

nodejs-github-bot commented May 12, 2024

nodejs-github-bot commented May 13, 2024

StefanStojanovic commented May 13, 2024

targos commented May 13, 2024

StefanStojanovic left a comment

Choose a reason for hiding this comment

targos commented May 13, 2024

targos commented May 13, 2024

lemire commented May 13, 2024

nodejs-github-bot commented May 13, 2024

StefanStojanovic commented May 14, 2024

targos commented May 7, 2024 •

edited