Skip to content
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

Back-port v8 changes for ppc64, Aix platform #23695

Conversation

V-for-Vasili
Copy link
Contributor

@V-for-Vasili V-for-Vasili commented Oct 16, 2018

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

s390, ppc64: Enable v8gen.py on Linux s390, ppc64
V8 Revision: abab9fbb643e97d1fa7ddee9bfc609e0e4a4f184
Link: https://chromium-review.googlesource.com/c/v8/v8/+/1135540

fix gn builds on aix.
V8 Revision: d9e78322d0370543342d52d58e4d81db67bb7203
Link: https://chromium-review.googlesource.com/c/v8/v8/+/1103533

Use gn from PATH on aix
V8 Revision: e7fad930a83b5b4f1bfee6bee620f35b988d28c3
Link: https://chromium-review.googlesource.com/c/v8/v8/+/1168087

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

disable failing cctest on AIX temporarily
V8 Revision: 67b549938d9060dde5cb557865f7451392a0f004
Link: https://chromium-review.googlesource.com/c/v8/v8/+/1174560

deps/v8/src/torque/file-visitor.h:
Add virtual dtor to avoid gcc error on Aix. (Not currently present in v8/master)

The following patch to v8/build solves this issue for v8/master:
a1a12ef3b343f9e75c630ed6dc8f1ea44a8a747b

However, the version of '/chromium/src/build.git' cannot be updated to include this
patch in v8/DEPS file. (could potentially cause issues for other platforms)

The change to deps/v8/src/torque/file-visitor.h is a workaround for origin/v10.x-staging
branch.

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v10.x v8 engine Issues and PRs related to the V8 dependency. labels Oct 16, 2018
@V-for-Vasili
Copy link
Contributor Author

cc @nodejs/v8

@mmarchini
Copy link
Contributor

/cc @nodejs/v8 @nodejs/v8-update

@mhdawson
Copy link
Member

I think you need to include e228033 in your description for the PR, but otherwise looks good.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@refack
Copy link
Contributor

refack commented Oct 16, 2018

@V-for-Vasili if you want you can teach GitHub to recognize you by following https://help.github.com/articles/setting-your-commit-email-address-on-github/

@V-for-Vasili
Copy link
Contributor Author

@mhdawson
e228033 included in PR description:

Use gn from PATH on aix
V8 Revision: e7fad930a83b5b4f1bfee6bee620f35b988d28c3
Link: https://chromium-review.googlesource.com/c/v8/v8/+/1168087

@BridgeAR
Copy link
Member

@V-for-Vasili
Copy link
Contributor Author

@BridgeAR according to the log a git exception (timeout) occurred for OSX1010
Would you be able to try and rerun the CI?

@mhdawson
Copy link
Member

@mhdawson
Copy link
Member

Resumed build again: https://ci.nodejs.org/job/node-test-pull-request/18215/

@refack
Copy link
Contributor

refack commented Oct 31, 2018

Is there anything else I need to do for this

IIUC no. It just needs to be accepted by @nodejs/lts

@mhdawson
Copy link
Member

@BethGriggs can you see if we can pull this in for the next 8.x release.

@mhdawson
Copy link
Member

@BethGriggs sorry I meant the next 10.x release

@BethGriggs
Copy link
Member

Could someone from @nodejs/v8 review please?

@MylesBorins
Copy link
Contributor

/cc @nodejs/v8-update

@V-for-Vasili V-for-Vasili force-pushed the Node-10-aix-backport-cherry-pick branch 2 times, most recently from 8b3326b to 9e969c3 Compare November 14, 2018 16:12
@V-for-Vasili
Copy link
Contributor Author

@refack Can the common.gypi conflict be resolved when this PR is being merged using "Resolve conflicts" button or do I need to resolve it? (simple version mismatch in common.gypi: -node.36 vs -node.38)

@refack
Copy link
Contributor

refack commented Nov 14, 2018

@refack Can the common.gypi conflict be resolved when this PR is being merged using "Resolve conflicts" button or do I need to resolve it? (simple version mismatch in common.gypi: -node.36 vs -node.38)

We don't use the "resolve conflicts" button because it creates a merge commit. The conflict could be solved during the manual landing process though. v8_embedder_string might also continue to change anyway while the the v10.x version is being staged.

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common.gypi should be bumped for each individual commit. (currently it is being bumped once at the end of all the commits).

Vasili Skurydzin added 4 commits November 21, 2018 14:23
  Original commit message:
  Use gn from PATH on aix

  Change-Id: I853f7899dbba9112ba1ca2ce78e2838b5a09c975
  Reviewed-on: https://chromium-review.googlesource.com/1168087
  Commit-Queue: John Barboza <jbarboza@ca.ibm.com>
  Reviewed-by: Michael Achenbach <machenbach@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#55028}
  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}
Original commit message:
  PPC: disable failing cctest on AIX temporarily

  Change-Id: I8a0081acb9c5eb662bf43eceb52218096eac327c
  Reviewed-on: https://chromium-review.googlesource.com/1174560
  Reviewed-by: Adam Klein <adamk@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@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#55229}
Add virtual dtor to avoid gcc error on Aix. (Not currently present in v8/master)

deps/v8/third_party/antlr4/BUILD.gn:
Use current_os variable to avoid is_aix being undefined.

The following patch to v8/build solves this issue for v8/master:
a1a12ef3b343f9e75c630ed6dc8f1ea44a8a747b

However, the version of '/chromium/src/build.git' cannot be updated to include this
patch in v8/DEPS file. (could potentially cause issues for other platforms)

The change to deps/v8/src/torque/file-visitor.h is a workaround for origin/v10.x-staging
branch.
@V-for-Vasili V-for-Vasili force-pushed the Node-10-aix-backport-cherry-pick branch from 9e969c3 to 21c50f1 Compare November 21, 2018 21:10
@V-for-Vasili
Copy link
Contributor Author

@MylesBorins done.

@MylesBorins
Copy link
Contributor

MylesBorins pushed a commit that referenced this pull request Nov 22, 2018
Original commit message:
  fix gn builds on aix

  Change-Id: I60aed7bf8207703fa6ceddb6165e173e68b5ff5f
  Reviewed-on: https://chromium-review.googlesource.com/1103533
  Commit-Queue: Michael Achenbach <machenbach@chromium.org>
  Reviewed-by: Michael Achenbach <machenbach@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#54386}

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2018
Original commit message:
  s390, ppc64: Enable v8gen.py on Linux s390, ppc64

  Change-Id: Ia05e949e1a823e30a45894c47f6f6df2e159befe
  Reviewed-on: https://chromium-review.googlesource.com/1135540
  Commit-Queue: Michael Achenbach <machenbach@chromium.org>
  Reviewed-by: Michael Achenbach <machenbach@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#54485}

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2018
Original commit message:

  Use gn from PATH on aix

  Change-Id: I853f7899dbba9112ba1ca2ce78e2838b5a09c975
  Reviewed-on: https://chromium-review.googlesource.com/1168087
  Commit-Queue: John Barboza <jbarboza@ca.ibm.com>
  Reviewed-by: Michael Achenbach <machenbach@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#55028}

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2018
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: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2018
Original commit message:
  PPC: disable failing cctest on AIX temporarily

  Change-Id: I8a0081acb9c5eb662bf43eceb52218096eac327c
  Reviewed-on: https://chromium-review.googlesource.com/1174560
  Reviewed-by: Adam Klein <adamk@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Junliang Yan <jyan@ca.ibm.com>
  Commit-Queue: Junliang Yan <jyan@ca.ibm.com>
  Cr-Commit-Position: refs/heads/master@{#55229}

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2018
deps/v8/src/torque/file-visitor.h:
* Not currently present in v8/master

deps/v8/third_party/antlr4/BUILD.gn:
* Use current_os variable to avoid is_aix being undefined.

The following patch to v8/build solves this issue for v8/master:
* a1a12ef3b343f9e75c630ed6dc8f1ea44a8a747b

However, the version of '/chromium/src/build.git' cannot be updated to
include this patch in v8/DEPS file. (could potentially cause issues for
other platforms)

The change to deps/v8/src/torque/file-visitor.h is a workaround for
origin/v10.x-staging branch.

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 469473d...20430ae

@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Original commit message:
  fix gn builds on aix

  Change-Id: I60aed7bf8207703fa6ceddb6165e173e68b5ff5f
  Reviewed-on: https://chromium-review.googlesource.com/1103533
  Commit-Queue: Michael Achenbach <machenbach@chromium.org>
  Reviewed-by: Michael Achenbach <machenbach@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#54386}

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Original commit message:
  s390, ppc64: Enable v8gen.py on Linux s390, ppc64

  Change-Id: Ia05e949e1a823e30a45894c47f6f6df2e159befe
  Reviewed-on: https://chromium-review.googlesource.com/1135540
  Commit-Queue: Michael Achenbach <machenbach@chromium.org>
  Reviewed-by: Michael Achenbach <machenbach@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#54485}

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Original commit message:

  Use gn from PATH on aix

  Change-Id: I853f7899dbba9112ba1ca2ce78e2838b5a09c975
  Reviewed-on: https://chromium-review.googlesource.com/1168087
  Commit-Queue: John Barboza <jbarboza@ca.ibm.com>
  Reviewed-by: Michael Achenbach <machenbach@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#55028}

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
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: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Original commit message:
  PPC: disable failing cctest on AIX temporarily

  Change-Id: I8a0081acb9c5eb662bf43eceb52218096eac327c
  Reviewed-on: https://chromium-review.googlesource.com/1174560
  Reviewed-by: Adam Klein <adamk@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Junliang Yan <jyan@ca.ibm.com>
  Commit-Queue: Junliang Yan <jyan@ca.ibm.com>
  Cr-Commit-Position: refs/heads/master@{#55229}

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
deps/v8/src/torque/file-visitor.h:
* Not currently present in v8/master

deps/v8/third_party/antlr4/BUILD.gn:
* Use current_os variable to avoid is_aix being undefined.

The following patch to v8/build solves this issue for v8/master:
* a1a12ef3b343f9e75c630ed6dc8f1ea44a8a747b

However, the version of '/chromium/src/build.git' cannot be updated to
include this patch in v8/DEPS file. (could potentially cause issues for
other platforms)

The change to deps/v8/src/torque/file-visitor.h is a workaround for
origin/v10.x-staging branch.

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Original commit message:
  fix gn builds on aix

  Change-Id: I60aed7bf8207703fa6ceddb6165e173e68b5ff5f
  Reviewed-on: https://chromium-review.googlesource.com/1103533
  Commit-Queue: Michael Achenbach <machenbach@chromium.org>
  Reviewed-by: Michael Achenbach <machenbach@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#54386}

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Original commit message:
  s390, ppc64: Enable v8gen.py on Linux s390, ppc64

  Change-Id: Ia05e949e1a823e30a45894c47f6f6df2e159befe
  Reviewed-on: https://chromium-review.googlesource.com/1135540
  Commit-Queue: Michael Achenbach <machenbach@chromium.org>
  Reviewed-by: Michael Achenbach <machenbach@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#54485}

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Original commit message:

  Use gn from PATH on aix

  Change-Id: I853f7899dbba9112ba1ca2ce78e2838b5a09c975
  Reviewed-on: https://chromium-review.googlesource.com/1168087
  Commit-Queue: John Barboza <jbarboza@ca.ibm.com>
  Reviewed-by: Michael Achenbach <machenbach@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#55028}

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
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: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Original commit message:
  PPC: disable failing cctest on AIX temporarily

  Change-Id: I8a0081acb9c5eb662bf43eceb52218096eac327c
  Reviewed-on: https://chromium-review.googlesource.com/1174560
  Reviewed-by: Adam Klein <adamk@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Junliang Yan <jyan@ca.ibm.com>
  Commit-Queue: Junliang Yan <jyan@ca.ibm.com>
  Cr-Commit-Position: refs/heads/master@{#55229}

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
deps/v8/src/torque/file-visitor.h:
* Not currently present in v8/master

deps/v8/third_party/antlr4/BUILD.gn:
* Use current_os variable to avoid is_aix being undefined.

The following patch to v8/build solves this issue for v8/master:
* a1a12ef3b343f9e75c630ed6dc8f1ea44a8a747b

However, the version of '/chromium/src/build.git' cannot be updated to
include this patch in v8/DEPS file. (could potentially cause issues for
other platforms)

The change to deps/v8/src/torque/file-visitor.h is a workaround for
origin/v10.x-staging branch.

PR-URL: #23695
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants