[debugger] Consider close-by functions when setting a breakpoint

This changes the behavior of SetBreakpointForScript to find more
accurate break positions.

Previously, setting a breakpoint would only consider the shared
function info that contained the requested position for setting a
breakpoint. More intuitively, a breakpoint should not necessarily
be set in a function that contains the position, but in the closest
breakable location that comes after the position we requested.

To achieve this we:
1. find the shared function info of the inner most function
that contains the requested_position.
This function's end position is used to find other shared function
infos in step 2.

2. search for all shared function infos that intersect with the
range [requested_position, inner_most_function.break_position[.

3. From the shared function infos extracted in 2, find the one
that has the closest breakable location to requested_position.

Also-By: bmeurer@chromium.org
Fixed: chromium:1137141
Change-Id: I4f4c6c3aac1ebea50cbcad9543b539ab1ded2b05
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2742198
Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73392}

src/debug/debug.h

@@ -294,8 +294,16 @@ class V8_EXPORT_PRIVATE Debug {
   void RemoveAllCoverageInfos();
 
   // This function is used in FunctionNameUsing* tests.
-  Handle<Object> FindSharedFunctionInfoInScript(Handle<Script> script,
-                                                int position);
+  Handle<Object> FindInnermostContainingFunctionInfo(Handle<Script> script,
+                                                     int position);
+
+  Handle<SharedFunctionInfo> FindClosestSharedFunctionInfoFromPosition(
+      int position, Handle<Script> script,
+      Handle<SharedFunctionInfo> outer_shared);
+
+  bool FindSharedFunctionInfosIntersectingRange(
+      Handle<Script> script, int start_position, int end_position,
+      std::vector<Handle<SharedFunctionInfo>>* candidates);
 
   static Handle<Object> GetSourceBreakLocations(
       Isolate* isolate, Handle<SharedFunctionInfo> shared);

test/cctest/test-func-name-inference.cc

@@ -80,7 +80,8 @@ static void CheckFunctionName(v8::Local<v8::Script> script,
   // Obtain SharedFunctionInfo for the function.
   Handle<SharedFunctionInfo> shared_func_info =
       Handle<SharedFunctionInfo>::cast(
-          isolate->debug()->FindSharedFunctionInfoInScript(i_script, func_pos));
+          isolate->debug()->FindInnermostContainingFunctionInfo(i_script,
+                                                                func_pos));
 
   // Verify inferred function name.
   std::unique_ptr<char[]> inferred_name =

test/debugger/debug/debug-break-class-fields.js

@@ -64,76 +64,82 @@ assertTrue(Debug.showBreakPoints(initializer).indexOf("y = [B0]2;") === -1);
 Debug.clearBreakPoint(b3);
 assertTrue(Debug.showBreakPoints(initializer).indexOf("z = [B1]3") === -1);
 
+// The computed properties are evaluated during class construction,
+// not as part of the initializer function. As a consequence of which,
+// they aren't breakable here in the initializer function, but
+// instead, are part of the enclosing function.
+
 function foo() {}
-var bar = "bar";
+var bar = 'bar';
 
 class X {
   [foo()] = 1;
-  [bar] = 2;
   baz = foo();
 }
 
-// The computed properties are evaluated during class construction,
-// not as part of the initializer function. As a consequence of which,
-// they aren't breakable here in the initializer function, but
-// instead, are part of the enclosing function.
-//
 // class X {
-//   [foo()] = [B0]1;
-//   [bar] = [B1]2;
-//   [baz] = [B2]foo();
+//  [foo()] = 1;
+//  baz = [B0]foo();
 // }
 
 initializer = %GetInitializerFunction(X);
 b1 = Debug.setBreakPoint(initializer, 0, 0);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[foo()] = [B0]1;") === 0);
+assertTrue(Debug.showBreakPoints(initializer).indexOf('[foo()] = 1;') === 0);
 Debug.clearBreakPoint(b1);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[foo()] = [B0]1;") === -1);
 
-b2 = Debug.setBreakPoint(initializer, 1, 0);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[bar] = [B0]2;") > 0);
+b1 = Debug.setBreakPoint(initializer, 1, 0);
+assertTrue(Debug.showBreakPoints(initializer).indexOf('baz = [B0]foo()') > 0);
+Debug.clearBreakPoint(b1);
+
+function t() {
+  class X {
+    [foo()] = 1;
+    [bar] = 2;
+    baz = foo();
+  }
+}
+
+// class X {
+//   [[B0]foo()] = 1;
+//   [[B1]bar] = 2;
+//   baz = foo();
+// }
+
+b1 = Debug.setBreakPoint(t, 2, 0);
+assertTrue(Debug.showBreakPoints(t).indexOf('[[B0]foo()] = 1;') > 0);
+Debug.clearBreakPoint(b1);
+assertTrue(Debug.showBreakPoints(t).indexOf('[[B0]foo()] = 1;') === -1);
+
+b2 = Debug.setBreakPoint(t, 3, 0);
+assertTrue(Debug.showBreakPoints(t).indexOf('[[B0]bar] = 2;') > 0);
 Debug.clearBreakPoint(b2);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[bar] = [B0]2;") === -1);
+assertTrue(Debug.showBreakPoints(t).indexOf('[[B0]bar] = [B0]2;') === -1);
 
-b3 = Debug.setBreakPoint(initializer, 2, 0);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("baz = [B0]foo()") > 0);
+b3 = Debug.setBreakPoint(t, 4, 0);
+assertTrue(Debug.showBreakPoints(t).indexOf('baz = foo()') > 0);
 Debug.clearBreakPoint(b3);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("baz = [B0]foo()") === -1);
 
-b1 = Debug.setBreakPoint(initializer, 0, 0);
-b2 = Debug.setBreakPoint(initializer, 1, 0);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[foo()] = [B0]1;") === 0);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[bar] = [B1]2;") > 0);
+b1 = Debug.setBreakPoint(t, 2, 0);
+b2 = Debug.setBreakPoint(t, 3, 0);
+assertTrue(Debug.showBreakPoints(t).indexOf('[[B0]foo()] = 1;') > 0);
+assertTrue(Debug.showBreakPoints(t).indexOf('[[B1]bar] = 2;') > 0);
 Debug.clearBreakPoint(b1);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[foo()] = [B0]1;") === -1);
+assertTrue(Debug.showBreakPoints(t).indexOf('[[B0]foo()] = 1;') === -1);
 Debug.clearBreakPoint(b2);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[bar] = [B1]2;") === -1);
+assertTrue(Debug.showBreakPoints(t).indexOf('[[B1]bar] = 2;') === -1);
 
-b1 = Debug.setBreakPoint(initializer, 0, 0);
-b3 = Debug.setBreakPoint(initializer, 2, 0);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[foo()] = [B0]1;") === 0);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("baz = [B1]foo()") > 0);
+b1 = Debug.setBreakPoint(t, 2, 0);
+b3 = Debug.setBreakPoint(initializer, 4, 0);
+assertTrue(Debug.showBreakPoints(t).indexOf('[[B0]foo()] = 1;') > 0);
+assertTrue(Debug.showBreakPoints(t).indexOf('baz = foo()') > 0);
 Debug.clearBreakPoint(b1);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[foo()] = [B0]1;") === -1);
+assertTrue(Debug.showBreakPoints(t).indexOf('[[B0]foo()] = 1;') === -1);
 Debug.clearBreakPoint(b3);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("baz = [B1]foo()") === -1);
 
-b2 = Debug.setBreakPoint(initializer, 1, 0);
-b3 = Debug.setBreakPoint(initializer, 2, 0);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[bar] = [B0]2;") > 0);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("baz = [B1]foo()") > 0);
+b2 = Debug.setBreakPoint(t, 3, 0);
+b3 = Debug.setBreakPoint(t, 4, 0);
+assertTrue(Debug.showBreakPoints(t).indexOf('[[B0]bar] = 2;') > 0);
+assertTrue(Debug.showBreakPoints(t).indexOf('baz = foo()') > 0);
 Debug.clearBreakPoint(b2);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[bar] = [B0]2;") === -1);
+assertTrue(Debug.showBreakPoints(t).indexOf('[[B0]bar] = 2;') === -1);
 Debug.clearBreakPoint(b3);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("baz = [B1]foo()") === -1);
-
-function t() {
-  class X {
-    [foo()] = 1;
-  }
-}
-
-b1 = Debug.setBreakPoint(t, 0, 0);
-assertTrue(Debug.showBreakPoints(t).indexOf("[[B0]foo()] = 1;")> 0);
-Debug.clearBreakPoint(b1);
-assertTrue(Debug.showBreakPoints(initializer).indexOf("[[B0]foo()] = 1;") === -1);

test/debugger/debug/es6/debug-step-into-class-extends.js

@@ -34,7 +34,7 @@ function f() {
   class Derived extends GetBase() {} // 0.
 }
 
-var bp = Debug.setBreakPoint(f, 0);
+var bp = Debug.setBreakPoint(f, 1, 20);
 f();
 assertEquals(4, stepCount);
 

test/inspector/debugger/set-breakpoint-breaks-on-first-breakable-location-expected.txt

@@ -2,7 +2,7 @@ Tests if breakpoint set is first breakable location
 Set breakpoint outside of any function: (0, 0).
 Setting breakpoint for id: 3 at 0, 0.
 No breakable location inside a function was found
-Set breakpoint adds a breakpoint at (8, 1).
+Set breakpoint adds a breakpoint at (4, 2).
 Set breakpoint at a breakable location: (4, 17).
 Setting breakpoint for id: 3 at 4, 17.
 Location match for (4, 17).
@@ -10,4 +10,4 @@ Initial location is expected to be breakable: true.
 Set breakpoint at non-breakable location: (7, 0).
 Setting breakpoint for id: 3 at 7, 0.
 Location match for (7, 2).
-Initial location is expected to be breakable: false.
+Initial location is expected to be breakable: false.
\ No newline at end of file

test/inspector/debugger/set-breakpoint-expected.txt

@@ -39,10 +39,6 @@ hitBreakpoints contains breakpoint: true
 
 Set breakpoint at empty line by url in top level function..
 Breakpoint resolved at:
-// last line#
-
-Breakpoint hit at:
-// last line#
-
-hitBreakpoints contains breakpoint: true
+function i2(){#}
+// last line
 

test/inspector/debugger/set-breakpoint-in-class-initializer-expected.txt

+Checks if we can set a breakpoint on a one-line inline functions.
+Setting breakpoint on `class X`
+
+function foo() {}
+var bar = "bar";
+
+class X {
+  constructor() {
+    |_|this.x = 1;
+  }
+  [bar] = 2;
+  baz = foo();
+}
+new X();
+
+Setting breakpoint on constructor, should resolve to same location
+
+function foo() {}
+var bar = "bar";
+
+class X {
+  constructor() {
+    |_|this.x = 1;
+  }
+  [bar] = 2;
+  baz = foo();
+}
+new X();
+
+Setting breakpoint on computed properties in class
+
+function foo() {}
+var bar = "bar";
+
+class X {
+  constructor() {
+    this.x = 1;
+  }
+  [|_|bar] = 2;
+  baz = foo();
+}
+new X();
+
+Setting breakpoint on initializer function
+
+function foo() {}
+var bar = "bar";
+
+class X {
+  constructor() {
+    this.x = 1;
+  }
+  [bar] = 2;
+  baz = |_|foo();
+}
+new X();
+
+Paused on location:
+(anonymous) (testInitializer.js:8:3)
+Paused on location:
+<instance_members_initializer> (testInitializer.js:9:8)
+X (testInitializer.js:5:13)
+(anonymous) (testInitializer.js:11:0)
+Paused on location:
+X (testInitializer.js:6:4)
+(anonymous) (testInitializer.js:11:0)

test/inspector/debugger/set-breakpoint-in-class-initializer.js

+// 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(
+    'Checks if we can set a breakpoint on a one-line inline functions.');
+
+session.setupScriptMap();
+
+const testClassInitializer = `
+function foo() {}
+var bar = "bar";
+
+class X {
+  constructor() {
+    this.x = 1;
+  }
+  [bar] = 2;
+  baz = foo();
+}
+new X();
+//# sourceURL=testInitializer.js`
+
+Protocol.Debugger.enable().then(onDebuggerEnabled);
+
+function onDebuggerEnabled() {
+  Protocol.Runtime.enable();
+  Protocol.Runtime.onExecutionContextCreated(onExecutionContextCreated);
+}
+
+async function onExecutionContextCreated(messageObject) {
+  const executionContextId = messageObject.params.context.id;
+  await runTest(executionContextId, testClassInitializer, 'testInitializer.js');
+  InspectorTest.completeTest();
+}
+
+async function runTest(executionContextId, func, url) {
+  const obj = await Protocol.Runtime.compileScript({
+    expression: func,
+    sourceURL: url,
+    persistScript: true,
+    executionContextId: executionContextId
+  });
+  const scriptId = obj.result.scriptId;
+
+  InspectorTest.log('Setting breakpoint on `class X`');
+  await setBreakpoint(4, 'testInitializer.js');
+
+  InspectorTest.log(
+      'Setting breakpoint on constructor, should resolve to same location');
+  await setBreakpoint(5, 'testInitializer.js');
+
+  InspectorTest.log('Setting breakpoint on computed properties in class');
+  await setBreakpoint(8, 'testInitializer.js');
+
+  InspectorTest.log('Setting breakpoint on initializer function');
+  await setBreakpoint(9, 'testInitializer.js');
+
+  Protocol.Runtime.runScript({scriptId});
+  const numBreaks = 3;
+  for (var i = 0; i < numBreaks; ++i) {
+    const {params: {callFrames}} = await Protocol.Debugger.oncePaused();
+    InspectorTest.log('Paused on location:');
+    session.logCallFrames(callFrames);
+    Protocol.Debugger.resume();
+  }
+
+  InspectorTest.completeTest();
+};
+
+async function setBreakpoint(lineNumber, url) {
+  const {result: {locations}} =
+      await Protocol.Debugger.setBreakpointByUrl({lineNumber, url});
+  await session.logBreakLocations(locations);
+}

test/inspector/debugger/set-breakpoint-inline-function-expected.txt

+Checks if we can set a breakpoint on a one-line inline functions.
+Setting breakpoint
+ function test() {
+    function func(a) {|_|console.log(a);}
+    func("hi");
+  }
+
+Paused on location:
+func (testFunction.js:1:22)
+test (testFunction.js:2:4)
+(anonymous) (:0:0)

test/inspector/debugger/set-breakpoint-inline-function.js

+// 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(
+    'Checks if we can set a breakpoint on a one-line inline functions.');
+
+session.setupScriptMap();
+const testFunction = ` function test() {
+    function func(a) {console.log(a);}
+    func("hi");
+  }
+  //# sourceURL=testFunction.js`;
+
+contextGroup.addScript(testFunction);
+
+(async function testSetBreakpoint() {
+  await Protocol.Debugger.enable();
+  await Protocol.Runtime.enable();
+
+  InspectorTest.log('Setting breakpoint');
+  const {result: {locations}} = await Protocol.Debugger.setBreakpointByUrl(
+      {lineNumber: 1, url: 'testFunction.js'});
+  await session.logBreakLocations(locations);
+
+  Protocol.Runtime.evaluate({expression: 'test()'});
+  const {params: {callFrames}} = await Protocol.Debugger.oncePaused();
+  InspectorTest.log('Paused on location:');
+  session.logCallFrames(callFrames);
+  InspectorTest.completeTest();
+})();

test/inspector/debugger/set-breakpoint.js

@@ -92,22 +92,10 @@ eval('function sourceUrlFunc() { a = 2; }\\n//# sourceURL=sourceUrlScript');`);
     Protocol.Runtime.evaluate({
       expression: `//# sourceURL=test-script\nfunction i1(){};\n\n\n\n\nfunction i2(){}\n// last line`
     });
-    const [{
-      params:{location}
-    }, {
-      params:{
-        callFrames:[topFrame],
-        hitBreakpoints
-      }
-    }] = await Promise.all([
-      Protocol.Debugger.onceBreakpointResolved(),
-      Protocol.Debugger.oncePaused()]);
+    const [{params: {location}}] =
+        await Promise.all([Protocol.Debugger.onceBreakpointResolved()]);
     InspectorTest.log('Breakpoint resolved at:');
     await session.logSourceLocation(location);
-    InspectorTest.log('Breakpoint hit at:');
-    await session.logSourceLocation(topFrame.location);
-    const hitBreakpoint = hitBreakpoints[0] === breakpointId;
-    InspectorTest.log(`hitBreakpoints contains breakpoint: ${hitBreakpoint}\n`);
   }
   await Protocol.Debugger.disable();
   InspectorTest.completeTest();