Back-port deps/v8 changes for ppc64, Aix platform (Node 8 backport) #23958

V-for-Vasili · 2018-10-29T16:01:47Z

Back-port v8 changes for ppc64, Aix platform
Changes included:

ppc64, aix: Pass CallFrequency object by const reference to avoid value copy error.
Bug: v8:8193
GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61976
V8 Revision: d2e0166ded485df126c765b9196f7edd1ce50f82
Link: https://chromium-review.googlesource.com/c/v8/v8/+/1228633

ppc64, aix: Fixes to V8 GN build process on aix platform
(Only changes to src/base/debug/stack_trace_posix.cc included)
V8 Revision: 6bc4bfea655723b6e71280e71a3dae07c37d7b44
Link: https://chromium-review.googlesource.com/c/v8/v8/+/927484

Fix gyp build on Aix platform. (Changes not currently present in Master)
Supperss -Wnonnull-compare, -Waddress warnings for deps/v8/testing/gtest project;
Suppress unused function warnings in deps/v8/src/compiler/store-store-elimination.cc,
deps/v8/src/conversions.cc in order to compile with newer (>4.8.5) gcc on Aix.

V-for-Vasili · 2018-10-29T16:02:18Z

cc @nodejs/v8 @mhdawson @refack
PTAL.

Simplified this pull request: #23718

bnoordhuis · 2018-10-29T17:10:17Z

The third commit isn't necessary; the first two commits should both update V8_PATCH_LEVEL.

Apropos the fourth commit, we don't normally merge out-of-tree code changes but it looks like it's for code that no longer exists upstream? LGTM in that case but please update the commit log to conform to the style guide.

V-for-Vasili · 2018-10-29T17:33:31Z

@bnoordhuis updated last commit description, ptal

refack · 2018-10-29T17:39:57Z

updated last commit description, ptal

@V-for-Vasili according to the guide it should be something like:

deps,v8: fix gyp build on AIX platform

Floating this patch since the code does not exist upstream anymore.

deps/v8/testing/gtest.gyp:
Supperss -Wnonnull-compare, -Waddress warnings for deps/v8/testing/gtest
project;

deps/v8/src/compiler/store-store-elimination.cc, deps/v8/src/conversions.cc:
Suppress unused function warnings in order to compile with newer (>4.8.5) gcc
on Aix.

refack

RSLGTM with comments addressed.
.gyp file change LGTM

V-for-Vasili · 2018-10-29T18:07:56Z

@refack @bnoordhuis Addressed the commit log.

bnoordhuis · 2018-10-30T10:30:58Z

@V-for-Vasili Can you update the first two commits to each bump V8_PATCH_LEVEL and drop the third?

MylesBorins · 2018-10-30T15:36:54Z

@V-for-Vasili where does e402a75 come from? Is there an upstream equivalent or this your own code?

/cc @nodejs/v8-update

V-for-Vasili · 2018-10-30T19:52:14Z

@MylesBorins this is my own code. The changes are made to avoid compile time error on Aix and affect Aix only.

V-for-Vasili · 2018-10-30T21:05:38Z

@bnoordhuis Done.
Resolved a conflict with V8_PATCH_LEVEL in v8-version.h

refack · 2018-10-30T21:57:03Z

@V-for-Vasili I had to rebase (instead of merge) so that we can run it on our CI:
~~https://ci.nodejs.org/job/node-test-pull-request/18242/~~
Resume CI: https://ci.nodejs.org/job/node-test-pull-request/18244/

refack · 2018-10-30T21:58:52Z

P.S. If you like to be recognized by GitHub properly, you could setup your email as per https://help.github.com/articles/setting-your-commit-email-address-on-github/

refack · 2018-10-31T02:11:45Z

Last ping was for wrong PR.

V-for-Vasili · 2018-10-31T13:47:16Z

@refack Is there anything else I need to do for this or #23695 ?

refack · 2018-10-31T15:00:18Z

Is there anything else I need to do for this

Well it seems like now there's a conflict with deps/v8/include/v8-version.h

<<<<<<< Node-8-backport-cherry-pick-final
#define V8_PATCH_LEVEL 70
=======
#define V8_PATCH_LEVEL 69
>>>>>>> v8.x-staging

After that this need to be accepted by @nodejs/lts

(If you fix this remember to git rebase, our CI doesn't handle merges, but you might want to wait until this is approved for landing on v8.x)

mhdawson

LGTM

mhdawson · 2018-10-31T19:24:01Z

@BethGriggs and this one for the next 8.x release

mhdawson · 2018-10-31T19:42:07Z

@MylesBorins in respect to #23958 (comment). There is no equivalent upstream as it is fixed a different way in master and we cannot land this change in the older V8 version.

MylesBorins · 2018-10-31T20:12:24Z

@mhdawson if that is the case I would like someone from @nodejs/v8-update to review this before we land it

/cc @hashseed @fhinkel

BethGriggs · 2018-11-15T16:55:18Z

This PR was reverted fromv8.x-staging (and v8.13.0-proposal) due to https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=ppcle-ubuntu1404,v8test=v8test/1846/console.

Based on #23974 (comment), the failure may not have been down to this PR, but I'll rerun some CIs to be sure before landing again.

New CI: https://ci.nodejs.org/job/node-test-pull-request/18651

/cc @nodejs/platform-aix

V-for-Vasili · 2018-11-19T21:44:48Z

@BethGriggs Updated the PR (corrections to deps/v8/testing/gtest.gyp). Please run the CI again if possible.

cc @MylesBorins @refack

refack · 2018-11-19T21:51:16Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1886/

V-for-Vasili · 2018-11-20T18:08:36Z

@refack do I need to resolve this conflict or will it be resolved while landing?

<<<<<<< Node-8-backport-cherry-pick-final
#define V8_PATCH_LEVEL 73
=======
#define V8_PATCH_LEVEL 72
>>>>>>> v8.x-staging

refack · 2018-11-20T18:30:40Z

Since this is a backport branch, I defer to the @nodejs/backporters team.

refack · 2018-11-20T20:20:37Z

P.S. @V-for-Vasili if you don't get a response now, I'd hold off until closer to the next planned release.
v8.13.0 just went out.

MylesBorins · 2018-11-21T16:27:20Z

one last ci: https://ci.nodejs.org/job/node-test-pull-request/18849/

if it is green I'll go ahead and land / take care of conflict

edit:

Conflict was breaking CI... I've gone ahead and fixed the conflict and force pushed the branch

https://ci.nodejs.org/job/node-test-pull-request/18850/

…debug/stack_trace_posix.cc included) Original commit message: Fixes to V8 GN build process on aix platform src/base/debug/stack_trace_posix.cc: suppressed unused function warnings for functions DemangleSymbols, OutputPointer(in order to compile with -Werror flag) test/cctest/test-isolate-independent-builtins.cc: corrections to make ByteInText test case compatible with aix. (affects aix only) Change-Id: I49e45e63545404c77aaed3f51b26557f6f03455e Reviewed-on: https://chromium-review.googlesource.com/927484 Reviewed-by: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Michael Achenbach <machenbach@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#52071}

Original commit message: ppc64, aix: Pass CallFrequency object by const reference to avoid value copy error. Bug: v8:8193 GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61976 Change-Id: I0d4efca4da03ef82651325e15ddf2160022bc8de Reviewed-on: https://chromium-review.googlesource.com/1228633 Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Reviewed-by: Daniel Clifford <danno@chromium.org> Reviewed-by: Junliang Yan <jyan@ca.ibm.com> Commit-Queue: Junliang Yan <jyan@ca.ibm.com> Cr-Commit-Position: refs/heads/master@{#56275}

Floating this patch since the code does not exist upstream anymore. deps/v8/testing/gtest.gyp: Supperss -Wnonnull-compare, -Waddress warnings for deps/v8/testing/gtest project; deps/v8/src/compiler/store-store-elimination.cc, deps/v8/src/conversions.cc: Suppress unused function warnings in order to compile with newer (>4.8.5) gcc on Aix.

Trott · 2018-11-21T18:37:12Z

https://ci.nodejs.org/job/node-test-pull-request/18850/

@MylesBorins It's yellow! Hooray?

Only changes to src/base/debug/stack_trace_posix.cc included Original commit message: Fixes to V8 GN build process on aix platform src/base/debug/stack_trace_posix.cc: suppressed unused function warnings for functions DemangleSymbols, OutputPointer(in order to compile with -Werror flag) test/cctest/test-isolate-independent-builtins.cc: corrections to make ByteInText test case compatible with aix. (affects aix only) Change-Id: I49e45e63545404c77aaed3f51b26557f6f03455e Reviewed-on: https://chromium-review.googlesource.com/927484 Reviewed-by: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Michael Achenbach <machenbach@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#52071} PR-URL: #23958 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>

Original commit message: ppc64, aix: Pass CallFrequency object by const reference to avoid value copy error. Bug: v8:8193 GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61976 Change-Id: I0d4efca4da03ef82651325e15ddf2160022bc8de Reviewed-on: https://chromium-review.googlesource.com/1228633 Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Reviewed-by: Daniel Clifford <danno@chromium.org> Reviewed-by: Junliang Yan <jyan@ca.ibm.com> Commit-Queue: Junliang Yan <jyan@ca.ibm.com> Cr-Commit-Position: refs/heads/master@{#56275} PR-URL: #23958 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>

Floating this patch since the code does not exist upstream anymore. deps/v8/testing/gtest.gyp: Supperss -Wnonnull-compare, -Waddress warnings for deps/v8/testing/gtest project; deps/v8/src/compiler/store-store-elimination.cc, deps/v8/src/conversions.cc: Suppress unused function warnings in order to compile with newer (>4.8.5) gcc on Aix. PR-URL: #23958 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>

MylesBorins · 2018-11-22T04:43:07Z

landed in ebe617e...4d00e1c

Only changes to src/base/debug/stack_trace_posix.cc included Original commit message: Fixes to V8 GN build process on aix platform src/base/debug/stack_trace_posix.cc: suppressed unused function warnings for functions DemangleSymbols, OutputPointer(in order to compile with -Werror flag) test/cctest/test-isolate-independent-builtins.cc: corrections to make ByteInText test case compatible with aix. (affects aix only) Change-Id: I49e45e63545404c77aaed3f51b26557f6f03455e Reviewed-on: https://chromium-review.googlesource.com/927484 Reviewed-by: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Michael Achenbach <machenbach@chromium.org> Commit-Queue: Jakob Gruber <jgruber@chromium.org> Cr-Commit-Position: refs/heads/master@{#52071} PR-URL: #23958 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>

Original commit message: ppc64, aix: Pass CallFrequency object by const reference to avoid value copy error. Bug: v8:8193 GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61976 Change-Id: I0d4efca4da03ef82651325e15ddf2160022bc8de Reviewed-on: https://chromium-review.googlesource.com/1228633 Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Reviewed-by: Daniel Clifford <danno@chromium.org> Reviewed-by: Junliang Yan <jyan@ca.ibm.com> Commit-Queue: Junliang Yan <jyan@ca.ibm.com> Cr-Commit-Position: refs/heads/master@{#56275} PR-URL: #23958 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>

Floating this patch since the code does not exist upstream anymore. deps/v8/testing/gtest.gyp: Supperss -Wnonnull-compare, -Waddress warnings for deps/v8/testing/gtest project; deps/v8/src/compiler/store-store-elimination.cc, deps/v8/src/conversions.cc: Suppress unused function warnings in order to compile with newer (>4.8.5) gcc on Aix. PR-URL: #23958 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: George Adams <george.adams@uk.ibm.com>

nodejs-github-bot added v8 engine Issues and PRs related to the V8 dependency. v8.x labels Oct 29, 2018

V-for-Vasili mentioned this pull request Oct 29, 2018

Back-port v8 changes for ppc64, Aix platform (Node 8 backport) #23718

Closed

V-for-Vasili force-pushed the Node-8-backport-cherry-pick-final branch from ae200cc to 615d0ae Compare October 29, 2018 17:31

refack approved these changes Oct 29, 2018

View reviewed changes

V-for-Vasili force-pushed the Node-8-backport-cherry-pick-final branch from 615d0ae to e402a75 Compare October 29, 2018 18:06

V-for-Vasili force-pushed the Node-8-backport-cherry-pick-final branch from e402a75 to 51304c2 Compare October 30, 2018 21:01

refack force-pushed the Node-8-backport-cherry-pick-final branch from 06b8e7d to 9260e0e Compare October 30, 2018 21:55

refack self-assigned this Oct 31, 2018

refack removed the v8.x label Oct 31, 2018

refack added the v8.x label Oct 31, 2018

refack removed their assignment Oct 31, 2018

mhdawson approved these changes Oct 31, 2018

View reviewed changes

BethGriggs mentioned this pull request Nov 12, 2018

v8.13.0 proposal #23974

Merged

BethGriggs reopened this Nov 15, 2018

gdams approved these changes Nov 15, 2018

View reviewed changes

V-for-Vasili force-pushed the Node-8-backport-cherry-pick-final branch from d95ed59 to 3933465 Compare November 19, 2018 21:40

Vasili Skurydzin added 3 commits November 21, 2018 11:31

MylesBorins force-pushed the Node-8-backport-cherry-pick-final branch from 3933465 to 96dbfb0 Compare November 21, 2018 16:33

MylesBorins closed this Nov 22, 2018

BethGriggs mentioned this pull request Dec 4, 2018

v8.14.1 proposal #24832

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Back-port deps/v8 changes for ppc64, Aix platform (Node 8 backport) #23958

Back-port deps/v8 changes for ppc64, Aix platform (Node 8 backport) #23958

V-for-Vasili commented Oct 29, 2018

V-for-Vasili commented Oct 29, 2018 •

edited

bnoordhuis commented Oct 29, 2018

V-for-Vasili commented Oct 29, 2018

refack commented Oct 29, 2018 •

edited

refack left a comment

V-for-Vasili commented Oct 29, 2018

bnoordhuis commented Oct 30, 2018

MylesBorins commented Oct 30, 2018

V-for-Vasili commented Oct 30, 2018

V-for-Vasili commented Oct 30, 2018

refack commented Oct 30, 2018 •

edited

refack commented Oct 30, 2018

refack commented Oct 31, 2018

V-for-Vasili commented Oct 31, 2018

refack commented Oct 31, 2018

mhdawson left a comment

mhdawson commented Oct 31, 2018

mhdawson commented Oct 31, 2018

MylesBorins commented Oct 31, 2018

BethGriggs commented Nov 15, 2018 •

edited

V-for-Vasili commented Nov 19, 2018

refack commented Nov 19, 2018

V-for-Vasili commented Nov 20, 2018 •

edited

refack commented Nov 20, 2018

refack commented Nov 20, 2018 •

edited

MylesBorins commented Nov 21, 2018 •

edited

Trott commented Nov 21, 2018

MylesBorins commented Nov 22, 2018

Back-port deps/v8 changes for ppc64, Aix platform (Node 8 backport) #23958

Back-port deps/v8 changes for ppc64, Aix platform (Node 8 backport) #23958

Conversation

V-for-Vasili commented Oct 29, 2018

V-for-Vasili commented Oct 29, 2018 • edited

bnoordhuis commented Oct 29, 2018

V-for-Vasili commented Oct 29, 2018

refack commented Oct 29, 2018 • edited

refack left a comment

Choose a reason for hiding this comment

V-for-Vasili commented Oct 29, 2018

bnoordhuis commented Oct 30, 2018

MylesBorins commented Oct 30, 2018

V-for-Vasili commented Oct 30, 2018

V-for-Vasili commented Oct 30, 2018

refack commented Oct 30, 2018 • edited

refack commented Oct 30, 2018

refack commented Oct 31, 2018

V-for-Vasili commented Oct 31, 2018

refack commented Oct 31, 2018

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Oct 31, 2018

mhdawson commented Oct 31, 2018

MylesBorins commented Oct 31, 2018

BethGriggs commented Nov 15, 2018 • edited

V-for-Vasili commented Nov 19, 2018

refack commented Nov 19, 2018

V-for-Vasili commented Nov 20, 2018 • edited

refack commented Nov 20, 2018

refack commented Nov 20, 2018 • edited

MylesBorins commented Nov 21, 2018 • edited

Trott commented Nov 21, 2018

MylesBorins commented Nov 22, 2018

V-for-Vasili commented Oct 29, 2018 •

edited

refack commented Oct 29, 2018 •

edited

refack commented Oct 30, 2018 •

edited

BethGriggs commented Nov 15, 2018 •

edited

V-for-Vasili commented Nov 20, 2018 •

edited

refack commented Nov 20, 2018 •

edited

MylesBorins commented Nov 21, 2018 •

edited