Skip to content

Fix building on Windows x86#12184

Closed
joaocgreis wants to merge 2 commits intonodejs:masterfrom
JaneaSystems:joaocgreis-H42-msvs_shard
Closed

Fix building on Windows x86#12184
joaocgreis wants to merge 2 commits intonodejs:masterfrom
JaneaSystems:joaocgreis-H42-msvs_shard

Conversation

@joaocgreis
Copy link
Copy Markdown
Member

Building on Windows x86 has been broken on master since the update of V8 to 5.7 (#11752, nodejs/v8#4, nodejs/build#669). This was not detected at the time because the CI matrix does not include a 32 bit build (which I plan to add after this lands).

When building V8 with Gyp, the library v8_base is divided into several shards because its size exceeds the limit. For each shard, a single cl.exe invocation is used to compile all .cc files into .obj files. In this invocation, some functions from runtime.cc are not getting compiled into runtime.obj. Using cl.exe with all the same arguments but to compile only runtime.cc compiles all functions as expected. I was not yet able to determine what set of files interferes, but none of the other files compiled alone with runtime.cc causes the problem. This seems to happens only when a big set of files (>110) is compiled simultaneously.

This might have passed unnoticed in V8 upstream because ninja compiles every source file one at a time.

This PR increases the number of shards to divide v8_base into. This increases the number of calls to cl.exe but decreases the number of files compiled each time.

Fixes: nodejs/v8#4

cc @nodejs/v8 @jasnell

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

Deps, V8, build, Windows.

Loading
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. v8 engine Issues and PRs related to the V8 dependency. windows Issues and PRs related to the Windows platform.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants