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 #23425

Closed

Conversation

V-for-Vasili
Copy link
Contributor

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

Checklist

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 in v8gen.py
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 v10.x v8 engine Issues and PRs related to the V8 dependency. labels Oct 11, 2018
@richardlau
Copy link
Member

@V-for-Vasili Can you follow the backporting section of the Maintaining V8 in Node.js guide: https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md#maintenance-process

At the very least this needs to bump the v8_embedder_string number in common.gypi.

cc @nodejs/v8

@V-for-Vasili
Copy link
Contributor Author

@richardlau Done.

@mhdawson
Copy link
Member

@V-for-Vasili is it possible to add links for the backported changes as opposed to just the hashes. That would make it easier to cross-check.

@refack
Copy link
Contributor

refack commented Oct 12, 2018

It seems like most of the changes are already part of #23424

@mhdawson
Copy link
Member

As some additional context, these fixes are needed to address some edge cases which are exposed by the v8 tests even though we don't see any failures in the Node.js tests.

Unfortunately, we had a gap in our internal v8 testing on AIX (We don't run the v8 tests on AIX right now in the Node.js project CI although we are working on contributing a new machine to OSU that might make that possible). We are now back to having regular runs in our internal IBM CI.

Once we closed the gap in our internal testing it uncovered a few issues that this PR resolves based changes which have already landed in the v8 repos.

@mhdawson
Copy link
Member

@refack correct, but this is to backport to 10.x

@V-for-Vasili
Copy link
Contributor Author

@mhdawson Added links to chromium-review.googlesource.com

@mhdawson
Copy link
Member

I'm ok with the changes being backported, but I think we need to cherry-pick each of the commits as outlined in https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md. They can probably be in the same PR, but I'd expect a commit for each of the V8 commits being backported. @V-for-Vasili see this guide https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md#backporting-to-abandoned-branches

@V-for-Vasili
Copy link
Contributor Author

@mhdawson @refack Created a new PR with cherry-picked commits: #23695
Closing this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants