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: V8: cherry-pick 385aa80 #26702

Merged
merged 1 commit into from
Mar 17, 2019
Merged

Conversation

refack
Copy link
Contributor

@refack refack commented Mar 16, 2019

Fixes: #26694

Original commit message:

Correct removal of redundant moves

The logic for removing while iterating is non-standard and
a left over from a previous index based loop. This patch
replaces it with a standard erase based version.

This fixes a runtime crash with MSVC that invalidates the
iterator and then asserts. This also makes the code safe
in case the last move can be redundant.

Change-Id: Ie6990e0d65a3b83a4b7da3e2e89ed4e60a6cd215
Reviewed-on: https://chromium-review.googlesource.com/c/1488762
Reviewed-by: Ben Titzer <titzer@chromium.org>
Commit-Queue: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59868}

Refs: v8/v8@385aa80

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Mar 16, 2019
@refack
Copy link
Contributor Author

refack commented Mar 16, 2019

V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2172/
PR-CI: https://ci.nodejs.org/job/node-test-pull-request/21589/

/CC @nodejs/v8-update @nodejs/platform-windows, since this resolves a regression (and is a backport of a validated change from upstream), I'd like this to be considered for fast-tracking.
Please 👍 here if you concur.

@refack refack added regression Issues related to regressions. fast-track PRs that do not need to wait for 48 hours to land. labels Mar 16, 2019
@refack
Copy link
Contributor Author

refack commented Mar 16, 2019

P.S. @hashseed do you know why this code path is only live in a debug build, and is it relevant for node debugging (i.e. can we just turn it off?)

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 17, 2019
Original commit message:

    Correct removal of redundant moves

    The logic for removing while iterating is non-standard and
    a left over from a previous index based loop. This patch
    replaces it with a standard erase based version.

    This fixes a runtime crash with MSVC that invalidates the
    iterator and then asserts. This also makes the code safe
    in case the last move can be redundant.

    Change-Id: Ie6990e0d65a3b83a4b7da3e2e89ed4e60a6cd215
    Reviewed-on: https://chromium-review.googlesource.com/c/1488762
    Reviewed-by: Ben Titzer <titzer@chromium.org>
    Commit-Queue: Ben Titzer <titzer@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#59868}

Refs: v8/v8@385aa80

PR-URL: nodejs#26702
Fixes: nodejs#26694
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land. regression Issues related to regressions. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debug build fails on Windows: Assertion failed: vector iterators incompatible
7 participants