Skip to content

deps: backport 7c3748a from upstream V8#10873

Closed
cristiancavalli wants to merge 1 commit intonodejs:v4.x-stagingfrom
cristiancavalli:backport-07d0f6d-0.4x
Closed

deps: backport 7c3748a from upstream V8#10873
cristiancavalli wants to merge 1 commit intonodejs:v4.x-stagingfrom
cristiancavalli:backport-07d0f6d-0.4x

Conversation

@cristiancavalli
Copy link

@cristiancavalli cristiancavalli commented Jan 18, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

deps

V8 Commit:

v8/v8@7c3748a

Description of change

Backport of bugfix from upstream V8

Original commit message:
[debug] load correct stack slot for frame details.

R=bmeurer@chromium.org
BUG=v8:5071

Review URL: https://codereview.chromium.org/2045863002 .

Cr-Commit-Position: refs/heads/master@{#36769}

@nodejs-github-bot nodejs-github-bot added v4.x v8 engine Issues and PRs related to the V8 dependency. labels Jan 18, 2017
@cristiancavalli
Copy link
Author

cristiancavalli commented Jan 18, 2017 •
edited
Loading

cc @ofrobots

@ofrobots
Copy link
Contributor

ofrobots commented Jan 18, 2017

@cristiancavalli I think the fix is also needed on v6.x, correct?

@cristiancavalli
Copy link
Author

cristiancavalli commented Jan 18, 2017

@ofrobots yep, packaging it up as this message submits

bnoordhuis
bnoordhuis approved these changes Jan 18, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but the commit hash in the status line is wrong (should be 7c3748a, not 2bd7464.)

jasnell
jasnell approved these changes Jan 19, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM

@cristiancavalli cristiancavalli changed the title deps: backport 2bd7464 from upstream V8 deps: backport 7c3748a from upstream V8 Jan 19, 2017
@cristiancavalli
Copy link
Author

cristiancavalli commented Jan 19, 2017

commit message fixed

@ofrobots
Copy link
Contributor

ofrobots commented Jan 19, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/5954/
V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/530/

@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 24, 2017

@nodejs/build are we not keeping CI results for at least 10 days?

new ci: https://ci.nodejs.org/job/node-test-pull-request/6020/

v8: https://ci.nodejs.org/job/node-test-commit-v8-linux/537/

@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 24, 2017

Seeing failures for the V8 tests. Running tests again on v4.x-staging to ensure there isn't a regression on the staging branch

https://ci.nodejs.org/job/node-test-commit-v8-linux/538/

@cristiancavalli
Copy link
Author

cristiancavalli commented Jan 31, 2017

Added 'use strict'; declaration to avoid let and const exposure errors in 0.4x

@ofrobots
Copy link
Contributor

ofrobots commented Feb 1, 2017

New V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/550/

@jasnell
Copy link
Member

jasnell commented Feb 2, 2017

@ofrobots ... there's red in that CI.

@ofrobots
Copy link
Contributor

ofrobots commented Feb 8, 2017

Running the debug-backtrace-text test myself with/without the change on top of V8 branch-heads/4.5 passes with d8. Not quite sure why it would fail in the CI. Perhaps we have a floating patch that causes a divergence. I will trying building the V8 in our deps next.

@ofrobots
Copy link
Contributor

ofrobots commented Feb 8, 2017 •
edited
Loading

This is failing because @cristiancavalli 's branch is missing #10732. @cristiancavalli can you rebase this onto v4.x-staging?

(No wonder I had this very strong déjà vu when looking at the test)

@cristiancavalli
deps: backport 7c3748a from upstream V8
6f5c421 
Original commit message:
  [debug] load correct stack slot for frame details.

  R=bmeurer@chromium.org
  BUG=v8:5071

  Review URL: https://codereview.chromium.org/2045863002 .

  Cr-Commit-Position: refs/heads/master@{nodejs#36769}

Signed-off-by: Cristian Cavalli <cristiancavall@google.com>
@cristiancavalli
Copy link
Author

cristiancavalli commented Feb 8, 2017 •
edited
Loading

@ofrobots Thanks for the find; updated accordingly

@cristiancavalli cristiancavalli changed the base branch from v4.x to v4.x-staging February 8, 2017 17:38
@cristiancavalli
Copy link
Author

cristiancavalli commented Feb 8, 2017

My bad on targeting the wrong branch initially

@ofrobots
Copy link
Contributor

ofrobots commented Feb 8, 2017

New V8 CI https://ci.nodejs.org/job/node-test-commit-v8-linux/561/

@ofrobots
Copy link
Contributor

ofrobots commented Feb 8, 2017

CI is green this time! Will land.

ofrobots pushed a commit that referenced this pull request Feb 8, 2017
@cristiancavalli @ofrobots
deps: backport 7c3748a from upstream V8
2be8145 
Original commit message:
  [debug] load correct stack slot for frame details.

  R=bmeurer@chromium.org
  BUG=v8:5071

  Review URL: https://codereview.chromium.org/2045863002 .

  Cr-Commit-Position: refs/heads/master@{#36769}

PR-URL: #10873
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
@ofrobots
Copy link
Contributor

ofrobots commented Feb 8, 2017

Landed on v4.x-staging as 2be8145. Thanks.

@ofrobots ofrobots closed this Feb 8, 2017
MylesBorins pushed a commit that referenced this pull request Feb 22, 2017
@cristiancavalli @MylesBorins
deps: backport 7c3748a from upstream V8
4a077cc 
Original commit message:
  [debug] load correct stack slot for frame details.

  R=bmeurer@chromium.org
  BUG=v8:5071

  Review URL: https://codereview.chromium.org/2045863002 .

  Cr-Commit-Position: refs/heads/master@{#36769}

PR-URL: #10873
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@cristiancavalli @MylesBorins
deps: backport 7c3748a from upstream V8
3516f35 
Original commit message:
  [debug] load correct stack slot for frame details.

  R=bmeurer@chromium.org
  BUG=v8:5071

  Review URL: https://codereview.chromium.org/2045863002 .

  Cr-Commit-Position: refs/heads/master@{#36769}

PR-URL: #10873
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
BethGriggs pushed a commit to ibmruntimes/node that referenced this pull request Mar 22, 2017
@cristiancavalli @BethGriggs
deps: backport 7c3748a from upstream V8
19822d0 
Original commit message:
  [debug] load correct stack slot for frame details.

  R=bmeurer@chromium.org
  BUG=v8:5071

  Review URL: https://codereview.chromium.org/2045863002 .

  Cr-Commit-Position: refs/heads/master@{#36769}

PR-URL: nodejs/node#10873
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

+1 more reviewer

@ofrobots ofrobots ofrobots 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.

6 participants

@cristiancavalli @ofrobots @MylesBorins @jasnell @bnoordhuis @nodejs-github-bot