[liftoff][debug] Generate missing source positions

We previously could not OSR a frame paused in a breakpoint with another
frame in which the same breakpoint was removed, because the latter was
missing the source position.
This change fixes this by iterating the stack to collect frame
positions, and emitting the corresponding source positions in Liftoff.

R=clemensb@chromium.org

Bug: v8:10321,v8:10147
Change-Id: I5a7950d5ce6e3cd5a0648b861db75f4f3dafa644
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2115433
Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66830}

src/wasm/baseline/liftoff-compiler.cc

@@ -300,7 +300,8 @@ class LiftoffCompiler {
                   CompilationEnv* env, Zone* compilation_zone,
                   std::unique_ptr<AssemblerBuffer> buffer,
                   DebugSideTableBuilder* debug_sidetable_builder,
-                  Vector<int> breakpoints = {})
+                  Vector<int> breakpoints = {},
+                  Vector<int> extra_source_pos = {})
       : asm_(std::move(buffer)),
         descriptor_(
             GetLoweredCallDescriptor(compilation_zone, call_descriptor)),
@@ -309,7 +310,9 @@ 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()),
+        next_extra_source_pos_ptr_(extra_source_pos.begin()),
+        next_extra_source_pos_end_(extra_source_pos.end()) {}
 
   bool did_bailout() const { return bailout_reason_ != kSuccess; }
   LiftoffBailoutReason bailout_reason() const { return bailout_reason_; }
@@ -683,11 +686,12 @@ class LiftoffCompiler {
   }
 
   void NextInstruction(FullDecoder* decoder, WasmOpcode opcode) {
+    bool breakpoint = false;
     if (V8_UNLIKELY(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_);
-        EmitBreakpoint(decoder);
+        breakpoint = true;
       } else {
         while (next_breakpoint_ptr_ != next_breakpoint_end_ &&
                *next_breakpoint_ptr_ < decoder->position()) {
@@ -697,10 +701,17 @@ class LiftoffCompiler {
         if (next_breakpoint_ptr_ == next_breakpoint_end_) {
           next_breakpoint_ptr_ = next_breakpoint_end_ = nullptr;
         } else if (*next_breakpoint_ptr_ == decoder->position()) {
-          EmitBreakpoint(decoder);
+          breakpoint = true;
         }
       }
     }
+    if (breakpoint) {
+      EmitBreakpoint(decoder);
+    } else if (opcode != kExprCallFunction) {
+      // There is no breakpoint and no call instruction, but we might still need
+      // a source position to get the return address for a removed breakpoint.
+      MaybeGenerateExtraSourcePos(decoder, false);
+    }
     TraceCacheState(decoder);
 #ifdef DEBUG
     SLOW_DCHECK(__ ValidateCacheState());
@@ -719,16 +730,9 @@ class LiftoffCompiler {
     source_position_table_builder_.AddPosition(
         __ pc_offset(), SourcePosition(decoder->position()), false);
     __ CallRuntimeStub(WasmCode::kWasmDebugBreak);
-    // This source position helps updating return addresses on the stack after
-    // installing a new Liftoff code object.
-    source_position_table_builder_.AddPosition(
-        __ pc_offset(), SourcePosition(decoder->position()), false);
     RegisterDebugSideTableEntry(DebugSideTableBuilder::kAllowRegisters);
+    MaybeGenerateExtraSourcePos(decoder);
     safepoint_table_builder_.DefineSafepoint(&asm_, Safepoint::kNoLazyDeopt);
-    // Add a nop after the breakpoint, such that following code has another PC
-    // and does not collide with the source position recorded above.
-    // TODO(thibaudm/clemens): Remove this.
-    __ nop();
   }
 
   void Block(FullDecoder* decoder, Control* block) {}
@@ -2126,16 +2130,7 @@ class LiftoffCompiler {
     RegisterDebugSideTableEntry(DebugSideTableBuilder::kDidSpill);
     safepoint_table_builder_.DefineSafepoint(&asm_, Safepoint::kNoLazyDeopt);
 
-    if (V8_UNLIKELY(env_->debug)) {
-      // This source position helps updating return addresses on the stack after
-      // installing a new Liftoff code object.
-      source_position_table_builder_.AddPosition(
-          __ pc_offset(), SourcePosition(decoder->position()), false);
-      // Add a nop after the call, such that following code has another PC
-      // and does not collide with the source position recorded above.
-      // TODO(thibaudm/clemens): Remove this.
-      __ nop();
-    }
+    MaybeGenerateExtraSourcePos(decoder);
 
     __ FinishCall(imm.sig, call_descriptor);
   }
@@ -2270,17 +2265,6 @@ class LiftoffCompiler {
     RegisterDebugSideTableEntry(DebugSideTableBuilder::kDidSpill);
     safepoint_table_builder_.DefineSafepoint(&asm_, Safepoint::kNoLazyDeopt);
 
-    if (V8_UNLIKELY(env_->debug)) {
-      // This source position helps updating return addresses on the stack after
-      // installing a new Liftoff code object.
-      source_position_table_builder_.AddPosition(
-          __ pc_offset(), SourcePosition(decoder->position()), false);
-      // Add a nop after the call, such that following code has another PC
-      // and does not collide with the source position recorded above.
-      // TODO(thibaudm/clemens): Remove this.
-      __ nop();
-    }
-
     __ FinishCall(imm.sig, call_descriptor);
   }
 
@@ -2975,6 +2959,38 @@ class LiftoffCompiler {
   }
 
  private:
+  // Emit additional source positions for return addresses. Used by debugging to
+  // OSR frames with different sets of breakpoints.
+  void MaybeGenerateExtraSourcePos(Decoder* decoder, bool after_call = true) {
+    if (V8_UNLIKELY(next_extra_source_pos_ptr_) &&
+        *next_extra_source_pos_ptr_ == static_cast<int>(decoder->position())) {
+      if (*next_extra_source_pos_ptr_ ==
+          static_cast<int>(decoder->position())) {
+        next_extra_source_pos_ptr_++;
+        if (next_extra_source_pos_ptr_ == next_extra_source_pos_end_) {
+          next_extra_source_pos_ptr_ = next_extra_source_pos_end_ = nullptr;
+        }
+        source_position_table_builder_.AddPosition(
+            __ pc_offset(), SourcePosition(decoder->position()), false);
+        // Add a nop after the breakpoint, such that following code has another
+        // PC and does not collide with the source position recorded above.
+        // TODO(thibaudm/clemens): Remove this.
+        __ nop();
+        if (!after_call) {
+          // A frame position is given by the last source position before the
+          // return address. If the return address does not follow a call, which
+          // happens after removing a breakpoint, this would be incorrect.
+          // Generate two source positions in this case. The second one will be
+          // used as the return address and the first one as the actual
+          // position.
+          source_position_table_builder_.AddPosition(
+              __ pc_offset(), SourcePosition(decoder->position()), false);
+          __ nop();
+        }
+      }
+    }
+  }
+
   static constexpr WasmOpcode kNoOutstandingOp = kExprUnreachable;
 
   LiftoffAssembler asm_;
@@ -3004,6 +3020,9 @@ class LiftoffCompiler {
   // function for stepping by flooding it with breakpoints.
   int* next_breakpoint_ptr_ = nullptr;
   int* next_breakpoint_end_ = nullptr;
+  // Use a similar approach to generate additional source positions.
+  int* next_extra_source_pos_ptr_ = nullptr;
+  int* next_extra_source_pos_end_ = nullptr;
 
   bool has_outstanding_op() const {
     return outstanding_op_ != kNoOutstandingOp;
@@ -3033,7 +3052,8 @@ WasmCompilationResult ExecuteLiftoffCompilation(
     AccountingAllocator* allocator, CompilationEnv* env,
     const FunctionBody& func_body, int func_index, Counters* counters,
     WasmFeatures* detected, Vector<int> breakpoints,
-    std::unique_ptr<DebugSideTable>* debug_sidetable) {
+    std::unique_ptr<DebugSideTable>* debug_sidetable,
+    Vector<int> extra_source_pos) {
   int func_body_size = static_cast<int>(func_body.end - func_body.start);
   TRACE_EVENT2(TRACE_DISABLED_BY_DEFAULT("v8.wasm"),
                "ExecuteLiftoffCompilation", "func_index", func_index,
@@ -3060,7 +3080,7 @@ 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(), breakpoints);
+      debug_sidetable_builder.get(), breakpoints, extra_source_pos);
   decoder.Decode();
   liftoff_compile_time_scope.reset();
   LiftoffCompiler* compiler = &decoder.interface();

src/wasm/baseline/liftoff-compiler.h

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

src/wasm/wasm-debug.cc

@@ -443,40 +443,56 @@ class InterpreterHandle {
   DISALLOW_COPY_AND_ASSIGN(InterpreterHandle);
 };
 
-Address FindNewPC(int offset, WasmCode* old_code, WasmCode* new_code) {
-  Vector<const uint8_t> old_pos_table = old_code->source_positions();
-  Vector<const uint8_t> new_pos_table = new_code->source_positions();
-
-  // Find the source position in the old code.
-  int old_source_pos = -1;
-  for (SourcePositionTableIterator old_it(old_pos_table); !old_it.done();
-       old_it.Advance()) {
-    if (old_it.code_offset() == offset) {
-      old_source_pos = old_it.source_position().ScriptOffset();
-      break;
-    }
+int FindByteOffset(int pc_offset, WasmCode* wasm_code) {
+  int position = 0;
+  SourcePositionTableIterator iterator(wasm_code->source_positions());
+  for (SourcePositionTableIterator iterator(wasm_code->source_positions());
+       !iterator.done() && iterator.code_offset() < pc_offset;
+       iterator.Advance()) {
+    position = iterator.source_position().ScriptOffset();
   }
-  DCHECK_LE(0, old_source_pos);
+  return position;
+}
 
-  // Find the matching source position in the new code.
-  SourcePositionTableIterator new_it(new_pos_table);
-  while (!new_it.done() &&
-         new_it.source_position().ScriptOffset() != old_source_pos) {
-    new_it.Advance();
-  }
-  if (new_it.done()) {
-    // TODO(thibaudm): Make Liftoff generate the missing source positions when
-    // removing breakpoints.
-    return 0;
+// 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;
+  WasmCodeRefScope code_ref_scope;
+  for (StackTraceFrameIterator it(isolate); !it.done(); it.Advance()) {
+    if (!it.is_wasm()) continue;
+    WasmCompiledFrame* frame = WasmCompiledFrame::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;
+    int pc_offset =
+        static_cast<int>(frame->pc() - wasm_code->instruction_start());
+    int byte_offset = FindByteOffset(pc_offset, wasm_code);
+    byte_offsets.push_back(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;
+}
 
-  // Each call instruction generates two source positions with the same source
-  // offset: one for the address of the call instruction and one for the
-  // return address. Skip the first one.
-  new_it.Advance();
-  DCHECK(!new_it.done());
-  DCHECK_EQ(new_it.source_position().ScriptOffset(), old_source_pos);
-  return new_code->instruction_start() + new_it.code_offset();
+Address FindNewPC(int byte_offset, WasmCode* wasm_code) {
+  Vector<const uint8_t> new_pos_table = wasm_code->source_positions();
+
+  DCHECK_LE(0, byte_offset);
+
+  SourcePositionTableIterator it(new_pos_table);
+  while (!it.done() && it.source_position().ScriptOffset() != byte_offset) {
+    it.Advance();
+  }
+  DCHECK(!it.done());
+  DCHECK_EQ(byte_offset, it.source_position().ScriptOffset());
+  // If there are two source positions with this offset, one is for the call and
+  // one for the return address. Get the return address.
+  it.Advance();
+  DCHECK(!it.done());
+  DCHECK_EQ(byte_offset, it.source_position().ScriptOffset());
+  return wasm_code->instruction_start() + it.code_offset();
 }
 
 }  // namespace
@@ -619,10 +635,15 @@ class DebugInfoImpl {
                       wire_bytes.begin() + function->code.end_offset()};
     std::unique_ptr<DebugSideTable> debug_sidetable;
 
+    // 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, current_isolate);
+
     WasmCompilationResult result;
-    result = ExecuteLiftoffCompilation(native_module_->engine()->allocator(),
-                                       &env, body, func_index, nullptr, nullptr,
-                                       offsets, &debug_sidetable);
+    result = ExecuteLiftoffCompilation(
+        native_module_->engine()->allocator(), &env, body, func_index, nullptr,
+        nullptr, offsets, &debug_sidetable, VectorOf(stack_frame_positions));
     // Liftoff compilation failure is a FATAL error. We rely on complete Liftoff
     // support for debugging.
     if (!result.succeeded()) FATAL("Liftoff compilation failed");
@@ -816,13 +837,12 @@ class DebugInfoImpl {
       if (frame->function_index() != new_code->index()) continue;
       WasmCode* old_code = frame->wasm_code();
       if (!old_code->is_liftoff()) return;
-      int offset =
+      int pc_offset =
           static_cast<int>(frame->pc() - old_code->instruction_start());
-      Address new_pc = FindNewPC(offset, old_code, new_code);
-      if (new_pc) {
-        PointerAuthentication::ReplacePC(frame->pc_address(), new_pc,
-                                         kSystemPointerSize);
-      }
+      int byte_offset = FindByteOffset(pc_offset, old_code);
+      Address new_pc = FindNewPC(byte_offset, new_code);
+      PointerAuthentication::ReplacePC(frame->pc_address(), new_pc,
+                                       kSystemPointerSize);
     }
   }