Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

V8 Lateral upgrade - float V8 patch to address infinite loop #14411

Closed
mhdawson opened this issue Apr 2, 2015 · 2 comments
Closed

V8 Lateral upgrade - float V8 patch to address infinite loop #14411

mhdawson opened this issue Apr 2, 2015 · 2 comments
Assignees
@mhdawson
Labels
P-1 v0.12 V8
Milestone
0.12.3

Comments

@mhdawson
Copy link
Member

mhdawson commented Apr 2, 2015

With the lateral upgrade to V8 discussed here: #9185

The following
node --harmony --use-strict -e 'for (let i = 0; i < 3; ++i) { if (i == 1) { continue; } }'

results in an infinite loop.

This is an instance of the problem reported in V8 Issue 3683 - https://code.google.com/p/v8/issues/detail?can=2&q=3683&colspec=ID%20Type%20Status%20Priority%20Owner%20Summary%20HW%20OS%20Area%20Stars&id=3683

The corresponding fix is: https://chromium.googlesource.com/v8/v8.git/+/b17eaaa5755e625493c5fe537f42b58838923c52

Cherry-picking that commit on top of 3.28.71.19 resolves the issue.

In V8 releases, the fix was rolled into 3.31.6:

commit 8b0d1187aaea32f767f0daf152434b3681c192ca
Author: v8-autoroll [email protected]
Date: Fri Nov 14 18:01:38 2014 -0800

Version 3.31.6 (based on 4a4158f3632ba2b18f6525a132891d480ea40bad)

Fix desugaring of let bindings in for loops to handle continue properly (issue 3683).

Performance and stability improvements on all platforms.

Cr-Commit-Position: refs/heads/candidates@{#25280}

Opening this issue to track floating this change once the lateral upgrade is in.

Along with cherry-picking the commit across we also need to add a test.
@mhdawson mhdawson added this to the 0.12.3 milestone Apr 2, 2015
@mhdawson mhdawson added the P-1 label Apr 2, 2015
@mhdawson mhdawson self-assigned this Apr 9, 2015
@misterdjules
Copy link

@mhdawson Just to make sure we're tracking this issue correctly, this floating patch would fix #9113 right?

@misterdjules misterdjules mentioned this issue Apr 28, 2015
Closed
misterdjules pushed a commit to misterdjules/node that referenced this issue Apr 28, 2015
@ajklein
v8: fix issue with let bindings in for loops
ebfd375 
Backport b17eaaa5755e625493c5fe537f42b58838923c52 from upstream v8.
Also add a regression test to make sure subsequent V8 upgrades include
this floating patch if needed.

Original commit message:
  Fix desugaring of let bindings in for loops to handle continue properly

  This requires putting the original loop's body inside an inner for loop (with
  the same labels as the original loop) and re-binding the temp variables in its
  "next" expression. A second flag is added to the desugared code to ensure the
  loop body executes at most once per loop.

  BUG=v8:3683
  LOG=y

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

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

Fixes nodejs#9113 and nodejs#14411.
misterdjules pushed a commit to misterdjules/node that referenced this issue Apr 28, 2015
@ajklein
v8: fix issue with let bindings in for loops
0724602 
Backport b17eaaa5755e625493c5fe537f42b58838923c52 from upstream v8.

Original commit message:
  Fix desugaring of let bindings in for loops to handle continue properly

  This requires putting the original loop's body inside an inner for loop (with
  the same labels as the original loop) and re-binding the temp variables in its
  "next" expression. A second flag is added to the desugared code to ensure the
  loop body executes at most once per loop.

  BUG=v8:3683
  LOG=y

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

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

Fixes nodejs#9113 and nodejs#14411.
@misterdjules misterdjules mentioned this issue Apr 28, 2015
Closed
misterdjules pushed a commit to misterdjules/node that referenced this issue Apr 30, 2015
@ajklein
v8: fix issue with let bindings in for loops
cb8c45f 
Backport b17eaaa5755e625493c5fe537f42b58838923c52 from upstream v8.

Original commit message:
  Fix desugaring of let bindings in for loops to handle continue properly

  This requires putting the original loop's body inside an inner for loop (with
  the same labels as the original loop) and re-binding the temp variables in its
  "next" expression. A second flag is added to the desugared code to ensure the
  loop body executes at most once per loop.

  BUG=v8:3683
  LOG=y

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

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

Fixes nodejs#9113 and nodejs#14411.
misterdjules pushed a commit that referenced this issue Apr 30, 2015
@ajklein
v8: fix issue with let bindings in for loops
2a5f4bd 
Backport b17eaaa5755e625493c5fe537f42b58838923c52 from upstream v8.

Original commit message:
  Fix desugaring of let bindings in for loops to handle continue properly

  This requires putting the original loop's body inside an inner for loop (with
  the same labels as the original loop) and re-binding the temp variables in its
  "next" expression. A second flag is added to the desugared code to ensure the
  loop body executes at most once per loop.

  BUG=v8:3683
  LOG=y

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

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

Fixes #9113 and #14411.

Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: #23948
@misterdjules
Copy link

Fixed in 2a5f4bd and cb55a05

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mhdawson mhdawson

Labels
P-1 v0.12 V8
Projects
None yet
Milestone
0.12.3
Development

No branches or pull requests
2 participants
@misterdjules @mhdawson