FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: update V8 to 7.2 by targos · Pull Request #24875 · nodejs/node · GitHub

/ node Public

deps: update V8 to 7.2 - #24875

Closed
targos wants to merge 10 commits into
nodejs:masterfrom
targos:v8-7.2
ConversationCommits (10)ChecksFiles changed
Closed

deps: update V8 to 7.2#24875
targos wants to merge 10 commits into
nodejs:masterfrom
targos:v8-7.2

Conversation

targos commented Dec 6, 2018

Copy link
Copy Markdown
Member

ETA: Jan 29th, 2019

targos added semver-major PRs that contain breaking changes and should be released in the next major version. blocked PRs that are blocked by other issues or PRs. labels Dec 6, 2018

nodejs-github-bot commented Dec 6, 2018

Copy link
Copy Markdown
Collaborator

@targos build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1853/pipeline

nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Dec 6, 2018
targos removed the build Issues and PRs related to build files or the CI. label Dec 6, 2018

targos commented Dec 6, 2018

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/19272/
V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/1920/

targos commented Dec 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

"unresolved external symbol" errors on Windows were already seen on canary. See nodejs/node-v8#89 (comment)
/cc @bzoz

targos commented Dec 6, 2018

Copy link
Copy Markdown
Member Author

Error on Linux with GCC 6:

17:00:46 g++: internal compiler error: Killed (program cc1plus)
17:00:46 Please submit a full bug report,
17:00:46 with preprocessed source if appropriate.
17:00:46 See <http://bugzilla.redhat.com/bugzilla> for instructions.
17:00:46 deps/v8/gypfiles/v8_initializers.target.mk:199: recipe for target '/home/iojs/build/workspace/node-test-commit-linux/nodes/centos7-64-gcc6/out/Release/obj.target/v8_initializers/gen/torque-generated/builtins-array-from-dsl-gen.o' failed
17:00:46 make[2]: *** [/home/iojs/build/workspace/node-test-commit-linux/nodes/centos7-64-gcc6/out/Release/obj.target/v8_initializers/gen/torque-generated/builtins-array-from-dsl-gen.o] Error 4

CI: https://ci.nodejs.org/job/node-test-commit-linux/23806/nodes=centos7-64-gcc6/

bnoordhuis commented Dec 6, 2018

Copy link
Copy Markdown
Member

Nine times out of ten that's the compiler getting OOM-killed. All the more likely because it was trying to compile a large generated file.

targos mentioned this pull request Dec 11, 2018
Closed
2 tasks
Flarna mentioned this pull request Dec 17, 2018
Closed
targos mentioned this pull request Dec 17, 2018
Closed

targos commented Jan 1, 2019

Copy link
Copy Markdown
Member Author

Updated.

New CI: https://ci.nodejs.org/job/node-test-pull-request/19904/
V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/1975/

targos and others added 7 commits January 20, 2019 08:45
deps: update V8 to 7.2.502.21
7d29926
build: reset embedder string to "-node.0"
bb0ffe7
src: update NODE_MODULE_VERSION to 69
a9cd973 
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 7.2.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md
deps: sync V8 gypfiles with 7.2
d1486e9 
Co-authored-by: Ujjwal Sharma <usharma1998@gmail.com>

win: add v8_init to dependencies

Fixes: nodejs/node-v8#89
Co-authored-by: Bartosz Sosnowski <bartosz@janeasystems.com>
deps: bump minimum icu version to 63
20721dc 
Bump minimum version of ICU needed to build node to 63.

Refs: v8/v8@30a350f
Co-authored-by: Steven R Loomis <srloomis@us.ibm.com>
test: remove apply calls over 65534 arg limit
8c3a8c8
test: update test-v8-stats
c04c8a2 
New heap space: code_large_object_space

targos commented Jan 20, 2019

Copy link
Copy Markdown
Member Author

Updated and fixed v8.gyp (contained a file that's not in 7.2).

CI: https://ci.nodejs.org/job/node-test-pull-request/20214/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2035/

targos commented Jan 20, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

/cc @nodejs/platform-ppc The executable produced for linuxone doesn't work (illegal instruction).
This is with V8 7.2.502.21

mhdawson commented Jan 21, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

@john-yan can you take a look.

mhdawson commented Jan 22, 2019

Copy link
Copy Markdown
Member

From@john-yan seems like V8 7.2 is green for us so new CI to see if it is repeatable: https://ci.nodejs.org/job/node-test-commit-v8-linux/2041/

mhdawson commented Jan 22, 2019

Copy link
Copy Markdown
Member

CI failed again. The node executable built seems to crash.

mhdawson commented Jan 22, 2019

Copy link
Copy Markdown
Member

I see that linuxone (s390x) also fails in the node CI in the same way it does for the v8 test. @sam-github can you help investigate what is going on? Looks like with the udpated version of V8 Node crashes on startup for some reason.

sam-github mentioned this pull request Jan 22, 2019
Closed

schuay commented Jan 24, 2019

Copy link
Copy Markdown

@schuay I'm wondering if there are any tests that could be added to the v8 testing that might have caught this problem? Maybe there are and we did not see them in the google CI because s390x testing is done through emulation?

Yes that is probably why we didn't see this before. IIRC runtime code is compiled with the host architecture in sim builds. On real s390 hosts we'd have seen this immediately.

john-yan commented Jan 24, 2019

Copy link
Copy Markdown

@schuay I'm wondering if there are any tests that could be added to the v8 testing that might have caught this problem? Maybe there are and we did not see them in the google CI because s390x testing is done through emulation?

Yes that is probably why we didn't see this before. IIRC runtime code is compiled with the host architecture in sim builds. On real s390 hosts we'd have seen this immediately.

Hello Jakob, we have v8 testing running locally on s390x host and we never see this either. Do you know which v8 test has coverage for this alignment issue?

schuay commented Jan 24, 2019

Copy link
Copy Markdown

Hello Jakob, we have v8 testing running locally on s390x host and we never see this either. Do you know which v8 test has coverage for this alignment issue?

From the backtrace I'd expect it to happen on every Isolate startup if the variable is unaligned. But I'd suggest moving this discussion to another bug to avoid cluttering here ;)

deps: cherry-pick fc0ddf5 from upstream V8
f9142b8 
Original commit message:

    [snapshot] Always align embedded blob code pointer and size

    Other platforms besides ARM64 Windows may also have alignment
    requirements, e.g. PPC and s390. These requirements may affect
    both the code pointer field and the size field, and so they
    each need alignment directives because they are stored in
    different sections.

    Since aligning wastes a handful of bytes at most, not making
    alignment conditional on the platform type seems like a good idea.

    Refs: nodejs#24875
    Change-Id: I1f58606af294be65e74a1f107cd05fc21e032704
    Reviewed-on: https://chromium-review.googlesource.com/c/1433778
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59058}

Refs: v8/v8@fc0ddf5
deps: backport bits of 9302db480e8 from upstream V8
85ffec2 
Backport parts of v8/v8@9302db480e8cd7c88fd948baf0cf that fix
compiler errors on Windows.

Original commit message:

    [ubsan] Port HeapObject to the new design

    Merging the temporary HeapObjectPtr back into HeapObject.

    Bug: v8:3770
    Change-Id: I5bcd23ca2f5ba862cf5b52955dca143e531c637b
    Reviewed-on: https://chromium-review.googlesource.com/c/1386492
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Reviewed-by: Michael Stanton <mvstanton@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{nodejs#58410}

Refs: v8/v8@9302db4

addaleax commented Jan 24, 2019

Copy link
Copy Markdown
Member

CI for addaleax/node@85ffec2 which might (:crossed_fingers:) fix the Windows compiler errors: https://ci.nodejs.org/job/node-test-commit/25276/

addaleax commented Jan 24, 2019

Copy link
Copy Markdown
Member

So, the compiler errors are fixed, but now there’s a linker error that’s probably going to need attention from somebody more knowledgeable with Windows/V8 snapshots than me:

v8_snapshot.lib(embedded.obj) : error LNK2026: module unsafe for SAFESEH image. [c:\workspace\node-compile-windows\node.vcxproj]

schuay commented Jan 25, 2019

Copy link
Copy Markdown

Haven't seen that one before. These Microsoft docs look relevant.

schuay commented Jan 25, 2019

Copy link
Copy Markdown

Is it an option to disable /SAFESEH? Here's another node issue where this has apparently been done before. Alternatively it may also be possible to add .SAFESEH MASM directives as explained here, but I have no experience with that.

paolosev commented Jan 26, 2019

Copy link
Copy Markdown

The problem seems to be in project v8_snapshot, where the file embedded.S is compiled with the MASM tool. We should pass /safeseh to ml.exe to make v8_snapshot.lib compatible with SAFESEH.

brn pushed a commit to brn/v8 that referenced this pull request Jan 30, 2019
[snapshot] Always align embedded blob code pointer and size
fc0ddf5 
Other platforms besides ARM64 Windows may also have alignment
requirements, e.g. PPC and s390. These requirements may affect
both the code pointer field and the size field, and so they
each need alignment directives because they are stored in
different sections.

Since aligning wastes a handful of bytes at most, not making
alignment conditional on the platform type seems like a good idea.

Refs: nodejs/node#24875
Change-Id: I1f58606af294be65e74a1f107cd05fc21e032704
Reviewed-on: https://chromium-review.googlesource.com/c/1433778
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59058}

targos commented Jan 31, 2019

Copy link
Copy Markdown
Member Author

Branch 7.3 was cut a week ago. I'll open a PR with it. We'll see if it's just easier to skip a version.

targos mentioned this pull request Jan 31, 2019
Closed

targos commented Jan 31, 2019

Copy link
Copy Markdown
Member Author

Attempte to update directly to 7.3: #25852

targos pushed a commit to targos/node that referenced this pull request Jan 31, 2019
deps: cherry-pick fc0ddf5 from upstream V8
1ccca91 
Original commit message:

    [snapshot] Always align embedded blob code pointer and size

    Other platforms besides ARM64 Windows may also have alignment
    requirements, e.g. PPC and s390. These requirements may affect
    both the code pointer field and the size field, and so they
    each need alignment directives because they are stored in
    different sections.

    Since aligning wastes a handful of bytes at most, not making
    alignment conditional on the platform type seems like a good idea.

    Refs: nodejs#24875
    Change-Id: I1f58606af294be65e74a1f107cd05fc21e032704
    Reviewed-on: https://chromium-review.googlesource.com/c/1433778
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59058}

Refs: v8/v8@fc0ddf5
targos closed this Feb 1, 2019
jkunkee mentioned this pull request Feb 8, 2019
Closed
cjihrig pushed a commit to cjihrig/node that referenced this pull request Mar 12, 2019
deps: cherry-pick fc0ddf5 from upstream V8
412f8d0 
Original commit message:

    [snapshot] Always align embedded blob code pointer and size

    Other platforms besides ARM64 Windows may also have alignment
    requirements, e.g. PPC and s390. These requirements may affect
    both the code pointer field and the size field, and so they
    each need alignment directives because they are stored in
    different sections.

    Since aligning wastes a handful of bytes at most, not making
    alignment conditional on the platform type seems like a good idea.

    Refs: nodejs#24875
    Change-Id: I1f58606af294be65e74a1f107cd05fc21e032704
    Reviewed-on: https://chromium-review.googlesource.com/c/1433778
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59058}

Refs: v8/v8@fc0ddf5
targos pushed a commit to targos/node that referenced this pull request Mar 12, 2019
deps: cherry-pick fc0ddf5 from upstream V8
b53dbc6 
Original commit message:

    [snapshot] Always align embedded blob code pointer and size

    Other platforms besides ARM64 Windows may also have alignment
    requirements, e.g. PPC and s390. These requirements may affect
    both the code pointer field and the size field, and so they
    each need alignment directives because they are stored in
    different sections.

    Since aligning wastes a handful of bytes at most, not making
    alignment conditional on the platform type seems like a good idea.

    Refs: nodejs#24875
    Change-Id: I1f58606af294be65e74a1f107cd05fc21e032704
    Reviewed-on: https://chromium-review.googlesource.com/c/1433778
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59058}

Refs: v8/v8@fc0ddf5
targos pushed a commit that referenced this pull request Mar 14, 2019
deps: cherry-pick fc0ddf5 from upstream V8
44d5401 
Original commit message:

    [snapshot] Always align embedded blob code pointer and size

    Other platforms besides ARM64 Windows may also have alignment
    requirements, e.g. PPC and s390. These requirements may affect
    both the code pointer field and the size field, and so they
    each need alignment directives because they are stored in
    different sections.

    Since aligning wastes a handful of bytes at most, not making
    alignment conditional on the platform type seems like a good idea.

    Refs: #24875
    Change-Id: I1f58606af294be65e74a1f107cd05fc21e032704
    Reviewed-on: https://chromium-review.googlesource.com/c/1433778
    Commit-Queue: Jakob Gruber <jgruber@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59058}

Refs: v8/v8@fc0ddf5

PR-URL: #25852
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos deleted the v8-7.2 branch March 14, 2019 17:53
richardlau mentioned this pull request Mar 27, 2019
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

blocked PRs that are blocked by other issues or PRs. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL