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

deps: backport 102e3e87e7 from V8 upstream #7442

Merged

Conversation

MylesBorins
Copy link
Contributor

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

deps

Description of change

This commit backports a fix to a crankshaft bug affects arm64 systems.

Original commit message:

[arm64] Fix jssp based spill slot accesses in Crankshaft

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

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

Fixes: #7417

@MylesBorins MylesBorins added v8 engine Issues and PRs related to the V8 dependency. v4.x labels Jun 27, 2016
@MylesBorins
Copy link
Contributor Author

/cc @nodejs/lts @nodejs/v8

This fixes a arm64 bug that has existed on v4.x for the duration of it's lifetime. I'd like to try and get this wrapped into v4.4.7 if possible

ci: https://ci.nodejs.org/job/node-test-pull-request/3093/
V8-ci: https://ci.nodejs.org/job/node-test-commit-v8-linux/153/

@mscdex
Copy link
Contributor

mscdex commented Jun 27, 2016

/cc @nodejs/v8 Is it possible (or does it make sense) to get this upstreamed at some point, even though I'm assuming that that v8 branch is no longer supported?

@MylesBorins
Copy link
Contributor Author

@mscdex I don't believe this branch is supported any longer, in the same way that v5.0.x is no longer supported

we have nodejs/Release#111 open with the V8 team which leads me to believe that @natorion is the person to talk to about this

@mscdex
Copy link
Contributor

mscdex commented Jun 27, 2016

@thealphanerd Ah yes, that LTS issue looks like what I had in mind.

@ofrobots
Copy link
Contributor

The change LGTM, but the V8 tests should be run. The V8-CI has two problems: 1) it doesn't support v4.x and 2) AFAIK it only runs on x86-64. This change in particular needs to be tested on arm64.

@MylesBorins
Copy link
Contributor Author

@ofrobots are you able to test this at all on your own infra? I can dig in a bit tomorrow, but I'm not sure that getting the V8 tests working on our CI for v4.x, specifically on arm64, is something we can get done quickly

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Jun 28, 2016

here is a run on the V8 test ci: https://ci.nodejs.org/job/thealphanerd-test-commit-v8-linux/2/

This is working by rebasing this backport against my WIP branch for backporting the test runner.

edit: it's green!

@jasnell
Copy link
Member

jasnell commented Jun 28, 2016

Lgtm

This commit backports a fix to a crankshaft bug affects arm64 systems.

Original commit message:

    [arm64] Fix jssp based spill slot accesses in Crankshaft

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

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

Fixes: nodejs#7417
PR-URL: nodejs#7442
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins merged commit 207ade7 into nodejs:v4.x-staging Jun 28, 2016
MylesBorins pushed a commit that referenced this pull request Jun 28, 2016
This commit backports a fix to a crankshaft bug affects arm64 systems.

Original commit message:

    [arm64] Fix jssp based spill slot accesses in Crankshaft

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

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

Fixes: #7417
PR-URL: #7442
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jun 28, 2016
MylesBorins pushed a commit that referenced this pull request Jun 28, 2016
This LTS release comes with 89 commits. This includes 46 commits that
are docs related, 11 commits that are test related, 8 commits that are
build related, and 4 commits that are benchmark related.

Notable Changes:

- debugger:
  - All properties of an array (aside from length) can now be printed
    in the repl (cjihrig)
    #6448
- npm:
  - Upgrade npm to 2.15.8 (Rebecca Turner)
    #7412
- stream:
  - Fix for a bug that became more prevalent with the stream changes
    that landed in v4.4.5. (Anna Henningsen)
    #7160
- V8:
  - Fix for a bug in crankshaft that was causing crashes on arm64
    (Myles Borins)
    #7442
  - Add missing classes to postmortem info such as JSMap and JSSet
    (evan.lucas)
    #3792
MylesBorins pushed a commit that referenced this pull request Jun 28, 2016
This LTS release comes with 89 commits. This includes 46 commits that
are docs related, 11 commits that are test related, 8 commits that are
build related, and 4 commits that are benchmark related.

Notable Changes:

- debugger:
  - All properties of an array (aside from length) can now be printed
    in the repl (cjihrig)
    #6448
- npm:
  - Upgrade npm to 2.15.8 (Rebecca Turner)
    #7412
- stream:
  - Fix for a bug that became more prevalent with the stream changes
    that landed in v4.4.5. (Anna Henningsen)
    #7160
- V8:
  - Fix for a bug in crankshaft that was causing crashes on arm64
    (Myles Borins)
    #7442
  - Add missing classes to postmortem info such as JSMap and JSSet
    (evan.lucas)
    #3792
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request Jun 29, 2016
This commit backports a fix to a crankshaft bug affects arm64 systems.

Original commit message:

    [arm64] Fix jssp based spill slot accesses in Crankshaft

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

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

Fixes: nodejs/node#7417
PR-URL: nodejs/node#7442
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins deleted the V8-crankshaft-arm64-bug branch June 29, 2016 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants