[liftoff][debug] Fix step in from JS

When stepping in from JS, the stepping frame ID will not be set.
Instead of ensuring to set it properly, we can just skip the check for
the frame ID. It was needed before, when we didn't properly reset
stepping information. Now, it's redundant anyway.

Also, ensure that we don't redirect to the interpreter if the
--debug-in-liftoff flag is set.

Drive-by: Fix and clang-format some parts of the test (no semantic
change).

R=thibaudm@chromium.org, szuend@chromium.org

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

src/debug/debug.cc

@@ -872,8 +872,9 @@ void Debug::PrepareStepIn(Handle<JSFunction> function) {
   if (in_debug_scope()) return;
   if (break_disabled()) return;
   Handle<SharedFunctionInfo> shared(function->shared(), isolate_);
-  // If stepping from JS into Wasm, prepare for it.
-  if (shared->HasWasmExportedFunctionData()) {
+  // If stepping from JS into Wasm, and we are using the wasm interpreter for
+  // debugging, prepare the interpreter for step in.
+  if (shared->HasWasmExportedFunctionData() && !FLAG_debug_in_liftoff) {
     auto imported_function = Handle<WasmExportedFunction>::cast(function);
     Handle<WasmInstanceObject> wasm_instance(imported_function->instance(),
                                              isolate_);
@@ -1053,7 +1054,8 @@ void Debug::PrepareStep(StepAction step_action) {
     wasm::WasmCodeRefScope code_ref_scope;
     wasm::WasmCode* code = wasm_frame->wasm_code();
     if (code->is_liftoff()) {
-      wasm_frame->native_module()->GetDebugInfo()->PrepareStep(isolate_);
+      wasm_frame->native_module()->GetDebugInfo()->PrepareStep(isolate_,
+                                                               frame_id);
     }
     // In case the wasm code returns, prepare the next frame (if JS) to break.
     step_action = StepOut;

src/wasm/wasm-debug.cc

@@ -754,8 +754,8 @@ class DebugInfoImpl {
                                     current_isolate);
   }
 
-  void PrepareStep(Isolate* isolate) {
-    StackTraceFrameIterator it(isolate);
+  void PrepareStep(Isolate* isolate, StackFrameId break_frame_id) {
+    StackTraceFrameIterator it(isolate, break_frame_id);
     DCHECK(!it.done());
     DCHECK(it.frame()->is_wasm_compiled());
     WasmCompiledFrame* frame = WasmCompiledFrame::cast(it.frame());
@@ -787,8 +787,7 @@ class DebugInfoImpl {
   bool IsStepping(WasmCompiledFrame* frame) {
     Isolate* isolate = frame->wasm_instance().GetIsolate();
     StepAction last_step_action = isolate->debug()->last_step_action();
-    return stepping_frame_ == frame->id() ||
-           (last_step_action == StepIn && stepping_frame_ != NO_ID);
+    return stepping_frame_ == frame->id() || last_step_action == StepIn;
   }
 
   void RemoveBreakpoint(int func_index, int position,
@@ -998,7 +997,9 @@ void DebugInfo::SetBreakpoint(int func_index, int offset,
   impl_->SetBreakpoint(func_index, offset, current_isolate);
 }
 
-void DebugInfo::PrepareStep(Isolate* isolate) { impl_->PrepareStep(isolate); }
+void DebugInfo::PrepareStep(Isolate* isolate, StackFrameId break_frame_id) {
+  impl_->PrepareStep(isolate, break_frame_id);
+}
 
 void DebugInfo::ClearStepping() { impl_->ClearStepping(); }
 

src/wasm/wasm-debug.h

@@ -153,7 +153,7 @@ class DebugInfo {
 
   void SetBreakpoint(int func_index, int offset, Isolate* current_isolate);
 
-  void PrepareStep(Isolate*);
+  void PrepareStep(Isolate*, StackFrameId);
 
   void ClearStepping();
 

test/inspector/debugger/wasm-stepping-in-from-js-expected.txt

@@ -4,7 +4,7 @@ Calling instantiate function.
 Waiting for wasm scripts to be parsed.
 Ignoring script with url v8://test/callInstantiate
 Got wasm script: wasm://wasm/7d022e0e
-Setting breakpoint on line 3 of wasm function
+Setting breakpoint on i32.const
 {
     columnNumber : 37
     lineNumber : 0

test/inspector/debugger/wasm-stepping-in-from-js.js

@@ -2,7 +2,8 @@
 // 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 stepping from javascript into wasm');
+let {session, contextGroup, Protocol} =
+    InspectorTest.start('Tests stepping from javascript into wasm');
 session.setupScriptMap();
 
 utils.load('test/mjsunit/wasm/wasm-module-builder.js');
@@ -11,14 +12,12 @@ let builder = new WasmModuleBuilder();
 
 // wasm_A
 let func = builder.addFunction('wasm_A', kSig_i_i)
-    .addBody([
-      // clang-format off
-      kExprLocalGet, 0,              // Line 1: get input
-      kExprI32Const, 1,              // Line 2: get constant 1
-      kExprI32Sub                    // Line 3: decrease
-      // clang-format on
-    ])
-    .exportAs('main');
+               .addBody([
+                 kExprLocalGet, 0,  // push param 0
+                 kExprI32Const, 1,  // push constant 1
+                 kExprI32Sub        // subtract
+               ])
+               .exportAs('main');
 
 let module_bytes = builder.toArray();
 
@@ -38,7 +37,7 @@ let evalWithUrl = (code, url) => Protocol.Runtime.evaluate(
     {'expression': code + '\n//# sourceURL=v8://test/' + url});
 
 Protocol.Debugger.onPaused(async message => {
-  InspectorTest.log("paused");
+  InspectorTest.log('paused');
   var frames = message.params.callFrames;
   await session.logSourceLocation(frames[0].location);
   let action = step_actions.shift() || 'resume';
@@ -50,8 +49,7 @@ let step_actions = [
   'stepInto',  // # debugger
   'stepInto',  // step into instance.exports.main(1)
   'resume',    // move to breakpoint
-  // then just resume.
-  'resume',
+  'resume',    // then just resume.
 ];
 
 contextGroup.addScript(`
@@ -69,13 +67,17 @@ function test() {
   evalWithUrl(
       'instantiate(' + JSON.stringify(module_bytes) + ')', 'callInstantiate');
   const scriptId = await waitForWasmScript();
-  InspectorTest.log(
-      'Setting breakpoint on line 3 of wasm function');
-  let msg = await Protocol.Debugger.setBreakpoint(
-      {'location': {'scriptId': scriptId, 'lineNumber': 0, 'columnNumber': 2 + func.body_offset}});
+  InspectorTest.log('Setting breakpoint on i32.const');
+  let msg = await Protocol.Debugger.setBreakpoint({
+    'location': {
+      'scriptId': scriptId,
+      'lineNumber': 0,
+      'columnNumber': 2 + func.body_offset
+    }
+  });
   printFailure(msg);
   InspectorTest.logMessage(msg.result.actualLocation);
-  await Protocol.Runtime.evaluate({ expression: 'test()' });
+  await Protocol.Runtime.evaluate({expression: 'test()'});
   InspectorTest.log('exports.main returned!');
   InspectorTest.log('Finished!');
   InspectorTest.completeTest();

test/inspector/inspector.status

@@ -23,7 +23,6 @@
   # differences to the old behaviour (in particular, anyref is not
   # implemented in Liftoff yet).
   # TODO(clemensb/thibaudm): Get this list to zero and remove this block.
-  'debugger/wasm-stepping-in-from-js': [FAIL],
   'debugger/wasm-anyref-global': [FAIL],
 }],