[liftoff][debug] Fix source position after OSR

The top wasm frame position can be inaccurate after removing a
breakpoint and OSRing the new code. This is because we are missing the
source position which was associated with that breakpoint in the old
code. Fix this by explicitly introducing the missing source position.

R=clemensb@chromium.org

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

src/wasm/baseline/liftoff-compiler.cc

@@ -686,10 +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_);
+        breakpoint = true;
         EmitBreakpoint(decoder);
       } else {
         while (next_breakpoint_ptr_ != next_breakpoint_end_ &&
@@ -700,12 +702,13 @@ class LiftoffCompiler {
         if (next_breakpoint_ptr_ == next_breakpoint_end_) {
           next_breakpoint_ptr_ = next_breakpoint_end_ = nullptr;
         } else if (*next_breakpoint_ptr_ == decoder->position()) {
+          breakpoint = true;
           EmitBreakpoint(decoder);
         }
       }
     }
     // Potentially generate the source position to OSR to this instruction.
-    MaybeGenerateExtraSourcePos(decoder);
+    MaybeGenerateExtraSourcePos(decoder, !breakpoint);
     TraceCacheState(decoder);
 #ifdef DEBUG
     SLOW_DCHECK(__ ValidateCacheState());
@@ -3085,7 +3088,8 @@ 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) {
+  void MaybeGenerateExtraSourcePos(Decoder* decoder,
+                                   bool emit_breakpoint_position = false) {
     if (V8_LIKELY(next_extra_source_pos_ptr_ == nullptr)) return;
     int position = static_cast<int>(decoder->position());
     while (*next_extra_source_pos_ptr_ < position) {
@@ -3096,6 +3100,26 @@ class LiftoffCompiler {
       }
     }
     if (*next_extra_source_pos_ptr_ != position) return;
+    if (emit_breakpoint_position) {
+      // Removing a breakpoint while paused on that breakpoint will OSR the
+      // return address as follows:
+      //   pos  instr
+      //   0    foo
+      //   1    call WasmDebugBreak
+      //   1    bar  // top frame return address
+      // becomes:
+      //   pos  instr
+      //   0    foo
+      //   1    nop  // top frame return address
+      //        bar
+      // {WasmCompiledFrame::position} would then return "0" as the source
+      // position of the top frame instead of "1".  This is fixed by explicitly
+      // emitting the missing position before the return address, with a nop so
+      // that code offsets do not collide.
+      source_position_table_builder_.AddPosition(
+          __ pc_offset(), SourcePosition(decoder->position()), false);
+      __ nop();
+    }
     source_position_table_builder_.AddPosition(
         __ pc_offset(), SourcePosition(decoder->position()), true);
     // Add a nop here, such that following code has another

src/wasm/wasm-debug.cc

@@ -922,10 +922,14 @@ class DebugInfoImpl {
       if (!old_code->is_liftoff()) return;
       int pc_offset =
           static_cast<int>(frame->pc() - old_code->instruction_start());
+      int position = frame->position();
       int byte_offset = FindByteOffset(pc_offset, old_code);
       Address new_pc = FindNewPC(new_code, byte_offset, return_location);
       PointerAuthentication::ReplacePC(frame->pc_address(), new_pc,
                                        kSystemPointerSize);
+      USE(position);
+      // The frame position should still be the same after OSR.
+      DCHECK_EQ(position, frame->position());
     }
   }