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

Cherry pick 588e15c and c0d4bb8 from upstream V8 #8038

Closed
wants to merge 3 commits into from
Closed

Cherry pick 588e15c and c0d4bb8 from upstream V8 #8038

wants to merge 3 commits into from

Conversation

cristiancavalli
Copy link

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

Backport of bugfix from upstream V8.
This is already fixed in versions 5.2 and 5.3 of V8 but is needed for 5.1.

cc @nodejs/v8

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Aug 9, 2016
@ofrobots ofrobots changed the title Cherry pick 588e15c and c0d4bb8 Cherry pick 588e15c and c0d4bb8 from upstream V8 Aug 9, 2016
@MylesBorins
Copy link
Contributor

@cristiancavalli can you bump the patch level in https://github.com/nodejs/node/blob/master/deps/v8/include/v8-version.h

@ofrobots
Copy link
Contributor

ofrobots commented Aug 9, 2016

/cc @nodejs/v8 as the original didn't seem to get linkified.

@cristiancavalli
Copy link
Author

@thealphanerd Bumped!

@ofrobots
Copy link
Contributor

ofrobots commented Aug 9, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/3604/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/262/

@bnoordhuis
Copy link
Member

LGTM but seeing how this only affects the disassembler it's not a very serious bug, is it?

Aside: the commit log for the second commit is a bit confusing in that it back-ports the test wholesale instead of just the fixes (which the commit log implies.) No action required, merely mentioning it for posterity.

@ofrobots
Copy link
Contributor

seeing how this only affects the disassembler it's not a very serious bug, is it?

Actually this is a crash bug in the x32 assembler. See chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=621926.

This was identified as missing from node as we were going through bugs that were deemed important enough for Chromium to backport, but Node.js master was still missing.

@bnoordhuis
Copy link
Member

This PR only contains changes to disasm-ia32.cc, it looks like it's missing the changes to assembler-ia32.cc from v8/v8@588e15c.

epertoso and others added 3 commits August 10, 2016 11:04
@cristiancavalli
deps: cherry-pick 588e15c from V8 upstream
55d2383 
Original commit message:
    Fixes a bug in cmpw.

    The opcodes for 'cmpw r/m16, r16' and 'cmpw r16, r/m16' were
    swapped, causing a few issues when less than/greater than
    comparison were performed.

    Adds a regression test.

    BUG=621926

    Committed: https://crrev.com/efa7095e3e360fbadbe909d831ac11b268ca26b0
    Review-Url: https://codereview.chromium.org/2103713003
    Cr-Original-Commit-Position: refs/heads/master@{nodejs#37339}
    Cr-Commit-Position: refs/heads/master@{nodejs#37345}
@cristiancavalli
deps: cherry-pick c0d4bb8 from V8 upstream
83e427d 
Original commit message:
    Fixes a wrong use of Operand in a test.

    Operand(reg) -> reg
    Operand(reg, 0) -> [reg]

    BUG=

    Review-Url: https://codereview.chromium.org/2111503002
    Cr-Commit-Position: refs/heads/master@{nodejs#37370}
@cristiancavalli
deps: Bump V8 version to 5.1.281.79
8c09bb9
@cristiancavalli
Copy link
Author

cristiancavalli commented Aug 10, 2016 •
edited
Loading

@bnoordhuis Thanks for the spot! PR is updated accordingly.

@fhinkel
Copy link
Member

fhinkel commented Aug 10, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/3612/

@bnoordhuis
Copy link
Member

LGTM. @fhinkel Can you also run the V8 test suite?

@ofrobots
Copy link
Contributor

LGTM, to be squashed at landing time. V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/264/

@mhdawson
Copy link
Member

LGTM

@ofrobots
Copy link
Contributor

Landed as 4dee6bd.

@ofrobots ofrobots closed this Aug 11, 2016
addaleax pushed a commit that referenced this pull request Aug 11, 2016
@addaleax
deps: V8: cherry-pick 588e15c, c0d4bb8
51d45db 
Pick up an upstream bugfix for https://crbug.com/621926 and bump V8
version to 5.1.281.80.

Original commit message for 588e15c:
    Fixes a bug in cmpw.

    The opcodes for 'cmpw r/m16, r16' and 'cmpw r16, r/m16' were
    swapped, causing a few issues when less than/greater than
    comparison were performed.

    Adds a regression test.

    BUG=621926

    Committed: https://crrev.com/efa7095e3e360fbadbe909d831ac11b268ca26b0
    Review-Url: https://codereview.chromium.org/2103713003
    Cr-Original-Commit-Position: refs/heads/master@{#37339}
    Cr-Commit-Position: refs/heads/master@{#37345}

Original commit message for c0d4bb8:
    Fixes a wrong use of Operand in a test.

    Operand(reg) -> reg
    Operand(reg, 0) -> [reg]

    BUG=

    Review-Url: https://codereview.chromium.org/2111503002
    Cr-Commit-Position: refs/heads/master@{#37370}

PR-URL: #8038
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: mhdawson - Michael Dawson <michael_dawson@ca.ibm.com>
@addaleax
Copy link
Member

Had to force-push to master, the commit SHA changed to 51d45db, sorry for the trouble.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 11, 2016

This should not be landed on v6.x, correct?

@ofrobots
Copy link
Contributor

Don't land on v6.x as long as #8054 is pending. I will add it to #8054.

ofrobots pushed a commit to ofrobots/node that referenced this pull request Aug 25, 2016
@ofrobots
deps: V8: cherry-pick 588e15c, c0d4bb8
fc2a89c 
Pick up an upstream bugfix for https://crbug.com/621926 and bump V8
version to 5.1.281.80.

Original commit message for 588e15c:
    Fixes a bug in cmpw.

    The opcodes for 'cmpw r/m16, r16' and 'cmpw r16, r/m16' were
    swapped, causing a few issues when less than/greater than
    comparison were performed.

    Adds a regression test.

    BUG=621926

    Committed: https://crrev.com/efa7095e3e360fbadbe909d831ac11b268ca26b0
    Review-Url: https://codereview.chromium.org/2103713003
    Cr-Original-Commit-Position: refs/heads/master@{nodejs#37339}
    Cr-Commit-Position: refs/heads/master@{nodejs#37345}

Original commit message for c0d4bb8:
    Fixes a wrong use of Operand in a test.

    Operand(reg) -> reg
    Operand(reg, 0) -> [reg]

    BUG=

    Review-Url: https://codereview.chromium.org/2111503002
    Cr-Commit-Position: refs/heads/master@{nodejs#37370}

PR-URL: nodejs#8038
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: mhdawson - Michael Dawson <michael_dawson@ca.ibm.com>
@evanlucas evanlucas mentioned this pull request Aug 26, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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.
9 participants
@cristiancavalli @MylesBorins @ofrobots @bnoordhuis @fhinkel @mhdawson @addaleax @cjihrig @nodejs-github-bot