[liftoff] Test stepping over a recursive call

And fix a few issues revealed by this new test. Incidentally, the test
uses removeBreakpoint which was still untested with Liftoff. But as
expected this seems to work out of the box.

R=clemensb@chromium.org

Bug: v8:10321
Change-Id: Ifa4e867737d925ea8c6c9731575a32f3da3e16dc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2106206
Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66752}

src/debug/debug.cc

@@ -1012,6 +1012,7 @@ void Debug::PrepareStep(StepAction step_action) {
     wasm::WasmCode* code = wasm_frame->wasm_code();
     if (code->is_liftoff()) {
       wasm_frame->native_module()->GetDebugInfo()->PrepareStep(isolate_);
+      return;
     }
   }
   // If this is wasm, but there are no interpreted frames on top, all we can do

src/wasm/wasm-debug.cc

@@ -665,6 +665,9 @@ class DebugInfoImpl {
       breakpoints.insert(insertion_point, offset);
     }
 
+    // No need to recompile if the function is already flooded.
+    if (func_index == flooded_function_index_) return;
+
     RecompileLiftoffWithBreakpoints(func_index, VectorOf(breakpoints),
                                     current_isolate);
   }
@@ -683,19 +686,19 @@ class DebugInfoImpl {
     DCHECK(!it.done());
     DCHECK(it.frame()->is_wasm_compiled());
     WasmCompiledFrame* frame = WasmCompiledFrame::cast(it.frame());
-    if (frame->id() != stepping_frame_) {
+    if (static_cast<int>(frame->function_index()) != flooded_function_index_) {
       FloodWithBreakpoints(frame->native_module(), frame->function_index(),
                            isolate);
-      stepping_frame_ = frame->id();
+      flooded_function_index_ = frame->function_index();
     }
-    stepping_ = true;
+    stepping_frame_ = frame->id();
   }
 
-  void ClearStepping() { stepping_ = false; }
+  void ClearStepping() { stepping_frame_ = NO_ID; }
 
   bool IsStepping(WasmCompiledFrame* frame) {
-    DCHECK_IMPLIES(stepping_, stepping_frame_ != NO_ID);
-    return stepping_;
+    DCHECK_IMPLIES(stepping_frame_ != NO_ID, flooded_function_index_ != -1);
+    return stepping_frame_ == frame->id();
   }
 
   void RemoveDebugSideTables(Vector<WasmCode* const> codes) {
@@ -812,7 +815,7 @@ class DebugInfoImpl {
   // Store the frame ID when stepping, to avoid breaking in recursive calls of
   // the same function.
   StackFrameId stepping_frame_ = NO_ID;
-  bool stepping_ = false;
+  int flooded_function_index_ = -1;
 
   DISALLOW_COPY_AND_ASSIGN(DebugInfoImpl);
 };

test/inspector/debugger/wasm-stepping-liftoff-expected.txt

 Tests stepping through wasm scripts by byte offsets
 Setting up global instance variable.
-Got wasm script: wasm://wasm/befe41aa
-Setting breakpoint on offset 59 (should be propagated to 60, the offset of the call), url wasm://wasm/befe41aa
+Got wasm script: wasm://wasm/42af3c82
+Setting breakpoint on offset 72 (should be propagated to 73, the offset of the call), url wasm://wasm/42af3c82
 {
-    columnNumber : 60
+    columnNumber : 73
     lineNumber : 0
     scriptId : <scriptId>
 }
-Paused at wasm://wasm/befe41aa:0:60
+Paused at wasm://wasm/42af3c82:0:73
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:62
+Paused at wasm://wasm/42af3c82:0:75
 Debugger.resume called
-Paused at wasm://wasm/befe41aa:0:60
+Paused at wasm://wasm/42af3c82:0:73
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:62
+Paused at wasm://wasm/42af3c82:0:75
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:46
+Paused at wasm://wasm/42af3c82:0:59
 Debugger.resume called
-Paused at wasm://wasm/befe41aa:0:60
+Paused at wasm://wasm/42af3c82:0:73
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:62
+Paused at wasm://wasm/42af3c82:0:75
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:46
+Paused at wasm://wasm/42af3c82:0:59
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:48
+Paused at wasm://wasm/42af3c82:0:61
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:50
+Paused at wasm://wasm/42af3c82:0:63
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:52
+Paused at wasm://wasm/42af3c82:0:65
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:54
+Paused at wasm://wasm/42af3c82:0:67
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:55
+Paused at wasm://wasm/42af3c82:0:68
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:57
+Paused at wasm://wasm/42af3c82:0:70
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:60
+Paused at wasm://wasm/42af3c82:0:73
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:62
+Paused at wasm://wasm/42af3c82:0:75
 Debugger.stepOver called
-Paused at wasm://wasm/befe41aa:0:46
+Paused at wasm://wasm/42af3c82:0:59
 Debugger.resume called
 exports.main returned!
+Test stepping over a recursive call
+Setting breakpoint on the recursive call instruction @+93, url wasm://wasm/42af3c82
+{
+    columnNumber : 93
+    lineNumber : 0
+    scriptId : <scriptId>
+}
+Paused at wasm://wasm/42af3c82:0:93
+Removing breakpoint
+Debugger.stepOver called
+Paused at wasm://wasm/42af3c82:0:95
+Debugger.resume called
 Finished!

test/inspector/debugger/wasm-stepping-liftoff.js

@@ -15,7 +15,7 @@ var func_a_idx =
     builder.addFunction('wasm_A', kSig_v_i).addBody([kExprNop, kExprNop]).index;
 
 // wasm_B calls wasm_A <param0> times.
-builder.addFunction('wasm_B', kSig_v_i)
+var func_b = builder.addFunction('wasm_B', kSig_v_i)
     .addBody([
       // clang-format off
       kExprLoop, kWasmStmt,               // while
@@ -34,6 +34,24 @@ builder.addFunction('wasm_B', kSig_v_i)
     ])
     .exportAs('main');
 
+let fact = builder.addFunction('fact', kSig_i_i)
+    .addLocals({i32_count: 1})
+    .addBody([
+    // clang-format off
+    kExprLocalGet, 0,
+    kExprIf, kWasmI32,               // if <param0> != 0
+      kExprLocalGet, 0,
+      kExprI32Const, 1,
+      kExprI32Sub,
+      kExprCallFunction, 2,
+      kExprLocalGet, 0,
+      kExprI32Mul,                   //   return fact(<param0> - 1) * <param0>
+    kExprElse,                       // else
+      kExprI32Const, 1,              //   return 1
+    kExprEnd,
+    // clang-format on
+  ])
+  .exportAs('fact');
 
 var module_bytes = builder.toArray();
 
@@ -65,15 +83,15 @@ function instantiate(bytes) {
 
   // Set the breakpoint on a non-breakable position. This should resolve to the
   // next instruction.
+  var offset = func_b.body_offset + 15;
   InspectorTest.log(
-      `Setting breakpoint on offset 59 (should be propagated to 60, the ` +
-      `offset of the call), url ${wasmScript.url}`);
-  const bpmsg = await Protocol.Debugger.setBreakpoint({
-    location: {scriptId: wasmScript.scriptId, lineNumber: 0, columnNumber: 59}
+      `Setting breakpoint on offset ` + offset + ` (should be propagated to ` +
+        (offset + 1) + `, the offset of the call), url ${wasmScript.url}`);
+  let bpmsg = await Protocol.Debugger.setBreakpoint({
+    location: {scriptId: wasmScript.scriptId, lineNumber: 0, columnNumber: offset}
   });
 
-  const actualLocation = bpmsg.result.actualLocation;
-  InspectorTest.logMessage(actualLocation);
+  InspectorTest.logMessage(bpmsg.result.actualLocation);
   Protocol.Runtime.evaluate({ expression: 'instance.exports.main(4)' });
   await waitForPauseAndStep('stepOver');  // over call to wasm_A
   await waitForPauseAndStep('resume');    // stop on breakpoint
@@ -86,14 +104,39 @@ function instantiate(bytes) {
   // Then just resume.
   await waitForPauseAndStep('resume');
   InspectorTest.log('exports.main returned!');
+
+  InspectorTest.log('Test stepping over a recursive call');
+  // Set a breakpoint at the recursive call and run.
+  offset = fact.body_offset + 9; // Offset of the recursive call instruction.
+  InspectorTest.log(
+      `Setting breakpoint on the recursive call instruction @+` + offset +
+      `, url ${wasmScript.url}`);
+  bpmsg = await Protocol.Debugger.setBreakpoint({
+    location: {scriptId: wasmScript.scriptId, lineNumber: 0, columnNumber: offset}
+  });
+  actualLocation = bpmsg.result.actualLocation;
+  InspectorTest.logMessage(actualLocation);
+  Protocol.Runtime.evaluate({ expression: 'instance.exports.fact(4)' });
+  await waitForPause();
+
+  // Remove the breakpoint before stepping over.
+  InspectorTest.log('Removing breakpoint');
+  let breakpointId = bpmsg.result.breakpointId;
+  await Protocol.Debugger.removeBreakpoint({breakpointId});
+  await Protocol.Debugger.stepOver();
+  await waitForPauseAndStep('resume');
   InspectorTest.log('Finished!');
 })().catch(reason => InspectorTest.log(`Failed: ${reason}`))
     .finally(InspectorTest.completeTest);
 
 async function waitForPauseAndStep(stepAction) {
+  await waitForPause();
+  Protocol.Debugger[stepAction]();
+}
+
+async function waitForPause() {
   const {params: {callFrames}} = await Protocol.Debugger.oncePaused();
   const topFrame = callFrames[0];
   InspectorTest.log(
       `Paused at ${topFrame.url}:${topFrame.location.lineNumber}:${topFrame.location.columnNumber}`);
-  Protocol.Debugger[stepAction]();
 }