[wasm] Fix stepping from a non-breakable position

... at function start. Otherwise we run into a position mismatch:
In a non-flooded function, we add the function-entry breakpoint (for
"hook on function call") with the position of the first opcode.
In the flooded function though, we skip that special breakpoint because
we will stop at the first instruction anyway. But then the first
instruction is non-breakable, so we don't actually emit a breakpoint for
it.
Hence during OSR we do not find a corresponding position in the new
code.

This CL fixes this by postponing the function-entry breakpoint until the
first breakable opcode is found, and only emits it if that position does
not have a breakpoint anyway.
This way, we can also move the handling for function-entry breakpoints
from {StartFunctionBody} to {EmitDebuggingInfo}, where it fits much
better.

R=thibaudm@chromium.org

Bug: chromium:1137710
Change-Id: Idfa658fa0897cca89ba5ee3066cd414f68864d06
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2474774Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70529}

src/wasm/baseline/liftoff-compiler.cc

@@ -690,30 +690,6 @@ class LiftoffCompiler {
     }
 
     if (FLAG_trace_wasm) TraceFunctionEntry(decoder);
-
-    // If we are generating debug code, do check the "hook on function call"
-    // flag. If set, trigger a break.
-    if (V8_UNLIKELY(for_debugging_)) {
-      // If there is a breakpoint set on the first instruction (== start of the
-      // function), then skip the check for "hook on function call", since we
-      // will unconditionally break there anyway.
-      bool has_breakpoint = next_breakpoint_ptr_ != nullptr &&
-                            (*next_breakpoint_ptr_ == 0 ||
-                             *next_breakpoint_ptr_ == decoder->position());
-      if (!has_breakpoint) {
-        DEBUG_CODE_COMMENT("check hook on function call");
-        Register flag = __ GetUnusedRegister(kGpReg, {}).gp();
-        LOAD_INSTANCE_FIELD(flag, HookOnFunctionCallAddress,
-                            kSystemPointerSize);
-        Label no_break;
-        __ Load(LiftoffRegister{flag}, flag, no_reg, 0, LoadType::kI32Load8U,
-                {});
-        // Unary "equal" means "equals zero".
-        __ emit_cond_jump(kEqual, &no_break, kWasmI32, flag);
-        EmitBreakpoint(decoder);
-        __ bind(&no_break);
-      }
-    }
   }
 
   void GenerateOutOfLineCode(OutOfLineCode* ool) {
@@ -799,14 +775,14 @@ class LiftoffCompiler {
   }
 
   V8_NOINLINE void EmitDebuggingInfo(FullDecoder* decoder, WasmOpcode opcode) {
-    DCHECK(V8_UNLIKELY(for_debugging_));
+    DCHECK(for_debugging_);
+    if (!WasmOpcodes::IsBreakable(opcode)) return;
+    bool has_breakpoint = false;
     if (next_breakpoint_ptr_) {
       if (*next_breakpoint_ptr_ == 0) {
         // A single breakpoint at offset 0 indicates stepping.
         DCHECK_EQ(next_breakpoint_ptr_ + 1, next_breakpoint_end_);
-        if (WasmOpcodes::IsBreakable(opcode)) {
-          EmitBreakpoint(decoder);
-        }
+        has_breakpoint = true;
       } else {
         while (next_breakpoint_ptr_ != next_breakpoint_end_ &&
                *next_breakpoint_ptr_ < decoder->position()) {
@@ -816,18 +792,34 @@ class LiftoffCompiler {
         if (next_breakpoint_ptr_ == next_breakpoint_end_) {
           next_breakpoint_ptr_ = next_breakpoint_end_ = nullptr;
         } else if (*next_breakpoint_ptr_ == decoder->position()) {
-          DCHECK(WasmOpcodes::IsBreakable(opcode));
-          EmitBreakpoint(decoder);
+          has_breakpoint = true;
         }
       }
     }
-    if (dead_breakpoint_ == decoder->position()) {
+    if (has_breakpoint) {
+      EmitBreakpoint(decoder);
+      // Once we emitted a breakpoint, we don't need to check the "hook on
+      // function call" any more.
+      checked_hook_on_function_call_ = true;
+    } else if (!checked_hook_on_function_call_) {
+      checked_hook_on_function_call_ = true;
+      // Check the "hook on function call" flag. If set, trigger a break.
+      DEBUG_CODE_COMMENT("check hook on function call");
+      Register flag = __ GetUnusedRegister(kGpReg, {}).gp();
+      LOAD_INSTANCE_FIELD(flag, HookOnFunctionCallAddress, kSystemPointerSize);
+      Label no_break;
+      __ Load(LiftoffRegister{flag}, flag, no_reg, 0, LoadType::kI32Load8U, {});
+      // Unary "equal" means "equals zero".
+      __ emit_cond_jump(kEqual, &no_break, kWasmI32, flag);
+      EmitBreakpoint(decoder);
+      __ bind(&no_break);
+    } else if (dead_breakpoint_ == decoder->position()) {
       DCHECK(!next_breakpoint_ptr_ ||
              *next_breakpoint_ptr_ != dead_breakpoint_);
       // The top frame is paused at this position, but the breakpoint was
-      // removed. Adding a dead breakpoint here ensures that the source position
-      // exists, and that the offset to the return address is the same as in the
-      // old code.
+      // removed. Adding a dead breakpoint here ensures that the source
+      // position exists, and that the offset to the return address is the
+      // same as in the old code.
       Label cont;
       __ emit_jump(&cont);
       EmitBreakpoint(decoder);
@@ -4076,6 +4068,11 @@ class LiftoffCompiler {
   // address in OSR is correct.
   int dead_breakpoint_ = 0;
 
+  // Remember whether the "hook on function call" has already been checked.
+  // This happens at the first breakable opcode in the function (if compiling
+  // for debugging).
+  bool checked_hook_on_function_call_ = false;
+
   bool has_outstanding_op() const {
     return outstanding_op_ != kNoOutstandingOp;
   }

test/inspector/debugger/wasm-step-from-non-breakable-position-expected.txt

+Step into a function that starts with a non-breakable opcode (i.e. block), then step from there. See https://crbug.com/1137710.
+Setting up global instance variable.
+Got wasm script: wasm://wasm/4658c40e
+Setting breakpoint on offset 44
+Running main function.
+Script wasm://wasm/4658c40e byte offset 44: Wasm opcode 0x10
+Debugger.stepInto called
+Script wasm://wasm/4658c40e byte offset 40: Wasm opcode 0x0b
+Debugger.stepInto called
+Script wasm://wasm/4658c40e byte offset 41: Wasm opcode 0x0b
+Debugger.resume called
+exports.main returned.

test/inspector/debugger/wasm-step-from-non-breakable-position.js

+// Copyright 2020 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.
+
+utils.load('test/inspector/wasm-inspector-test.js');
+
+let {session, contextGroup, Protocol} = InspectorTest.start(
+    'Step into a function that starts with a non-breakable opcode (i.e. ' +
+    'block), then step from there. See https://crbug.com/1137710.');
+session.setupScriptMap();
+
+var builder = new WasmModuleBuilder();
+
+var callee = builder.addFunction('callee', kSig_v_v)
+                 .addBody([kExprBlock, kWasmStmt, kExprEnd])
+                 .index;
+
+var main = builder.addFunction('main', kSig_v_i)
+               .addBody([kExprCallFunction, callee])
+               .exportFunc();
+
+var module_bytes = builder.toArray();
+
+(async function test() {
+  InspectorTest.logProtocolCommandCalls('Debugger.stepInto');
+  InspectorTest.logProtocolCommandCalls('Debugger.resume');
+
+  await Protocol.Debugger.enable();
+  InspectorTest.log('Setting up global instance variable.');
+  WasmInspectorTest.instantiate(module_bytes);
+  const [, {params: wasmScript}] = await Protocol.Debugger.onceScriptParsed(2);
+
+  InspectorTest.log(`Got wasm script: ${wasmScript.url}`);
+
+  // Set a breakpoint in 'main', at the call.
+  InspectorTest.log(`Setting breakpoint on offset ${main.body_offset}`);
+  await Protocol.Debugger.setBreakpoint({
+    location: {
+      scriptId: wasmScript.scriptId,
+      lineNumber: 0,
+      columnNumber: main.body_offset
+    }
+  });
+
+  InspectorTest.log('Running main function.');
+  Protocol.Runtime.evaluate({ expression: 'instance.exports.main()' });
+  for (let action of ['stepInto', 'stepInto', 'resume']) {
+    const {params: {callFrames}} = await Protocol.Debugger.oncePaused();
+    await session.logSourceLocation(callFrames[0].location);
+    Protocol.Debugger[action]();
+  }
+  InspectorTest.log('exports.main returned.');
+})().catch(reason => InspectorTest.log(`Failed: ${reason}`))
+    .finally(InspectorTest.completeTest);