[wasm][debug] Leave stepping code early

Stepping code that is left on the stack will repeatedly call the
WasmDebugBreak function. This has no observable effect, except for
severe slowdown of execution. In the linked bug, we were executing at
least another few million instructions in the same frame, so it appeared
that it never finishes.

This CL fixes that by replacing stepping code with non-stepping code if
the WasmDebugBreak runtime function is called from stepping code but we
are not stepping (any more).
Adding a test for this is difficult, since this only has an effect on
performance.

R=thibaudm@chromium.org

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

src/runtime/runtime-wasm.cc

@@ -552,12 +552,10 @@ RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
   DCHECK_EQ(0, args.length());
   FrameFinder<WasmFrame> frame_finder(
       isolate, {StackFrame::EXIT, StackFrame::WASM_DEBUG_BREAK});
-  auto instance = handle(frame_finder.frame()->wasm_instance(), isolate);
-  auto script = handle(instance->module_object().script(), isolate);
   WasmFrame* frame = frame_finder.frame();
-  int position = frame->position();
-  auto frame_id = frame->id();
-  auto* debug_info = frame->native_module()->GetDebugInfo();
+  auto instance = handle(frame->wasm_instance(), isolate);
+  auto script = handle(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
@@ -572,8 +570,9 @@ RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
   DCHECK_EQ(script->break_on_entry(), instance->break_on_entry());
   if (script->break_on_entry()) {
     MaybeHandle<FixedArray> maybe_on_entry_breakpoints =
-        WasmScript::CheckBreakPoints(
-            isolate, script, WasmScript::kOnEntryBreakpointPosition, frame_id);
+        WasmScript::CheckBreakPoints(isolate, script,
+                                     WasmScript::kOnEntryBreakpointPosition,
+                                     frame->id());
     script->set_break_on_entry(false);
     // Update the "break_on_entry" flag on all live instances.
     i::WeakArrayList weak_instance_list = script->wasm_weak_instance_list();
@@ -606,7 +605,8 @@ RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
 
   // Check whether we hit a breakpoint.
   Handle<FixedArray> breakpoints;
-  if (WasmScript::CheckBreakPoints(isolate, script, position, frame_id)
+  if (WasmScript::CheckBreakPoints(isolate, script, frame->position(),
+                                   frame->id())
           .ToHandle(&breakpoints)) {
     debug_info->ClearStepping(isolate);
     StepAction step_action = isolate->debug()->last_step_action();
@@ -615,8 +615,14 @@ 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();
   }
 
+  // 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);
+
   return ReadOnlyRoots(isolate).undefined_value();
 }
 

src/wasm/wasm-debug.cc

@@ -217,16 +217,13 @@ class DebugInfoImpl {
     return field_names_->GetName(struct_index, field_index);
   }
 
-  // If the top frame is a Wasm frame and its position is not in the list of
-  // breakpoints, return that position. Return 0 otherwise.
+  // If the frame position is not in the list of breakpoints, return that
+  // position. Return 0 otherwise.
   // This is used to generate a "dead breakpoint" in Liftoff, which is necessary
   // for OSR to find the correct return address.
-  int DeadBreakpoint(int func_index, std::vector<int>& breakpoints,
-                     Isolate* isolate) {
-    StackTraceFrameIterator it(isolate);
-    if (it.done() || !it.is_wasm()) return 0;
-    WasmFrame* frame = WasmFrame::cast(it.frame());
-    const auto& function = native_module_->module()->functions[func_index];
+  int DeadBreakpoint(WasmFrame* frame, Vector<const int> breakpoints) {
+    const auto& function =
+        native_module_->module()->functions[frame->function_index()];
     int offset = frame->position() - function.code.offset();
     if (std::binary_search(breakpoints.begin(), breakpoints.end(), offset)) {
       return 0;
@@ -234,6 +231,17 @@ class DebugInfoImpl {
     return offset;
   }
 
+  // Find the dead breakpoint (see above) for the top wasm frame, if that frame
+  // is in the function of the given index.
+  int DeadBreakpoint(int func_index, Vector<const int> breakpoints,
+                     Isolate* isolate) {
+    StackTraceFrameIterator it(isolate);
+    if (it.done() || !it.is_wasm()) return 0;
+    auto* wasm_frame = WasmFrame::cast(it.frame());
+    if (static_cast<int>(wasm_frame->function_index()) != func_index) return 0;
+    return DeadBreakpoint(wasm_frame, breakpoints);
+  }
+
   WasmCode* RecompileLiftoffWithBreakpoints(int func_index,
                                             Vector<const int> offsets,
                                             int dead_breakpoint) {
@@ -355,7 +363,7 @@ class DebugInfoImpl {
     } else {
       all_breakpoints.insert(insertion_point, offset);
       int dead_breakpoint =
-          DeadBreakpoint(func_index, all_breakpoints, isolate);
+          DeadBreakpoint(func_index, VectorOf(all_breakpoints), isolate);
       new_code = RecompileLiftoffWithBreakpoints(
           func_index, VectorOf(all_breakpoints), dead_breakpoint);
     }
@@ -411,6 +419,19 @@ class DebugInfoImpl {
     FloodWithBreakpoints(frame, kAfterWasmCall);
   }
 
+  void ClearStepping(WasmFrame* frame) {
+    WasmCodeRefScope wasm_code_ref_scope;
+    base::MutexGuard guard(&mutex_);
+    auto* code = frame->wasm_code();
+    if (code->for_debugging() != kForStepping) return;
+    int func_index = code->index();
+    std::vector<int> breakpoints = FindAllBreakpoints(func_index);
+    int dead_breakpoint = DeadBreakpoint(frame, VectorOf(breakpoints));
+    WasmCode* new_code = RecompileLiftoffWithBreakpoints(
+        func_index, VectorOf(breakpoints), dead_breakpoint);
+    UpdateReturnAddress(frame, new_code, kAfterBreakpoint);
+  }
+
   void ClearStepping(Isolate* isolate) {
     base::MutexGuard guard(&mutex_);
     auto it = per_isolate_data_.find(isolate);
@@ -452,7 +473,8 @@ class DebugInfoImpl {
     // If the breakpoint is still set in another isolate, don't remove it.
     DCHECK(std::is_sorted(remaining.begin(), remaining.end()));
     if (std::binary_search(remaining.begin(), remaining.end(), offset)) return;
-    int dead_breakpoint = DeadBreakpoint(func_index, remaining, isolate);
+    int dead_breakpoint =
+        DeadBreakpoint(func_index, VectorOf(remaining), isolate);
     UpdateBreakpoints(func_index, VectorOf(remaining), isolate,
                       isolate_data.stepping_frame, dead_breakpoint);
   }
@@ -816,6 +838,8 @@ void DebugInfo::ClearStepping(Isolate* isolate) {
   impl_->ClearStepping(isolate);
 }
 
+void DebugInfo::ClearStepping(WasmFrame* frame) { impl_->ClearStepping(frame); }
+
 bool DebugInfo::IsStepping(WasmFrame* frame) {
   return impl_->IsStepping(frame);
 }

src/wasm/wasm-debug.h

@@ -208,6 +208,11 @@ class V8_EXPORT_PRIVATE DebugInfo {
 
   void ClearStepping(Isolate*);
 
+  // Remove stepping code from a single frame; this is a performance
+  // optimization only, hitting debug breaks while not stepping and not at a set
+  // breakpoint would be unobservable otherwise.
+  void ClearStepping(WasmFrame*);
+
   bool IsStepping(WasmFrame*);
 
   void RemoveBreakpoint(int func_index, int offset, Isolate* current_isolate);