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

deps: backport 7c3748a from upstream V8 #10873

Closed
wants to merge 1 commit into from
Closed

deps: backport 7c3748a from upstream V8 #10873

wants to merge 1 commit into from

Conversation

cristiancavalli
Copy link

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=[email protected]
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

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

@cristiancavalli
Copy link
Author

@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

commit message fixed

@ofrobots
Copy link
Contributor

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

@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

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

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.

  [email protected]
  BUG=v8:5071

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

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

Signed-off-by: Cristian Cavalli <[email protected]>
@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

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.

  [email protected]
  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 <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]>
@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.

  [email protected]
  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 <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]>
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.

  [email protected]
  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 <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]>
@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.

  [email protected]
  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 <[email protected]>
Reviewed-By: jasnell - James M Snell <[email protected]>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ofrobots ofrobots ofrobots approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell 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.
6 participants
@cristiancavalli @ofrobots @MylesBorins @jasnell @bnoordhuis @nodejs-github-bot