Repository navigation
Fix V8 5.4 for Node #1
Description
Activity
cc @nodejs/v8
edit: Hmmm, looks like you cannot cc a team from the parent repository?
cc @fhinkel and @bnoordhuis in case you're interested
It's possible you need to add
'want_separate_host_toolset_mkpeephole%': 0in common.gypi like we do forwant_separate_host_toolset.cc @misterdjules for the smartos build error.
- FreeBSD issue with test-tick-processor
From looking at the stack trace, I suspect that the fix is this:
diff --git a/src/base/platform/semaphore.cc b/src/base/platform/semaphore.cc index f6a518c..3713153 100644 --- a/src/base/platform/semaphore.cc +++ b/src/base/platform/semaphore.cc @@ -72,18 +72,18 @@ bool Semaphore::WaitFor(const TimeDelta& rel_time) { #elif V8_OS_POSIX Semaphore::Semaphore(int count) { // The sem_init() does not check for alignment of the native handle. // Unaligned native handle can later cause a failure in semaphore signal. // Check the alignment here to catch the failure earlier. // Context: crbug.com/605349. -#if V8_OS_AIX - // On aix sem_t is of type int +#if V8_OS_AIX || V8_OS_FREEBSD + // On AIX, sem_t is of type int. On FreeBSD, it's a struct of 32 bits fields. const uintptr_t kSemaphoreAlignmentMask = sizeof(int) - 1; #else const uintptr_t kSemaphoreAlignmentMask = sizeof(void*) - 1; #endif CHECK_EQ( 0, reinterpret_cast<uintptr_t>(&native_handle_) & kSemaphoreAlignmentMask); DCHECK(count >= 0);
On freebsd, sem_t is a struct with a couple of uint32_t fields. It only needs dword alignment on 64 bits architectures, not the qword (pointer) alignment that the CHECK currently enforces.
Will look at this asap.
Based on @bnoordhuis' comment, it doesn't look like the tick processor failure is an issue in the code that the tick processor is testing but I'm happy to help out with any questions/work related to the test.
The following patch fixes the build issue on SmartOS:
diff --git a/deps/v8/src/base/debug/stack_trace_posix.cc b/deps/v8/src/base/debug/stack_trace_posix.cc index cbd722d..45d1e8f 100644 --- a/deps/v8/src/base/debug/stack_trace_posix.cc +++ b/deps/v8/src/base/debug/stack_trace_posix.cc @@ -32,7 +32,9 @@ #if V8_OS_MACOSX #include <AvailabilityMacros.h> #endif - +#if V8_OS_SOLARIS +#include <execinfo.h> +#endif #include "src/base/build_config.h" #include "src/base/free_deleter.h" #include "src/base/logging.h"
@targos sorry for the late reply, I was on vacation. From the new CI, it seems like that the
common.gypisuggestion from @bnoordhuis didn't help. I will take a look at this today.@misterdjules thanks !
cc @mhart for the similar issue with https://ci.nodejs.org/job/node-test-commit-linux/4734/nodes=ubuntu1604_docker_alpine34-64/console
Do you know if there is a V8 macro applicable to this platform ?@targos: I have submitted this CL for the
mkpeepholeissue on windows: https://codereview.chromium.org/2276733002.@ofrobots your fix seems to break the build on my local machine (Linux).
Running ./configure:
$ ./configure creating ./icu_config.gypi * Using ICU in deps/icu-small creating ./icu_config.gypi { 'target_defaults': { 'cflags': [], 'default_configuration': 'Release', 'defines': [], 'include_dirs': [], 'libraries': []}, 'variables': { 'asan': 0, 'debug_devtools': 'node', 'force_dynamic_crt': 0, 'gas_version': '2.26', 'host_arch': 'x64', 'icu_data_file': 'icudt57l.dat', 'icu_data_in': '../../deps/icu-small/source/data/in/icudt57l.dat', 'icu_endianness': 'l', 'icu_gyp_path': 'tools/icu/icu-generic.gyp', 'icu_locales': 'en,root', 'icu_path': 'deps/icu-small', 'icu_small': 'true', 'icu_ver_major': '57', 'node_byteorder': 'little', 'node_enable_d8': 'false', 'node_enable_v8_vtunejit': 'false', 'node_install_npm': 'true', 'node_module_version': 48, 'node_no_browser_globals': 'false', 'node_prefix': '/usr/local', 'node_release_urlbase': '', 'node_shared': 'false', 'node_shared_cares': 'false', 'node_shared_http_parser': 'false', 'node_shared_libuv': 'false', 'node_shared_openssl': 'false', 'node_shared_zlib': 'false', 'node_tag': '', 'node_use_bundled_v8': 'true', 'node_use_dtrace': 'false', 'node_use_etw': 'false', 'node_use_lttng': 'false', 'node_use_openssl': 'true', 'node_use_perfctr': 'false', 'node_use_v8_platform': 'true', 'openssl_fips': '', 'openssl_no_asm': 0, 'shlib_suffix': 'so.48', 'target_arch': 'x64', 'uv_parent_path': '/deps/uv/', 'uv_use_dtrace': 'false', 'v8_enable_gdbjit': 0, 'v8_enable_i18n_support': 1, 'v8_inspector': 'true', 'v8_no_strict_aliasing': 1, 'v8_optimized_debug': 0, 'v8_random_seed': 0, 'v8_use_snapshot': 'true', 'want_separate_host_toolset': 0}} creating ./config.gypi creating ./config.mk gyp: Dependency '/home/mzasso/git/forks/node/deps/v8/src/v8.gyp:v8_libsampler#host' not found while trying to load target /home/mzasso/git/forks/node/deps/v8/src/v8.gyp:v8_base#host Error running GYP@targos: You still need
want_separate_host_toolset_mkpeephole%': 0in common.gypi as @bnoordhuis suggested above.@targo: It seems that the variable definition in
common.gypiis having no effect. Here's how I "fixed" it: ofrobots@b4ad793 on my branch. Today was my day off so I didn't have too much time to look into whycommon.gypiis not having an effect.OK thanks!
143 remaining items
- added 5 commits that reference this issue
on Apr 30, 2021 - added 2 commits that reference this issue
on May 17, 2021 - added 2 commits that reference this issue
on Nov 12, 2023 - added 2 commits that reference this issue
on Dec 13, 2023 - added a commit that references this issue
on Jun 3, 2026
I don't want to pollute the node issue tracker that soon, so I'm opening a ticket here to track the issues that need to be solved on V8's side.
Here is a fresh CI run of targos/v8-5.4 as reference: https://ci.nodejs.org/job/node-test-commit/4668/
1. Compilation issue on BSD
Already fixed on my branch
See https://codereview.chromium.org/2251603004/
2. Compilation issue on SmartOS
See https://ci.nodejs.org/job/node-test-commit-smartos/3808/nodes=smartos14-32/console
I think it's due to the recent https://codereview.chromium.org/2248393002
3. Windows issue with mkpeephole
See https://ci.nodejs.org/job/node-compile-windows/3798/label=win-vcbt2015/console
@ofrobots: do you have any news about this?
4. FreeBSD issue with test-tick-processor
The process is probably crashing before the skip, at https://github.com/nodejs/node/blob/8ff3d61d8ba90fde01827643db3d87ee97f502e6/test/parallel/test-tick-processor.js#L21-L29I don't understand what this test is doing so help would be much appreciated here.
Fixed with https://codereview.chromium.org/2268993002
Edit: new CI: https://ci.nodejs.org/job/node-test-commit/4686/