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: fix MSVC 2022 Release compilation #46228

Closed
wants to merge 3 commits into from

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Jan 16, 2023

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.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/v8-update

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

@targos targos left a comment

Choose a reason for hiding this comment

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

It looks like a reasonable fix. Thank you!

@targos
Copy link
Member

targos commented Jan 17, 2023

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

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 17, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@eukarpov
Copy link

#46231 fixes the problem differently

@targos
Copy link
Member

targos commented Jan 18, 2023

@eukarpov do you think the fix here is wrong?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@eukarpov
Copy link

@eukarpov do you think the fix here is wrong?

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

@StefanStojanovic
Copy link
Contributor

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?

@eukarpov
Copy link

@StefanStojanovic PR #46231 should fix this issue

@StefanStojanovic
Copy link
Contributor

@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.

@vmoroz
Copy link
Member Author

vmoroz commented Jan 21, 2023

@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).

@vmoroz 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
@vmoroz vmoroz force-pushed the PR/Fix_MSVC_2022_Release_build branch from 8ab4121 to ffe17f8 Compare January 27, 2023 15:04
@mhdawson 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 pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. labels Jan 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 27, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM based on approval from Stefan from Microsoft team

@mhdawson
Copy link
Member

Landed in 19bcba0

@mhdawson mhdawson closed this Jan 27, 2023
mhdawson pushed a commit that referenced this pull request Jan 27, 2023
PR-URL: #46228
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@vmoroz vmoroz deleted the PR/Fix_MSVC_2022_Release_build branch January 27, 2023 21:01
@vmoroz
Copy link
Member Author

vmoroz commented Jan 27, 2023

Landed in 19bcba0

Thank you! :)

juanarbol pushed a commit that referenced this pull request Mar 3, 2023
PR-URL: #46228
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@juanarbol juanarbol mentioned this pull request Mar 3, 2023
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
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
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
Fixes Release build on VS2022 ARM64
sparist added a commit to Distributive-Network/node that referenced this pull request Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants