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

Support building with win-clang #19630

Closed
5 tasks
hashseed opened this issue Mar 27, 2018 · 25 comments
Closed
5 tasks

Support building with win-clang #19630

hashseed opened this issue Mar 27, 2018 · 25 comments
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.

Comments

@hashseed
Copy link
Member

hashseed commented Mar 27, 2018

Chrome and V8 support building with Clang on windows. Node.js should include support for this too.

Benefits:

  • Future proof. Chrome is already deprecating MSVC build. V8 might do the same in the long-term.
  • No more dependency on installing Visual Studio. Clang is open source.

Blockers:

  • Expose clang build in configure.
  • Official way to build V8 on Windows is to use ninja. However, gyp backend for ninja only supports MSVC.
  • openssl.gyp requires llvm_version to be set.
  • Actually fetching Clang. This is done in V8 through python .\deps\v8\tools\clang\scripts\update.py.
  • Version and macro detection does not work with clang-cl.exe.
@hashseed
Copy link
Member Author

/cc @targos

@addaleax addaleax added windows Issues and PRs related to the Windows platform. build Issues and PRs related to build files or the CI. labels Mar 27, 2018
@targos
Copy link
Member

targos commented Mar 27, 2018

/cc @nodejs/platform-windows

@seishun
Copy link
Contributor

seishun commented Mar 30, 2018

Official way to build V8 on Windows is to use ninja. However, gyp backend for ninja only supports MSVC.

How can we solve this?

@hashseed
Copy link
Member Author

Good question. One of the reasons for moving off gyp is scenarios like this where new toolchains are not supported. I guess implementing support for ninja with clang on Windows would be a way. It might be not all that complicated, if clang on Windows works the same way as clang on linux.

@seishun
Copy link
Contributor

seishun commented Mar 30, 2018

You mean implementing it in gyp?

@bnoordhuis
Copy link
Member

I suspect this already works when you use clang-cl, the cl.exe compatible driver.

Untested, but configure would need to write out a make_global_settings stanza to config.gypi that looks like this and will be picked up by the ninja generator:

# ...
'make_global_settings': {
  'CC': '/path/to/clang-cl',   # or just 'clang-cl' if it's on the path
  'CXX': '/path/to/clang-cl',  # ditto
}
# ...

@nico
Copy link

nico commented Apr 5, 2018

bnoordhuis's comment is correct, except you don't need to set CXX since the ninja generator doesn't look at that in msvc compat mode (cf https://chromium.googlesource.com/external/gyp/+/e540e6ab6291d4e63f262c8169ef0d0aae11dbec / https://codereview.chromium.org/147083011 for how we hooked up clang-cl in the Chromium gyp win build back in the day).

@hashseed
Copy link
Member Author

The amazing @agrieve from the Chrome team has come up with this PR (against the fork of Node.js that V8 is maintaining) to make building with Clang on Windows work!

We could use some feedback on whether this way to build works well for everyone, similar to when I initially implemented the --build-v8-with-gn configure flag. @seishun @targos

@refack
Copy link
Contributor

refack commented Apr 17, 2018

Although as @bnoordhuis mentions there are workarounds for cmake/ninja/GYP/Windows, I'll look into getting that to work out of the box.

  • This still does not remove the VisualStudio dependency per se, since clang-win / clang-cl and the Windows SDK are still interdependent, so IMHO the more "interesting" issue IMHO is smooth integration with the Windows SDK.
    image1

  • There might be conflicting interests with the @nodejs/chakracore team. But IMHO that should be upstreamed to V8 & Chromium.

@hashseed
Copy link
Member Author

There might be conflicting interests with the @nodejs/chakracore team. But IMHO that should be upstreamed to V8 & Chromium.

My interpretation is that having alternatives is always great :)

@gibfahn
Copy link
Member

gibfahn commented Apr 19, 2018

So if this doesn't remove the dependency on Visual Studio, the only benefit would be that we'd have to do this at some point if V8 drop support for MSVC?

@hashseed
Copy link
Member Author

hashseed commented Apr 30, 2018

FWIW we have a fork of Node that can fully build with clang-cl on Windows.

It still requires Visual Studio and some dependencies to be installed.

The benefit of that build is that the gyp-gn bridge works on that build. I.e. gyp files do not need to be maintained when updated to a new V8 version. It is also guaranteed to build if V8 decides to deprecate MSVC support in the future. This is relevant when MSVC and Clang differ in how they interpret C++ syntax.

Dependencies:

  • Git
  • Python 2.7
  • Pywin32
  • Visual Studio 2017, "Desktop development with C++". Make sure to include:
    • Windows 10 SDK (10.0.15063) for Desktop C++. This version is hardcoded in to deps/v8/build/toolchain/win/setup_toolchain.py.
    • Visual C++ ATL support
  • Debugging Tools for Windows SDK (as found here)

Build steps:

python configure --build-v8-with-gn --use-clang-cl --ninja
deps\v8\_depot_tools\ninja -C out\Release node

There is obviously still a bit of a way to go here.

What also doesn't work is that add-on tests do not have a ninja target, and npm install probably doesn't work with clang-cl.

Edit: missing dependency.

@seishun
Copy link
Contributor

seishun commented Apr 30, 2018

The list of dependencies seems incomplete. I'm getting the following error:

FAILED: obj/deps/v8/gypfiles/v8_monolith.gen/gn/build.ninja
C:\Python27\python.exe gyp-win-tool action-wrapper environment.x64 v8_monolith_target_build_with_gn_generate_build_files_f3270641800ff32515e443515bf752cc..rsp ..\..\deps\v8\gypfiles
Traceback (most recent call last):
  File "C:/Users/Nikolai/node/deps/v8/build/vs_toolchain.py", line 482, in <module>
    sys.exit(main())
  File "C:/Users/Nikolai/node/deps/v8/build/vs_toolchain.py", line 478, in main
    return commands[sys.argv[1]](*sys.argv[2:])
  File "C:/Users/Nikolai/node/deps/v8/build/vs_toolchain.py", line 306, in CopyDlls
    _CopyDebugger(target_dir, target_cpu)
  File "C:/Users/Nikolai/node/deps/v8/build/vs_toolchain.py", line 337, in _CopyDebugger
    ' 10 SDK.' % (debug_file, full_path))
Exception: dbghelp.dll not found in "C:\Program Files (x86)\Windows Kits\10\Debuggers\x64\dbghelp.dll"
You must install the "Debugging Tools for Windows" feature from the Windows 10 SDK.
ERROR at //build/toolchain/win/BUILD.gn:43:3: Script returned non-zero exit code.
  exec_script("../../vs_toolchain.py",
  ^----------
Current dir: C:/Users/Nikolai/node/out/Release/obj/deps/v8/gypfiles/v8_monolith.gen/gn/
Command: C:/Python27/python.exe -- C:/Users/Nikolai/node/deps/v8/build/vs_toolchain.py copy_dlls C:/Users/Nikolai/node/out/Release/obj/deps/v8/gypfiles/v8_monolith.gen/gn Release x64
Returned 1.
See //BUILD.gn:618:1: which caused the file to be included.
action("js2c") {
^---------------
Traceback (most recent call last):
  File "..\tools\node\build_gn.py", line 141, in <module>
    GenerateBuildFiles(options)
  File "..\tools\node\build_gn.py", line 75, in GenerateBuildFiles
    subprocess.check_call(args)
  File "C:\Python27\lib\subprocess.py", line 540, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['C:\\Users\\Nikolai\\node\\deps\\v8\\buildtools\\win\\gn', 'gen', 'C:\\Users\\Nikolai\\node\\out\\Release\\obj\\deps\\v8\\gypfiles\\v8_monolith.gen\\gn', '-q', '--args=v8_monolithic=true is_component_build=false v8_use_external_startup_data=false use_custom_libcxx=false v8_promise_internal_field_count=true target_cpu="x64" target_os="win" v8_target_cpu="x64" v8_embedder_string="-node.4" v8_use_snapshot=true v8_optimized_debug=false v8_enable_disassembler=true v8_postmortem_support=false is_debug=false clang_base_path="C:\\Users\\Nikolai\\node\\deps\\v8\\third_party\\llvm-build\\Release+Asserts"']' returned non-zero exit status 1

I'm also getting a warning on every file, is that a known issue?

[20/664] CC obj\deps\openssl\openssl\crypto\ecdsa\openssl.ecs_err.obj
clang-cl.exe: warning: argument unused during compilation: '/GL' [-Wunused-command-line-argument]
[21/664] CC obj\deps\openssl\openssl\crypto\engine\openssl.tb_rand.obj
clang-cl.exe: warning: argument unused during compilation: '/GL' [-Wunused-command-line-argument]
[22/664] CC obj\deps\openssl\openssl\crypto\ecdh\openssl.ech_kdf.obj
clang-cl.exe: warning: argument unused during compilation: '/GL' [-Wunused-command-line-argument]
[23/664] CC obj\deps\openssl\openssl\crypto\ec\openssl.ecp_oct.obj
clang-cl.exe: warning: argument unused during compilation: '/GL' [-Wunused-command-line-argument]

@hashseed
Copy link
Member Author

hashseed commented Apr 30, 2018

That might indeed by true. I've seen that error too and solved it by following the hint in the error log, and installed Debugging Tools for Windows.

I've seen the same warning too. The issue here is that Clang does not support the whole program optimization flag. V8 files are not affected by this.

@seishun
Copy link
Contributor

seishun commented Apr 30, 2018

Getting the following error now:

C:\Users\Nikolai\node>deps\v8\_depot_tools\ninja -C out\Release node
ninja: Entering directory `out\Release'
[0/13] ACTION v8_monolith: build_with_gn_f3270641800ff32515e443515bf752cc
Using depot tools in ..\_depot_tools
ninja: Entering directory `C:\Users\Nikolai\node\out\Release\obj\deps\v8\gypfiles\v8_monolith.gen\gn'
ninja: no work to do.
[8/12] CXX obj\src\node_lib.node_win32_perfctr_provider.obj
FAILED: obj/src/node_lib.node_win32_perfctr_provider.obj
ninja -t msvc -e environment.x64 -- "C:\Users\Nikolai\node\deps\v8\third_party\llvm-build\Release+Asserts\bin\clang-cl" -m64 /nologo /showIncludes /FC @obj\src\node_lib.node_win32_perfctr_provider.obj.rsp /c ..\..\src\node_win32_perfctr_provider.cc /Foobj\src\node_lib.node_win32_perfctr_provider.obj /Fdobj\node_lib.cc.pdb
clang-cl.exe: warning: argument unused during compilation: '/GL' [-Wunused-command-line-argument]
In file included from ..\..\src\node_win32_perfctr_provider.cc:28:
..\..\tools\msvs\genfiles\node_perfctr_provider.h(19,90):  warning: suggest braces around initialization of subobject [-Wmissing-braces]
EXTERN_C DECLSPEC_SELECTANY GUID NodeCounterProviderGuid = { 0x793c9b44, 0x3d6b, 0x4f57, 0xb5, 0xd7, 0x4f, 0xf8, 0xa, 0xdc, 0xf9, 0xa2 };
                                                                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                                                         {                                            }
..\..\tools\msvs\genfiles\node_perfctr_provider.h(21,85):  warning: suggest braces around initialization of subobject [-Wmissing-braces]
EXTERN_C DECLSPEC_SELECTANY GUID NodeCounterSetGuid = { 0x3a22a8ec, 0x297c, 0x48ac, 0xab, 0x15, 0x33, 0xec, 0x93, 0x3, 0x3f, 0xd8 };
                                                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                                                    {                                            }
..\..\tools\msvs\genfiles\node_perfctr_provider.h(39,37):  warning: suggest braces around initialization of subobject [-Wmissing-braces]
    { { 0x3a22a8ec, 0x297c, 0x48ac, 0xab, 0x15, 0x33, 0xec, 0x93, 0x3, 0x3f, 0xd8 }, { 0x793c9b44, 0x3d6b, 0x4f57, 0xb5, 0xd7, 0x4f, 0xf8, 0xa, 0xdc, 0xf9, 0xa2 }, 10, PERF_COUNTERSET_MULTI_AGGREGATE },
                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                    {                                            }
..\..\tools\msvs\genfiles\node_perfctr_provider.h(39,116):  warning: suggest braces around initialization of subobject [-Wmissing-braces]
    { { 0x3a22a8ec, 0x297c, 0x48ac, 0xab, 0x15, 0x33, 0xec, 0x93, 0x3, 0x3f, 0xd8 }, { 0x793c9b44, 0x3d6b, 0x4f57, 0xb5, 0xd7, 0x4f, 0xf8, 0xa, 0xdc, 0xf9, 0xa2 }, 10, PERF_COUNTERSET_MULTI_AGGREGATE },
                                                                                                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                                                                                   {                                            }
..\..\tools\msvs\genfiles\node_perfctr_provider.h(45,77):  warning: implicit conversion from 'long long' to 'LONG' (aka 'long') changes value from 4294967293 to -3 [-Wconstant-conversion]
    { 6, PERF_COUNTER_BULK_COUNT, 0, sizeof(ULONGLONG), PERF_DETAIL_NOVICE, 4294967293, 0 },
    ~                                                                       ^~~~~~~~~~
..\..\tools\msvs\genfiles\node_perfctr_provider.h(46,77):  warning: implicit conversion from 'long long' to 'LONG' (aka 'long') changes value from 4294967293 to -3 [-Wconstant-conversion]
    { 7, PERF_COUNTER_BULK_COUNT, 0, sizeof(ULONGLONG), PERF_DETAIL_NOVICE, 4294967293, 0 },
    ~                                                                       ^~~~~~~~~~
..\..\tools\msvs\genfiles\node_perfctr_provider.h(48,77):  warning: implicit conversion from 'long long' to 'LONG' (aka 'long') changes value from 4294967293 to -3 [-Wconstant-conversion]
    { 9, PERF_COUNTER_BULK_COUNT, 0, sizeof(ULONGLONG), PERF_DETAIL_NOVICE, 4294967293, 0 },
    ~                                                                       ^~~~~~~~~~
..\..\tools\msvs\genfiles\node_perfctr_provider.h(49,78):  warning: implicit conversion from 'long long' to 'LONG' (aka 'long') changes value from 4294967293 to -3 [-Wconstant-conversion]
    { 10, PERF_COUNTER_BULK_COUNT, 0, sizeof(ULONGLONG), PERF_DETAIL_NOVICE, 4294967293, 0 },
    ~                                                                        ^~~~~~~~~~
..\..\src\node_win32_perfctr_provider.cc(122,36):  error: declaration of 'NodeCounterProvider' has a different language linkage
EXTERN_C DECLSPEC_SELECTANY HANDLE NodeCounterProvider = nullptr;
                                   ^
..\..\src/node_win32_perfctr_provider.h(35,15):  note: previous declaration is here
extern HANDLE NodeCounterProvider;
              ^
8 warnings and 1 error generated.
[9/12] CXX obj\gen\node_lib.node_javascript.obj
clang-cl.exe: warning: argument unused during compilation: '/GL' [-Wunused-command-line-argument]
ninja: build stopped: subcommand failed.

@hashseed
Copy link
Member Author

Thanks for making it this far! That's the last remaining issue. The fix is here. I wasn't sure whether it's the right fix, which is why I haven't landed it yet.

@seishun
Copy link
Contributor

seishun commented May 1, 2018

It builds now. A few questions:

  1. How do I run tests?
  2. Any ideas about the warnings? They are quite noisy.
  3. Are there plans to shorten the list of dependencies? Requiring double the number of dependencies compared to MSVC seems like a step backwards to me.

@hashseed
Copy link
Member Author

hashseed commented May 1, 2018

Yup. It's not perfect yet. I'm working on making all of this smoother.

You should be able to run tests by using the test script in tools directly.

addaleax pushed a commit that referenced this issue May 5, 2018
NodeCounterProvider is declared as extern but defined as EXTERN_C.
This confuses clang-cl.

PR-URL: #20436
Refs: #19630
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue May 8, 2018
NodeCounterProvider is declared as extern but defined as EXTERN_C.
This confuses clang-cl.

PR-URL: #20436
Refs: #19630
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue May 8, 2018
NodeCounterProvider is declared as extern but defined as EXTERN_C.
This confuses clang-cl.

PR-URL: #20436
Refs: #19630
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this issue May 9, 2018
NodeCounterProvider is declared as extern but defined as EXTERN_C.
This confuses clang-cl.

PR-URL: #20436
Refs: #19630
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@seishun
Copy link
Contributor

seishun commented May 11, 2018

So what's the plan now? Are you going to create a PR here?

@hashseed
Copy link
Member Author

I want to figure out how far we can go with reducing the dependencies before upstreaming anything. Also, this is not particularly urgent.

@refack refack self-assigned this Oct 17, 2018
@refack
Copy link
Contributor

refack commented Oct 17, 2018

This is an interesting issue, I'll try to pick it...

@refack
Copy link
Contributor

refack commented Oct 23, 2018

Actually fetching Clang. This is done in V8 through python .\deps\v8\tools\clang\scripts\update.py.

IIUC non googlers can't do that. The docs say "install MSVS and set DEPOT_TOOLS_WIN_TOOLCHAIN=0"

But I'm making progress using the "official" prebuilt llvm binaries, and the ninja-msvs GYP generator.

@refack
Copy link
Contributor

refack commented Oct 26, 2018

found a bug in V8's src/base/debug/stack_trace_win.cc (mix and max MACROS breaking compilation with "official" prebuilt llvm binaries).
CL: https://chromium-review.googlesource.com/c/v8/v8/+/1297479

refack added a commit to refack/node that referenced this issue Nov 3, 2018
Original commit message:
  undef min,max macros on windows

  This blocks building with official clang-cl and Windows SDK

  Refs: nodejs#19630
  Change-Id: I41fdf934f486c660df7a9e0dd284f6eb3c294dd4
  Reviewed-on: https://chromium-review.googlesource.com/c/1297479
  Commit-Queue: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#57053}

PR-URL: nodejs#23985
Refs: v8/v8@dc70449
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this issue Nov 3, 2018
Original commit message:
  undef min,max macros on windows

  This blocks building with official clang-cl and Windows SDK

  Refs: #19630
  Change-Id: I41fdf934f486c660df7a9e0dd284f6eb3c294dd4
  Reviewed-on: https://chromium-review.googlesource.com/c/1297479
  Commit-Queue: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#57053}

PR-URL: #23985
Refs: v8/v8@dc70449
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
joyeecheung pushed a commit to joyeecheung/v8 that referenced this issue Nov 5, 2018
This blocks building with official clang-cl and Windows SDK

Refs: nodejs/node#19630
Change-Id: I41fdf934f486c660df7a9e0dd284f6eb3c294dd4
Reviewed-on: https://chromium-review.googlesource.com/c/1297479
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57053}
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

There's been no further activity on this and it's not clear if it's going to move forward. Closing but we can reopen if necessary

@jasnell jasnell closed this as completed Jun 25, 2020
@aminya
Copy link

aminya commented Oct 1, 2020

This is a very necessary feature. MSVC is not an optimized compiler. Can we open this again?

https://www.youtube.com/watch?v=8e7IdHG5fhQ

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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

10 participants