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 6.7 #19989

Closed
wants to merge 15 commits into
base: master
from

Conversation

@targos
Member

targos commented Apr 12, 2018

ETA: May 29th.

There are still 2 known issues to fix (help would be appreciated):

/cc @nodejs/v8-update

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@devsnek

This comment has been minimized.

Member

devsnek commented Apr 17, 2018

@nodejs/v8 do you guys know if there are any plans to expose the value of a bigint assuming it fits in an int64 or similar?

@jakobkummerow

This comment has been minimized.

Contributor

jakobkummerow commented Apr 17, 2018

@devsnek : No plans currently, but we're open to suggestions if there is demand for such an API :-)

@devsnek

This comment has been minimized.

Member

devsnek commented Apr 17, 2018

@jakobkummerow i was just looking at integrating bigint into our napi after we bump to 6.7 (https://gist.github.com/devsnek/34c3243a650ef5f4419a16961bca5cd8) and it seems like a bit of an inconsistency to not have some sort of method to see what the value of a bigint is. (https://gist.github.com/devsnek/34c3243a650ef5f4419a16961bca5cd8#file-napi_bigint-diff-L145) i'm probably not the best person to come up with a solution to that problem as i'm no c++ guru, but i personally do have a use case for passing 64bit integers between c++ and js.

@jakobkummerow

This comment has been minimized.

Contributor

jakobkummerow commented Apr 17, 2018

@devsnek : it's an intentional omission/postponing -- we didn't want to introduce some random API that we didn't have a use case for. I absolutely agree that for working with BigInts via the C++ API, more methods are needed, and I'd be happy to collaborate on a reasonable design for V8 6.8. We should probably take that discussion elsewhere though.

@ofrobots

This comment has been minimized.

Contributor

ofrobots commented Apr 23, 2018

What's going on with 731b4ad ? It seems bigger than the commit it is reverting anyway.

@targos

This comment has been minimized.

Member

targos commented Apr 23, 2018

@ofrobots 731b4ad is an exact revert of f02b74d.
0e99791 replaces it.

@targos

This comment has been minimized.

@jakobkummerow

This comment has been minimized.

Contributor

jakobkummerow commented May 1, 2018

Re earlier discussion: anyone who has opinions on what V8's BigInt API should look like, please chime in at crbug.com/v8/7712.

@targos

This comment has been minimized.

Member

targos commented May 3, 2018

/cc @nodejs/platform-freebsd @nodejs/platform-macos

We have a compiler issue with clang. Can someone please have a look?

FreeBSD error:

64/out/Release/obj.target/v8_base/deps/v8/src/inspector/wasm-translation.o ../deps/v8/src/inspector/wasm-translation.cc
08:08:42 ../deps/v8/src/inspector/wasm-translation.cc:91:40: error: default initialization of an object of const type 'const v8_inspector::WasmSourceInformation' requires a user-provided default constructor
08:08:42     static const WasmSourceInformation singleEmptySourceInformation;
08:08:42                                        ^
08:08:42 1 error generated.
08:08:42 deps/v8/gypfiles/v8_base.target.mk:625: recipe for target '/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/out/Release/obj.target/v8_base/deps/v8/src/inspector/wasm-translation.o' failed
08:08:42 gmake[2]: *** [/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/out/Release/obj.target/v8_base/deps/v8/src/inspector/wasm-translation.o] Error 1

macOS error:

../deps/v8/src/inspector/wasm-translation.cc:91:40: error: default initialization of an object of const type 'const v8_inspector::WasmSourceInformation' without a user-provided default constructor
    static const WasmSourceInformation singleEmptySourceInformation;
                                       ^
../deps/v8/src/inspector/wasm-translation.cc:91:68: note: add an explicit initializer to initialize 'singleEmptySourceInformation'
    static const WasmSourceInformation singleEmptySourceInformation;
                                                                   ^
                                                                   {}
1 error generated.
make[2]: *** [/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/obj.target/v8_base/deps/v8/src/inspector/wasm-translation.o] Error 1
@targos

This comment has been minimized.

Member

targos commented May 8, 2018

ping

@mmarchini

This comment has been minimized.

Member

mmarchini commented May 8, 2018

@targos those errors are likely related to an older version of clang being used in our OSX/FreeBSD servers. I was able to build without problem on my machine (Mac OS 10.13.4). Therefore we need to either:

  • Update our clang to a version which won't complain about this struct not having a user-provided constructor
  • Float a patch on V8 to make this struct compatible with older versions of clang
  • Maybe there's a flag to disable this check on clang, but I couldn't find it :(

I'll try to reproduce this issue in a FreeBSD box and figure out which clang version fixes this issue.

/cc @nodejs/v8 @nodejs/build

@mmarchini

This comment has been minimized.

Member

mmarchini commented May 8, 2018

FWIW I was able to reproduce this error on Ubuntu 16.04 with clang 3.8.

Also a reference to why I think this is a clang version issue: https://stackoverflow.com/a/28338265/2956796

@mmarchini

This comment has been minimized.

Member

mmarchini commented May 8, 2018

Upstream bug: https://bugs.chromium.org/p/v8/issues/detail?id=7743

I'll try to open a CL fixing it by the end of the day

@mmarchini

This comment has been minimized.

@mmarchini

This comment has been minimized.

Member

mmarchini commented May 11, 2018

Fix landed upstream. Should we try to backport it to 6.7 or should we float it?

@targos

This comment has been minimized.

Member

targos commented May 13, 2018

Thanks for your help @mmarchini !
It would be nice to get it backported to 6.7. A merge request has been made in https://bugs.chromium.org/p/v8/issues/detail?id=7743

@targos

This comment has been minimized.

Member

targos commented May 13, 2018

@jasnell, @cjihrig I just read your comments in #20662 about semver-ness of adding new globals.

This version of V8 adds 3 of them: BigInt, BigInt64Array, BigUint64Array. Are we going to have to disable the BigInt feature to be able to land V8 6.7 on Node 10?

@devsnek

This comment has been minimized.

Member

devsnek commented May 13, 2018

I would say no due to the relative non-existence of them in code, and anyone who is using those globals is running with harmony since you can't really polyfill bigint. I would personally consider this extenuating circumstances but I understand if it would be semver-major anyway.

@addaleax

This comment has been minimized.

Member

addaleax commented May 13, 2018

+1 to not considering the BigInt global addition semver-major.

v8::HeapProfiler::SetWrapperClassInfoProvider is deprecated

I think I can do that, or guide somebody else do address this. What’s the time frame? Does it have to happen before the ETA date (May 29)? It looks like for now it’s just a deprecation.

An issue with this might be the interaction with addons. I’m not aware of addons using the old API, but I wouldn’t be surprised if there are. The new API seems to assume a monolithic embedder (like Chromium), but in Node’s case we cannot provide information about objects created by userland addons. I’ll try to figure out what we can do here.

(@ChALkeR I know Gzemnid is JS-focused, but would it be possible to see if SetWrapperClassInfoProvider is something that addons use?)

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 13, 2018

I'm also +1 to allowing BigInt and company.

@targos

This comment has been minimized.

Member

targos commented May 13, 2018

What’s the time frame? Does it have to happen before the ETA date (May 29)? It looks like for now it’s just a deprecation.

Yes, it's just a deprecation. It doesn't have to block us landing the PR.
@hashseed do you know if there is an ETA for removal of the API (v8::HeapProfiler::SetWrapperClassInfoProvider)?

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

util: add type check function for BigIntObject
PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

Revert "src: fix GetCpuProfiler() deprecation warning"
This reverts commit f02b74d.

PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

src: fix GetCpuProfiler() deprecation warning
Refs: nodejs/node-v8#46

PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

test: add read_only_space heap space
v8.getHeapSpaceStatistics() now includes read_only_space
in its results. Update test-v8-stats.js to account for this.

Fixes: nodejs/node-v8#44

PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

test: update postmortem metadata test
This commit updates the following postmortem metadata constants:

- v8dbg_class_SharedFunctionInfo__code__Code
  - This is now combined with SharedFunctionInfo's function_data.
  - Renamed: v8dbg_class_SharedFunctionInfo__function_data__Object
  - V8 commit: v8/v8@51ded9d

- v8dbg_class_SharedFunctionInfo__raw_name__Object and
- v8dbg_class_SharedFunctionInfo__scope_info__ScopeInfo
  - These are now combined as name_or_scope_info.
  - Renamed: v8dbg_class_SharedFunctionInfo__name_or_scope_info__Object
  - V8 commit: v8/v8@74a663b

- v8dbg_class_SharedFunctionInfo__compiler_hints__int
  - Renamed: v8dbg_class_SharedFunctionInfo__flags__int
  - V8 commit: v8/v8@071b01b#diff-f3f182b0510ba2ee39ae87e421ff110b

Fixes: nodejs/node-v8#44
Fixes: nodejs/node-v8#48

PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

src: update postmortem constant name
This commit renames V8DBG_CLASS_SHAREDFUNCTIONINFO__RAW_NAME__OBJECT
to V8DBG_CLASS_SHAREDFUNCTIONINFO__NAME_OR_SCOPE_INFO__OBJECT
following an upstream change.

PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

2018-06-06, Version 10.4.0 (Current)
Notable Changes:

* **deps**:
 - update V8 to 6.7.288.43 (Michaël Zasso)
   nodejs#19989
* **stream**:
  - ensure Stream.pipeline re-throws errors without callback (Blaine Bublitz)
    nodejs#20437

PR-URL: nodejs#21167

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

deps: update V8 to 6.7.288.43
PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

build: reset embedder string to "-node.0"
PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

deps: update v8.gyp
Synchronize source files list with upstream's BUILD.gn.

PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

deps: backport 91ddb65d from upstream V8
Original commit message:

    Revert promises optimizations due to regressions in async hooks

    Revert "[async-await] Eliminate throwaway promise in async functions."

    This reverts commit a840f1f8f7e6c9ab32dec456a954612619855093.

    Revert "[async-generators] Also avoid throwaway promise here."

    This reverts commit feb545ceba5cb477ba883b46b60142f441ad5a3d.

    Revert "[async-await] Turn await closures into intrinsics."

    This reverts commit d97bb317386506a6798386b74647cbe0502fce35.

    Revert "[async-generators] Add fast-path for primitives in AsyncGeneratorYield."

    This reverts commit e57b500eb26dc595609613963468910eb4bcae2e.

    Revert "[async-generators] Add fast-path to skip "then" lookup in AsyncGeneratorResolve."

    This reverts commit c15802e11e30976528d1b711a9b9e6ff6b490117.

    Revert "[promises] Correctly run before/after hooks for await."

    This reverts commit ca7639239fc13e992e857710cbf7dbaac87163bf.

    Bug: v8:7253, v8:7745
    Change-Id: I25ad0d2df3cfbc84dbb431aa25b268bce8a39e89
    Reviewed-on: https://chromium-review.googlesource.com/1049975
    Commit-Queue: Maya Lekova <mslekova@chromium.org>
    Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#53139}

Refs: v8/v8@91ddb65
Fixes: nodejs#20516
Co-authored-by: Matteo Collina <matteo.collina@gmail.com>

PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

util: add type check function for BigIntObject
PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

Revert "src: fix GetCpuProfiler() deprecation warning"
This reverts commit f02b74d.

PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

src: fix GetCpuProfiler() deprecation warning
Refs: nodejs/node-v8#46

PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

test: add read_only_space heap space
v8.getHeapSpaceStatistics() now includes read_only_space
in its results. Update test-v8-stats.js to account for this.

Fixes: nodejs/node-v8#44

PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

test: update postmortem metadata test
This commit updates the following postmortem metadata constants:

- v8dbg_class_SharedFunctionInfo__code__Code
  - This is now combined with SharedFunctionInfo's function_data.
  - Renamed: v8dbg_class_SharedFunctionInfo__function_data__Object
  - V8 commit: v8/v8@51ded9d

- v8dbg_class_SharedFunctionInfo__raw_name__Object and
- v8dbg_class_SharedFunctionInfo__scope_info__ScopeInfo
  - These are now combined as name_or_scope_info.
  - Renamed: v8dbg_class_SharedFunctionInfo__name_or_scope_info__Object
  - V8 commit: v8/v8@74a663b

- v8dbg_class_SharedFunctionInfo__compiler_hints__int
  - Renamed: v8dbg_class_SharedFunctionInfo__flags__int
  - V8 commit: v8/v8@071b01b#diff-f3f182b0510ba2ee39ae87e421ff110b

Fixes: nodejs/node-v8#44
Fixes: nodejs/node-v8#48

PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

src: update postmortem constant name
This commit renames V8DBG_CLASS_SHAREDFUNCTIONINFO__RAW_NAME__OBJECT
to V8DBG_CLASS_SHAREDFUNCTIONINFO__NAME_OR_SCOPE_INFO__OBJECT
following an upstream change.

PR-URL: nodejs#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

2018-06-06, Version 10.4.0 (Current)
Notable Changes:

* **deps**:
 - update V8 to 6.7.288.43 (Michaël Zasso)
   nodejs#19989
* **stream**:
  - ensure Stream.pipeline re-throws errors without callback (Blaine Bublitz)
    nodejs#20437

PR-URL: nodejs#21167

watson added a commit to watson/apm-agent-nodejs that referenced this pull request Jul 17, 2018

docs: remove reference to Node 10 incompatibility
The bug in V8 that meant we couldn't instrument Node 10 was fixed and
Node 10.4.0 shipped with a newer version of V8.

This is the original bug report in Node:
nodejs/node#20516

This is the PR that fixed it in Node:
nodejs/node#19989

watson added a commit to watson/apm-agent-nodejs that referenced this pull request Jul 17, 2018

docs: remove reference to Node 10 incompatibility
The bug in V8 that meant we couldn't instrument Node 10 was fixed and
Node 10.4.0 shipped with a newer version of V8.

This is the original bug report in Node:
nodejs/node#20516

This is the PR that fixed it in Node:
nodejs/node#19989

Closes elastic#448

hferreiro added a commit to brave/node that referenced this pull request Jul 18, 2018

src: fix GetCpuProfiler() deprecation warning
Refs: nodejs/node-v8#46

PR-URL: nodejs/node#19989
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matheus Marchini <matheus@sthima.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
(cherry picked from commit fbe6b85)

watson added a commit to elastic/apm-agent-nodejs that referenced this pull request Jul 18, 2018

docs: remove reference to Node 10 incompatibility (#449)
The bug in V8 that meant we couldn't instrument Node 10 was fixed and
Node 10.4.0 shipped with a newer version of V8.

This is the original bug report in Node:
nodejs/node#20516

This is the PR that fixed it in Node:
nodejs/node#19989

Closes #448

mcollina added a commit that referenced this pull request Aug 23, 2018

async_hooks: adding regression test case for async/await
The actual bug was fixed by a V8 update in Node v10.4.0.

See: #19989
PR-URL: #22374
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos added a commit that referenced this pull request Aug 24, 2018

async_hooks: adding regression test case for async/await
The actual bug was fixed by a V8 update in Node v10.4.0.

See: #19989
PR-URL: #22374
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos added a commit that referenced this pull request Sep 3, 2018

async_hooks: adding regression test case for async/await
The actual bug was fixed by a V8 update in Node v10.4.0.

See: #19989
PR-URL: #22374
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018

async_hooks: adding regression test case for async/await
The actual bug was fixed by a V8 update in Node v10.4.0.

See: nodejs#19989
PR-URL: nodejs#22374
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment