[debug] Consistent Step-In behavior for generator functions.

This change addresses inconsistencies wrt. to stepping into generator
functions and breaking on the implicit initial yield. The new behavior
is the following:

 1. Stepping into a generator function doesn't trigger "generator
    stepping", but rather pauses right before the initial yield
    (assuming there a no non-simple parameters in between).
 2. When paused on the initial yield and stepping into or over, we also
    don't turn on "generator stepping" immediately, but rather return to
    the caller and only enter "generator stepping" on SuspendGenerator
    bytecodes that correspond to `yield`s or `await`s in the source
    code.

This matches the stepping behavior of regular functions more closely and
seems like a good compromise.

Fixed: chromium:901814
Change-Id: Ifc6c174011df1afea183e2c6ec21de27d72b17a7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2949099
Commit-Queue: Yang Guo <yangguo@chromium.org>
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75066}

src/debug/debug.cc

@@ -311,6 +311,7 @@ BreakLocation BreakIterator::GetBreakLocation() {
       AbstractCode::cast(debug_info_->DebugBytecodeArray()), isolate());
   DebugBreakType type = GetDebugBreakType();
   int generator_object_reg_index = -1;
+  int generator_suspend_id = -1;
   if (type == DEBUG_BREAK_SLOT_AT_SUSPEND) {
     // For suspend break, we'll need the generator object to be able to step
     // over the suspend as if it didn't return. We get the interpreter register
@@ -325,9 +326,13 @@ BreakLocation BreakIterator::GetBreakLocation() {
               interpreter::Bytecode::kSuspendGenerator);
     interpreter::Register generator_obj_reg = iterator.GetRegisterOperand(0);
     generator_object_reg_index = generator_obj_reg.index();
+
+    // Also memorize the suspend ID, to be able to decide whether
+    // we are paused on the implicit initial yield later.
+    generator_suspend_id = iterator.GetUnsignedImmediateOperand(3);
   }
   return BreakLocation(code, type, code_offset(), position_,
-                       generator_object_reg_index);
+                       generator_object_reg_index, generator_suspend_id);
 }
 
 Isolate* BreakIterator::isolate() { return debug_info_->GetIsolate(); }
@@ -510,8 +515,12 @@ void Debug::Break(JavaScriptFrame* frame, Handle<JSFunction> break_target) {
       V8_FALLTHROUGH;
     case StepInto: {
       // Special case StepInto and StepOver for generators that are about to
-      // suspend.
-      if (location.IsSuspend()) {
+      // suspend, in which case we go into "generator stepping" mode. The
+      // exception here is the initial implicit yield in generators (which
+      // always has a suspend ID of 0), where we return to the caller first,
+      // instead of triggering "generator stepping" mode straight away.
+      if (location.IsSuspend() && (!IsGeneratorFunction(shared->kind()) ||
+                                   location.generator_suspend_id() > 0)) {
         DCHECK(!has_suspended_generator());
         thread_local_.suspended_generator_ =
             location.GetGeneratorObjectForSuspendedFrame(frame);
@@ -1065,7 +1074,9 @@ void Debug::PrepareStep(StepAction step_action) {
     // Any step at a return is a step-out, and a step-out at a suspend behaves
     // like a return.
     if (location.IsReturn() ||
-        (location.IsSuspend() && step_action == StepOut)) {
+        (location.IsSuspend() &&
+         (step_action == StepOut || (IsGeneratorFunction(shared->kind()) &&
+                                     location.generator_suspend_id() == 0)))) {
       // On StepOut we'll ignore our further calls to current function in
       // PrepareStepIn callback.
       if (last_step_action() == StepOut) {

src/debug/debug.h

@@ -16,6 +16,7 @@
 #include "src/execution/isolate.h"
 #include "src/handles/handles.h"
 #include "src/objects/debug-objects.h"
+#include "src/objects/shared-function-info.h"
 
 namespace v8 {
 namespace internal {
@@ -83,6 +84,7 @@ class BreakLocation {
 
   bool HasBreakPoint(Isolate* isolate, Handle<DebugInfo> debug_info) const;
 
+  inline int generator_suspend_id() { return generator_suspend_id_; }
   inline int position() const { return position_; }
 
   debug::BreakLocationType type() const;
@@ -92,12 +94,14 @@ class BreakLocation {
 
  private:
   BreakLocation(Handle<AbstractCode> abstract_code, DebugBreakType type,
-                int code_offset, int position, int generator_obj_reg_index)
+                int code_offset, int position, int generator_obj_reg_index,
+                int generator_suspend_id)
       : abstract_code_(abstract_code),
         code_offset_(code_offset),
         type_(type),
         position_(position),
-        generator_obj_reg_index_(generator_obj_reg_index) {
+        generator_obj_reg_index_(generator_obj_reg_index),
+        generator_suspend_id_(generator_suspend_id) {
     DCHECK_NE(NOT_DEBUG_BREAK, type_);
   }
 
@@ -105,7 +109,8 @@ class BreakLocation {
       : code_offset_(0),
         type_(type),
         position_(position),
-        generator_obj_reg_index_(0) {}
+        generator_obj_reg_index_(0),
+        generator_suspend_id_(-1) {}
 
   static int BreakIndexFromCodeOffset(Handle<DebugInfo> debug_info,
                                       Handle<AbstractCode> abstract_code,
@@ -119,6 +124,7 @@ class BreakLocation {
   DebugBreakType type_;
   int position_;
   int generator_obj_reg_index_;
+  int generator_suspend_id_;
 
   friend class BreakIterator;
 };

test/inspector/debugger/step-into-expected.txt

@@ -682,6 +682,31 @@ function testGenerator() {
   var gen = #idMaker();
   return42();
 
+break at:
+
+function* idMaker#() {
+  yield 1;
+
+break at:
+  var gen = idMaker();
+  #return42();
+  gen.next().value;
+
+break at:
+function return42() {
+  #return 42;
+}
+
+break at:
+function return42() {
+  return 42;#
+}
+
+break at:
+  return42();
+  gen.#next().value;
+  debugger;
+
 break at:
 function* idMaker() {
   #yield 1;

test/inspector/debugger/stepping-async-generator-expected.txt

+Async generator stepping
+
+Running test: testStepOverFromInitialYield
+Setting breakpoint on implicit initial yield
+Calling callGenerator()
+
+async function* generator#() {
+  var a = 42;
+
+Stepping over while paused on the initial yield
+function callGenerator() {
+  return generator();#
+}
+
+
+Running test: testStepIntoInitialYield
+Setting breakpoint on call to generator()
+Calling callGenerator()
+function callGenerator() {
+  #return generator();
+}
+
+Stepping into the generator()
+
+async function* generator#() {
+  var a = 42;
+
+Stepping into while paused on the initial yield
+function callGenerator() {
+  return generator();#
+}
+

test/inspector/debugger/stepping-async-generator.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('Async generator stepping');
+
+const url = 'stepping-async-generator.js';
+contextGroup.addScript(`
+async function* generator() {
+  var a = 42;
+  yield a;
+}
+function callGenerator() {
+  return generator();
+}
+`, 0, 0, url);
+
+session.setupScriptMap();
+
+InspectorTest.runAsyncTestSuite([
+  async function testStepOverFromInitialYield() {
+    await Promise.all([Protocol.Debugger.enable(), Protocol.Runtime.enable()]);
+    InspectorTest.log(`Setting breakpoint on implicit initial yield`);
+    const {result: {breakpointId}} = await Protocol.Debugger.setBreakpointByUrl({
+      url,
+      lineNumber: 1,
+      columnNumber: 0,
+    })
+    InspectorTest.log(`Calling callGenerator()`);
+    const pausedPromise = Protocol.Debugger.oncePaused();
+    const evalPromise = Protocol.Runtime.evaluate({expression: 'callGenerator()'});
+    const {method, params} = await Promise.race([pausedPromise, evalPromise]);
+    if (method === 'Debugger.paused') {
+      await session.logSourceLocation(params.callFrames[0].location);
+
+      InspectorTest.log('Stepping over while paused on the initial yield');
+      const [{params: {callFrames:[{location}]}}] = await Promise.all([
+        Protocol.Debugger.oncePaused(),
+        Protocol.Debugger.stepOver(),
+      ]);
+      await session.logSourceLocation(location);
+
+      await Promise.all([Protocol.Debugger.resume(), evalPromise]);
+    } else {
+      InspectorTest.log('Did not pause');
+    }
+    await Protocol.Debugger.removeBreakpoint({breakpointId});
+    await Promise.all([Protocol.Debugger.disable(), Protocol.Runtime.disable()]);
+  },
+
+  async function testStepIntoInitialYield() {
+    await Promise.all([Protocol.Debugger.enable(), Protocol.Runtime.enable()]);
+    InspectorTest.log(`Setting breakpoint on call to generator()`);
+    const {result: {breakpointId}} = await Protocol.Debugger.setBreakpointByUrl({
+      url,
+      lineNumber: 5,
+      columnNumber: 0,
+    })
+    InspectorTest.log(`Calling callGenerator()`);
+    const pausedPromise = Protocol.Debugger.oncePaused();
+    const evalPromise = Protocol.Runtime.evaluate({expression: 'callGenerator()'});
+    const {method, params} = await Promise.race([pausedPromise, evalPromise]);
+    if (method === 'Debugger.paused') {
+      await session.logSourceLocation(params.callFrames[0].location);
+
+      InspectorTest.log('Stepping into the generator()');
+      let [{params: {callFrames:[{location}]}}] = await Promise.all([
+        Protocol.Debugger.oncePaused(),
+        Protocol.Debugger.stepInto(),
+      ]);
+      await session.logSourceLocation(location);
+
+      InspectorTest.log('Stepping into while paused on the initial yield');
+      ([{params: {callFrames:[{location}]}}] = await Promise.all([
+        Protocol.Debugger.oncePaused(),
+        Protocol.Debugger.stepInto(),
+      ]));
+      await session.logSourceLocation(location);
+
+      await Promise.all([Protocol.Debugger.resume(), evalPromise]);
+    } else {
+      InspectorTest.log('Did not pause');
+    }
+    await Protocol.Debugger.removeBreakpoint({breakpointId});
+    await Promise.all([Protocol.Debugger.disable(), Protocol.Runtime.disable()]);
+  }
+]);

test/inspector/debugger/stepping-generator-expected.txt

+Generator stepping
+
+Running test: testStepOverFromInitialYield
+Setting breakpoint on implicit initial yield
+Calling callGenerator()
+
+function* generator#() {
+  var a = 42;
+
+Stepping over while paused on the initial yield
+function callGenerator() {
+  return generator();#
+}
+
+
+Running test: testStepIntoInitialYield
+Setting breakpoint on call to generator()
+Calling callGenerator()
+function callGenerator() {
+  #return generator();
+}
+
+Stepping into the generator()
+
+function* generator#() {
+  var a = 42;
+
+Stepping into while paused on the initial yield
+function callGenerator() {
+  return generator();#
+}
+

test/inspector/debugger/stepping-generator-parameters-expected.txt

+Generator stepping with non-simple parameters
+
+Running test: testStepOverFromInitialYield
+Setting breakpoint on implicit initial yield
+Calling callGenerator()
+
+function* generator(a = (x => x)(42)) #{
+  yield a;
+
+Stepping over while paused on the initial yield
+function callGenerator() {
+  return generator(1);#
+}
+
+
+Running test: testStepIntoInitialYield
+Setting breakpoint on call to generator()
+Calling callGenerator()
+function callGenerator() {
+  #return generator(1);
+}
+
+Stepping into the generator()
+
+function* generator(a = (x => x)(42)) #{
+  yield a;
+
+Stepping into while paused on the initial yield
+function callGenerator() {
+  return generator(1);#
+}
+

test/inspector/debugger/stepping-generator-parameters.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('Generator stepping with non-simple parameters');
+
+const url = 'stepping-generator.js';
+contextGroup.addScript(`
+function* generator(a = (x => x)(42)) {
+  yield a;
+}
+function callGenerator() {
+  return generator(1);
+}
+`, 0, 0, url);
+
+session.setupScriptMap();
+
+InspectorTest.runAsyncTestSuite([
+  async function testStepOverFromInitialYield() {
+    await Promise.all([Protocol.Debugger.enable(), Protocol.Runtime.enable()]);
+    InspectorTest.log(`Setting breakpoint on implicit initial yield`);
+    const {result: {breakpointId}} = await Protocol.Debugger.setBreakpointByUrl({
+      url,
+      lineNumber: 1,
+      columnNumber: 38,
+    })
+    InspectorTest.log(`Calling callGenerator()`);
+    const pausedPromise = Protocol.Debugger.oncePaused();
+    const evalPromise = Protocol.Runtime.evaluate({expression: 'callGenerator()'});
+    const {method, params} = await Promise.race([pausedPromise, evalPromise]);
+    if (method === 'Debugger.paused') {
+      await session.logSourceLocation(params.callFrames[0].location);
+
+      InspectorTest.log('Stepping over while paused on the initial yield');
+      const [{params: {callFrames:[{location}]}}] = await Promise.all([
+        Protocol.Debugger.oncePaused(),
+        Protocol.Debugger.stepOver(),
+      ]);
+      await session.logSourceLocation(location);
+
+      await Promise.all([Protocol.Debugger.resume(), evalPromise]);
+    } else {
+      InspectorTest.log('Did not pause');
+    }
+    await Protocol.Debugger.removeBreakpoint({breakpointId});
+    await Promise.all([Protocol.Debugger.disable(), Protocol.Runtime.disable()]);
+  },
+
+  async function testStepIntoInitialYield() {
+    await Promise.all([Protocol.Debugger.enable(), Protocol.Runtime.enable()]);
+    InspectorTest.log(`Setting breakpoint on call to generator()`);
+    const {result: {breakpointId}} = await Protocol.Debugger.setBreakpointByUrl({
+      url,
+      lineNumber: 5,
+      columnNumber: 0,
+    })
+    InspectorTest.log(`Calling callGenerator()`);
+    const pausedPromise = Protocol.Debugger.oncePaused();
+    const evalPromise = Protocol.Runtime.evaluate({expression: 'callGenerator()'});
+    const {method, params} = await Promise.race([pausedPromise, evalPromise]);
+    if (method === 'Debugger.paused') {
+      await session.logSourceLocation(params.callFrames[0].location);
+
+      InspectorTest.log('Stepping into the generator()');
+      let [{params: {callFrames:[{location}]}}] = await Promise.all([
+        Protocol.Debugger.oncePaused(),
+        Protocol.Debugger.stepInto(),
+      ]);
+      await session.logSourceLocation(location);
+
+      InspectorTest.log('Stepping into while paused on the initial yield');
+      ([{params: {callFrames:[{location}]}}] = await Promise.all([
+        Protocol.Debugger.oncePaused(),
+        Protocol.Debugger.stepInto(),
+      ]));
+      await session.logSourceLocation(location);
+
+      await Promise.all([Protocol.Debugger.resume(), evalPromise]);
+    } else {
+      InspectorTest.log('Did not pause');
+    }
+    await Protocol.Debugger.removeBreakpoint({breakpointId});
+    await Promise.all([Protocol.Debugger.disable(), Protocol.Runtime.disable()]);
+  }
+]);

test/inspector/debugger/stepping-generator.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('Generator stepping');
+
+const url = 'stepping-generator.js';
+contextGroup.addScript(`
+function* generator() {
+  var a = 42;
+  yield a;
+}
+function callGenerator() {
+  return generator();
+}
+`, 0, 0, url);
+
+session.setupScriptMap();
+
+InspectorTest.runAsyncTestSuite([
+  async function testStepOverFromInitialYield() {
+    await Promise.all([Protocol.Debugger.enable(), Protocol.Runtime.enable()]);
+    InspectorTest.log(`Setting breakpoint on implicit initial yield`);
+    const {result: {breakpointId}} = await Protocol.Debugger.setBreakpointByUrl({
+      url,
+      lineNumber: 1,
+      columnNumber: 0,
+    })
+    InspectorTest.log(`Calling callGenerator()`);
+    const pausedPromise = Protocol.Debugger.oncePaused();
+    const evalPromise = Protocol.Runtime.evaluate({expression: 'callGenerator()'});
+    const {method, params} = await Promise.race([pausedPromise, evalPromise]);
+    if (method === 'Debugger.paused') {
+      await session.logSourceLocation(params.callFrames[0].location);
+
+      InspectorTest.log('Stepping over while paused on the initial yield');
+      const [{params: {callFrames:[{location}]}}] = await Promise.all([
+        Protocol.Debugger.oncePaused(),
+        Protocol.Debugger.stepOver(),
+      ]);
+      await session.logSourceLocation(location);
+
+      await Promise.all([Protocol.Debugger.resume(), evalPromise]);
+    } else {
+      InspectorTest.log('Did not pause');
+    }
+    await Protocol.Debugger.removeBreakpoint({breakpointId});
+    await Promise.all([Protocol.Debugger.disable(), Protocol.Runtime.disable()]);
+  },
+
+  async function testStepIntoInitialYield() {
+    await Promise.all([Protocol.Debugger.enable(), Protocol.Runtime.enable()]);
+    InspectorTest.log(`Setting breakpoint on call to generator()`);
+    const {result: {breakpointId}} = await Protocol.Debugger.setBreakpointByUrl({
+      url,
+      lineNumber: 5,
+      columnNumber: 0,
+    })
+    InspectorTest.log(`Calling callGenerator()`);
+    const pausedPromise = Protocol.Debugger.oncePaused();
+    const evalPromise = Protocol.Runtime.evaluate({expression: 'callGenerator()'});
+    const {method, params} = await Promise.race([pausedPromise, evalPromise]);
+    if (method === 'Debugger.paused') {
+      await session.logSourceLocation(params.callFrames[0].location);
+
+      InspectorTest.log('Stepping into the generator()');
+      let [{params: {callFrames:[{location}]}}] = await Promise.all([
+        Protocol.Debugger.oncePaused(),
+        Protocol.Debugger.stepInto(),
+      ]);
+      await session.logSourceLocation(location);
+
+      InspectorTest.log('Stepping into while paused on the initial yield');
+      ([{params: {callFrames:[{location}]}}] = await Promise.all([
+        Protocol.Debugger.oncePaused(),
+        Protocol.Debugger.stepInto(),
+      ]));
+      await session.logSourceLocation(location);
+
+      await Promise.all([Protocol.Debugger.resume(), evalPromise]);
+    } else {
+      InspectorTest.log('Did not pause');
+    }
+    await Protocol.Debugger.removeBreakpoint({breakpointId});
+    await Promise.all([Protocol.Debugger.disable(), Protocol.Runtime.disable()]);
+  }
+]);