[debugger] fix break locations for assignments and return.

We used to emit debug break location on block entry. This cannot be
ported to the interpreted as we do not emit bytecode for block entry.
This made no sense to begin with though, but accidentally added
break locations for var declarations.

With this change, the debugger no longer breaks at var declarations
without initialization. This is in accordance with the fact that the
interpreter does not emit bytecode for uninitialized var declarations.

Also fix the bytecode to match full-codegen's behavior wrt return
positions:
- there is a break location before the return statement, with the source
  position of the return statement.
- right before the actual return, there is another break location. The
  source position points to the end of the function.

R=rmcilroy@chromium.org, vogelheim@chromium.org
TBR=rossberg@chromium.org
BUG=v8:4690
LOG=N

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

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

src/debug/debug.cc

@@ -84,6 +84,15 @@ BreakLocation::Iterator::Iterator(Handle<DebugInfo> debug_info)
       position_(1),
       statement_position_(1) {}
 
+int BreakLocation::Iterator::ReturnPosition() {
+  if (debug_info_->shared()->HasSourceCode()) {
+    return debug_info_->shared()->end_position() -
+           debug_info_->shared()->start_position() - 1;
+  } else {
+    return 0;
+  }
+}
+
 BreakLocation::CodeIterator::CodeIterator(Handle<DebugInfo> debug_info,
                                           BreakLocatorType type)
     : Iterator(debug_info),
@@ -137,13 +146,7 @@ void BreakLocation::CodeIterator::Next() {
 
     if (RelocInfo::IsDebugBreakSlotAtReturn(rmode())) {
       // Set the positions to the end of the function.
-      if (debug_info_->shared()->HasSourceCode()) {
-        position_ = debug_info_->shared()->end_position() -
-                    debug_info_->shared()->start_position() - 1;
-      } else {
-        position_ = 0;
-      }
-      statement_position_ = position_;
+      statement_position_ = position_ = ReturnPosition();
     }
 
     break;
@@ -201,8 +204,10 @@ void BreakLocation::BytecodeArrayIterator::Next() {
     if (break_locator_type_ == ALL_BREAK_LOCATIONS) break;
 
     DCHECK_EQ(CALLS_AND_RETURNS, break_locator_type_);
-    if (type == DEBUG_BREAK_SLOT_AT_CALL ||
-        type == DEBUG_BREAK_SLOT_AT_RETURN) {
+    if (type == DEBUG_BREAK_SLOT_AT_CALL) break;
+    if (type == DEBUG_BREAK_SLOT_AT_RETURN) {
+      DCHECK_EQ(ReturnPosition(), position_);
+      DCHECK_EQ(ReturnPosition(), statement_position_);
       break;
     }
   }

src/debug/debug.h

@@ -142,6 +142,7 @@ class BreakLocation {
 
    protected:
     explicit Iterator(Handle<DebugInfo> debug_info);
+    int ReturnPosition();
 
     Handle<DebugInfo> debug_info_;
     int break_index_;

src/full-codegen/full-codegen.cc

@@ -851,7 +851,6 @@ void FullCodeGenerator::VisitForTypeofValue(Expression* expr) {
 void FullCodeGenerator::VisitBlock(Block* stmt) {
   Comment cmnt(masm_, "[ Block");
   NestedBlock nested_block(this, stmt);
-  SetStatementPosition(stmt);
 
   {
     EnterBlockScopeIfNeeded block_scope_state(

src/interpreter/bytecode-array-builder.cc

@@ -67,7 +67,8 @@ class BytecodeArrayBuilder::PreviousBytecodeHelper BASE_EMBEDDED {
 
 BytecodeArrayBuilder::BytecodeArrayBuilder(Isolate* isolate, Zone* zone,
                                            int parameter_count,
-                                           int context_count, int locals_count)
+                                           int context_count, int locals_count,
+                                           FunctionLiteral* literal)
     : isolate_(isolate),
       zone_(zone),
       bytecodes_(zone),
@@ -87,6 +88,9 @@ BytecodeArrayBuilder::BytecodeArrayBuilder(Isolate* isolate, Zone* zone,
   DCHECK_GE(parameter_count_, 0);
   DCHECK_GE(context_register_count_, 0);
   DCHECK_GE(local_register_count_, 0);
+  return_position_ =
+      literal ? std::max(literal->start_position(), literal->end_position() - 1)
+              : RelocInfo::kNoPosition;
 }
 
 BytecodeArrayBuilder::~BytecodeArrayBuilder() { DCHECK_EQ(0, unbound_jumps_); }
@@ -961,6 +965,7 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::ReThrow() {
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::Return() {
+  SetReturnPosition();
   Output(Bytecode::kReturn);
   exit_seen_in_block_ = true;
   return *this;
@@ -1042,10 +1047,9 @@ void BytecodeArrayBuilder::LeaveBasicBlock() {
   exit_seen_in_block_ = false;
 }
 
-void BytecodeArrayBuilder::EnsureReturn(FunctionLiteral* literal) {
+void BytecodeArrayBuilder::EnsureReturn() {
   if (!exit_seen_in_block_) {
     LoadUndefined();
-    SetReturnPosition(literal);
     Return();
   }
   DCHECK(exit_seen_in_block_);
@@ -1176,12 +1180,11 @@ size_t BytecodeArrayBuilder::GetConstantPoolEntry(Handle<Object> object) {
   return constant_array_builder()->Insert(object);
 }
 
-void BytecodeArrayBuilder::SetReturnPosition(FunctionLiteral* fun) {
-  // Don't emit dead code.
+void BytecodeArrayBuilder::SetReturnPosition() {
+  if (return_position_ == RelocInfo::kNoPosition) return;
   if (exit_seen_in_block_) return;
-
-  int pos = std::max(fun->start_position(), fun->end_position() - 1);
-  source_position_table_builder_.AddStatementPosition(bytecodes_.size(), pos);
+  source_position_table_builder_.AddStatementPosition(bytecodes_.size(),
+                                                      return_position_);
 }
 
 void BytecodeArrayBuilder::SetStatementPosition(Statement* stmt) {

src/interpreter/bytecode-array-builder.h

@@ -27,7 +27,8 @@ class Register;
 class BytecodeArrayBuilder final : public ZoneObject, private RegisterMover {
  public:
   BytecodeArrayBuilder(Isolate* isolate, Zone* zone, int parameter_count,
-                       int context_count, int locals_count);
+                       int context_count, int locals_count,
+                       FunctionLiteral* literal = nullptr);
   ~BytecodeArrayBuilder();
 
   Handle<BytecodeArray> ToBytecodeArray();
@@ -257,6 +258,8 @@ class BytecodeArrayBuilder final : public ZoneObject, private RegisterMover {
   // entry, so that it can be referenced by above exception handling support.
   int NewHandlerEntry() { return handler_table_builder()->NewHandlerEntry(); }
 
+  void InitializeReturnPosition(FunctionLiteral* literal);
+
   void SetStatementPosition(Statement* stmt);
   void SetExpressionPosition(Expression* expr);
 
@@ -269,7 +272,7 @@ class BytecodeArrayBuilder final : public ZoneObject, private RegisterMover {
     return &temporary_allocator_;
   }
 
-  void EnsureReturn(FunctionLiteral* literal);
+  void EnsureReturn();
 
  private:
   class PreviousBytecodeHelper;
@@ -334,8 +337,8 @@ class BytecodeArrayBuilder final : public ZoneObject, private RegisterMover {
   bool NeedToBooleanCast();
   bool IsRegisterInAccumulator(Register reg);
 
-  // Set position for implicit return.
-  void SetReturnPosition(FunctionLiteral* fun);
+  // Set position for return.
+  void SetReturnPosition();
 
   // Gets a constant pool entry for the |object|.
   size_t GetConstantPoolEntry(Handle<Object> object);
@@ -371,6 +374,7 @@ class BytecodeArrayBuilder final : public ZoneObject, private RegisterMover {
   int parameter_count_;
   int local_register_count_;
   int context_register_count_;
+  int return_position_;
   TemporaryRegisterAllocator temporary_allocator_;
   RegisterTranslator register_translator_;
 

src/interpreter/bytecode-generator.cc

@@ -571,7 +571,8 @@ Handle<BytecodeArray> BytecodeGenerator::MakeBytecode(CompilationInfo* info) {
   // Initialize bytecode array builder.
   set_builder(new (zone()) BytecodeArrayBuilder(
       isolate(), zone(), info->num_parameters_including_this(),
-      scope()->MaxNestedContextChainLength(), scope()->num_stack_slots()));
+      scope()->MaxNestedContextChainLength(), scope()->num_stack_slots(),
+      info->literal()));
 
   // Initialize the incoming context.
   ContextScope incoming_context(this, scope(), false);
@@ -590,7 +591,7 @@ Handle<BytecodeArray> BytecodeGenerator::MakeBytecode(CompilationInfo* info) {
     MakeBytecodeBody();
   }
 
-  builder()->EnsureReturn(info->literal());
+  builder()->EnsureReturn();
   set_scope(nullptr);
   set_info(nullptr);
   return builder()->ToBytecodeArray();
@@ -877,8 +878,8 @@ void BytecodeGenerator::VisitBreakStatement(BreakStatement* stmt) {
 
 
 void BytecodeGenerator::VisitReturnStatement(ReturnStatement* stmt) {
-  VisitForAccumulatorValue(stmt->expression());
   builder()->SetStatementPosition(stmt);
+  VisitForAccumulatorValue(stmt->expression());
   execution_control()->ReturnAccumulator();
 }
 

src/parsing/pattern-rewriter.cc

@@ -266,7 +266,7 @@ void Parser::PatternRewriter::VisitVariableProxy(VariableProxy* pattern) {
 
     if (initialize != NULL) {
       block_->statements()->Add(
-          factory()->NewExpressionStatement(initialize, RelocInfo::kNoPosition),
+          factory()->NewExpressionStatement(initialize, initialize->position()),
           zone());
     }
   } else if (value != nullptr && (descriptor_->mode == CONST_LEGACY ||
@@ -282,7 +282,8 @@ void Parser::PatternRewriter::VisitVariableProxy(VariableProxy* pattern) {
     DCHECK_NOT_NULL(proxy->var());
     DCHECK_NOT_NULL(value);
     // Add break location for destructured sub-pattern.
-    int pos = IsSubPattern() ? pattern->position() : RelocInfo::kNoPosition;
+    int pos =
+        IsSubPattern() ? pattern->position() : descriptor_->initialization_pos;
     Assignment* assignment =
         factory()->NewAssignment(Token::INIT, proxy, value, pos);
     block_->statements()->Add(
@@ -299,7 +300,8 @@ void Parser::PatternRewriter::VisitVariableProxy(VariableProxy* pattern) {
     // property).
     VariableProxy* proxy = initialization_scope->NewUnresolved(factory(), name);
     // Add break location for destructured sub-pattern.
-    int pos = IsSubPattern() ? pattern->position() : RelocInfo::kNoPosition;
+    int pos =
+        IsSubPattern() ? pattern->position() : descriptor_->initialization_pos;
     Assignment* assignment =
         factory()->NewAssignment(Token::INIT, proxy, value, pos);
     block_->statements()->Add(

src/parsing/rewriter.cc

@@ -355,14 +355,7 @@ bool Rewriter::Rewrite(ParseInfo* info) {
     if (processor.HasStackOverflow()) return false;
 
     if (processor.result_assigned()) {
-      DCHECK(function->end_position() != RelocInfo::kNoPosition);
-      // Set the position of the assignment statement one character past the
-      // source code, such that it definitely is not in the source code range
-      // of an immediate inner scope. For example in
-      //   eval('with ({x:1}) x = 1');
-      // the end position of the function generated for executing the eval code
-      // coincides with the end of the with scope which is the position of '1'.
-      int pos = function->end_position();
+      int pos = RelocInfo::kNoPosition;
       VariableProxy* result_proxy =
           processor.factory()->NewVariableProxy(result, pos);
       Statement* result_statement =

test/cctest/cctest.status

@@ -502,15 +502,12 @@
 ##############################################################################
 ['ignition == True', {
   # TODO(yangguo,4690): Test failures in debugger tests.
-  'test-debug/DebugStepLocals': [FAIL],
   'test-debug/DebugStepKeyedLoadLoop': [FAIL],
   'test-debug/DebugStepKeyedStoreLoop': [FAIL],
   'test-debug/DebugStepIf': [FAIL],
   'test-debug/DebugStepNamedLoadLoop': [FAIL],
-  'test-debug/DebugStepDeclarations': [FAIL],
   'test-debug/BreakPointConstructCallWithGC': [PASS, FAIL],
   'test-debug/DebugStepNamedStoreLoop': [FAIL],
-  'test-debug/DebugStepLinearMixedICs': [FAIL],
   'test-debug/DebugStepSwitch': [FAIL],
   'test-debug/DebugStepWhile': [FAIL],
   'test-debug/DebugStepFor': [FAIL],

test/cctest/test-debug.cc

@@ -2747,7 +2747,7 @@ TEST(DebugStepKeyedLoadLoop) {
   foo->Call(context, env->Global(), kArgc, args).ToLocalChecked();
 
   // With stepping all break locations are hit.
-  CHECK_EQ(45, break_point_hit_count);
+  CHECK_EQ(44, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(env->GetIsolate(), nullptr);
   CheckDebuggerUnloaded(env->GetIsolate());
@@ -2917,7 +2917,7 @@ TEST(DebugStepLinearMixedICs) {
   foo->Call(context, env->Global(), 0, NULL).ToLocalChecked();
 
   // With stepping all break locations are hit.
-  CHECK_EQ(11, break_point_hit_count);
+  CHECK_EQ(10, break_point_hit_count);
 
   v8::Debug::SetDebugEventListener(env->GetIsolate(), nullptr);
   CheckDebuggerUnloaded(env->GetIsolate());
@@ -2964,7 +2964,7 @@ TEST(DebugStepDeclarations) {
   step_action = StepIn;
   break_point_hit_count = 0;
   foo->Call(context, env->Global(), 0, NULL).ToLocalChecked();
-  CHECK_EQ(6, break_point_hit_count);
+  CHECK_EQ(5, break_point_hit_count);
 
   // Get rid of the debug event listener.
   v8::Debug::SetDebugEventListener(env->GetIsolate(), nullptr);
@@ -2998,7 +2998,7 @@ TEST(DebugStepLocals) {
   step_action = StepIn;
   break_point_hit_count = 0;
   foo->Call(context, env->Global(), 0, NULL).ToLocalChecked();
-  CHECK_EQ(6, break_point_hit_count);
+  CHECK_EQ(5, break_point_hit_count);
 
   // Get rid of the debug event listener.
   v8::Debug::SetDebugEventListener(env->GetIsolate(), nullptr);
@@ -3471,14 +3471,14 @@ TEST(DebugConditional) {
   step_action = StepIn;
   break_point_hit_count = 0;
   foo->Call(context, env->Global(), 0, NULL).ToLocalChecked();
-  CHECK_EQ(4, break_point_hit_count);
+  CHECK_EQ(3, break_point_hit_count);
 
   step_action = StepIn;
   break_point_hit_count = 0;
   const int argc = 1;
   v8::Local<v8::Value> argv_true[argc] = {v8::True(isolate)};
   foo->Call(context, env->Global(), argc, argv_true).ToLocalChecked();
-  CHECK_EQ(4, break_point_hit_count);
+  CHECK_EQ(3, break_point_hit_count);
 
   // Get rid of the debug event listener.
   v8::Debug::SetDebugEventListener(isolate, nullptr);

test/mjsunit/debug-step-end-of-script.js

@@ -5,7 +5,7 @@
 // Flags: --expose-debug-as debug --allow-natives-syntax
 
 var Debug = debug.Debug;
-var expected = ["debugger;", "", "debugger;"];
+var expected = ["debugger;", "debugger;"];
 
 function listener(event, exec_state, event_data, data) {
   if (event != Debug.DebugEvent.Break) return;

test/mjsunit/debug-step.js

@@ -64,9 +64,9 @@ function f() {
 // Set a breakpoint on the for statement (line 1).
 bp1 = Debug.setBreakPoint(f, 1);
 
-// Check that performing 1000 steps will make i 499.
+// Check that performing 1000 steps will make i 333.
 var step_count = 1000;
 result = -1;
 f();
-assertEquals(332, result);
+assertEquals(333, result);
 Debug.setListener(null);

test/mjsunit/es6/debug-stepnext-for.js

@@ -91,7 +91,7 @@ print("log:\n"+ JSON.stringify(log));
 // based on other values.
 var expected = [
   // Entry
-  "a2","b2",
+  "a2",
   // Empty for-in-var: get enumerable
   "c16",
   // Empty for-in: get enumerable

test/mjsunit/harmony/debug-step-destructuring-assignment.js

@@ -32,55 +32,55 @@ function f() {
   var a, b, c, d;
   debugger;                                       // B0
   [                                               // B1
-    a,                                            // B3
-    b,                                            // B4
-    c = 3                                         // B5
-  ] = [1, 2];                                     // B2
-  assertEquals({a:1,b:2,c:3}, {a, b, c});         // B6
+    a,                                            // B2
+    b,                                            // B3
+    c = 3                                         // B4
+  ] = [1, 2];
+  assertEquals({a:1,b:2,c:3}, {a, b, c});         // B5
 
-  [                                               // B7
-    a,                                            // B9
+  [                                               // B6
+    a,                                            // B7
     [
-      b,                                          // B10
-      c                                           // B11
+      b,                                          // B8
+      c                                           // B9
     ],
-    d                                             // B12
-  ] = [5, [6, 7], 8];                             // B8
-  assertEquals({a:5,b:6,c:7,d:8}, {a, b, c, d});  // B13
+    d                                             // B10
+  ] = [5, [6, 7], 8];
+  assertEquals({a:5,b:6,c:7,d:8}, {a, b, c, d});  // B11
 
-  [                                               // B14
-    a,                                            // B16
-    b,                                            // B17
-    ...c                                          // B18
-  ] = [1, 2, 3, 4];                               // B15
-  assertEquals({a:1,b:2,c:[3,4]}, {a, b, c});     // B19
+  [                                               // B12
+    a,                                            // B13
+    b,                                            // B14
+    ...c                                          // B15
+  ] = [1, 2, 3, 4];
+  assertEquals({a:1,b:2,c:[3,4]}, {a, b, c});     // B16
 
-  ({                                              // B20
-    a,                                            // B22
-    b,                                            // B23
-    c = 7                                         // B24
-  } = {a: 5, b: 6});                              // B21
-  assertEquals({a:5,b:6,c:7}, {a, b, c});         // B25
+  ({                                              // B17
+    a,                                            // B18
+    b,                                            // B19
+    c = 7                                         // B20
+  } = {a: 5, b: 6});
+  assertEquals({a:5,b:6,c:7}, {a, b, c});         // B21
 
-  ({                                              // B26
-    a,                                            // B28
-    b = return1(),                                // B29
-    c = return1()                                 // B30
-  } = {a: 5, b: 6});                              // B27
-  assertEquals({a:5,b:6,c:1}, {a, b, c});         // B33
+  ({                                              // B22
+    a,                                            // B23
+    b = return1(),                                // B24
+    c = return1()                                 // B25
+  } = {a: 5, b: 6});
+  assertEquals({a:5,b:6,c:1}, {a, b, c});         // B28
 
-  ({                                              // B34
-    x : a,                                        // B36
-    y : b,                                        // B37
-    z : c = 3                                     // B38
-  } = {x: 1, y: 2});                              // B35
-  assertEquals({a:1,b:2,c:3}, {a, b, c});         // B39
-}                                                 // B40
+  ({                                              // B29
+    x : a,                                        // B30
+    y : b,                                        // B31
+    z : c = 3                                     // B32
+  } = {x: 1, y: 2});
+  assertEquals({a:1,b:2,c:3}, {a, b, c});         // B33
+}                                                 // B34
 
 function return1() {
-  return 1;                                       // B31
-}                                                 // B32
+  return 1;                                       // B26
+}                                                 // B27
 
 f();
-Debug.setListener(null);                          // B41
+Debug.setListener(null);                          // B35
 assertNull(exception);

test/mjsunit/harmony/debug-step-destructuring-bind.js

@@ -98,13 +98,13 @@ function test() {
     assertEquals([3, 4, 6], [a, b, c]);           // B45
   }
 
-  var {                                           // B46
-    x: a,                                         // B47
-    y: b = 9                                      // B48
+  var {
+    x: a,                                         // B46
+    y: b = 9                                      // B47
   } = { x: 4 };
-  assertEquals([4, 9], [a, b]);                   // B49
-}                                                 // B50
+  assertEquals([4, 9], [a, b]);                   // B48
+}                                                 // B49
 
 test();
-Debug.setListener(null);                          // B51
+Debug.setListener(null);                          // B50
 assertNull(exception);

test/mjsunit/mjsunit.status

@@ -789,31 +789,18 @@
   'debug-liveedit-literals': [FAIL],
   'debug-liveedit-3': [FAIL],
   'debug-liveedit-1': [FAIL],
-  'debug-step-into-json': [FAIL],
   'debug-liveedit-patch-positions-replace': [FAIL],
-  'debug-step-into-valueof': [FAIL],
   'debug-liveedit-patch-positions': [FAIL],
   'debug-liveedit-stepin': [FAIL],
-  'debug-step-4': [FAIL],
   'debug-liveedit-newsource': [FAIL],
   'debug-liveedit-stack-padding': [FAIL],
-  'debug-stepframe': [FAIL],
-  'debug-negative-break-points': [FAIL],
-  'debug-stepin-accessor': [FAIL],
-  'debug-step-stub-callfunction': [FAIL],
   'debug-liveedit-breakpoints': [FAIL],
   'debug-stepin-accessor-ic': [FAIL],
-  'debug-stepin-builtin': [FAIL],
-  'debug-stepin-foreach': [FAIL],
   'debug-stepnext-do-while': [FAIL],
-  'debug-stepin-builtin-callback-opt': [FAIL],
-  'debug-stepin-function-call': [FAIL],
 
   # TODO(yangguo,4690): Check failure in debug.cc BreakLocation::SetBreakPoint
   # DCHECK(IsDebugBreak() || IsDebuggerStatement());
   'regress/regress-1523': [FAIL],
-  'regress/regress-102153': [FAIL],
-  'regress/regress-2825': [FAIL],
   'regress/regress-crbug-119800': [FAIL],
   'regress/regress-crbug-467180': [FAIL],
   'regress/regress-opt-after-debug-deopt': [FAIL],
@@ -872,8 +859,6 @@
   'regress/regress-4266': [FAIL],
   'regress/regress-crbug-109362': [FAIL],
   'regress/regress-crbug-568477-2': [FAIL],
-  'regress/regress-crbug-568477-3': [FAIL],
-  'regress/regress-crbug-568477-1': [FAIL],
 
   # TODO(rmcilroy, 4680): new ES6 instanceof support
   'harmony/instanceof-es6': [SKIP],

test/mjsunit/regress/regress-crbug-568477-2.js

@@ -8,7 +8,7 @@ var Debug = debug.Debug;
 var expected = ["debugger;",
                 "var x = y;",
                 "new Promise(f).catch(call_f_with_deeper_stack);",
-                "var a = 1;", "", "var a = 1;",
+                "var a = 1;", "var a = 1;",
                 "debugger;",
                 "var x = y;"];