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

deps: update V8 to 13.3.415.20 #56959

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

deps: update V8 to 13.3.415.20 #56959

wants to merge 23 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Feb 8, 2025

Notable changes:

@nodejs/v8-update @nodejs/tsc

Supersedes #56842

targos and others added 18 commits February 7, 2025 19:00
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 13.3.

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 <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#54077
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs#53134
Refs: nodejs#52809
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#55014
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
It's causing compiler errors with some classes on Xcode 11
and the attribute should have no runtime effect.

PR-URL: nodejs#54077
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
PR-URL: nodejs#55014
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
It's causing linker errors with node.lib in node-gyp and potentially
breaks other 3rd party tools

PR-URL: nodejs#56238
Refs: nodejs#55784
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#55014
Reviewed-By: Matteo Collina <[email protected]>
The API was removed from V8.
@targos targos added 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. labels Feb 8, 2025
@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. labels Feb 8, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

New V8 version includes more information about regular expressions.
@nodejs-github-bot

This comment was marked as resolved.

@nodejs-github-bot

This comment was marked as resolved.

@nodejs-github-bot

This comment was marked as resolved.

@targos
Copy link
Member Author

targos commented Feb 8, 2025

@nodejs/platform-windows There are new compiler errors (including with clang-cl this time):
https://ci.nodejs.org/job/node-compile-windows/60341/
https://ci.nodejs.org/job/node-compile-windows-debug/26107/nodes=win-vs2022/

This comment was marked as resolved.

@targos
Copy link
Member Author

targos commented Feb 8, 2025

There are some crashes in debug mode: https://ci.nodejs.org/job/node-test-commit-arm-debug/17147/

/cc @joyeecheung, seems related to cppgc

@targos targos added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Feb 9, 2025
@joyeecheung
Copy link
Member

The crash looks similar to #56327 - we probably have been forgetting to put the locker somewhere and it's somehow exposed by recent V8 changes. Looking..

@targos
Copy link
Member Author

targos commented Feb 12, 2025

Ping @nodejs/platform-windows

@joyeecheung
Copy link
Member

I cannot reproduce test-child-process-exec-maxbuf locally though #57031 fixes test-heapsnapshot-near-heap-limit-bounded locally for me. From the stack trace it seems they are caused by something similar so it may fix the other one as well.

Otherwise it may fail the DCHECK that uses the locked thread
as a fast path to get the current thread.
@joyeecheung
Copy link
Member

Pushed the commit from #57031

@joyeecheung
Copy link
Member

joyeecheung commented Feb 13, 2025

I don't have a Windows machine at hand at the hour, but I think this should fix it for the windows-clang build from a look at the error

diff --git a/deps/v8/src/wasm/wasm-module-sourcemap.cc b/deps/v8/src/wasm/wasm-module-sourcemap.cc
index f95107faba..c65389839e 100644
--- a/deps/v8/src/wasm/wasm-module-sourcemap.cc
+++ b/deps/v8/src/wasm/wasm-module-sourcemap.cc
@@ -69,7 +69,7 @@ WasmModuleSourceMap::WasmModuleSourceMap(v8::Isolate* v8_isolate,
     size_t file_name_sz = file_name->Utf8LengthV2(v8_isolate) + 1;
     std::unique_ptr<char[]> file_name_buf(new char[file_name_sz]);
     file_name->WriteUtf8V2(v8_isolate, file_name_buf.get(), file_name_sz,
-                           String::WriteFlags::kNullTerminate);
+                           v8::String::WriteFlags::kNullTerminate);
     filenames.emplace_back(file_name_buf.get());
   }

@@ -84,7 +84,7 @@ WasmModuleSourceMap::WasmModuleSourceMap(v8::Isolate* v8_isolate,
   size_t mappings_sz = mappings->Utf8LengthV2(v8_isolate) + 1;
   std::unique_ptr<char[]> mappings_buf(new char[mappings_sz]);
   mappings->WriteUtf8V2(v8_isolate, mappings_buf.get(), mappings_sz,
-                        String::WriteFlags::kNullTerminate);
+                        v8::String::WriteFlags::kNullTerminate);

   valid_ = DecodeMapping(mappings_buf.get());
 }

The MSVC one looks different though.

@targos
Copy link
Member Author

targos commented Feb 14, 2025

I don't have a Windows machine at hand at the hour, but I think this should fix it for the windows-clang build from a look at the error

The MSVC one looks different though.

It looks like we could upstream it (other uses of String in that file have the namespace)

@joyeecheung
Copy link
Member

joyeecheung commented Feb 14, 2025

Weird enough I cannot reproduce the compilation error with clangcl 19.1.1 on Windows. I suspect the clang version might make a difference? What is the version being run in the CI?

@richardlau
Copy link
Member

Weird enough I cannot reproduce the compilation error with clangcl 19.1.1 on Windows. I suspect the clang version might make a difference? What is the version being run in the CI?

18.1.8, according to https://ci.nodejs.org/job/node-compile-windows/60341/nodes=win-vs2022_clang/consoleFull

14:31:14 > set "clang_path=C:\Program Files\Microsoft Visual Studio\2022\Community\VC\\Tools\Llvm\x64\bin\clang.exe" 
14:31:14 
14:31:14 > for /F "tokens=3" %i in ('"C:\Program Files\Microsoft Visual Studio\2022\Community\VC\\Tools\Llvm\x64\bin\clang.exe" --version') do (
14:31:14 set clang_version=%i  
14:31:14  goto clang-found 
14:31:14 ) 
14:31:14 
14:31:14 > (
14:31:14 set clang_version=18.1.8  
14:31:14  goto clang-found 
14:31:14 ) 
14:31:14 
14:31:14 > echo Found Clang version 18.1.8 
14:31:14 Found Clang version 18.1.8
14:31:14 
14:31:14 > set configure_flags= --with-ltcg "--download=all" --with-intl=full-icu --dest-cpu=x64 --verbose --clang-cl=18.1.8

@joyeecheung
Copy link
Member

joyeecheung commented Feb 14, 2025

Pushed the namespace fix to see if it fixes the clang-cl error: https://ci.nodejs.org/job/node-compile-windows/60465/nodes=win-vs2022_clang/

@joyeecheung
Copy link
Member

Upstreamed the patch for clangcl to https://chromium-review.googlesource.com/c/v8/v8/+/6271543

@targos
Copy link
Member Author

targos commented Feb 15, 2025

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. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. 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.

5 participants