build: enable Clang-cl Windows builds #35433

targos · 2020-09-30T18:33:36Z

This is very hacky at the moment. I'm trying to find out what needs to be done to enable it and where.
I'm opening a PR to discuss the changes and seek some help, because it doesn't work yet!

To try it: .\vcbuild.bat

targos · 2020-09-30T18:36:59Z

deps/icu-small/source/tools/toolutil/pkg_genc.cpp

 #   else
-        *pCPU = IMAGE_FILE_MACHINE_UNKNOWN;
+        *pCPU = IMAGE_FILE_MACHINE_AMD64;


This is for #34201

deps/zlib/crc_folding.c

node.gypi

tools/gyp/pylib/gyp/generator/msvs.py

targos · 2020-09-30T18:43:18Z

With all the above hacks, I am blocked on this error:

lld-link : error : undefined symbol: public: virtual __cdecl v8::internal::Relocatable::~Relocatable(void) [D:\Git\nodejs\node\tools\v8_gypfiles\mksnapshot.vcxproj]

Related warning in the build output:

..\..\deps\v8\src/objects/objects.h(818,18): warning : inline function 'v8::internal::Relocatable::~Relocatable' is not defined [-Wundefined-inline] [D:\a\node\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]
..\..\deps\v8\src/objects/string.h(821,25): message : used here [D:\a\node\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]

node/deps/v8/src/objects/objects.h

Lines 815 to 833 in 4a6005c

    
           class Relocatable { 
        
            public: 
        
             explicit inline Relocatable(Isolate* isolate); 
        
             inline virtual ~Relocatable(); 
        
             virtual void IterateInstance(RootVisitor* v) {} 
        
             virtual void PostGarbageCollection() {} 
        
             static void PostGarbageCollectionProcessing(Isolate* isolate); 
        
             static int ArchiveSpacePerThread(); 
        
             static char* ArchiveState(Isolate* isolate, char* to); 
        
             static char* RestoreState(Isolate* isolate, char* from); 
        
             static void Iterate(Isolate* isolate, RootVisitor* v); 
        
             static void Iterate(RootVisitor* v, Relocatable* top); 
        
             static char* Iterate(RootVisitor* v, char* t); 
        
            private: 
        
             Isolate* isolate_; 
        
             Relocatable* prev_; 
        
           };

node/deps/v8/src/objects/string.h

Lines 821 to 836 in 4a6005c

    
           class V8_EXPORT_PRIVATE FlatStringReader : public Relocatable { 
        
            public: 
        
             FlatStringReader(Isolate* isolate, Handle<String> str); 
        
             FlatStringReader(Isolate* isolate, Vector<const char> input); 
        
             void PostGarbageCollection() override; 
        
             inline uc32 Get(int index); 
        
             template <typename Char> 
        
             inline Char Get(int index); 
        
             int length() { return length_; } 
        
            private: 
        
             Address* str_; 
        
             bool is_one_byte_; 
        
             int length_; 
        
             const void* start_; 
        
           };

targos · 2020-10-02T08:28:22Z

/cc @nodejs/v8

gengjiawen · 2020-10-02T14:56:11Z

Another tricky thing is that we can't even test this on github action due to space limit https://github.com/nodejs/node/pull/35433/checks?check_run_id=1189469828 LIB : LLVM error : IO failure on output stream: no space on device.

dennisameling · 2020-10-03T20:08:43Z

Regarding the disk space issue: the D:\ drive on Windows VMs apparently has 14GB available - this is also described in the official docs. A workaround is to move things to the C:\ drive which has 80GB+ as described in actions/runner-images#1341 (comment)

gengjiawen · 2020-10-19T00:45:45Z

@targos Can you rebase this, so we can working on new V8 ?

gengjiawen · 2020-10-19T01:14:00Z

Regarding the disk space issue: the D:\ drive on Windows VMs apparently has 14GB available - this is also described in the official docs. A workaround is to move things to the C:\ drive which has 80GB+ as described in actions/virtual-environments#1341 (comment)

I investigate a little bit, looks like not an easy thing to do: actions/checkout#197.

Also, below won't work either on windows CI on github action.

    defaults:
      run:
        working-directory: C:/nodejs

Not sure why they default to drive D with such a limited disk space.

targos · 2020-10-19T06:09:02Z

Rebased on #35700

targos · 2020-10-19T06:23:38Z

I don't know if CI has clang-cl. Let's try: https://ci.nodejs.org/job/node-test-commit-windows-fanned/38888/

Edit: well, it doesn't 😄

08:23:59 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Microsoft\VC\v160\Microsoft.CppBuild.targets(411,5): error MSB8020: The build tools for ClangCL (Platform Toolset = 'ClangCL') cannot be found. To build using the ClangCL build tools, please install ClangCL build tools.  Alternatively, you may upgrade to the current Visual Studio tools by selecting the Project menu or right-click the solution, and then selecting "Retarget solution". [C:\workspace\node-compile-windows\node\node_etw.vcxproj]

targos · 2020-10-19T06:29:42Z

@nodejs/build-infra Would it be easy to install ClangCL build tools on the Windows machines?

gengjiawen · 2020-10-19T06:45:46Z

@nodejs/build-infra Would it be easy to install ClangCL build tools on the Windows machines?

Not sure this will help: https://github.com/appveyor/build-images/blob/27bde614bc60d7ef7a8bc46182f4d7582fa11b56/scripts/Windows/install_vs2019.ps1#L192.

targos · 2020-10-19T06:52:23Z

So I tried to build it on my local machine.

There's one warning that is very noisy (I think it's printed for every V8 source file:

..\..\deps\v8\src/base/safe_conversions_impl.h(158,46): warning : implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808 [-Wimplicit-int-float-conversion] [D:\Git\no 
dejs\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]
..\..\deps\v8\src/base/safe_conversions_impl.h(213,52): message : in instantiation of member function 'v8::base::internal::DstRangeRelationToSrcRangeImpl<long long, double, v8::base::internal::INTEGER_REPRESENTATION_SIGNE 
D, v8::base::internal::INTEGER_REPRESENTATION_SIGNED, v8::base::internal::NUMERIC_RANGE_NOT_CONTAINED>::Check' requested here [D:\Git\nodejs\node\tools\v8_gypfiles\v8_base_without_compiler.vcxproj]
..\..\deps\v8\src/base/safe_conversions.h(44,21): message : in instantiation of function template specialization 'v8::base::internal::DstRangeRelationToSrcRange<long long, double>' requested here [D:\Git\nodejs\node\tools 
\v8_gypfiles\v8_base_without_compiler.vcxproj]
..\..\deps\v8\src/base/platform/time.h(228,20): message : in instantiation of function template specialization 'v8::base::saturated_cast<long long, double>' requested here [D:\Git\nodejs\node\tools\v8_gypfiles\v8_base_wit 
hout_compiler.vcxproj]

Then it fails with 3 errors (related warnings included):

lld-link : error : undefined symbol: public: virtual __cdecl v8::internal::Relocatable::~Relocatable(void) [D:\Git\nodejs\node\tools\v8_gypfiles\mksnapshot.vcxproj]
  >>> referenced by ..\..\out\Release\obj\mksnapshot\\deps\v8\src\snapshot\embedded\platform-embedded-file-writer-win.obj
  >>> referenced by ..\..\out\Release\obj\mksnapshot\\deps\v8\src\snapshot\mksnapshot.obj
  >>> referenced by v8_base_without_compiler.lib(stack-guard.obj)

..\..\deps\v8\src/objects/objects.h(818,18): warning : inline function 'v8::internal::Relocatable::~Relocatable' is not defined [-Wundefined-inline] [D:\Git\nodejs\node\tools\v8_gypfiles\mksnapshot.vcxproj]
..\..\deps\v8\src/objects/string.h(832,25): message : used here [D:\Git\nodejs\node\tools\v8_gypfiles\mksnapshot.vcxproj]

lld-link : error : undefined symbol: public: class v8::internal::Object __cdecl v8::internal::FixedArray::get(int) const [D:\Git\nodejs\node\tools\v8_gypfiles\mksnapshot.vcxproj]
  >>> referenced by ..\..\out\Release\obj\mksnapshot\\deps\v8\src\snapshot\embedded\platform-embedded-file-writer-win.obj
  >>> referenced by ..\..\out\Release\obj\mksnapshot\\deps\v8\src\snapshot\mksnapshot.obj
  >>> referenced by v8_base_without_compiler.lib(flush-instruction-cache.obj)

..\..\deps\v8\src/objects/fixed-array.h(102,17): warning : inline function 'v8::internal::FixedArray::get' is not defined [-Wundefined-inline] [D:\Git\nodejs\node\tools\v8_gypfiles\mksnapshot.vcxproj]
..\..\deps\v8\src/objects/ordered-hash-table.h(88,23): message : used here [D:\Git\nodejs\node\tools\v8_gypfiles\mksnapshot.vcxproj]

lld-link : error : undefined symbol: public: void __cdecl v8::internal::FixedArray::set(int, class v8::internal::Smi) [D:\Git\nodejs\node\tools\v8_gypfiles\mksnapshot.vcxproj]
  >>> referenced by ..\..\out\Release\obj\mksnapshot\\deps\v8\src\snapshot\embedded\platform-embedded-file-writer-win.obj
  >>> referenced by ..\..\out\Release\obj\mksnapshot\\deps\v8\src\snapshot\mksnapshot.obj
  >>> referenced by v8_base_without_compiler.lib(flush-instruction-cache.obj)

..\..\deps\v8\src/objects/fixed-array.h(134,15): warning : inline function 'v8::internal::FixedArray::set' is not defined [-Wundefined-inline] [D:\Git\nodejs\node\tools\v8_gypfiles\mksnapshot.vcxproj]
..\..\deps\v8\src/objects/ordered-hash-table.h(210,5): message : used here [D:\Git\nodejs\node\tools\v8_gypfiles\mksnapshot.vcxproj]

gengjiawen · 2020-10-19T06:59:02Z

Looks like all related to inline function on windows platform.

gengjiawen · 2020-11-09T07:46:16Z

Now stucks with

  ..\..\out\Release\obj\v8_libsampler\\deps\v8\src\libsampler\sampler.obj: no such file or directory
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(1309,5): error MSB6006: "llvm-lib.exe" exited with code 1. [D:\code\node\tools\v8_gypfiles\v8_libsampler.vcxproj]
  ..\..\out\Release\obj\v8_zlib\\deps\v8\third_party\zlib\adler32.obj: no such file or directory
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(1309,5): error MSB6006: "llvm-lib.exe" exited with code 1. [D:\code\node\tools\v8_gypfiles\v8_zlib.vcxproj]

Maybe related: https://developercommunity.visualstudio.com/content/problem/1110835/clangcl-ltcg-is-passed-to-llvm-lib.html

This occurred when I update my visual studio to latest.

I also get some ideas to patch V8 to make the windows build works.

dennisameling · 2020-11-11T08:41:42Z

Just for the record, Visual Studio 16.8 was released yesterday and has the following mentioned in its release notes:

Support for ARM64 projects using clang-cl.

Thought I'd mention it here as it might help for this PR. I have a Surface Pro X based on the arm64 architecture, let me know if you'd like me to test something 👍

dennisameling · 2020-11-11T10:28:05Z

Just tried to build using .\vcbuild.bat noetw on x64 with Visual Studio 16.8, getting the same errors as @targos regarding mksnapshot, but there are actually quite some things that build correctly, so that's promising 🚀

@gengjiawen Does it make any difference if you update to Visual Studio 16.8, which was released yesterday?

gengjiawen · 2020-11-11T11:00:46Z

@gengjiawen Does it make any difference if you update to Visual Studio 16.8, which was released yesterday?

I forget to remove '/P'. I should try my build now.

I have a Surface Pro X based on the arm64 architecture

Have you tried the official arm64 build for windows. Will it run ? (I have no device to test it)

dennisameling · 2020-11-11T11:17:12Z

Have you tried the official arm64 build for windows. Will it run ? (I have no device to test it)

Yes (see nodejs/build#2450 (comment)), and it's blazing fast 🚀 I use it on a daily basis and to work on things like an arm64 build for GitHub Desktop: desktop/desktop#9691

gengjiawen · 2020-12-01T09:04:53Z

This is more likely a bug from what I test (refactor the inline implementation). Not sure this need to reported to clang team or msvc team.

StefanStojanovic

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.

StefanStojanovic · 2024-04-26T13:50:19Z

deps/icu-small/source/tools/toolutil/pkg_genc.cpp

+// TODO: detect ARM64
 #   else
-        *pCPU = IMAGE_FILE_MACHINE_UNKNOWN;
+        *pCPU = IMAGE_FILE_MACHINE_AMD64;


I tried an ARM64 build and got an error connected to these changes.

After some investigation, I found that dealing with a TODO: detect ARM64 added here would be tricky. This file is a part of a host project, thus it is always compiled for x64 in our case. Ideally, we'd use _M_ARM64/_M_AMD64 and set different IMAGE_FILE_MACHINE_... based on that in a target architecture project, but that cannot be used here.

What we can do, is add another option to genccode_host (genccode.c) that we'd use for setting the *pCPU. The issue with it is that the files I'd change in the process are part of the deps so we'd be making a floating patch on similar to the ones we have on V8, which is not great, but currently I see no other way (although there might be some).

targos · 2024-04-26T14:25:56Z

I rebased the branch.

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?

I see the same as you: an optional clang option that we can pass to vcbuild.bat.

lemire · 2024-04-29T13:17:23Z

Note that #52714 should help a bit once merged.

deps/base64/base64.gyp

TODO: upstream change to Chromium

1. To avoid many warnings, this PR declares the C and C++ standards separately. 2. This PR extends gyp so that we can build with AVX-512. Nevertheless, getting runtime dispatching with ClangCl through Visual Studio is challenging, so we disable it. It only affects one component of zip, so the effect on runtime performance should be negligible. Note that other dependencies such as simdutf do not need to this build support for runtime dispatching (so you still get AVX2, AVX-512 support in these dependencies).

anonrig · 2024-04-30T18:46:24Z

It seems Windows build is passing?

targos · 2024-05-01T05:34:33Z

Yes, it's been passing for some time. Now what we need to do is go back to all the "hacks" and either convert them to acceptable code or upstream the changes.

lemire · 2024-05-01T18:11:11Z

@targos There are three dependencies affected. The zlib changes are not, in my estimation, needed. I have a comment above regarding this. The v8 change should not be needed, indeed Chrome is compiled using ClangCL so it seems unlikely that we would need to patch v8. The remaining one (icu) is mysterious to me... and is the most likely to be strictly needed...

lemire · 2024-05-02T19:39:55Z

I ran some limited benchmarks and the results are encouraging, see https://lemire.me/blog/2024/05/02/should-node-js-be-built-with-clangcl-under-windows/

It seems possible that compiling Node.js with ClangCL under Windows would improve the performance. Of course, this requires further validation.

@RafaelGSS @anonrig

tools/gyp/pylib/gyp/generator/msvs.py

tools/gyp/pylib/gyp/msvs_emulation.py

targos · 2024-05-07T05:51:53Z

I opened #52870 to do things correctly and incrementally.

nodejs-github-bot added build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. zlib Issues and PRs related to the zlib subsystem. labels Sep 30, 2020

targos commented Sep 30, 2020

View reviewed changes

deps/zlib/crc_folding.c Show resolved Hide resolved

targos commented Sep 30, 2020

View reviewed changes

node.gypi Outdated Show resolved Hide resolved

targos commented Sep 30, 2020

View reviewed changes

tools/gyp/pylib/gyp/generator/msvs.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 Sep 30, 2020

targos force-pushed the clang-cl branch from e0b3e1c to 647f0a6 Compare October 19, 2020 06:08

This comment has been minimized.

Sign in to view

gengjiawen mentioned this pull request Oct 23, 2020

tools: add msvc /P output to .gitignore #35735

Closed

gengjiawen mentioned this pull request Jan 5, 2021

Use Clang on Windows nodejs/node-gyp#2295

Open

StefanStojanovic reviewed Apr 26, 2024

View reviewed changes

targos force-pushed the clang-cl branch from bc718f2 to 2b19112 Compare April 26, 2024 14:25

anonrig reviewed Apr 30, 2024

View reviewed changes

deps/base64/base64.gyp Outdated Show resolved Hide resolved

targos and others added 12 commits April 30, 2024 17:50

tools: hack gyp to generate clang-cl solutions

ff3a4ec

deps: add missing zlib includes

26e51ff

TODO: upstream change to Chromium

hack ICU

711a822

disable some workflows

bbf0efc

remove macros

3ccb17d

set clang to 1

273cb2b

fix pch

6ec7554

fix: embed snapshot asm into a source file like v8 does

d4fe780

fix: lib paths and remove unneeded masm

9d7c136

fix: only build embedded.cc when under Windows. (#11)

1a5726a

fix: only generate embedded.cc under Windows. (#12)

d5302f5

targos force-pushed the clang-cl branch from 2b19112 to d5302f5 Compare April 30, 2024 15:51

lemire mentioned this pull request May 2, 2024

build node.js using clangcl when under Windows #52809

Open

12 tasks

targos commented May 3, 2024

View reviewed changes

tools/gyp/pylib/gyp/generator/msvs.py Show resolved Hide resolved

targos commented May 3, 2024

View reviewed changes

tools/gyp/pylib/gyp/generator/msvs.py Show resolved Hide resolved

targos commented May 3, 2024

View reviewed changes

tools/gyp/pylib/gyp/msvs_emulation.py Show resolved Hide resolved

targos mentioned this pull request May 7, 2024

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

Merged

targos closed this May 8, 2024

targos deleted the clang-cl branch June 10, 2024 09:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: enable Clang-cl Windows builds #35433

build: enable Clang-cl Windows builds #35433

targos commented Sep 30, 2020 •

edited

targos Sep 30, 2020

targos commented Sep 30, 2020 •

edited

targos commented Oct 2, 2020

gengjiawen commented Oct 2, 2020

dennisameling commented Oct 3, 2020

gengjiawen commented Oct 19, 2020

gengjiawen commented Oct 19, 2020 •

edited

targos commented Oct 19, 2020

targos commented Oct 19, 2020 •

edited

targos commented Oct 19, 2020

gengjiawen commented Oct 19, 2020

targos commented Oct 19, 2020

gengjiawen commented Oct 19, 2020

This comment has been minimized.

gengjiawen commented Nov 9, 2020 •

edited

dennisameling commented Nov 11, 2020

dennisameling commented Nov 11, 2020

gengjiawen commented Nov 11, 2020

dennisameling commented Nov 11, 2020 •

edited

gengjiawen commented Dec 1, 2020

StefanStojanovic left a comment

StefanStojanovic Apr 26, 2024

targos commented Apr 26, 2024

lemire commented Apr 29, 2024

anonrig commented Apr 30, 2024

targos commented May 1, 2024

lemire commented May 1, 2024

lemire commented May 2, 2024

targos commented May 7, 2024

build: enable Clang-cl Windows builds #35433

build: enable Clang-cl Windows builds #35433

Conversation

targos commented Sep 30, 2020 • edited

targos Sep 30, 2020

Choose a reason for hiding this comment

targos commented Sep 30, 2020 • edited

targos commented Oct 2, 2020

gengjiawen commented Oct 2, 2020

dennisameling commented Oct 3, 2020

gengjiawen commented Oct 19, 2020

gengjiawen commented Oct 19, 2020 • edited

targos commented Oct 19, 2020

targos commented Oct 19, 2020 • edited

targos commented Oct 19, 2020

gengjiawen commented Oct 19, 2020

targos commented Oct 19, 2020

gengjiawen commented Oct 19, 2020

This comment has been minimized.

gengjiawen commented Nov 9, 2020 • edited

dennisameling commented Nov 11, 2020

dennisameling commented Nov 11, 2020

gengjiawen commented Nov 11, 2020

dennisameling commented Nov 11, 2020 • edited

gengjiawen commented Dec 1, 2020

StefanStojanovic left a comment

Choose a reason for hiding this comment

StefanStojanovic Apr 26, 2024

Choose a reason for hiding this comment

targos commented Apr 26, 2024

lemire commented Apr 29, 2024

anonrig commented Apr 30, 2024

targos commented May 1, 2024

lemire commented May 1, 2024

lemire commented May 2, 2024

targos commented May 7, 2024

targos commented Sep 30, 2020 •

edited

targos commented Sep 30, 2020 •

edited

gengjiawen commented Oct 19, 2020 •

edited

targos commented Oct 19, 2020 •

edited

gengjiawen commented Nov 9, 2020 •

edited

dennisameling commented Nov 11, 2020 •

edited