Back-port deps/v8 changes for ppc64, Aix platform (Node 8 backport) by V-for-Vasili · Pull Request #23958 · nodejs/node · GitHub

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.

cc @nodejs/v8 @mhdawson @refack
PTAL.

Simplified this pull request: #23718

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.

@bnoordhuis updated last commit description, ptal

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.

RSLGTM with comments addressed.
.gyp file change LGTM

@refack @bnoordhuis Addressed the commit log.

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

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

/cc @nodejs/v8-update

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

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

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

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/

Last ping was for wrong PR.

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

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)

LGTM

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

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

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

/cc @hashseed @fhinkel

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

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

cc @MylesBorins @refack

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

@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

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

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.

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@{nodejs#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.

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>

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

Back-port deps/v8 changes for ppc64, Aix platform (Node 8 backport) by V-for-Vasili · Pull Request #23958 · nodejs/node · GitHub

Uh oh!

Uh oh!

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)#23958V-for-Vasili wants to merge 3 commits intonodejs:v8.x-stagingnodejs/node:v8.x-stagingfrom V-for-Vasili:Node-8-backport-cherry-pick-finalV-for-Vasili/node:Node-8-backport-cherry-pick-finalCopy head branch name to clipboard

Conversation

V-for-Vasili commented Oct 29, 2018

Uh oh!

V-for-Vasili commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Oct 29, 2018

Uh oh!

V-for-Vasili commented Oct 29, 2018

Uh oh!

refack commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

V-for-Vasili commented Oct 29, 2018

Uh oh!

bnoordhuis commented Oct 30, 2018

Uh oh!

MylesBorins commented Oct 30, 2018

Uh oh!

V-for-Vasili commented Oct 30, 2018

Uh oh!

V-for-Vasili commented Oct 30, 2018

Uh oh!

refack commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Oct 30, 2018

Uh oh!

refack commented Oct 31, 2018

Uh oh!

V-for-Vasili commented Oct 31, 2018

Uh oh!

refack commented Oct 31, 2018

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Oct 31, 2018

Uh oh!

mhdawson commented Oct 31, 2018

Uh oh!

MylesBorins commented Oct 31, 2018

Uh oh!

BethGriggs commented Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

V-for-Vasili commented Nov 19, 2018

Uh oh!

refack commented Nov 19, 2018

Uh oh!

V-for-Vasili commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Nov 20, 2018

Uh oh!

refack commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Nov 21, 2018

Uh oh!

MylesBorins commented Nov 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Back-port deps/v8 changes for ppc64, Aix platform (Node 8 backport)#23958
V-for-Vasili wants to merge 3 commits into
nodejs:v8.x-stagingfrom
V-for-Vasili:Node-8-backport-cherry-pick-final

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

refack commented Oct 29, 2018 •
edited
Loading

refack commented Oct 30, 2018 •
edited
Loading

BethGriggs commented Nov 15, 2018 •
edited
Loading

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

refack commented Nov 20, 2018 •
edited
Loading

MylesBorins commented Nov 21, 2018 •
edited
Loading