[wasm][debug] Cleanup debugging source positions

Remove extra source positions added by Liftoff to help with OSR. Compute
the return address based on the call source position instead.

R=clemensb@chromium.org

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

src/wasm/baseline/liftoff-compiler.h

@@ -56,8 +56,7 @@ 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> extra_source_pos = {});
+    Vector<int> breakpoints = {}, std::unique_ptr<DebugSideTable>* = nullptr);
 
 V8_EXPORT_PRIVATE std::unique_ptr<DebugSideTable> GenerateLiftoffDebugSideTable(
     AccountingAllocator*, CompilationEnv*, const FunctionBody&, int func_index);

src/wasm/wasm-debug.cc

@@ -160,32 +160,28 @@ MaybeHandle<String> GetLocalNameString(Isolate* isolate,
   return isolate->factory()->NewStringFromUtf8(name);
 }
 
-// Generate a sorted and deduplicated list of byte offsets for this function's
-// current positions on the stack.
-std::vector<int> StackFramePositions(int func_index, Isolate* isolate) {
-  std::vector<int> byte_offsets;
-  for (StackTraceFrameIterator it(isolate); !it.done(); it.Advance()) {
-    if (!it.is_wasm()) continue;
-    WasmFrame* frame = WasmFrame::cast(it.frame());
-    if (static_cast<int>(frame->function_index()) != func_index) continue;
-    WasmCode* wasm_code = frame->wasm_code();
-    if (!wasm_code->is_liftoff()) continue;
-    byte_offsets.push_back(frame->byte_offset());
-  }
-  std::sort(byte_offsets.begin(), byte_offsets.end());
-  auto last = std::unique(byte_offsets.begin(), byte_offsets.end());
-  byte_offsets.erase(last, byte_offsets.end());
-  return byte_offsets;
-}
-
 enum ReturnLocation { kAfterBreakpoint, kAfterWasmCall };
 
-Address FindNewPC(WasmCode* wasm_code, int byte_offset,
+Address FindNewPC(WasmFrame* frame, WasmCode* wasm_code, int byte_offset,
                   ReturnLocation return_location) {
   Vector<const uint8_t> new_pos_table = wasm_code->source_positions();
 
   DCHECK_LE(0, byte_offset);
 
+  // Find the size of the call instruction by computing the distance from the
+  // source position entry to the return address.
+  WasmCode* old_code = frame->wasm_code();
+  int pc_offset = static_cast<int>(frame->pc() - old_code->instruction_start());
+  Vector<const uint8_t> old_pos_table = old_code->source_positions();
+  SourcePositionTableIterator old_it(old_pos_table);
+  int call_offset = -1;
+  while (!old_it.done() && old_it.code_offset() < pc_offset) {
+    call_offset = old_it.code_offset();
+    old_it.Advance();
+  }
+  DCHECK_LE(0, call_offset);
+  int call_instruction_size = pc_offset - call_offset;
+
   // If {return_location == kAfterBreakpoint} we search for the first code
   // offset which is marked as instruction (i.e. not the breakpoint).
   // If {return_location == kAfterWasmCall} we return the last code offset
@@ -197,7 +193,8 @@ Address FindNewPC(WasmCode* wasm_code, int byte_offset,
   if (return_location == kAfterBreakpoint) {
     while (!it.is_statement()) it.Advance();
     DCHECK_EQ(byte_offset, it.source_position().ScriptOffset());
-    return wasm_code->instruction_start() + it.code_offset();
+    return wasm_code->instruction_start() + it.code_offset() +
+           call_instruction_size;
   }
 
   DCHECK_EQ(kAfterWasmCall, return_location);
@@ -206,7 +203,7 @@ Address FindNewPC(WasmCode* wasm_code, int byte_offset,
     code_offset = it.code_offset();
     it.Advance();
   } while (!it.done() && it.source_position().ScriptOffset() == byte_offset);
-  return wasm_code->instruction_start() + code_offset;
+  return wasm_code->instruction_start() + code_offset + call_instruction_size;
 }
 
 }  // namespace
@@ -405,8 +402,8 @@ class DebugInfoImpl {
     return local_names_->GetName(func_index, local_index);
   }
 
-  WasmCode* RecompileLiftoffWithBreakpoints(
-      int func_index, Vector<int> offsets, Vector<int> extra_source_positions) {
+  WasmCode* RecompileLiftoffWithBreakpoints(int func_index,
+                                            Vector<int> offsets) {
     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_}.
@@ -425,8 +422,7 @@ class DebugInfoImpl {
     WasmFeatures unused_detected;
     WasmCompilationResult result = ExecuteLiftoffCompilation(
         native_module_->engine()->allocator(), &env, body, func_index,
-        for_debugging, counters, &unused_detected, offsets, &debug_sidetable,
-        extra_source_positions);
+        for_debugging, counters, &unused_detected, offsets, &debug_sidetable);
     // Liftoff compilation failure is a FATAL error. We rely on complete Liftoff
     // support for debugging.
     if (!result.succeeded()) FATAL("Liftoff compilation failed");
@@ -447,11 +443,6 @@ class DebugInfoImpl {
     // hold the mutex while freeing code.
     WasmCodeRefScope wasm_code_ref_scope;
 
-    // Generate additional source positions for current stack frame positions.
-    // These source positions are used to find return addresses in the new code.
-    std::vector<int> stack_frame_positions =
-        StackFramePositions(func_index, isolate);
-
     // Hold the mutex while modifying breakpoints, to ensure consistency when
     // multiple isolates set/remove breakpoints at the same time.
     base::MutexGuard guard(&mutex_);
@@ -480,19 +471,16 @@ class DebugInfoImpl {
                                        all_breakpoints.end(), offset);
     bool breakpoint_exists =
         insertion_point != all_breakpoints.end() && *insertion_point == offset;
-    // If the breakpoint was already set before *and* we don't need any special
-    // positions for OSR, then we can just reuse the old code. Otherwise,
-    // recompile it. In any case, rewrite this isolate's stack to make sure that
-    // it uses up-to-date code containing the breakpoint.
+    // If the breakpoint was already set before, then we can just reuse the old
+    // code. Otherwise, recompile it. In any case, rewrite this isolate's stack
+    // to make sure that it uses up-to-date code containing the breakpoint.
     WasmCode* new_code;
-    if (breakpoint_exists && stack_frame_positions.empty()) {
+    if (breakpoint_exists) {
       new_code = native_module_->GetCode(func_index);
     } else {
-      // Add the new offset to the set of all breakpoints, then recompile.
-      if (!breakpoint_exists) all_breakpoints.insert(insertion_point, offset);
-      new_code =
-          RecompileLiftoffWithBreakpoints(func_index, VectorOf(all_breakpoints),
-                                          VectorOf(stack_frame_positions));
+      all_breakpoints.insert(insertion_point, offset);
+      new_code = RecompileLiftoffWithBreakpoints(func_index,
+                                                 VectorOf(all_breakpoints));
     }
     UpdateReturnAddresses(isolate, new_code, isolate_data.stepping_frame);
   }
@@ -511,13 +499,8 @@ class DebugInfoImpl {
   void UpdateBreakpoints(int func_index, Vector<int> breakpoints,
                          Isolate* isolate, StackFrameId stepping_frame) {
     DCHECK(!mutex_.TryLock());  // Mutex is held externally.
-    // Generate additional source positions for current stack frame positions.
-    // These source positions are used to find return addresses in the new code.
-    std::vector<int> stack_frame_positions =
-        StackFramePositions(func_index, isolate);
-
-    WasmCode* new_code = RecompileLiftoffWithBreakpoints(
-        func_index, breakpoints, VectorOf(stack_frame_positions));
+    WasmCode* new_code =
+        RecompileLiftoffWithBreakpoints(func_index, breakpoints);
     UpdateReturnAddresses(isolate, new_code, stepping_frame);
   }
 
@@ -527,11 +510,9 @@ class DebugInfoImpl {
     WasmCodeRefScope wasm_code_ref_scope;
     DCHECK(frame->wasm_code()->is_liftoff());
     // Generate an additional source position for the current byte offset.
-    int byte_offset = frame->byte_offset();
     base::MutexGuard guard(&mutex_);
     WasmCode* new_code = RecompileLiftoffWithBreakpoints(
-        frame->function_index(), VectorOf(&offset, 1),
-        VectorOf(&byte_offset, 1));
+        frame->function_index(), VectorOf(&offset, 1));
     UpdateReturnAddress(frame, new_code, return_location);
   }
 
@@ -577,6 +558,18 @@ 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;
@@ -647,7 +640,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));
       }
     }
   }
@@ -809,7 +802,8 @@ class DebugInfoImpl {
 #ifdef DEBUG
     int old_position = frame->position();
 #endif
-    Address new_pc = FindNewPC(new_code, frame->byte_offset(), return_location);
+    Address new_pc =
+        FindNewPC(frame, new_code, frame->byte_offset(), return_location);
     PointerAuthentication::ReplacePC(frame->pc_address(), new_pc,
                                      kSystemPointerSize);
     // The frame position should still be the same after OSR.