build: fix MSVC 2022 Release compilation by vmoroz · Pull Request #46228 · nodejs/node · GitHub

This PR targets to address the Linker errors mentioned in Issue #43092 when we build Node.JS main branch in MSVC 2022 (Version 17.4.4).
Then, it fixes the VS2022 ARM64 build issue with file access violation originally fixed by PR #46231.

The fix adds PCH file to mksnapshot project in the v8.gyp file to solve #43092.
directory.build.props is added to override PreprocessedFileName output path to address the file access violation issue.

Details

When we build Node.JS Release in MSVC 2022 we see linker errors that mksnapshot cannot find FixedArray::get method. This is an example of the linker error from issue #43092:

platform-embedded-file-writer-aix.obj : error LNK2001: unresolved external symbol "public: class v8::internal::Object __cdecl v8::internal::FixedArray::get(int)const " (?get@FixedArray@internal@v8@@QEBA?AVObject@23@H@Z) [F:\node\tools\v8_gypfiles\mksna
pshot.vcxproj]

It seemed that the inlined method FixedArray::get is optimized out.
We can either change V8 files and explicitly include fixed-array-inl.h file as in PR #46231, or we can use the forced file inclusion used by PCH files. Adding the MSVC specific PCH for mksnapshot project addresses the issue.

The root cause of the access violation issue is that the embedded.S file is generated in the $(IntDir) folder while the preprocessed file name uses exactly the same path: <PreprocessedFileName Condition="'%(PreprocessedFileName)' == ''">$(IntDir)%(FileName)%(Extension)</PreprocessedFileName> from "c:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Microsoft\VC\v170\BuildCustomizations\marmasm.props". Thus, we are getting error because we try to change the embedded.S file while reading it.

In this PR we add new directory.build.props file that locally overrides the marmasm.props definition by adding .pp extension to the file name. It generates output file name that is different in from the input file name and addresses the file access violation issue.

Review requested:

@nodejs/gyp
@nodejs/v8-update

It looks like a reasonable fix. Thank you!

I cherry-picked my commit from #46125 to test it.

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

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1804-intel-64,v8test=v8test/5125/

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5125/

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5125/

#46231 fixes the problem differently

@eukarpov do you think the fix here is wrong?

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

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

@eukarpov do you think the fix here is wrong?

It is different than I proposed in my PR, however it looks good for me.

I've tested this PR and it fixes x64 and x86 release builds nicely. However, when cross-compiling for ARM64 (eg. running vcbuild.bat release arm64 vs2022) I notice a new error:

  Microsoft (R) C/C++ Optimizing Compiler Version 19.34.31935 for ARM64
  Copyright (C) Microsoft Corporation.  All rights reserved.
  
  cl /c /P /Fi"..\..\out\Release\obj\v8_snapshot\embedded.S" /TC ..\..\out\Release\obj\v8_snapshot\\embedded.S

  embedded.S
..\..\out\Release\obj\v8_snapshot\\embedded.S : fatal error C1083: Cannot open compiler generated file: '..\..\out\Release\obj\v8_snapshot\embedded.S': Permission denied [E:\work\node\tools\v8_gypfiles\v8_snapshot.vcxproj]

When cross-compiling on the main branch, I get the same linking errors as for the x64/x86 release builds:

mksnapshot.obj : error LNK2001: unresolved external symbol "public: class v8::internal::Object __cdecl v8::internal::FixedArray::get(int)const " (?get@FixedArray@internal@v8@@QEBA?AVObject@23@H@Z) [E:\work\node\tools\v8_gypfiles\mksnapshot_host.vcxproj]
platform-embedded-file-writer-aix.obj : error LNK2001: unresolved external symbol "public: class v8::internal::Object __cdecl v8::internal::FixedArray::get(int)const " (?get@FixedArray@internal@v8@@QEBA?AVObject@23@H@Z) [E:\work\node\tools\v8_gypfiles\mksnapshot_host.v
cxproj]
platform-embedded-file-writer-generic.obj : error LNK2001: unresolved external symbol "public: class v8::internal::Object __cdecl v8::internal::FixedArray::get(int)const " (?get@FixedArray@internal@v8@@QEBA?AVObject@23@H@Z) [E:\work\node\tools\v8_gypfiles\mksnapshot_ho 
st.vcxproj]
platform-embedded-file-writer-mac.obj : error LNK2001: unresolved external symbol "public: class v8::internal::Object __cdecl v8::internal::FixedArray::get(int)const " (?get@FixedArray@internal@v8@@QEBA?AVObject@23@H@Z) [E:\work\node\tools\v8_gypfiles\mksnapshot_host.v 
cxproj]
platform-embedded-file-writer-win.obj : error LNK2001: unresolved external symbol "public: class v8::internal::Object __cdecl v8::internal::FixedArray::get(int)const " (?get@FixedArray@internal@v8@@QEBA?AVObject@23@H@Z) [E:\work\node\tools\v8_gypfiles\mksnapshot_host.v 
cxproj]
..\..\out\Release\mksnapshot_host.exe : fatal error LNK1120: 1 unresolved externals [E:\work\node\tools\v8_gypfiles\mksnapshot_host.vcxproj]

Could you please investigate this issue further and see what's the root cause?

@StefanStojanovic PR #46231 should fix this issue

@StefanStojanovic PR #46231 should fix this issue

Yes, it looks like it does. I tried building releases for all x86, x64, and ARM64 and all builds were successful.

@StefanStojanovic Could you please investigate this issue further and see what's the root cause?

I played a little bit with it, and you are right: my solution worked fine for x86 and x64, but it failed for ARM64.

Adding the metadata PreprocessedFileName for the MARMASM MSBuild items from #46231 implemented by @eukarpov fixes the problem. Though, I see that the solution is a bit incomplete: it generates save_registers_masm.S.pp file in the source folder instead of $(IntDir).

The root cause of the access violation issue is that the embedded.S file is generated in the $(IntDir) folder while the preprocessed file name uses exactly the same path: <PreprocessedFileName Condition="'%(PreprocessedFileName)' == ''">$(IntDir)%(FileName)%(Extension)</PreprocessedFileName> from "c:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Microsoft\VC\v170\BuildCustomizations\marmasm.props".

My last commit implements the same fix as in PR #46231 by adding new directory.build.props file that locally overrides the marmasm.props definition by adding .pp extension. It also ensures that we always output .pp files to the $(IntDir).

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

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5140/

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5140/

LGTM based on approval from Stefan from Microsoft team

Landed in 19bcba0

PR-URL: #46228 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Landed in 19bcba0

Thank you! :)

PR-URL: #46228 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1804-intel-64,v8test=v8test/5140/

PR-URL: #46228 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Fixes Release build on VS2022 ARM64

Apply: nodejs#46228

nodejs-github-bot added needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Jan 16, 2023

targos approved these changes Jan 17, 2023

View reviewed changes

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2023

eukarpov mentioned this pull request Jan 17, 2023

build: fix arm64 compilation #46231

Closed

github-actions Bot mentioned this pull request Jan 18, 2023

CI Reliability 2023-01-18 nodejs/reliability#487

Open

18 tasks

This was referenced Jan 19, 2023

CI Reliability 2023-01-19 nodejs/reliability#488

Open

CI Reliability 2023-01-20 nodejs/reliability#489

Open

github-actions Bot mentioned this pull request Jan 21, 2023

CI Reliability 2023-01-21 nodejs/reliability#490

Open

14 tasks

vmoroz force-pushed the PR/Fix_MSVC_2022_Release_build branch 2 times, most recently from 109f0f7 to 8ab4121 Compare January 21, 2023 21:09

github-actions Bot mentioned this pull request Jan 22, 2023

CI Reliability 2023-01-22 nodejs/reliability#491

Open

13 tasks

StefanStojanovic mentioned this pull request Jan 23, 2023

Impossible to build with vs2022 #46300

Closed

vmoroz and others added 3 commits January 27, 2023 07:04

build: fix MSVC 2022 Release compilation

baa38dd

tools: enable windows-2022 in actions

2de21bd

resolve access violation error for MARMASM

ffe17f8

vmoroz force-pushed the PR/Fix_MSVC_2022_Release_build branch from 8ab4121 to ffe17f8 Compare January 27, 2023 15:04

mhdawson added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. build Issues and PRs related to build files or the CI. labels Jan 27, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 27, 2023

mhdawson approved these changes Jan 27, 2023

View reviewed changes

mhdawson closed this Jan 27, 2023

mhdawson pushed a commit that referenced this pull request Jan 27, 2023

build: fix MSVC 2022 Release compilation

19bcba0

PR-URL: #46228 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

vmoroz deleted the PR/Fix_MSVC_2022_Release_build branch January 27, 2023 21:01

ruyadorno pushed a commit that referenced this pull request Feb 1, 2023

build: fix MSVC 2022 Release compilation

d795d93

PR-URL: #46228 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

ruyadorno mentioned this pull request Feb 1, 2023

v19.6.0 proposal #46455

Merged

juanarbol mentioned this pull request Mar 3, 2023

V18.15.0 proposal #46920

Merged

juanarbol pushed a commit that referenced this pull request Mar 3, 2023

build: fix MSVC 2022 Release compilation

b4ea389

PR-URL: #46228 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

juanarbol pushed a commit that referenced this pull request Mar 5, 2023

build: fix MSVC 2022 Release compilation

7bae433

PR-URL: #46228 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

sparist added a commit to Distributive-Network/node that referenced this pull request Dec 9, 2023

Apply: nodejs#46228

12ccaa4

Fixes Release build on VS2022 ARM64

sparist added a commit to Distributive-Network/node that referenced this pull request Dec 9, 2023

Merge pull request #3 from Distributive-Network/dcp/bugfix-vs2022

cfaa0e5

Apply: nodejs#46228

build: fix MSVC 2022 Release compilation by vmoroz · Pull Request #46228 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: fix MSVC 2022 Release compilation - #46228

build: fix MSVC 2022 Release compilation#46228vmoroz wants to merge 3 commits intonodejs:mainnodejs/node:mainfrom vmoroz:PR/Fix_MSVC_2022_Release_buildvmoroz/nodejs-node:PR/Fix_MSVC_2022_Release_buildCopy head branch name to clipboard

Conversation

vmoroz commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Uh oh!

nodejs-github-bot commented Jan 16, 2023

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Jan 17, 2023

Uh oh!

nodejs-github-bot commented Jan 17, 2023

Uh oh!

nodejs-github-bot commented Jan 17, 2023

Uh oh!

nodejs-github-bot commented Jan 17, 2023

Uh oh!

nodejs-github-bot commented Jan 17, 2023

Uh oh!

eukarpov commented Jan 17, 2023

Uh oh!

targos commented Jan 18, 2023

Uh oh!

nodejs-github-bot commented Jan 18, 2023

Uh oh!

nodejs-github-bot commented Jan 18, 2023

Uh oh!

eukarpov commented Jan 18, 2023

Uh oh!

StefanStojanovic commented Jan 20, 2023

Uh oh!

eukarpov commented Jan 20, 2023

Uh oh!

StefanStojanovic commented Jan 20, 2023

Uh oh!

vmoroz commented Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 27, 2023

Uh oh!

nodejs-github-bot commented Jan 27, 2023

Uh oh!

nodejs-github-bot commented Jan 27, 2023

Uh oh!

mhdawson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Jan 27, 2023

Uh oh!

vmoroz commented Jan 27, 2023

Uh oh!

nodejs-github-bot commented Feb 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

build: fix MSVC 2022 Release compilation#46228
vmoroz wants to merge 3 commits into
nodejs:mainfrom
vmoroz:PR/Fix_MSVC_2022_Release_build

vmoroz commented Jan 16, 2023 •
edited
Loading

vmoroz commented Jan 21, 2023 •
edited
Loading

mhdawson left a comment •
edited
Loading