Skip to content
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

build: enable Clang-cl Windows builds #35433

Closed
wants to merge 12 commits into from
Closed

Conversation

targos
Copy link
Member

@targos targos commented Sep 30, 2020

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

@nodejs-github-bot 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
# else
*pCPU = IMAGE_FILE_MACHINE_UNKNOWN;
*pCPU = IMAGE_FILE_MACHINE_AMD64;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for #34201

node.gypi Outdated Show resolved Hide resolved
@targos targos added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Sep 30, 2020
@targos
Copy link
Member Author

targos commented Sep 30, 2020

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]

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_;
};

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
Copy link
Member Author

targos commented Oct 2, 2020

/cc @nodejs/v8

@gengjiawen
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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

@gengjiawen
Copy link
Member

gengjiawen commented Oct 19, 2020

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
Copy link
Member Author

targos commented Oct 19, 2020

Rebased on #35700

@targos
Copy link
Member Author

targos commented Oct 19, 2020

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
Copy link
Member Author

targos commented Oct 19, 2020

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

@gengjiawen
Copy link
Member

@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
Copy link
Member Author

targos commented Oct 19, 2020

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
Copy link
Member

Looks like all related to inline function on windows platform.

@codecov-io

This comment has been minimized.

@gengjiawen
Copy link
Member

gengjiawen commented Nov 9, 2020

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
Copy link
Contributor

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
Copy link
Contributor

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?

image

@gengjiawen
Copy link
Member

@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
Copy link
Contributor

dennisameling commented Nov 11, 2020

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
Copy link
Member

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.

Copy link
Contributor

@StefanStojanovic StefanStojanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +849 to +851
// TODO: detect ARM64
# else
*pCPU = IMAGE_FILE_MACHINE_UNKNOWN;
*pCPU = IMAGE_FILE_MACHINE_AMD64;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

targos commented Apr 26, 2024

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
Copy link
Member

lemire commented Apr 29, 2024

Note that #52714 should help a bit once merged.

deps/base64/base64.gyp Outdated Show resolved Hide resolved
targos and others added 12 commits April 30, 2024 17:50
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
Copy link
Member

anonrig commented Apr 30, 2024

It seems Windows build is passing?

@targos
Copy link
Member Author

targos commented May 1, 2024

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
Copy link
Member

lemire commented May 1, 2024

@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
Copy link
Member

lemire commented May 2, 2024

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

@targos
Copy link
Member Author

targos commented May 7, 2024

I opened #52870 to do things correctly and incrementally.

@targos targos closed this May 8, 2024
@targos targos deleted the clang-cl branch June 10, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. tools Issues and PRs related to the tools directory. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.