[WIP] Update V8 to 5.2 by targos · Pull Request #7824 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

v8

Description of change

This is almost ready, but I am still waiting for https://bugs.chromium.org/p/v8/issues/detail?id=5174 to be fixed in the 5.2 branch.
I'm creating the PR now to start a CI run.

CI: https://ci.nodejs.org/job/node-test-pull-request/3370/
CI: https://ci.nodejs.org/job/node-test-pull-request/3383/
CI: https://ci.nodejs.org/job/node-test-pull-request/3420/

In which next node version it is possible to see that's new shiny V8? 🤔

@bricss Probably not until v7.0.0, unless the ABI incompatibility issues can be smoothed over before August.

Noting some errors.

freebsd

  c++ '-DV8_TARGET_ARCH_X64' '-DENABLE_DISASSEMBLER' '-DV8_I18N_SUPPORT' '-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_STATIC' '-DUCONFIG_NO_SERVICE=1' '-DUCONFIG_NO_REGULAR_EXPRESSIONS=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=0' '-DUCONFIG_NO_BREAK_ITERATION=0' '-DUCONFIG_NO_LEGACY_CONVERSION=1' '-DUCONFIG_NO_CONVERSION=1' -I../deps/v8 -I../deps -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -fno-strict-aliasing -I/usr/local/include -fdata-sections -ffunction-sections -O3 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/out/Release/.deps//usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/out/Release/obj.target/v8_base/deps/v8/src/profiler/sampling-heap-profiler.o.d.raw   -c -o /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/out/Release/obj.target/v8_base/deps/v8/src/profiler/sampling-heap-profiler.o ../deps/v8/src/profiler/sampling-heap-profiler.cc
../deps/v8/src/profiler/sampler.cc:216:10: error: static_cast from 'pthread_t' (aka 'pthread *') to 'uint32_t' (aka 'unsigned int') is not allowed
  return static_cast<uint32_t>(thread_id);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
deps/v8/src/v8_base.target.mk:506: recipe for target '/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/out/Release/obj.target/v8_base/deps/v8/src/profiler/sampler.o' failed

smartos 64

  g++ '-DV8_TARGET_ARCH_X64' '-D__C99FEATURES__=1' '-DENABLE_DISASSEMBLER' '-DV8_I18N_SUPPORT' '-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_STATIC' '-DUCONFIG_NO_SERVICE=1' '-DUCONFIG_NO_REGULAR_EXPRESSIONS=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=0' '-DUCONFIG_NO_BREAK_ITERATION=0' '-DUCONFIG_NO_LEGACY_CONVERSION=1' '-DUCONFIG_NO_CONVERSION=1' -I../deps/v8 -I../deps -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common  -Wall -Wextra -Wno-unused-parameter -m64 -pthreads -fno-strict-aliasing -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/.deps//home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/obj.target/v8_base/deps/v8/src/interface-descriptors.o.d.raw   -c -o /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/obj.target/v8_base/deps/v8/src/interface-descriptors.o ../deps/v8/src/interface-descriptors.cc
../deps/v8/src/ic/ic.cc: In member function 'v8::internal::MaybeHandle<v8::internal::Object> v8::internal::BinaryOpIC::Transition(v8::internal::Handle<v8::internal::AllocationSite>, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>)':
../deps/v8/src/ic/ic.cc:2547:68: error: 'alloca' was not declared in this scope
     byte* instructions = static_cast<byte*>(alloca(instruction_size));
                                                                    ^
deps/v8/src/v8_base.target.mk:498: recipe for target '/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/obj.target/v8_base/deps/v8/src/ic/ic.o' failed

V8 bug: https://bugs.chromium.org/p/v8/issues/detail?id=5227

smartos 32

parallel/test-tick-processor.js failed

v8 CI run - https://ci.nodejs.org/job/node-test-commit-v8-linux/228/. To check for PPC, s390, should be ok there.

nice job

@targos I independently ran into the the ICU transliteration issue. Here's the fix I put together with help from @srl295: ofrobots@fcd0595. We should reconcile the two fixes.

deps: edit v8 gitignore to allow trace event copy deps: update v8 trace event to 54b8455be9505c2cb0cf5c26bb86739c236471aa

V8 needs it for case conversion. Ref: https://codereview.chromium.org/1812673005

The location of various gypfiles has changed in V8 5.2.

The next major release will make it a fatal error to use non-primitive values in function templates and object templates. Print a warning that includes the C and JS stack trace to tell people to upgrade their add-ons. The C stack trace is only printed on platforms that support it (the BSDs, OS X and Linux+glibc.) The warning can be disabled with the new `--nowarn_template_set` flag. Refs: nodejs#6216 PR-URL: nodejs#6277 Reviewed-By: James M Snell <jasnell@gmail.com>

Original commit message: S390:Update inline asm constraint in test-platform The GetStackPointer() routine in test-platform uses an inline assembly code to store the current stack pointer value into a static variable sp_addr. The existing asm code for S390 uses an ST/STG instruction, with the memory operand associated with the general ('=g') constraint to sp_addr. On GCC 4.8.5, the GCC compiler got confused and treated sp_addr as an integer operand instead of memory operand, resulting in a store being emitted that writes to an invalid meory location. Given the specific store instructions being inlined here, we should restict the sp_addr operand to explicitly be a memory operand using '=m' instead of '=g'. R=bmeurer@chromium.org,jkummerow@chormium.org,rmcilroy@chromium.org,yangguo@chromium.org BUG= Review-Url: https://codereview.chromium.org/2158523002 Cr-Commit-Position: refs/heads/master@{nodejs#37809} Fixes: nodejs#7659 PR-URL: nodejs#7771 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

@ofrobots thanks, it's done.

I made an attempt to fix the compiling issues.
New CI: https://ci.nodejs.org/job/node-test-pull-request/3420/ with the following errors:

smartos-64

g++ '-DV8_TARGET_ARCH_X64' '-D__C99FEATURES__=1' '-DENABLE_DISASSEMBLER' '-DV8_I18N_SUPPORT' '-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_STATIC' '-DUCONFIG_NO_SERVICE=1' '-DUCONFIG_NO_REGULAR_EXPRESSIONS=1' '-DU_ENABLE_DYLOAD=0' '-DU_STATIC_IMPLEMENTATION=1' '-DU_HAVE_STD_STRING=0' '-DUCONFIG_NO_BREAK_ITERATION=0' '-DUCONFIG_NO_LEGACY_CONVERSION=1' '-DUCONFIG_NO_CONVERSION=1' -I../deps/v8 -I../deps -I../deps/icu-small/source/i18n -I../deps/icu-small/source/common  -Wall -Wextra -Wno-unused-parameter -m64 -pthreads -fno-strict-aliasing -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/.deps//home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/obj.target/v8_base/deps/v8/src/profiler/strings-storage.o.d.raw   -c -o /home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/obj.target/v8_base/deps/v8/src/profiler/strings-storage.o ../deps/v8/src/profiler/strings-storage.cc
../deps/v8/src/profiler/sampler.cc: In member function 'virtual void v8::internal::SamplerThread::Run()':
../deps/v8/src/profiler/sampler.cc:600:69: error: cast from 'void*' to 'pthread_t {aka unsigned int}' loses precision [-fpermissive]
             pthread_t thread_id = reinterpret_cast<pthread_t>(p->key);
                                                                     ^
deps/v8/src/v8_base.target.mk:498: recipe for target '/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos14-64/out/Release/obj.target/v8_base/deps/v8/src/profiler/sampler.o' failed

freebsd

not ok 914 parallel/test-tick-processor
# 
# 
# #
# # Fatal error in ../deps/v8/src/base/platform/semaphore.cc, line 88
# # Check failed: ( 0)==(reinterpret_cast<uintptr_t>(&native_handle_) & kSemaphoreAlignmentMask).
# #
# 
# ==== C stack trace ===============================
# 
#  1: _Fatal volatile
#  2: v8::base::Semaphore::Semaphore(int)
#  3: v8::internal::Logger::SetUp(v8::internal::Isolate*)
#  4: v8::internal::Isolate::Init(v8::internal::Deserializer*)
#  5: v8::internal::Snapshot::Initialize(v8::internal::Isolate*)
#  6: v8::Isolate::New(v8::Isolate::CreateParams const&)
#  7: node::Start(int, char**)
#  8: _start
# 
# assert.js:89
#   throw new assert.AssertionError({
#   ^
# AssertionError: /LazyCompile.*\[eval\]:1|.*%  UNKNOWN/ not matching Statistical profiling result from isolate-0x803c4d000-v8.log, (0 ticks, 0 unaccounted, 0 excluded).
# 
#     at runTest (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-tick-processor.js:58:3)
#     at Object.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-tick-processor.js:21:1)
#     at Module._compile (module.js:541:32)
#     at Object.Module._extensions..js (module.js:550:10)
#     at Module.load (module.js:458:32)
#     at tryModuleLoad (module.js:417:12)
#     at Function.Module._load (module.js:409:3)
#     at Module.runMain (module.js:575:10)
#     at run (bootstrap_node.js:340:7)
#     at startup (bootstrap_node.js:132:9)

Edit: V8 bug ? https://bugs.chromium.org/p/chromium/issues/detail?id=605349

While playing with this branch in one of my projects, I found a bug:

async function test() {
    await 1;
    throw new Error('bad');
}

test().catch(function(e){
        e.stack;
});

This code fails at a check:

#
# Fatal error in ../deps/v8/src/frames-inl.h, line 163
# Check failed: -1 <= index && index < param_count.
#

==== C stack trace ===============================

 1: V8_Fatal
 2: v8::internal::JavaScriptFrame::GetParameterSlot(int) const
 3: v8::internal::JavaScriptFrame::GetParameter(int) const
 4: v8::internal::JavaScriptFrame::receiver() const
 5: v8::internal::JavaScriptFrame::Summarize(v8::internal::List<v8::internal::FrameSummary, v8::internal::FreeStoreAllocationPolicy>*, v8::internal::FrameSummary::Mode) const
 6: v8::internal::OptimizedFrame::Summarize(v8::internal::List<v8::internal::FrameSummary, v8::internal::FreeStoreAllocationPolicy>*, v8::internal::FrameSummary::Mode) const
 7: v8::internal::Isolate::CaptureSimpleStackTrace(v8::internal::Handle<v8::internal::JSReceiver>, v8::internal::Handle<v8::internal::Object>)
 8: v8::internal::Isolate::CaptureAndSetSimpleStackTrace(v8::internal::Handle<v8::internal::JSReceiver>, v8::internal::Handle<v8::internal::Object>)
 9: 0x1b21737
10: v8::internal::Runtime_CollectStackTrace(int, v8::internal::Object**, v8::internal::Isolate*)
11: 0x1bd2a4008bc7

The bug is fixed in V8's master branch but not in 5.2 or 5.3.
We might want to try and backport the fix, as 5.3 will probably be in the v7 release and I'm sure a lot of people will start using --harmony-async-await as soon as it is available.

I am hoping we can get V8 5.4 in Node v7.x, but I guess it depends on the exact timing of its release.

Yes let's please coordinate. @ofrobots is there a separate issue tracking this?

yeah - "locales" may not be quite right here in fact. It's more complex which is why I'd like a separate issue tracking this (translit enablement for v8) For now, I'd actually remove the translit line altogether from this file.

I commented on some outdated diffs for discussion.

It's not clear exactly what translit data is needed. either locales or deleting the line will probably add 640k to the minimal node build, most or all of which (99% or 100%)is unneeded and not beneficial to node.

What I'd like to see is a separate PR that enables transliteration support if to the v8 version requires it. This could go in prior to v8 going in and would make node.js always build ICU the right way.

Related here is 7de59ef#diff-eda374a7b1c3fccd439785b07caa1372 from @jasnell ’s #7355 which also enabled some additional ICU code and data, but for a core feature and not for a v8 requirement.

The V8 crash bug mentioned above makes debugging challenging in bigger projects. Programming errors and typos in async functions, e.g. something that normally prints errorTypeError: Cannot read property 'foo' of null leads to a crash without any information about the location in V8 5.2.

If possible, it'd be nice to see the fix backported.

More specifically I keep seeing the crash in this situation:

#!/usr/bin/env node --harmony_async_await

process.on('unhandledRejection', function(reason, p) {
    console.log("Unhandled Rejection at: Promise ", p, " reason: ", reason); // [1]
    process.exit(1);
});

async function foo() {
    await 42;
    consXole.log('Typo on this line');
}

(async function() {
    await foo();
})();

If I comment out [1] then the app doesn't crash anymore. But then you don't know what the problem is. This looks like a variation of the test case @targos already posted.

meta: given that v5 is EOL and there will be no further releases in that line, I'm going to go ahead and remove the dont-land-on-v5.x label and likely delete it.

This (and a bug 5174 fix in 5.2) appear to be going nowhere? Could be better to skip 5.2 and go straight to 5.3? It has been out for a month now, and there will soon be a 5.4...

@Kovensky I plan to skip directly to 5.4 when the branch is cut.

Closing (superseded by #8317)

targos added v8 engine Issues and PRs related to the V8 dependency. dont-land-on-v5.x labels Jul 21, 2016

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 21, 2016

mscdex removed the build Issues and PRs related to build files or the CI. label Jul 21, 2016

ChALkeR added the wip Issues and PRs that are still a work in progress. label Jul 21, 2016

targos and others added 6 commits July 26, 2016 07:57

deps: update V8 5.2.361.46

1399906

deps: edit v8 gitignore to allow trace event copy deps: update v8 trace event to 54b8455be9505c2cb0cf5c26bb86739c236471aa

tools: do not disable ICU's transliteration

8c13873

V8 needs it for case conversion. Ref: https://codereview.chromium.org/1812673005

build: update V8 gypfile paths

2d7abd9

The location of various gypfiles has changed in V8 5.2.

temporarily remove proxy test

c3d8bcf

targos force-pushed the v8-5.2 branch from 4f57a8f to c3d8bcf Compare July 26, 2016 05:59

targos added 2 commits July 26, 2016 08:31

attempt to fix freebsd compilation

406dd07

remove crash instrumentation to fix smartos64 build

e6ac388

srl295 reviewed Jul 27, 2016
View reviewed changes

srl295 added the i18n-api Issues and PRs related to the i18n implementation. label Jul 27, 2016

targos mentioned this pull request Jul 28, 2016

V8 version for 7.0.0 release #7904

Closed

jasnell removed the dont-land-on-v5.x label Aug 5, 2016

targos mentioned this pull request Aug 29, 2016

deps: update V8 to 5.4 #8317

Merged

2 tasks

targos closed this Aug 29, 2016

targos deleted the v8-5.2 branch September 29, 2016 16:21

                   "converters": "none",
                   "stringprep": "locales",
-                  "translit": "none",
+                  "translit": "locales",

[WIP] Update V8 to 5.2 by targos · Pull Request #7824 · nodejs/node · GitHub

Uh oh!

Uh oh!

[WIP] Update V8 to 5.2 - #7824

[WIP] Update V8 to 5.2#7824targos wants to merge 8 commits intonodejs:masternodejs/node:masterfrom targos:v8-5.2targos/node:v8-5.2Copy head branch name to clipboard

Conversation

targos commented Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

bricss commented Jul 21, 2016

Uh oh!

mscdex commented Jul 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Jul 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

freebsd

smartos 64

smartos 32

Uh oh!

mhdawson commented Jul 22, 2016

Uh oh!

i5ting commented Jul 25, 2016

Uh oh!

ofrobots commented Jul 25, 2016

Uh oh!

targos commented Jul 26, 2016

Uh oh!

targos commented Jul 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

smartos-64

freebsd

Uh oh!

targos commented Jul 27, 2016

Uh oh!

ofrobots commented Jul 27, 2016

Uh oh!

srl295 commented Jul 27, 2016

Uh oh!

srl295 Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srl295 commented Jul 27, 2016

Uh oh!

ilkkao commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilkkao commented Jul 31, 2016

Uh oh!

jasnell commented Aug 5, 2016

Uh oh!

Jessidhia commented Aug 18, 2016

Uh oh!

targos commented Aug 18, 2016

Uh oh!

targos commented Aug 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

[WIP] Update V8 to 5.2#7824
targos wants to merge 8 commits into
nodejs:masterfrom
targos:v8-5.2

targos commented Jul 21, 2016 •
edited
Loading

mscdex commented Jul 21, 2016 •
edited
Loading

targos commented Jul 22, 2016 •
edited
Loading

targos commented Jul 26, 2016 •
edited
Loading

srl295 Jul 27, 2016 •
edited
Loading

ilkkao commented Jul 29, 2016 •
edited
Loading