[wasm] Fix termination on breakpoint

If the debug handler (called via {OnDebugBreak}) requests termination of
the isolate, this would only get considered on the next stack check,
where it is turned into a proper termination exception.

Handling this correctly is further complicated by the {DebugScope}
blocking any handling of interrupts via the included
{PostponeInterruptsScope}.

Hence this CL refactors the code to call any debug handlers in a second
function which has the {DebugScope}, and to check for interrupts after
leaving that scope.

R=thibaudm@chromium.org
CC=bmeurer@chromium.org

Bug: chromium:1319343
Change-Id: Ia2df0f2610d50eedc6437841c4bf1d2ad3ac9125
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3605228Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80235}

src/runtime/runtime-wasm.cc

@@ -568,33 +568,20 @@ RUNTIME_FUNCTION(Runtime_WasmTableFill) {
   return ReadOnlyRoots(isolate).undefined_value();
 }
 
-RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
-  ClearThreadInWasmScope flag_scope(isolate);
-  HandleScope scope(isolate);
-  DCHECK_EQ(0, args.length());
-  FrameFinder<WasmFrame> frame_finder(
-      isolate, {StackFrame::EXIT, StackFrame::WASM_DEBUG_BREAK});
-  WasmFrame* frame = frame_finder.frame();
-  auto instance = handle(frame->wasm_instance(), isolate);
-  auto script = handle(instance->module_object().script(), isolate);
+namespace {
+// Returns true if any breakpoint was hit, false otherwise.
+bool ExecuteWasmDebugBreaks(Isolate* isolate,
+                            Handle<WasmInstanceObject> instance,
+                            WasmFrame* frame) {
+  Handle<Script> script{instance->module_object().script(), isolate};
   auto* debug_info = instance->module_object().native_module()->GetDebugInfo();
-  isolate->set_context(instance->native_context());
-
-  // Stepping can repeatedly create code, and code GC requires stack guards to
-  // be executed on all involved isolates. Proactively do this here.
-  StackLimitCheck check(isolate);
-  if (check.InterruptRequested()) {
-    Object interrupt_object = isolate->stack_guard()->HandleInterrupts();
-    // Interrupt handling can create an exception, including the
-    // termination exception.
-    if (interrupt_object.IsException(isolate)) return interrupt_object;
-    DCHECK(interrupt_object.IsUndefined(isolate));
-  }
 
   // Enter the debugger.
   DebugScope debug_scope(isolate->debug());
+
+  // Check for instrumentation breakpoints first, but still execute regular
+  // breakpoints afterwards.
   bool paused_on_instrumentation = false;
-  // Check for instrumentation breakpoint.
   DCHECK_EQ(script->break_on_entry(), !!instance->break_on_entry());
   if (script->break_on_entry()) {
     MaybeHandle<FixedArray> maybe_on_entry_breakpoints =
@@ -622,7 +609,7 @@ RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
     isolate->debug()->ClearStepping();
     isolate->debug()->OnDebugBreak(isolate->factory()->empty_fixed_array(),
                                    step_action);
-    return ReadOnlyRoots(isolate).undefined_value();
+    return true;
   }
 
   // Check whether we hit a breakpoint.
@@ -637,19 +624,43 @@ RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
       // We hit one or several breakpoints. Notify the debug listeners.
       isolate->debug()->OnDebugBreak(breakpoints, step_action);
     }
-    return ReadOnlyRoots(isolate).undefined_value();
+    return true;
   }
 
-  // We only hit the instrumentation breakpoint, and there is no other reason to
-  // break.
-  if (paused_on_instrumentation) {
-    return ReadOnlyRoots(isolate).undefined_value();
+  return paused_on_instrumentation;
+}
+}  // namespace
+
+RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
+  ClearThreadInWasmScope flag_scope(isolate);
+  HandleScope scope(isolate);
+  DCHECK_EQ(0, args.length());
+  FrameFinder<WasmFrame> frame_finder(
+      isolate, {StackFrame::EXIT, StackFrame::WASM_DEBUG_BREAK});
+  WasmFrame* frame = frame_finder.frame();
+  auto instance = handle(frame->wasm_instance(), isolate);
+  isolate->set_context(instance->native_context());
+
+  if (!ExecuteWasmDebugBreaks(isolate, instance, frame)) {
+    // We did not hit a breakpoint. If we are in stepping code, but the user did
+    // not request stepping, clear this (to save further calls into this runtime
+    // function).
+    auto* debug_info =
+        instance->module_object().native_module()->GetDebugInfo();
+    debug_info->ClearStepping(frame);
   }
 
-  // We did not hit a breakpoint. If we are in stepping code, but the user did
-  // not request stepping, clear this (to save further calls into this runtime
-  // function).
-  debug_info->ClearStepping(frame);
+  // Execute a stack check before leaving this function. This is to handle any
+  // interrupts set by the debugger (e.g. termination), but also to execute Wasm
+  // code GC to get rid of temporarily created Wasm code.
+  StackLimitCheck check(isolate);
+  if (check.InterruptRequested()) {
+    Object interrupt_object = isolate->stack_guard()->HandleInterrupts();
+    // Interrupt handling can create an exception, including the
+    // termination exception.
+    if (interrupt_object.IsException(isolate)) return interrupt_object;
+    DCHECK(interrupt_object.IsUndefined(isolate));
+  }
 
   return ReadOnlyRoots(isolate).undefined_value();
 }

test/inspector/debugger/wasm-terminate-execution-on-pause-expected.txt

+Tests termination on pause in Wasm
+
+Running test: testTerminateOnBreakpoint
+Instantiating.
+Waiting for wasm script (ignoring first non-wasm script).
+Setting breakpoint.
+{
+    id : <messageId>
+    result : {
+        actualLocation : {
+            columnNumber : 33
+            lineNumber : 0
+            scriptId : <scriptId>
+        }
+        breakpointId : <breakpointId>
+    }
+}
+Calling wasm export.
+{
+    type : string
+    value : Before Wasm execution
+}
+Paused:
+Script wasm://wasm/8c388106 byte offset 33: Wasm opcode 0x01 (kExprNop)

test/inspector/debugger/wasm-terminate-execution-on-pause.js

+// Copyright 2022 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('Tests termination on pause in Wasm');
+session.setupScriptMap();
+
+Protocol.Runtime.enable();
+Protocol.Runtime.onConsoleAPICalled(
+    message => InspectorTest.logMessage(message.params.args[0]));
+Protocol.Debugger.enable();
+
+const builder = new WasmModuleBuilder();
+const func = builder.addFunction('func', kSig_v_v)
+                 .addBody([kExprNop])
+                 .exportFunc();
+const module_bytes = builder.toArray();
+
+InspectorTest.runAsyncTestSuite([
+  async function testTerminateOnBreakpoint() {
+    InspectorTest.log('Instantiating.');
+    // Spawn asynchronously:
+    WasmInspectorTest.instantiate(module_bytes);
+    InspectorTest.log(
+        'Waiting for wasm script (ignoring first non-wasm script).');
+    const [, {params: wasm_script}] = await Protocol.Debugger.onceScriptParsed(2);
+    InspectorTest.log('Setting breakpoint.');
+    InspectorTest.logMessage(await Protocol.Debugger.setBreakpoint({
+      'location': {
+        'scriptId': wasm_script.scriptId,
+        'lineNumber': 0,
+        'columnNumber': func.body_offset
+      }
+    }));
+    InspectorTest.log('Calling wasm export.');
+    const evaluation = Protocol.Runtime.evaluate({
+      'expression': `console.log('Before Wasm execution');` +
+          `instance.exports.func();` +
+          `console.log('After Wasm execution (should not be reached)');`
+    });
+    const pause_msg = await Protocol.Debugger.oncePaused();
+    InspectorTest.log('Paused:');
+    await session.logSourceLocation(pause_msg.params.callFrames[0].location);
+    const terminated = Protocol.Runtime.terminateExecution();
+    await Protocol.Debugger.resume();
+    await terminated;
+    await evaluation;
+  },
+]);