Do not assign positions to parser-generated desugarings.

Do not assign positions to parser-generated desugarings.

The root cause for the bug is that the positions assigned to desugared
code was inconsistent with the source ranges of block scopes.
Since the fact that the position is assigned causes the debugger to
break at the parser-generated statement, the fix is to remove positions
from those nodes that we do not want to break on.

The CL also teaches Hydrogen to tolerate these cases.

R=adamk@chromium.org,rossberg@chromium.org
BUG=chromium:468661
LOG=Y

Committed: https://crrev.com/49c3a606514379e536491ce91fd732161b3c8357
Cr-Commit-Position: refs/heads/master@{#27424}

[Dmitry Lomov (no reviews), 2015-03-24 14:37:51]

ptal

[adamk, 2015-03-24 15:57:41]

Parser changes lgtm, but I have a few questions.

https://codereview.chromium.org/1032653002/diff/1/src/hydrogen.h
File src/hydrogen.h (left):

https://codereview.chromium.org/1032653002/diff/1/src/hydrogen.h#oldcode1871
src/hydrogen.h:1871: DCHECK(position != RelocInfo::kNoPosition);
Was this for/let desugaring really the first place to run into this DCHECK?

https://codereview.chromium.org/1032653002/diff/1/test/mjsunit/es6/regress/re...
File test/mjsunit/es6/regress/regress-468661.js (right):

https://codereview.chromium.org/1032653002/diff/1/test/mjsunit/es6/regress/re...
test/mjsunit/es6/regress/regress-468661.js:1: // Copyright 2014 the V8 project
authors. All rights reserved.
Nit: 2015

https://codereview.chromium.org/1032653002/diff/1/test/mjsunit/es6/regress/re...
test/mjsunit/es6/regress/regress-468661.js:6: // Tests stepping into through
Array.prototype.forEach callbacks
This comment either needs updating or removal.

https://codereview.chromium.org/1032653002/diff/1/test/mjsunit/es6/regress/re...
test/mjsunit/es6/regress/regress-468661.js:67: i++) {
Can you explain why we don't break on this line? I'm not sure I understand,
especially now that you're properly threading through next->position().

[Dmitry Lomov (no reviews), 2015-03-24 16:42:53]

Addressed comments

https://codereview.chromium.org/1032653002/diff/1/src/hydrogen.h
File src/hydrogen.h (left):

https://codereview.chromium.org/1032653002/diff/1/src/hydrogen.h#oldcode1871
src/hydrogen.h:1871: DCHECK(position != RelocInfo::kNoPosition);
On 2015/03/24 15:57:41, adamk wrote:
> Was this for/let desugaring really the first place to run into this DCHECK?


Seems to be. This for/let desugaring added comparisons and hydrogen randomly
calls SetSourcePosition for these nodes. It does not call SetSourcePostition for
other nodes.
The entire notion of source positons is needed for --hydrogen-track-positions
flag, which is mostly unused.

https://codereview.chromium.org/1032653002/diff/1/test/mjsunit/es6/regress/re...
File test/mjsunit/es6/regress/regress-468661.js (right):

https://codereview.chromium.org/1032653002/diff/1/test/mjsunit/es6/regress/re...
test/mjsunit/es6/regress/regress-468661.js:1: // Copyright 2014 the V8 project
authors. All rights reserved.
On 2015/03/24 15:57:41, adamk wrote:
> Nit: 2015

Done.

https://codereview.chromium.org/1032653002/diff/1/test/mjsunit/es6/regress/re...
test/mjsunit/es6/regress/regress-468661.js:6: // Tests stepping into through
Array.prototype.forEach callbacks
On 2015/03/24 15:57:41, adamk wrote:
> This comment either needs updating or removal.

Done.

https://codereview.chromium.org/1032653002/diff/1/test/mjsunit/es6/regress/re...
test/mjsunit/es6/regress/regress-468661.js:67: i++) {
On 2015/03/24 15:57:41, adamk wrote:
> Can you explain why we don't break on this line? I'm not sure I understand,
> especially now that you're properly threading through next->position().

No I do not know why do not break on this line. I know we do not break on this
line when there is no 'let'. This test just codifies the current behavior to
provide regression testing.

[adamk, 2015-03-24 16:48:48]

still lgtm, thanks for the answers

https://codereview.chromium.org/1032653002/diff/1/test/mjsunit/es6/regress/re...
File test/mjsunit/es6/regress/regress-468661.js (right):

https://codereview.chromium.org/1032653002/diff/1/test/mjsunit/es6/regress/re...
test/mjsunit/es6/regress/regress-468661.js:67: i++) {
On 2015/03/24 16:42:52, Dmitry Lomov (chromium) wrote:
> On 2015/03/24 15:57:41, adamk wrote:
> > Can you explain why we don't break on this line? I'm not sure I understand,
> > especially now that you're properly threading through next->position().
> 
> No I do not know why do not break on this line. I know we do not break on this
> line when there is no 'let'. This test just codifies the current behavior to
> provide regression testing.

Yeah, I tried this myself and saw the same behavior without let.

[Dmitry Lomov (no reviews), 2015-03-24 16:50:56]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2015-03-24 16:51:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1032653002/20001

[commit-bot: I haz the power, 2015-03-24 16:52:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-24 16:52:52]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/4319)

[Dmitry Lomov (no reviews), 2015-03-24 16:54:07]

The CQ bit was checked by dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-03-24 16:54:07]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/1032653002/#ps40001
(title: "Rebased for landing")

[commit-bot: I haz the power, 2015-03-24 16:54:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1032653002/40001

[commit-bot: I haz the power, 2015-03-24 17:16:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-24 17:17:00]

Patchset 3 (id:??) landed as
https://crrev.com/49c3a606514379e536491ce91fd732161b3c8357
Cr-Commit-Position: refs/heads/master@{#27424}