From 49c3a606514379e536491ce91fd732161b3c8357 Mon Sep 17 00:00:00 2001 From: dslomov Date: Tue, 24 Mar 2015 10:16:45 -0700 Subject: [PATCH] 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 Review URL: https://codereview.chromium.org/1032653002 Cr-Commit-Position: refs/heads/master@{#27424} --- src/hydrogen.h | 6 +- src/parser.cc | 26 ++++---- test/mjsunit/es6/regress/regress-468661.js | 75 ++++++++++++++++++++++ test/mjsunit/mjsunit.status | 1 + 4 files changed, 94 insertions(+), 14 deletions(-) create mode 100644 test/mjsunit/es6/regress/regress-468661.js diff --git a/src/hydrogen.h b/src/hydrogen.h index f1a6aa1509..268976af3d 100644 --- a/src/hydrogen.h +++ b/src/hydrogen.h @@ -1868,8 +1868,10 @@ class HGraphBuilder { protected: void SetSourcePosition(int position) { - DCHECK(position != RelocInfo::kNoPosition); - position_.set_position(position - start_position_); + if (position != RelocInfo::kNoPosition) { + position_.set_position(position - start_position_); + } + // Otherwise position remains unknown. } void EnterInlinedSource(int start_position, int id) { diff --git a/src/parser.cc b/src/parser.cc index afe51b2882..8ed20ee212 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -3201,7 +3201,6 @@ Statement* Parser::DesugarLexicalBindingsInForStatement( Block* inner_block = factory()->NewBlock(NULL, names->length() + 4, false, RelocInfo::kNoPosition); - int pos = scanner()->location().beg_pos; ZoneList inner_vars(names->length(), zone()); // For each let variable x: @@ -3214,8 +3213,9 @@ Statement* Parser::DesugarLexicalBindingsInForStatement( Declare(declaration, true, CHECK_OK); inner_vars.Add(declaration->proxy()->var(), zone()); VariableProxy* temp_proxy = factory()->NewVariableProxy(temps.at(i)); - Assignment* assignment = factory()->NewAssignment( - is_const ? Token::INIT_CONST : Token::INIT_LET, proxy, temp_proxy, pos); + Assignment* assignment = + factory()->NewAssignment(is_const ? Token::INIT_CONST : Token::INIT_LET, + proxy, temp_proxy, RelocInfo::kNoPosition); Statement* assignment_statement = factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition); proxy->var()->set_initializer_position(init->position()); @@ -3230,8 +3230,8 @@ Statement* Parser::DesugarLexicalBindingsInForStatement( { Expression* const1 = factory()->NewSmiLiteral(1, RelocInfo::kNoPosition); VariableProxy* first_proxy = factory()->NewVariableProxy(first); - compare = - factory()->NewCompareOperation(Token::EQ, first_proxy, const1, pos); + compare = factory()->NewCompareOperation(Token::EQ, first_proxy, const1, + RelocInfo::kNoPosition); } Statement* clear_first = NULL; // Make statement: first = 0. @@ -3243,8 +3243,8 @@ Statement* Parser::DesugarLexicalBindingsInForStatement( clear_first = factory()->NewExpressionStatement(assignment, RelocInfo::kNoPosition); } - Statement* clear_first_or_next = factory()->NewIfStatement( - compare, clear_first, next, RelocInfo::kNoPosition); + Statement* clear_first_or_next = + factory()->NewIfStatement(compare, clear_first, next, next->position()); inner_block->AddStatement(clear_first_or_next, zone()); } @@ -3265,8 +3265,8 @@ Statement* Parser::DesugarLexicalBindingsInForStatement( { Expression* const1 = factory()->NewSmiLiteral(1, RelocInfo::kNoPosition); VariableProxy* flag_proxy = factory()->NewVariableProxy(flag); - flag_cond = - factory()->NewCompareOperation(Token::EQ, flag_proxy, const1, pos); + flag_cond = factory()->NewCompareOperation(Token::EQ, flag_proxy, const1, + RelocInfo::kNoPosition); } // Create chain of expressions "flag = 0, temp_x = x, ..." @@ -3282,9 +3282,11 @@ Statement* Parser::DesugarLexicalBindingsInForStatement( } // Make the comma-separated list of temp_x = x assignments. + int inner_var_proxy_pos = scanner()->location().beg_pos; for (int i = 0; i < names->length(); i++) { VariableProxy* temp_proxy = factory()->NewVariableProxy(temps.at(i)); - VariableProxy* proxy = factory()->NewVariableProxy(inner_vars.at(i), pos); + VariableProxy* proxy = + factory()->NewVariableProxy(inner_vars.at(i), inner_var_proxy_pos); Assignment* assignment = factory()->NewAssignment( Token::ASSIGN, temp_proxy, proxy, RelocInfo::kNoPosition); compound_next = factory()->NewBinaryOperation( @@ -3318,8 +3320,8 @@ Statement* Parser::DesugarLexicalBindingsInForStatement( { Expression* const1 = factory()->NewSmiLiteral(1, RelocInfo::kNoPosition); VariableProxy* flag_proxy = factory()->NewVariableProxy(flag); - compare = - factory()->NewCompareOperation(Token::EQ, flag_proxy, const1, pos); + compare = factory()->NewCompareOperation(Token::EQ, flag_proxy, const1, + RelocInfo::kNoPosition); } Statement* stop = factory()->NewBreakStatement(outer_loop, RelocInfo::kNoPosition); diff --git a/test/mjsunit/es6/regress/regress-468661.js b/test/mjsunit/es6/regress/regress-468661.js new file mode 100644 index 0000000000..e3886ca181 --- /dev/null +++ b/test/mjsunit/es6/regress/regress-468661.js @@ -0,0 +1,75 @@ +// Copyright 2015 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --expose-debug-as debug + +Debug = debug.Debug +var exception = null; +var break_count = 0; + +var expected_values = + [ReferenceError, ReferenceError, 0, 0, 0, 0, 0, 1, ReferenceError, ReferenceError]; + +function listener(event, exec_state, event_data, data) { + try { + if (event == Debug.DebugEvent.Break) { + assertTrue(exec_state.frameCount() != 0, "FAIL: Empty stack trace"); + // Count number of expected breakpoints in this source file. + if (!break_count) { + var source_text = exec_state.frame(0).func().script().source(); + expected_breaks = source_text.match(/\/\/\s*Break\s+\d+\./g).length; + print("Expected breaks: " + expected_breaks); + } + var frameMirror = exec_state.frame(0); + + var v = null;; + try { + v = frameMirror.evaluate('i').value(); + } catch(e) { + v = e; + } + + var source = frameMirror.sourceLineText(); + print("paused at: " + source); + assertTrue(source.indexOf("// Break " + break_count + ".") > 0, + "Unexpected pause at: " + source + "\n" + + "Expected: // Break " + break_count + "."); + if (expected_values[break_count] === ReferenceError) { + assertTrue(v instanceof ReferenceError); + } else { + assertSame(expected_values[break_count], v); + } + ++break_count; + + if (break_count !== expected_breaks) { + exec_state.prepareStep(Debug.StepAction.StepIn, 1); + print("Next step prepared"); + } + } + } catch(e) { + exception = e; + print(e, e.stack); + } +}; + +Debug.setListener(listener); + +var sum = 0; +(function (){ + 'use strict'; + + debugger; // Break 0. + + for (let i=0; // Break 1. + i < 1; // Break 2. // Break 3. // Break 6. // Break 7. + i++) { + let key = i; // Break 4. + sum += key; // Break 5. + } +}()); // Break 8. + +assertNull(exception); // Break 9. +assertEquals(expected_breaks, break_count); + +Debug.setListener(null); diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status index 697c56ef6f..830617bbf8 100644 --- a/test/mjsunit/mjsunit.status +++ b/test/mjsunit/mjsunit.status @@ -113,6 +113,7 @@ 'es6/debug-stepnext-for': [PASS, NO_VARIANTS], 'es6/debug-evaluate-blockscopes': [PASS, NO_VARIANTS], 'regress/regress-crbug-259300': [PASS, NO_VARIANTS], + 'es6/regress/regress-468661': [PASS, NO_VARIANTS], ############################################################################## # Too slow in debug mode with --stress-opt mode.