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.

Sign up for GitHub

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

Jump to bottom

[v10.x] deps: V8: backport 442977e #25242

Closed
wants to merge 1 commit into from
Closed

[v10.x] deps: V8: backport 442977e #25242

wants to merge 1 commit into from

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Dec 28, 2018 •
edited
Loading

Original commit message:

Merged: [wasm] Fix dispatch table instance update

This CL fixes a bug where the receiving instance was updated improperly
in the dispatch table(s) of an imported table.

BUG=chromium:875322
R=​mstarzinger@chromium.org
CC=titzer@chromium.org

Change-Id: Iff24953a1fb6a8ab794e12a7a976d544b56fc3c2
Originally-reviewed-on: https://chromium-review.googlesource.com/1196886
No-Try: true
No-Presubmit: true
No-Treechecks: true
Reviewed-on: https://chromium-review.googlesource.com/1212922
Reviewed-by: Michael Starzinger mstarzinger@chromium.org
Commit-Queue: Clemens Hammacher clemensh@chromium.org
Cr-Commit-Position: refs/branch-heads/6.9@{#45}
Cr-Branched-From: d7b61abe7b48928aed739f02bf7695732d359e7e-refs/heads/6.9.427@{#1}
Cr-Branched-From: b7e108d6016bf6b7de3a34e6d61cb522f5193460-refs/heads/master@{#54504}

Refs: v8/v8@442977e

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1964/ https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1989/
CI: https://ci.nodejs.org/job/node-test-pull-request/19848/ https://ci.nodejs.org/job/node-test-pull-request/19974/

@nodejs-github-bot
Copy link
Collaborator

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

@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 Dec 28, 2018
@ofrobots
Copy link
Contributor Author

@joyeecheung do you have opinions on the strict application of core-validate-commit on quoted text for upstream back-ports? Having to reflow upstream commits in quotes seems overly strict. Thoughts?

@richardlau
Copy link
Member

@joyeecheung do you have opinions on the strict application of core-validate-commit on quoted text for upstream back-ports? Having to reflow upstream commits in quotes seems overly strict. Thoughts?

@ofrobots in theory nodejs/core-validate-commit#30 should have allowed it (there's a specific comment in that PR about V8 backports).

@joyeecheung
Copy link
Member

@ofrobots Have you tried with the latest version of core-validate-commit? (Or update the node_modules of node-core-utils)? What does the failure look like?

@ofrobots
Copy link
Contributor Author

ofrobots commented Jan 29, 2019 •
edited
Loading

Perhaps the CI happened before the latest version of core-validate-commit was available in the builds. Relaunched: https://ci.nodejs.org/job/node-test-pull-request/20395/ https://ci.nodejs.org/job/node-test-pull-request/20396/

@codebytere codebytere force-pushed the v10.x-staging branch 3 times, most recently from 2d6e145 to 7840f71 Compare January 29, 2019 18:12
@ofrobots
Copy link
Contributor Author

@nodejs/build: the FreeBSD build failed compiling a file that was unchanged by this PR. Did we change the compilers on FreeBSD machines? Note that this PR is for v10.x. @nodejs/release as FYI.

@ofrobots
deps: V8: backport 442977e
5a6353f 
Original commit message:

  Merged: [wasm] Fix dispatch table instance update

  This CL fixes a bug where the receiving instance was updated improperly
  in the dispatch table(s) of an imported table.

  BUG=chromium:875322
  R=​mstarzinger@chromium.org
  CC=titzer@chromium.org

  Change-Id: Iff24953a1fb6a8ab794e12a7a976d544b56fc3c2
  Originally-reviewed-on: https://chromium-review.googlesource.com/1196886
  No-Try: true
  No-Presubmit: true
  No-Treechecks: true
  Reviewed-on: https://chromium-review.googlesource.com/1212922
  Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
  Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
  Cr-Commit-Position: refs/branch-heads/6.9@{nodejs#45}
  Cr-Branched-From: d7b61abe7b48928aed739f02bf7695732d359e7e-refs/heads/6.9.427@{#1}
  Cr-Branched-From: b7e108d6016bf6b7de3a34e6d61cb522f5193460-refs/heads/master@{nodejs#54504}

Refs: v8/v8@442977e
@ofrobots
Copy link
Contributor Author

Rebased and launched CI in case this has already been fixed on v10.x: https://ci.nodejs.org/job/node-test-pull-request/20415/

@ofrobots
Copy link
Contributor Author

ofrobots commented Jan 30, 2019 •
edited
Loading

Resume build: https://ci.nodejs.org/job/node-test-pull-request/20430/. Green.

@nodejs/backporters this is ready to land (I understand that only backporters are supposed to land things on -staging branches).

@ofrobots ofrobots mentioned this pull request Feb 1, 2019
Closed
2 tasks
@BethGriggs
Copy link
Member

Could we get a review from @nodejs/v8?

@ofrobots ofrobots requested a review from targos February 4, 2019 17:59
targos
targos approved these changes Feb 4, 2019
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM if V8 CI was green

@BethGriggs
Copy link
Member

As cannot see the original result, rerun of V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2075/ [Green]

@BethGriggs
Copy link
Member

BethGriggs commented Feb 4, 2019 •
edited
Loading

Landed on v10.x-staging in e20e347

@BethGriggs BethGriggs closed this Feb 4, 2019
BethGriggs pushed a commit that referenced this pull request Feb 4, 2019
@ofrobots @BethGriggs
deps: V8: backport 442977e
e20e347 
Original commit message:

  Merged: [wasm] Fix dispatch table instance update

  This CL fixes a bug where the receiving instance was updated improperly
  in the dispatch table(s) of an imported table.

  BUG=chromium:875322
  R=​mstarzinger@chromium.org
  CC=titzer@chromium.org

  Change-Id: Iff24953a1fb6a8ab794e12a7a976d544b56fc3c2
  Originally-reviewed-on: https://chromium-review.googlesource.com/1196886
  No-Try: true
  No-Presubmit: true
  No-Treechecks: true
  Reviewed-on: https://chromium-review.googlesource.com/1212922
  Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
  Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
  Cr-Commit-Position: refs/branch-heads/6.9@{#45}
  Cr-Branched-From: d7b61abe7b48928aed739f02bf7695732d359e7e-refs/heads/6.9.427@{#1}
  Cr-Branched-From: b7e108d6016bf6b7de3a34e6d61cb522f5193460-refs/heads/master@{#54504}

Refs: v8/v8@442977e

PR-URL: #25242
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@ofrobots @rvagg
deps: V8: backport 442977e
fa12b24 
Original commit message:

  Merged: [wasm] Fix dispatch table instance update

  This CL fixes a bug where the receiving instance was updated improperly
  in the dispatch table(s) of an imported table.

  BUG=chromium:875322
  R=​mstarzinger@chromium.org
  CC=titzer@chromium.org

  Change-Id: Iff24953a1fb6a8ab794e12a7a976d544b56fc3c2
  Originally-reviewed-on: https://chromium-review.googlesource.com/1196886
  No-Try: true
  No-Presubmit: true
  No-Treechecks: true
  Reviewed-on: https://chromium-review.googlesource.com/1212922
  Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
  Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
  Cr-Commit-Position: refs/branch-heads/6.9@{#45}
  Cr-Branched-From: d7b61abe7b48928aed739f02bf7695732d359e7e-refs/heads/6.9.427@{#1}
  Cr-Branched-From: b7e108d6016bf6b7de3a34e6d61cb522f5193460-refs/heads/master@{#54504}

Refs: v8/v8@442977e

PR-URL: #25242
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ofrobots @nodejs-github-bot @richardlau @joyeecheung @BethGriggs @targos