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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: update V8 to 12.5 #52651

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

deps: update V8 to 12.5 #52651

wants to merge 14 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Apr 23, 2024

I know, it never ends 馃槄

targos and others added 13 commits April 23, 2024 07:13
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 12.5.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md
dllexport introduces issues when compiling with MSVC.

PR-URL: nodejs#47251
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
It introduces process hangs on some platforms because Node.js doesn't
tear down V8 correctly.
Disable it while we work on a solution.

Refs: nodejs#47297
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=13902
PR-URL: nodejs#47450
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14221
PR-URL: nodejs#49639
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
We are not ready to compile with C++20 support yet.
This is only a DCHECK that can be removed without affecting the behavior
of release builds.

PR-URL: nodejs#49639
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#49639
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: nodejs#52293
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
After enabling -std:c++20 on Windows, patch is now much smaller.

PR-URL: nodejs#52465
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
On Windows debug builds, it is not allowed to dereference empty
iterators.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5331688
PR-URL: nodejs#52465
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Namely:
  - `v8::ObjectTemplate::SetAccessor(v8::Local<v8::String>, ...);`
  - `v8::ObjectTemplate::SetNativeDataProperty` with `AccessControl`

Refs: v8/v8@46c241e
Refs: v8/v8@6ec8839
Co-authored-by: Micha毛l Zasso <targos@protonmail.com>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Apr 23, 2024
@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 23, 2024
@targos
Copy link
Member Author

targos commented Apr 23, 2024

@nodejs/platform-windows there's one little error for now:

D:\a\node\node\deps\v8\src\flags\flag-definitions.h(3061,1): error C1070: mismatched #if/#endif pair in file 'D:\a\node\node\deps\v8\src\flags\flag-definitions.h' [D:\a\node\node\tools\v8_gypfiles\torque_base.vcxproj]

@StefanStojanovic
Copy link
Contributor

@targos, with these changes the compilation passes (should probably become part of 0b738c9)

From f52e086c0142311f1c4b8b2ccf160050d8f2bdd5 Mon Sep 17 00:00:00 2001
From: StefanStojanovic <stefan.stojanovic@janeasystems.com>
Date: Wed, 24 Apr 2024 16:30:39 +0200
Subject: [PATCH 1/1] V8 v12.5 Win fix

---
 deps/v8/src/api/api.cc               | 6 ++----
 deps/v8/src/flags/flag-definitions.h | 3 ++-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/deps/v8/src/api/api.cc b/deps/v8/src/api/api.cc
index e99e7d9d135..bd2254d82a7 100644
--- a/deps/v8/src/api/api.cc
+++ b/deps/v8/src/api/api.cc
@@ -6323,8 +6323,7 @@ void v8::Object::SetAlignedPointerInInternalFields(int argc, int indices[],
 // static
 void* v8::Object::Unwrap(v8::Isolate* isolate, i::Address wrapper_obj,
                          CppHeapPointerTag tag) {
-  return i::JSApiWrapper(i::JSObject::cast(i::Tagged<i::Object>(
-                             reinterpret_cast<i::Address>(wrapper_obj))))
+  return i::JSApiWrapper(i::JSObject::cast(i::Tagged<i::Object>((wrapper_obj))))
       .GetCppHeapWrappable(reinterpret_cast<i::Isolate*>(isolate),
                            static_cast<i::ExternalPointerTag>(tag));
 }
@@ -6332,8 +6331,7 @@ void* v8::Object::Unwrap(v8::Isolate* isolate, i::Address wrapper_obj,
 // static
 void v8::Object::Wrap(v8::Isolate* isolate, i::Address wrapper_obj,
                       CppHeapPointerTag tag, void* wrappable) {
-  return i::JSApiWrapper(i::JSObject::cast(i::Tagged<i::Object>(
-                             reinterpret_cast<i::Address>(wrapper_obj))))
+  return i::JSApiWrapper(i::JSObject::cast(i::Tagged<i::Object>((wrapper_obj))))
       .SetCppHeapWrappable(reinterpret_cast<i::Isolate*>(isolate), wrappable,
                            static_cast<i::ExternalPointerTag>(tag));
 }
diff --git a/deps/v8/src/flags/flag-definitions.h b/deps/v8/src/flags/flag-definitions.h
index 68d98ae63e2..cd469b7537e 100644
--- a/deps/v8/src/flags/flag-definitions.h
+++ b/deps/v8/src/flags/flag-definitions.h
@@ -848,10 +848,11 @@ DEFINE_INT(invocation_count_for_feedback_allocation, 8,
 // Tiering: Maglev.
 #if defined(ANDROID)
 DEFINE_INT(invocation_count_for_maglev, 1000,
+           "invocation count required for optimizing with Maglev")
 #else
 DEFINE_INT(invocation_count_for_maglev, 400,
-#endif  // ANDROID
            "invocation count required for optimizing with Maglev")
+#endif  // ANDROID
 DEFINE_INT(invocation_count_for_maglev_osr, 100,
            "invocation count required for maglev OSR")
 DEFINE_BOOL(osr_from_maglev, false,
-- 
2.44.0.windows.1

However, there is another issue I got when running basic debug build sanity check (fails with Release too) .\Debug\node.exe deps\npm\node_modules\node-gyp\bin\node-gyp rebuild --directory=test\addons\hello-world --nodedir="E:\work\node".

Here is the log
gyp info it worked if it ends with ok
gyp info using node-gyp@10.1.0
gyp info using node@22.0.0-pre | win32 | x64
gyp info chdir test\addons\hello-world      
gyp info find Python using Python version 3.10.11 found at "C:\Program Files\Python310\python.exe"
gyp info find VS using VS2022 (17.9.34728.123) found at:
gyp info find VS "C:\Program Files\Microsoft Visual Studio\2022\Community"
gyp info find VS run with --verbose for detailed information
gyp WARN read config.gypi ENOENT: no such file or directory, open 'E:\work\node\include\node\config.gypi'
gyp info spawn C:\Program Files\Python310\python.exe
gyp info spawn args [
gyp info spawn args 'E:\\work\\node_main\\deps\\npm\\node_modules\\node-gyp\\gyp\\gyp_main.py',
gyp info spawn args 'binding.gyp',
gyp info spawn args '-f',
gyp info spawn args 'msvs',
gyp info spawn args '-I',
gyp info spawn args 'E:\\work\\node_main\\test\\addons\\hello-world\\build\\config.gypi',      
gyp info spawn args '-I',
gyp info spawn args 'E:\\work\\node_main\\deps\\npm\\node_modules\\node-gyp\\addon.gypi',      
gyp info spawn args '-I',
gyp info spawn args 'E:\\work\\node\\common.gypi',
gyp info spawn args '-Dlibrary=shared_library',
gyp info spawn args '-Dvisibility=default',
gyp info spawn args '-Dnode_root_dir=E:\\work\\node',
gyp info spawn args '-Dnode_gyp_dir=E:\\work\\node_main\\deps\\npm\\node_modules\\node-gyp',   
gyp info spawn args '-Dnode_lib_file=E:\\\\work\\\\node\\\\$(Configuration)\\\\node.lib',      
gyp info spawn args '-Dmodule_root_dir=E:\\work\\node_main\\test\\addons\\hello-world',        
gyp info spawn args '-Dnode_engine=v8',
gyp info spawn args '--depth=.',
gyp info spawn args '--no-parallel',
gyp info spawn args '--generator-output',
gyp info spawn args 'E:\\work\\node_main\\test\\addons\\hello-world\\build',
gyp info spawn args '-Goutput_dir=.'
gyp info spawn args ]
Warning: unrecognized setting VCCLCompilerTool/LanguageStandard while converting to MSBuild.
Warning: unrecognized setting VCCLCompilerTool/LanguageStandard_C while converting to MSBuild.
Warning: unrecognized setting VCCLCompilerTool/LanguageStandard while converting to MSBuild.  
Warning: unrecognized setting VCCLCompilerTool/LanguageStandard_C while converting to MSBuild.
gyp info spawn C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Current\Bin\MSBuild.exe
gyp info spawn args [
gyp info spawn args 'build\\binding.sln',
gyp info spawn args '/clp:Verbosity=minimal',
gyp info spawn args '/nologo',
gyp info spawn args '/p:Configuration=Debug;Platform=x64'
gyp info spawn args ]

binding.cc
E:\work\node\deps\v8\include\v8-handle-base.h(10,28): error C2429: language feature 'nested-namespace-definition' requires compiler flag '/std:c++17' [E:\work\node_main\test\addons\hello-world\build\binding.vcxproj]
(compiling source file '../binding.cc')

E:\work\node\deps\v8\include\v8-template.h(973,47): error C2039: 'string_view': is not a member of 'std' [E:\work\node_main\test\addons\hello-world\build\binding.vcxproj]
(compiling source file '../binding.cc')
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\include\array(19,1):
see declaration of 'std'

E:\work\node\deps\v8\include\v8-template.h(973,58): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [E:\work\node_main\test\addons\hello-world\build\binding.vcxproj]
(compiling source file '../binding.cc')

E:\work\node\deps\v8\include\v8-template.h(973,47): error C2146: syntax error: missing '>' before identifier 'string_view' [E:\work\node_main\test\addons\hello-world\build\binding.vcxproj]
(compiling source file '../binding.cc')

C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\include\string_view(12): warning STL4038: The contents of <string_view> are available only with C++17 or later. [E:\work\node_main\test\addons\hello-world\build\binding.vcxproj]
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\include\optional(11): warning STL4038: The contents of <optional> are available only with C++17 or later. [E:\work\node_main\test\addons\hello-world\build\binding.vcxproj]
E:\work\node\src\node.h(687,8): error C2039: 'optional': is not a member of 'std' [E:\work\node_main\test\addons\hello-world\build\binding.vcxproj]
(compiling source file '../binding.cc')
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\include\ostream(25,1):
see declaration of 'std'

E:\work\node\src\node.h(687,16): error C2143: syntax error: missing ';' before '<' [E:\work\node_main\test\addons\hello-world\build\binding.vcxproj]
(compiling source file '../binding.cc')

E:\work\node\src\node.h(687,16): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [E:\work\node_main\test\addons\hello-world\build\binding.vcxproj]
(compiling source file '../binding.cc')

E:\work\node\src\node.h(687,49): error C2238: unexpected token(s) preceding ';' [E:\work\node_main\test\addons\hello-world\build\binding.vcxproj]
(compiling source file '../binding.cc')

E:\work\node\src\node.h(759,10): error C2039: 'string_view': is not a member of 'std' [E:\work\node_main\test\addons\hello-world\build\binding.vcxproj]
(compiling source file '../binding.cc')
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\include\ostream(25,1):
see declaration of 'std'

E:\work\node\src\node.h(759,10): error C2061: syntax error: identifier 'string_view' [E:\work\node_main\test\addons\hello-world\build\binding.vcxproj]
(compiling source file '../binding.cc')

gyp ERR! build error 
gyp ERR! stack Error: `C:\Program Files\Microsoft Visual Studio\2022\Community\MSBuild\Current\Bin\MSBuild.exe` failed with exit code: 1
gyp ERR! stack at ChildProcess.<anonymous> (E:\work\node_main\deps\npm\node_modules\node-gyp\lib\build.js:209:23)
gyp ERR! stack at ChildProcess.emit (node:events:520:28)
gyp ERR! stack at ChildProcess._handle.onexit (node:internal/child_process:294:12)
gyp ERR! System Windows_NT 10.0.19045
gyp ERR! command "E:\\work\\node_main\\Debug\\node.exe" "E:\\work\\node_main\\deps\\npm\\node_modules\\node-gyp\\bin\\node-gyp" "rebuild" "--directory=test\\addons\\hello-world" "--nodedir=E:\\work\\node"
gyp ERR! cwd E:\work\node_main\test\addons\hello-world
gyp ERR! node -v v22.0.0-pre
gyp ERR! node-gyp -v v10.1.0
gyp ERR! not ok

Based on the errors, looks like not using std:c++17 is an issue, but I'm not sure what changes from V8 v12.5 caused this.

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 25, 2024
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 25, 2024

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@targos targos removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 25, 2024
@targos
Copy link
Member Author

targos commented Apr 25, 2024

@StefanStojanovic Not sure what's wrong with your local tests but Windows CI is green!

@StefanStojanovic
Copy link
Contributor

@StefanStojanovic Not sure what's wrong with your local tests but Windows CI is green!

This makes me both happy and sad... 馃槀 Hopefully it was something on my machine, I'll see if I can reproduce it again.

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. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants