Fix V8 5.4 for Node

[targos]

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-L29
I 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/

[matthewloring]

cc @nodejs/v8

edit: Hmmm, looks like you cannot cc a team from the parent repository?

[targos]

cc @fhinkel and @bnoordhuis in case you're interested

[bnoordhuis]

It's possible you need to add 'want_separate_host_toolset_mkpeephole%': 0 in common.gypi like we do for want_separate_host_toolset.

cc @misterdjules for the smartos build error.

[bnoordhuis]

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

[misterdjules]

Will look at this asap.

[matthewloring]

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.

[misterdjules]

@targos

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"

[ofrobots]

@targos sorry for the late reply, I was on vacation. From the new CI, it seems like that the common.gypi suggestion from @bnoordhuis didn't help. I will take a look at this today.

[targos]

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

[ofrobots]

@targos: I have submitted this CL for the mkpeephole issue on windows: https://codereview.chromium.org/2276733002.

[targos]

@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

[ofrobots]

@targos: You still need want_separate_host_toolset_mkpeephole%': 0 in common.gypi as @bnoordhuis suggested above.

[targos]

@ofrobots I'm still seeing this error with 'want_separate_host_toolset_mkpeephole%': 0;

[ofrobots]

@targo: It seems that the variable definition in common.gypi is 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 why common.gypi is not having an effect.

[targos]

OK thanks!

New CI: https://ci.nodejs.org/job/node-test-commit/4759/

[targos]

cc @ncopa for the Alpine issue

[targos]

cc @Starefossen / @jbergstroem

[jbergstroem]

Regarding alpine34; execinfo isn't provided by musl. Not sure what the correct path is here, but I recall seeing libunwind being available in the alpine ecosystem. Do we want a macro for that or should we find a simpler way?

[jbergstroem]

I guess the other option could be not making the assumption that linux equals glibc when checking defines, but if this is the only case it might be overkill.

[ncopa]

execinfo isn't provided by musl.

Correct, excinfo.h, backtrace and those are GNU extensions.

I guess the other option could be not making the assumption that linux equals glibc when checking defines

I think this would be the technically correct thing to do.

[targos]

@ofrobots it seems that the 5.4 branch does not include your fix from https://codereview.chromium.org/2276733002

[targos]

I guess the other option could be not making the assumption that linux equals glibc when checking defines

I think this would be the technically correct thing to do.

Should we escalate this issue to the V8 team or does anyone have a concrete way to fix it that we can submit to them ?

[jbergstroem]

@targos would v8 treat usage of backtrace_symbols/execinfo.h as required?

[targos]

@jbergstroem I don't know that, but I think not. I've been investigating a bit and here is what I found:

• The code in stack_trace_posix.cc comes initially from Chromium. There are a lot of checks there about the presence of execinfo.h.
• Alpine Linux has a floating patch on top of Chromium to make it work with musl.

I'm trying a fix here: nodejs@5cca3d2

[jbergstroem]

@targos good news. A patch similar to the one Alpine has should be good to go then. I'll start a new CI for alpine34.

[jbergstroem]

(run here: https://ci.nodejs.org/job/node-test-commit-jbergstroem-alpine34/13/nodes=alpine34-x64/)

[jbergstroem]

@targos:

/home/iojs/build/workspace/node-test-commit-jbergstroem-alpine34/nodes/alpine34-x64/out/Release/obj.target/v8_libbase/deps/v8/src/base/debug/stack_trace.o: In function `v8::base::debug::StackTrace::ToString[abi:cxx11]() const':
stack_trace.cc:(.text._ZNK2v84base5debug10StackTrace8ToStringB5cxx11Ev+0x20e): undefined reference to `v8::base::debug::StackTrace::OutputToStream(std::ostream*) const'

Error looks straightforward.

[targos]

oh I see. I missed the change in http://git.alpinelinux.org/cgit/aports/tree/community/chromium/musl-hacks.patch

New CI run: https://ci.nodejs.org/job/node-test-commit-jbergstroem-alpine34/14/nodes=alpine34-x64/

[ofrobots]

@targos It seems that the mkpeephole fix missed the branch cut-off. I have submitted a merge request to get the fix merged into 5.4: https://bugs.chromium.org/p/v8/issues/detail?id=5318.