[wasm][debug] Handle OSR edge case

When the top frame is paused at a breakpoint, and this breakpoint is
being removed or was already removed, introduce a "dead breakpoint" in
the new code. This ensures that:
- The source position for the new frame is correct, otherwise it would
just pick the source position of the previous call,
- The offset between the source position and return address is the same
in the new and old code, which is necessary for OSR to find the correct
return address.

R=clemensb@chromium.org

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

src/wasm/baseline/liftoff-compiler.cc

@@ -339,7 +339,7 @@ class LiftoffCompiler {
                   std::unique_ptr<AssemblerBuffer> buffer,
                   DebugSideTableBuilder* debug_sidetable_builder,
                   ForDebugging for_debugging, int func_index,
-                  Vector<int> breakpoints = {})
+                  Vector<int> breakpoints = {}, int dead_breakpoint = 0)
       : asm_(std::move(buffer)),
         descriptor_(
             GetLoweredCallDescriptor(compilation_zone, call_descriptor)),
@@ -353,7 +353,8 @@ class LiftoffCompiler {
         compilation_zone_(compilation_zone),
         safepoint_table_builder_(compilation_zone_),
         next_breakpoint_ptr_(breakpoints.begin()),
-        next_breakpoint_end_(breakpoints.end()) {
+        next_breakpoint_end_(breakpoints.end()),
+        dead_breakpoint_(dead_breakpoint) {
     if (breakpoints.empty()) {
       next_breakpoint_ptr_ = next_breakpoint_end_ = nullptr;
     }
@@ -819,6 +820,18 @@ class LiftoffCompiler {
         }
       }
     }
+    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.
+      Label cont;
+      __ emit_jump(&cont);
+      EmitBreakpoint(decoder);
+      __ bind(&cont);
+    }
   }
 
   void NextInstruction(FullDecoder* decoder, WasmOpcode opcode) {
@@ -3919,6 +3932,10 @@ class LiftoffCompiler {
   int* next_breakpoint_ptr_ = nullptr;
   int* next_breakpoint_end_ = nullptr;
 
+  // Introduce a dead breakpoint to ensure that the calculation of the return
+  // address in OSR is correct.
+  int dead_breakpoint_ = 0;
+
   bool has_outstanding_op() const {
     return outstanding_op_ != kNoOutstandingOp;
   }
@@ -3953,7 +3970,7 @@ WasmCompilationResult ExecuteLiftoffCompilation(
     AccountingAllocator* allocator, CompilationEnv* env,
     const FunctionBody& func_body, int func_index, ForDebugging for_debugging,
     Counters* counters, WasmFeatures* detected, Vector<int> breakpoints,
-    std::unique_ptr<DebugSideTable>* debug_sidetable) {
+    std::unique_ptr<DebugSideTable>* debug_sidetable, int dead_breakpoint) {
   int func_body_size = static_cast<int>(func_body.end - func_body.start);
   TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("v8.wasm.detailed"),
                "wasm.CompileBaseline", "func_index", func_index, "body_size",
@@ -3980,7 +3997,8 @@ WasmCompilationResult ExecuteLiftoffCompilation(
   WasmFullDecoder<Decoder::kValidate, LiftoffCompiler> decoder(
       &zone, env->module, env->enabled_features, detected, func_body,
       call_descriptor, env, &zone, instruction_buffer->CreateView(),
-      debug_sidetable_builder.get(), for_debugging, func_index, breakpoints);
+      debug_sidetable_builder.get(), for_debugging, func_index, breakpoints,
+      dead_breakpoint);
   decoder.Decode();
   liftoff_compile_time_scope.reset();
   LiftoffCompiler* compiler = &decoder.interface();

src/wasm/baseline/liftoff-compiler.h

@@ -56,7 +56,8 @@ enum LiftoffBailoutReason : int8_t {
 V8_EXPORT_PRIVATE WasmCompilationResult ExecuteLiftoffCompilation(
     AccountingAllocator*, CompilationEnv*, const FunctionBody&, int func_index,
     ForDebugging, Counters*, WasmFeatures* detected_features,
-    Vector<int> breakpoints = {}, std::unique_ptr<DebugSideTable>* = nullptr);
+    Vector<int> breakpoints = {}, std::unique_ptr<DebugSideTable>* = nullptr,
+    int dead_breakpoint = 0);
 
 V8_EXPORT_PRIVATE std::unique_ptr<DebugSideTable> GenerateLiftoffDebugSideTable(
     AccountingAllocator*, CompilationEnv*, const FunctionBody&, int func_index);

src/wasm/wasm-debug.cc

@@ -402,8 +402,25 @@ class DebugInfoImpl {
     return local_names_->GetName(func_index, local_index);
   }
 
-  WasmCode* RecompileLiftoffWithBreakpoints(int func_index,
-                                            Vector<int> offsets) {
+  // If the top frame is a Wasm frame and its 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 offset = frame->position() - function.code.offset();
+    if (std::binary_search(breakpoints.begin(), breakpoints.end(), offset)) {
+      return 0;
+    }
+    return offset;
+  }
+
+  WasmCode* RecompileLiftoffWithBreakpoints(int func_index, Vector<int> offsets,
+                                            int dead_breakpoint) {
     DCHECK(!mutex_.TryLock());  // Mutex is held externally.
     // Recompile the function with Liftoff, setting the new breakpoints.
     // Not thread-safe. The caller is responsible for locking {mutex_}.
@@ -422,7 +439,8 @@ class DebugInfoImpl {
     WasmFeatures unused_detected;
     WasmCompilationResult result = ExecuteLiftoffCompilation(
         native_module_->engine()->allocator(), &env, body, func_index,
-        for_debugging, counters, &unused_detected, offsets, &debug_sidetable);
+        for_debugging, counters, &unused_detected, offsets, &debug_sidetable,
+        dead_breakpoint);
     // Liftoff compilation failure is a FATAL error. We rely on complete Liftoff
     // support for debugging.
     if (!result.succeeded()) FATAL("Liftoff compilation failed");
@@ -479,8 +497,10 @@ class DebugInfoImpl {
       new_code = native_module_->GetCode(func_index);
     } else {
       all_breakpoints.insert(insertion_point, offset);
-      new_code = RecompileLiftoffWithBreakpoints(func_index,
-                                                 VectorOf(all_breakpoints));
+      int dead_breakpoint =
+          DeadBreakpoint(func_index, all_breakpoints, isolate);
+      new_code = RecompileLiftoffWithBreakpoints(
+          func_index, VectorOf(all_breakpoints), dead_breakpoint);
     }
     UpdateReturnAddresses(isolate, new_code, isolate_data.stepping_frame);
   }
@@ -497,10 +517,11 @@ class DebugInfoImpl {
   }
 
   void UpdateBreakpoints(int func_index, Vector<int> breakpoints,
-                         Isolate* isolate, StackFrameId stepping_frame) {
+                         Isolate* isolate, StackFrameId stepping_frame,
+                         int dead_breakpoint) {
     DCHECK(!mutex_.TryLock());  // Mutex is held externally.
-    WasmCode* new_code =
-        RecompileLiftoffWithBreakpoints(func_index, breakpoints);
+    WasmCode* new_code = RecompileLiftoffWithBreakpoints(
+        func_index, breakpoints, dead_breakpoint);
     UpdateReturnAddresses(isolate, new_code, stepping_frame);
   }
 
@@ -512,7 +533,7 @@ class DebugInfoImpl {
     // Generate an additional source position for the current byte offset.
     base::MutexGuard guard(&mutex_);
     WasmCode* new_code = RecompileLiftoffWithBreakpoints(
-        frame->function_index(), VectorOf(&offset, 1));
+        frame->function_index(), VectorOf(&offset, 1), 0);
     UpdateReturnAddress(frame, new_code, return_location);
   }
 
@@ -558,18 +579,6 @@ class DebugInfoImpl {
   }
 
   void RemoveBreakpoint(int func_index, int position, Isolate* isolate) {
-    // TODO(thibaudm): Cannot remove the breakpoint we are currently paused at,
-    // because the new code would be missing the call instruction and the
-    // corresponding source position that we rely on for OSR.
-    StackTraceFrameIterator it(isolate);
-    if (!it.done() && it.is_wasm()) {
-      WasmFrame* frame = WasmFrame::cast(it.frame());
-      if (static_cast<int32_t>(frame->function_index()) == func_index &&
-          frame->position() == position) {
-        return;
-      }
-    }
-
     // Put the code ref scope outside of the mutex, so we don't unnecessarily
     // hold the mutex while freeing code.
     WasmCodeRefScope wasm_code_ref_scope;
@@ -595,8 +604,9 @@ 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);
     UpdateBreakpoints(func_index, VectorOf(remaining), isolate,
-                      isolate_data.stepping_frame);
+                      isolate_data.stepping_frame, dead_breakpoint);
   }
 
   void RemoveDebugSideTables(Vector<WasmCode* const> codes) {
@@ -640,7 +650,7 @@ class DebugInfoImpl {
       std::vector<int>& removed = entry.second;
       std::vector<int> remaining = FindAllBreakpoints(func_index);
       if (HasRemovedBreakpoints(removed, remaining)) {
-        RecompileLiftoffWithBreakpoints(func_index, VectorOf(remaining));
+        RecompileLiftoffWithBreakpoints(func_index, VectorOf(remaining), 0);
       }
     }
   }