Skip to content

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

Closed
Drieger wants to merge 1 commit intonodejs:v8.x-stagingfrom
Drieger:backport-22338-to-v8.x
Closed

[v8.x backport] deps: backport 9a23bdd from upstream V8#22418
Drieger wants to merge 1 commit intonodejs:v8.x-stagingfrom
Drieger:backport-22338-to-v8.x

Conversation

@Drieger
Copy link
Copy Markdown
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 <[email protected]>
Commit-Queue: Ross McIlroy <[email protected]>
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 force-pushed the backport-22338-to-v8.x branch from f7e7a28 to 280f095 Compare August 21, 2018 11:02
@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
@Drieger Drieger force-pushed the backport-22338-to-v8.x branch from 280f095 to fad8bc7 Compare August 21, 2018 11:39
@mmarchini
Copy link
Copy Markdown
Contributor

mmarchini commented Aug 21, 2018 •
edited
Loading

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
Copy Markdown
Contributor

mmarchini commented Aug 21, 2018

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

ryzokuken
ryzokuken approved these changes Aug 21, 2018
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Drieger commented Aug 21, 2018

@richardlau updated patch level in v8-version.h

@Drieger Drieger force-pushed the backport-22338-to-v8.x branch from caf2708 to 308fb6e Compare August 21, 2018 16:55
@ofrobots
Copy link
Copy Markdown
Contributor

ofrobots commented Aug 21, 2018

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
Copy Markdown
Contributor

mmarchini commented Aug 22, 2018

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 <[email protected]>
    Commit-Queue: Ross McIlroy <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#54253}

Refs: v8/v8@9a23bdd
Fixes: nodejs#21988
@Drieger Drieger force-pushed the backport-22338-to-v8.x branch from 308fb6e to 94402b0 Compare September 10, 2018 13:34
@mmarchini mmarchini removed the blocked PRs that are blocked by other issues or PRs. label Sep 13, 2018
@mmarchini
Copy link
Copy Markdown
Contributor

mmarchini commented Sep 17, 2018 •
edited
Loading

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 <[email protected]>
    Commit-Queue: Ross McIlroy <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#54253}

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

PR-URL: #22418
Refs: #22338
Reviewed-By: Matheus Marchini <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@BethGriggs
Copy link
Copy Markdown
Member

BethGriggs commented Oct 2, 2018

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

@richardlau richardlau richardlau approved these changes

+2 more reviewers

@mmarchini mmarchini mmarchini approved these changes

@ryzokuken ryzokuken ryzokuken approved these changes

Reviewers whose approvals may not affect merge requirements

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.

7 participants

@Drieger @mmarchini @ofrobots @BethGriggs @richardlau @ryzokuken @nodejs-github-bot