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: cherry-pick 79aee39 from upstream v8 #12412

Merged
merged 1 commit into from
Apr 19, 2017

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Apr 14, 2017

This is a chery-pick if you consider reducing the context to -C2
a cherry-pick; WordIsSmi has been renamed to TaggedIsSmi upstream.

Original commit message:

[builtins] Fix pointer comparison in ToString builtin.

This fixes the bogus {Word32Equal} comparison in the ToString
builtin implementing Object.prototype.toString to be a pointer-size
{WordEqual} comparison instead. Comparing just the lower half-word
is insufficient on 64-bit architectures.

R=jgruber@chromium.org
TEST=mjsunit/regress/regress-crbug-664506
BUG=chromium:664506

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

Node CI: https://ci.nodejs.org/job/node-test-pull-request/7400/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/647/

@nodejs-github-bot nodejs-github-bot added v7.x v8 engine Issues and PRs related to the V8 dependency. labels Apr 14, 2017
@jasnell
Copy link
Member

jasnell commented Apr 17, 2017

The one failure in CI looks unrelated but @bnoordhuis can you please take a look and confirm

@thefourtheye
Copy link
Contributor

@jasnell I think that is seen in #12392 as well

@bnoordhuis
Copy link
Member Author

The one failure in CI looks unrelated but @bnoordhuis can you please take a look and confirm

It's that no member named 'max_align_t' error that happens at random on the ubuntu1204-clang341-64 buildbot. The file it complains about hasn't changed in years.

This is a chery-pick if you consider reducing the context to -C2
a cherry-pick; WordIsSmi has been renamed to TaggedIsSmi upstream.

Original commit message:

    [builtins] Fix pointer comparison in ToString builtin.

    This fixes the bogus {Word32Equal} comparison in the ToString
    builtin implementing Object.prototype.toString to be a pointer-size
    {WordEqual} comparison instead. Comparing just the lower half-word
    is insufficient on 64-bit architectures.

    R=jgruber@chromium.org
    TEST=mjsunit/regress/regress-crbug-664506
    BUG=chromium:664506

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

Fixes: nodejs#12411
PR-URL: nodejs#12412
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@bnoordhuis bnoordhuis merged commit f882f47 into nodejs:v7.x-staging Apr 19, 2017
@bnoordhuis bnoordhuis deleted the fix12411 branch April 19, 2017 08:55
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
This is a chery-pick if you consider reducing the context to -C2
a cherry-pick; WordIsSmi has been renamed to TaggedIsSmi upstream.

Original commit message:

    [builtins] Fix pointer comparison in ToString builtin.

    This fixes the bogus {Word32Equal} comparison in the ToString
    builtin implementing Object.prototype.toString to be a pointer-size
    {WordEqual} comparison instead. Comparing just the lower half-word
    is insufficient on 64-bit architectures.

    R=jgruber@chromium.org
    TEST=mjsunit/regress/regress-crbug-664506
    BUG=chromium:664506

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

Fixes: #12411
PR-URL: #12412
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins
Copy link
Contributor

should this be backported to v6.x?

@bnoordhuis
Copy link
Member Author

No, v4.x and v6.x aren't affected, AFAIK.

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.

6 participants