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

Jump to bottom

[v8.x backport] deps: backport 9a23bdd from upstream V8 #22418

Closed
wants to merge 1 commit into from
Closed

[v8.x backport] deps: backport 9a23bdd from upstream V8 #22418

wants to merge 1 commit into from

Conversation

Drieger
Copy link
Contributor

@Drieger Drieger commented Aug 20, 2018

Original commit message:

[Isolate] Fix Isolate::PrintCurrentStackTrace for interpreted frames

Previously we were getting the code object from the stack, so printed incorrect
position details for interpreted frames.

BUG=v8:7916

Change-Id: I2f87584117d88b7db3f3b9bdbfe793c4d3e33fe9
Reviewed-on: https://chromium-review.googlesource.com/1126313
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Commit-Queue: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54253}

Refs: v8/v8@9a23bdd
Refs: #22338
Fixes: #21988

Checklist

[x] make -j4 test (UNIX), or vcbuild test (Windows) passes
[x] commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added v8 engine Issues and PRs related to the V8 dependency. v8.x labels Aug 20, 2018
@Drieger Drieger changed the title [v8.x backport] deps: cherry-pick 9a23bdd from upstream V8 [v8.x backport] deps: backport 9a23bdd from upstream V8 Aug 21, 2018
@mmarchini
Copy link
Contributor

mmarchini commented Aug 21, 2018 •
edited

CI: https://ci.nodejs.org/job/node-test-pull-request/16628/ ❌
CI: https://ci.nodejs.org/job/node-test-pull-request/16633/ ❌
CI: https://ci.nodejs.org/job/node-test-pull-request/16642/
CI: https://ci.nodejs.org/job/node-test-pull-request/17294/ ❌
CI: https://ci.nodejs.org/job/node-test-pull-request/17319/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1622/ ✅

@mmarchini
Copy link
Contributor

@nodejs/v8 @nodejs/v8-update @nodejs/release PTAL

ryzokuken
ryzokuken approved these changes Aug 21, 2018
View reviewed changes
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

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

LGTM

richardlau
richardlau requested changes Aug 21, 2018
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

Please bump the patch level version in v8-version.h as per the maintaining V8 guide:

https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md#backporting-to-abandoned-branches

@Drieger
Copy link
Contributor Author

Drieger commented Aug 21, 2018

@richardlau updated patch level in v8-version.h

@ofrobots
Copy link
Contributor

The change LGTM, but the general policy is that we allow fixes to be released on Current first before back-porting to LTS. AFAICT, this has not been released on Current yet.

@mmarchini mmarchini added the blocked PRs that are blocked by other issues or PRs. label Aug 22, 2018
@mmarchini
Copy link
Contributor

Marked as blocked until #22338 lands or https://bugs.chromium.org/p/v8/issues/detail?id=7916 gets backported upstream.

@Drieger Drieger closed this Sep 10, 2018
@Drieger Drieger reopened this Sep 10, 2018
@Drieger
deps: backport 9a23bdd from upstream V8
94402b0 
Original commit message:

    [Isolate] Fix Isolate::PrintCurrentStackTrace for interpreted frames

    Previously we were getting the code object from the stack, so printed incorrect
    position details for interpreted frames.

    BUG=v8:7916

    Change-Id: I2f87584117d88b7db3f3b9bdbfe793c4d3e33fe9
    Reviewed-on: https://chromium-review.googlesource.com/1126313
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Commit-Queue: Ross McIlroy <rmcilroy@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#54253}

Refs: v8/v8@9a23bdd
Fixes: nodejs#21988
@mmarchini mmarchini removed the blocked PRs that are blocked by other issues or PRs. label Sep 13, 2018
@mmarchini
Copy link
Contributor

mmarchini commented Sep 17, 2018 •
edited

v8/v8@9a23bdd landed on master through #21983, which means we can move forward with this PR.

CI: https://ci.nodejs.org/job/node-test-pull-request/17236/ ❌
CI: https://ci.nodejs.org/job/node-test-pull-request/17259/ ❌
CI: https://ci.nodejs.org/job/node-test-pull-request/17326/ ❌
CI: https://ci.nodejs.org/job/node-test-pull-request/17359/ ❌
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1690/ ❌

CI: https://ci.nodejs.org/job/node-test-pull-request/17404/
CI (FreeBSD Rebuild): https://ci.nodejs.org/job/node-test-commit-freebsd/20724/
CI (Linux Rebuild): https://ci.nodejs.org/job/node-test-commit-linux/21811/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1692/ ✅

This was referenced Sep 18, 2018
Closed
Closed
BethGriggs pushed a commit that referenced this pull request Oct 2, 2018
@Drieger @BethGriggs
deps: backport 9a23bdd from upstream V8
9e2077a 
Original commit message:

    [Isolate] Fix Isolate::PrintCurrentStackTrace for interpreted frames

    Previously we were getting the code object from the stack, so printed incorrect
    position details for interpreted frames.

    BUG=v8:7916

    Change-Id: I2f87584117d88b7db3f3b9bdbfe793c4d3e33fe9
    Reviewed-on: https://chromium-review.googlesource.com/1126313
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Commit-Queue: Ross McIlroy <rmcilroy@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#54253}

Refs: v8/v8@9a23bdd
Fixes: #21988

PR-URL: #22418
Refs: #22338
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@BethGriggs
Copy link
Member

Landed in 9e2077a

@BethGriggs BethGriggs closed this Oct 2, 2018
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Merged
@beevelop beevelop mentioned this pull request Nov 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmarchini mmarchini mmarchini approved these changes

@richardlau richardlau richardlau approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

Assignees
No one assigned
Labels
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.

None yet
7 participants
@Drieger @mmarchini @ofrobots @BethGriggs @richardlau @ryzokuken @nodejs-github-bot