[debug][inspector] Use first rather than closest break location.

In case there's no exact match for the breakable location in
SetBreakpoint(), don't try to find the syntactically closest break
location, but rather find the first possible break location in bytecode
order. In particular when trying to set a breakpoint in a line with
for-of or an array destruction, there's no point in going for the
syntactically closest to the beginning of the line, but rather go for
the semantically first, as the intiution for setting a breakpoint on a
line is that the debugger stops before it executes anything on said
line. In the example

```
var [^a, ^b] = ^func();
```

there are three possible break locations, and the correct one is the
last one as the call to func will happen first at runtime.

For generators that's currently broken because of the implicit initial
yield, and same with modules (see crbug.com/901819), so we keep the
previous behavior of finding the closest breakable location, and will
fix that independently in a follow up CL.

Bug: chromium:901819
Fixed: chromium:782461
Also-By: yangguo@chromium.org
Change-Id: Ie724c5cb08e5f4edd90a450d99e001dff06bbe7a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2696586
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72813}

src/debug/debug.cc

@@ -217,20 +217,41 @@ BreakIterator::BreakIterator(Handle<DebugInfo> debug_info)
 }
 
 int BreakIterator::BreakIndexFromPosition(int source_position) {
-  int distance = kMaxInt;
-  int closest_break = break_index();
+  // TODO(crbug.com/901819): When there's no exact match, we
+  // should always pick the first match (in execution order)
+  // to ensure that when setting a breakpoint on a line, we
+  // really break as early as possible in that line. With
+  // generators that's currently broken because of the way
+  // the implicit yield is handled, this will be fixed in
+  // a follow up CL.
+  if (IsGeneratorFunction(debug_info_->shared().kind()) ||
+      IsModule(debug_info_->shared().kind())) {
+    int distance = kMaxInt;
+    int closest_break = break_index();
+    while (!Done()) {
+      int next_position = position();
+      if (source_position <= next_position &&
+          next_position - source_position < distance) {
+        closest_break = break_index();
+        distance = next_position - source_position;
+        if (distance == 0) break;
+      }
+      Next();
+    }
+    return closest_break;
+  }
+  int first_break = break_index();
+  bool first = true;
   while (!Done()) {
     int next_position = position();
-    if (source_position <= next_position &&
-        next_position - source_position < distance) {
-      closest_break = break_index();
-      distance = next_position - source_position;
-      // Check whether we can't get any closer.
-      if (distance == 0) break;
+    if (source_position == next_position) return break_index();
+    if (source_position <= next_position && first) {
+      first_break = break_index();
+      first = false;
     }
     Next();
   }
-  return closest_break;
+  return first_break;
 }
 
 void BreakIterator::Next() {

test/inspector/debugger/destructuring-expected.txt

@@ -0,0 +1,34 @@
+Tests breakable locations in destructuring.
+
+Running test: testBreakLocations
+
+function testFunction() {
+  function func() {
+    |_|return [1, 2];|R|
+  }
+
+  var [|_|a, |_|b] = |C|func();
+|R|}
+
+
+Running test: testSetBreakpoint
+Setting breakpoint at test.js:6:0
+
+  var [a, b] = #func();
+}
+
+Setting breakpoint at test.js:6:7
+
+  var [#a, b] = func();
+}
+
+Setting breakpoint at test.js:6:10
+
+  var [a, #b] = func();
+}
+
+Setting breakpoint at test.js:6:15
+
+  var [a, b] = #func();
+}
+

test/inspector/debugger/destructuring.js

@@ -0,0 +1,47 @@
+// Copyright 2021 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.
+
+let {session, contextGroup, Protocol} =
+  InspectorTest.start('Tests breakable locations in destructuring.');
+
+let source = `
+function testFunction() {
+  function func() {
+    return [1, 2];
+  }
+
+  var [a, b] = func();
+}
+//# sourceURL=test.js`;
+
+contextGroup.addScript(source);
+session.setupScriptMap();
+
+InspectorTest.runAsyncTestSuite([
+  async function testBreakLocations() {
+    Protocol.Debugger.enable();
+    let {params:{scriptId}} = await Protocol.Debugger.onceScriptParsed();
+    let {result:{locations}} = await Protocol.Debugger.getPossibleBreakpoints({
+      start: {lineNumber: 0, columnNumber : 0, scriptId}});
+    await session.logBreakLocations(locations);
+  },
+
+  async function testSetBreakpoint() {
+    const SOURCE_LOCATIONS = [
+      {lineNumber: 6, columnNumber: 0},
+      {lineNumber: 6, columnNumber: 7},
+      {lineNumber: 6, columnNumber: 10},
+      {lineNumber: 6, columnNumber: 15},
+    ];
+    for (const {lineNumber, columnNumber} of SOURCE_LOCATIONS) {
+      const url = 'test.js';
+      InspectorTest.log(`Setting breakpoint at ${url}:${lineNumber}:${columnNumber}`);
+      const {result: {breakpointId, locations}} = await Protocol.Debugger.setBreakpointByUrl({
+        lineNumber, columnNumber, url
+      });
+      locations.forEach(location => session.logSourceLocation(location));
+      await Protocol.Debugger.removeBreakpoint({breakpointId});
+    }
+  }
+]);

test/inspector/debugger/for-of-loops-expected.txt

@@ -2,8 +2,6 @@ Tests breakable locations in for-of loops.
 
 Running test: testBreakLocations
 
-Running test: testStepInto
-
 function testFunction() {
   var obj = |_|{a : 1};
   var arr = |_|[1];
@@ -33,6 +31,8 @@ function testFunction() {
   for (let |C|k of |_|iterable) { all.|C|push(k); }
 |R|}
 
+
+Running test: testStepInto
 (anonymous) (expr.js:0:0)
 
 
@@ -405,3 +405,25 @@ testFunction (test.js:25:11)
           if (this.#i < 1) {
             return { value: this.i++, done: false };
 
+
+Running test: testSetBreakpoint
+Setting breakpoint at test.js:25:0
+  };
+  for (var k of #iterable) { all.push(k); }
+  iterable.i = 0;
+
+Setting breakpoint at test.js:25:11
+  };
+  for (var #k of iterable) { all.push(k); }
+  iterable.i = 0;
+
+Setting breakpoint at test.js:25:16
+  };
+  for (var k of #iterable) { all.push(k); }
+  iterable.i = 0;
+
+Setting breakpoint at test.js:25:28
+  };
+  for (var k of iterable) { all.#push(k); }
+  iterable.i = 0;
+

test/inspector/debugger/for-of-loops.js

@@ -45,7 +45,7 @@ InspectorTest.runAsyncTestSuite([
     let {params:{scriptId}} = await Protocol.Debugger.onceScriptParsed();
     let {result:{locations}} = await Protocol.Debugger.getPossibleBreakpoints({
       start: {lineNumber: 0, columnNumber : 0, scriptId}});
-    session.logBreakLocations(locations);
+    await session.logBreakLocations(locations);
   },
 
   async function testStepInto() {
@@ -65,18 +65,39 @@ InspectorTest.runAsyncTestSuite([
   },
 
   async function testStepIntoAfterBreakpoint() {
-    Protocol.Debugger.setBreakpointByUrl({lineNumber: 25, url: 'test.js'});
+    const {result: {breakpointId}} = await Protocol.Debugger.setBreakpointByUrl({
+      lineNumber: 25, columnNumber: 11, url: 'test.js'
+    });
     Protocol.Runtime.evaluate({
       expression: 'testFunction()//# sourceURL=expr.js'});
     await awaitPausedAndDump();
     Protocol.Debugger.stepInto();
     await awaitPausedAndDump();
     await Protocol.Debugger.resume();
+    await Protocol.Debugger.removeBreakpoint({breakpointId});
 
     async function awaitPausedAndDump() {
       let {params:{callFrames}} = await Protocol.Debugger.oncePaused();
       session.logCallFrames(callFrames);
       session.logSourceLocation(callFrames[0].location);
     }
+  },
+
+  async function testSetBreakpoint() {
+    const SOURCE_LOCATIONS = [
+      {lineNumber: 25, columnNumber: 0},
+      {lineNumber: 25, columnNumber: 11},
+      {lineNumber: 25, columnNumber: 16},
+      {lineNumber: 25, columnNumber: 28},
+    ];
+    for (const {lineNumber, columnNumber} of SOURCE_LOCATIONS) {
+      const url = 'test.js';
+      InspectorTest.log(`Setting breakpoint at ${url}:${lineNumber}:${columnNumber}`);
+      const {result: {breakpointId, locations}} = await Protocol.Debugger.setBreakpointByUrl({
+        lineNumber, columnNumber, url
+      });
+      locations.forEach(location => session.logSourceLocation(location));
+      await Protocol.Debugger.removeBreakpoint({breakpointId});
+    }
   }
 ]);