[wasm][debug] Support multi-threaded breakpoints

This adds support for multiple isolates sharing the same module but
setting different breakpoints. This is simulated by having a debugger
test that runs in the "--isolates" variant, i.e. two isolates running
the same test at the same time. Both isolates will set and remove
breakpoints.

The DebugInfo will keep a separate list of breakpoints per isolate, and
when recompiling a function for debugging it will respect all
breakpoints in all isolates.
In order to ensure consistency if multiple isolates are setting or
removing breakpoints simultaneously, we go back to a more coarse-grained
locking scheme, where the DebugInfo lock is held while re-compiling
Liftoff functions.

While recompilation will install the code in the module-global code
table and jump table (and hence all isolates will use it for future
calls), only the stack of the requesting isolate is rewritten to
immediately use new code. This is OK, because other isolates are not
interested in the new breakpoint(s) anyway.
On {SetBreakpoint}, we always need to rewrite the stack of the
requesting isolate though, even if the breakpoint was set before by
another isolate.

Drive-by: Some fixes in SharedFunctionInfo in order to support setting
breakpoints via the Debug mirror.

R=thibaudm@chromium.org

Bug: v8:10359
Change-Id: If659afb273260fc5e8124b4b617fb4322de473c7
Cq-Include-Trybots: luci.v8.try:v8_linux64_tsan_rel
Cq-Include-Trybots: luci.v8.try:v8_linux64_tsan_isolates_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2218059Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68096}

src/debug/debug.cc

@@ -820,7 +820,10 @@ void Debug::ClearAllBreakPoints() {
       HeapObject raw_wasm_script;
       if (wasm_scripts_with_breakpoints_->Get(idx).GetHeapObject(
               &raw_wasm_script)) {
-        WasmScript::ClearAllBreakpoints(Script::cast(raw_wasm_script));
+        Script wasm_script = Script::cast(raw_wasm_script);
+        WasmScript::ClearAllBreakpoints(wasm_script);
+        wasm_script.wasm_native_module()->GetDebugInfo()->RemoveIsolate(
+            isolate_);
       }
     }
     wasm_scripts_with_breakpoints_ = Handle<WeakArrayList>{};

src/objects/objects.cc

@@ -5527,13 +5527,21 @@ int SharedFunctionInfo::StartPosition() const {
     if (info.HasPositionInfo()) {
       return info.StartPosition();
     }
-  } else if (HasUncompiledData()) {
+  }
+  if (HasUncompiledData()) {
     // Works with or without scope.
     return uncompiled_data().start_position();
-  } else if (IsApiFunction() || HasBuiltinId()) {
+  }
+  if (IsApiFunction() || HasBuiltinId()) {
     DCHECK_IMPLIES(HasBuiltinId(), builtin_id() != Builtins::kCompileLazy);
     return 0;
   }
+  if (HasWasmExportedFunctionData()) {
+    WasmInstanceObject instance = wasm_exported_function_data().instance();
+    int func_index = wasm_exported_function_data().function_index();
+    auto& function = instance.module()->functions[func_index];
+    return static_cast<int>(function.code.offset());
+  }
   return kNoSourcePosition;
 }
 
@@ -5544,13 +5552,21 @@ int SharedFunctionInfo::EndPosition() const {
     if (info.HasPositionInfo()) {
       return info.EndPosition();
     }
-  } else if (HasUncompiledData()) {
+  }
+  if (HasUncompiledData()) {
     // Works with or without scope.
     return uncompiled_data().end_position();
-  } else if (IsApiFunction() || HasBuiltinId()) {
+  }
+  if (IsApiFunction() || HasBuiltinId()) {
     DCHECK_IMPLIES(HasBuiltinId(), builtin_id() != Builtins::kCompileLazy);
     return 0;
   }
+  if (HasWasmExportedFunctionData()) {
+    WasmInstanceObject instance = wasm_exported_function_data().instance();
+    int func_index = wasm_exported_function_data().function_index();
+    auto& function = instance.module()->functions[func_index];
+    return static_cast<int>(function.code.end_offset());
+  }
   return kNoSourcePosition;
 }
 

src/runtime/runtime-debug.cc

@@ -495,7 +495,8 @@ int ScriptLinePosition(Handle<Script> script, int line) {
   if (line < 0) return -1;
 
   if (script->type() == Script::TYPE_WASM) {
-    return GetWasmFunctionOffset(script->wasm_native_module()->module(), line);
+    // Wasm positions are relative to the start of the module.
+    return 0;
   }
 
   Script::InitLineEnds(script->GetIsolate(), script);

src/wasm/wasm-code-manager.cc

@@ -1890,14 +1890,19 @@ void NativeModule::FreeCode(Vector<WasmCode* const> codes) {
   // Free the code space.
   code_allocator_.FreeCode(codes);
 
-  base::MutexGuard guard(&allocation_mutex_);
-  // Remove debug side tables for all removed code objects.
-  if (debug_info_) debug_info_->RemoveDebugSideTables(codes);
-  // Free the {WasmCode} objects. This will also unregister trap handler data.
-  for (WasmCode* code : codes) {
-    DCHECK_EQ(1, owned_code_.count(code->instruction_start()));
-    owned_code_.erase(code->instruction_start());
+  DebugInfo* debug_info = nullptr;
+  {
+    base::MutexGuard guard(&allocation_mutex_);
+    debug_info = debug_info_.get();
+    // Free the {WasmCode} objects. This will also unregister trap handler data.
+    for (WasmCode* code : codes) {
+      DCHECK_EQ(1, owned_code_.count(code->instruction_start()));
+      owned_code_.erase(code->instruction_start());
+    }
   }
+  // Remove debug side tables for all removed code objects, after releasing our
+  // lock. This is to avoid lock order inversion.
+  if (debug_info) debug_info->RemoveDebugSideTables(codes);
 }
 
 size_t NativeModule::GetNumberOfCodeSpacesForTesting() const {

src/wasm/wasm-debug.cc

@@ -241,7 +241,6 @@ class InterpreterHandle {
 // 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;
     WasmFrame* frame = WasmFrame::cast(it.frame());
@@ -485,6 +484,7 @@ class DebugInfoImpl {
 
   WasmCode* RecompileLiftoffWithBreakpoints(
       int func_index, Vector<int> offsets, Vector<int> extra_source_positions) {
+    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_}.
     CompilationEnv env = native_module_->CreateCompilationEnv();
@@ -510,54 +510,61 @@ class DebugInfoImpl {
         native_module_->AddCompiledCode(std::move(result)));
 
     DCHECK(new_code->is_inspectable());
-    {
-      base::MutexGuard guard(&mutex_);
-      DCHECK_EQ(0, debug_side_tables_.count(new_code));
-      debug_side_tables_.emplace(new_code, std::move(debug_sidetable));
-    }
+    DCHECK_EQ(0, debug_side_tables_.count(new_code));
+    debug_side_tables_.emplace(new_code, std::move(debug_sidetable));
 
     return new_code;
   }
 
   void SetBreakpoint(int func_index, int offset, Isolate* isolate) {
-    std::vector<int> breakpoints_copy;
-    StackFrameId stepping_frame = NO_ID;
-    {
-      // Hold the mutex while modifying the set of breakpoints, but release it
-      // before compiling the new code (see comment in
-      // {RecompileLiftoffWithBreakpoints}). This needs to be revisited once we
-      // support setting different breakpoints in different isolates
-      // (https://crbug.com/v8/10351).
-      base::MutexGuard guard(&mutex_);
-
-      // offset == 0 indicates flooding and should not happen here.
-      DCHECK_NE(0, offset);
+    // 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;
 
-      std::vector<int>& breakpoints = breakpoints_per_function_[func_index];
-      auto insertion_point =
-          std::lower_bound(breakpoints.begin(), breakpoints.end(), offset);
-      if (insertion_point != breakpoints.end() && *insertion_point == offset) {
-        // The breakpoint is already set.
-        return;
-      }
-      breakpoints.insert(insertion_point, offset);
-      breakpoints_copy = breakpoints;
+    // Hold the mutex while modifying breakpoints, to ensure consistency when
+    // multiple isolates set/remove breakpoints at the same time.
+    base::MutexGuard guard(&mutex_);
 
-      stepping_frame = per_isolate_data_[isolate].stepping_frame;
+    // offset == 0 indicates flooding and should not happen here.
+    DCHECK_NE(0, offset);
+
+    auto& isolate_data = per_isolate_data_[isolate];
+    std::vector<int>& breakpoints =
+        isolate_data.breakpoints_per_function[func_index];
+    auto insertion_point =
+        std::lower_bound(breakpoints.begin(), breakpoints.end(), offset);
+    if (insertion_point != breakpoints.end() && *insertion_point == offset) {
+      // The breakpoint is already set for this isolate.
+      return;
     }
+    breakpoints.insert(insertion_point, offset);
+
+    // TODO(clemensb): Avoid recompilation if the breakpoint was already set in
+    // another isolate.
+    std::vector<int> all_breakpoints = FindAllBreakpoints(func_index);
+    UpdateBreakpoints(func_index, VectorOf(all_breakpoints), isolate,
+                      isolate_data.stepping_frame);
+  }
 
-    UpdateBreakpoints(func_index, VectorOf(breakpoints_copy), isolate,
-                      stepping_frame);
+  std::vector<int> FindAllBreakpoints(int func_index) {
+    DCHECK(!mutex_.TryLock());  // Mutex must be held externally.
+    std::set<int> breakpoints;
+    for (auto& data : per_isolate_data_) {
+      auto it = data.second.breakpoints_per_function.find(func_index);
+      if (it == data.second.breakpoints_per_function.end()) continue;
+      for (int offset : it->second) breakpoints.insert(offset);
+    }
+    return {breakpoints.begin(), breakpoints.end()};
   }
 
   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);
 
-    WasmCodeRefScope wasm_code_ref_scope;
     WasmCode* new_code = RecompileLiftoffWithBreakpoints(
         func_index, breakpoints, VectorOf(stack_frame_positions));
     UpdateReturnAddresses(isolate, new_code, stepping_frame);
@@ -570,6 +577,7 @@ class DebugInfoImpl {
     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));
@@ -618,27 +626,33 @@ class DebugInfoImpl {
   }
 
   void RemoveBreakpoint(int func_index, int position, Isolate* isolate) {
-    std::vector<int> breakpoints_copy;
-    StackFrameId stepping_frame = NO_ID;
-    {
-      base::MutexGuard guard(&mutex_);
-      const auto& function = native_module_->module()->functions[func_index];
-      int offset = position - function.code.offset();
-
-      std::vector<int>& breakpoints = breakpoints_per_function_[func_index];
-      DCHECK_LT(0, offset);
-      auto insertion_point =
-          std::lower_bound(breakpoints.begin(), breakpoints.end(), offset);
-      if (insertion_point == breakpoints.end()) return;
-      if (*insertion_point != offset) return;
-      breakpoints.erase(insertion_point);
-      breakpoints_copy = breakpoints;
-
-      stepping_frame = per_isolate_data_[isolate].stepping_frame;
-    }
+    // 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;
+
+    // Hold the mutex while modifying breakpoints, to ensure consistency when
+    // multiple isolates set/remove breakpoints at the same time.
+    base::MutexGuard guard(&mutex_);
 
-    UpdateBreakpoints(func_index, VectorOf(breakpoints_copy), isolate,
-                      stepping_frame);
+    const auto& function = native_module_->module()->functions[func_index];
+    int offset = position - function.code.offset();
+
+    auto& isolate_data = per_isolate_data_[isolate];
+    std::vector<int>& breakpoints =
+        isolate_data.breakpoints_per_function[func_index];
+    DCHECK_LT(0, offset);
+    auto insertion_point =
+        std::lower_bound(breakpoints.begin(), breakpoints.end(), offset);
+    if (insertion_point == breakpoints.end()) return;
+    if (*insertion_point != offset) return;
+    breakpoints.erase(insertion_point);
+
+    std::vector<int> remaining = FindAllBreakpoints(func_index);
+    // 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;
+    UpdateBreakpoints(func_index, VectorOf(remaining), isolate,
+                      isolate_data.stepping_frame);
   }
 
   void RemoveDebugSideTables(Vector<WasmCode* const> codes) {
@@ -654,9 +668,36 @@ class DebugInfoImpl {
     return it == debug_side_tables_.end() ? nullptr : it->second.get();
   }
 
+  static bool HasRemovedBreakpoints(const std::vector<int>& removed,
+                                    const std::vector<int>& remaining) {
+    DCHECK(std::is_sorted(remaining.begin(), remaining.end()));
+    for (int offset : removed) {
+      // Return true if we removed a breakpoint which is not part of remaining.
+      if (!std::binary_search(remaining.begin(), remaining.end(), offset)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
   void RemoveIsolate(Isolate* isolate) {
+    // 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;
+
     base::MutexGuard guard(&mutex_);
-    per_isolate_data_.erase(isolate);
+    auto per_isolate_data_it = per_isolate_data_.find(isolate);
+    if (per_isolate_data_it == per_isolate_data_.end()) return;
+    std::unordered_map<int, std::vector<int>> removed_per_function;
+    for (auto& entry : per_isolate_data_it->second.breakpoints_per_function) {
+      int func_index = entry.first;
+      std::vector<int>& removed = entry.second;
+      std::vector<int> remaining = FindAllBreakpoints(func_index);
+      if (HasRemovedBreakpoints(removed, remaining)) {
+        RecompileLiftoffWithBreakpoints(func_index, VectorOf(remaining), {});
+      }
+    }
+    per_isolate_data_.erase(per_isolate_data_it);
   }
 
  private:
@@ -838,11 +879,13 @@ class DebugInfoImpl {
   // Isolate-specific data, for debugging modules that are shared by multiple
   // isolates.
   struct PerIsolateDebugData {
+    // Keeps track of the currently set breakpoints (by offset within that
+    // function).
+    std::unordered_map<int, std::vector<int>> breakpoints_per_function;
+
     // Store the frame ID when stepping, to avoid overwriting that frame when
     // setting or removing a breakpoint.
     StackFrameId stepping_frame = NO_ID;
-
-    // TODO(clemensb): Also move breakpoint here.
   };
 
   NativeModule* const native_module_;
@@ -857,11 +900,6 @@ class DebugInfoImpl {
   // Names of locals, lazily decoded from the wire bytes.
   std::unique_ptr<LocalNames> local_names_;
 
-  // Keeps track of the currently set breakpoints (by offset within that
-  // function).
-  // TODO(clemensb): Move this into {PerIsolateDebugData}.
-  std::unordered_map<int, std::vector<int>> breakpoints_per_function_;
-
   // Isolate-specific data.
   std::unordered_map<Isolate*, PerIsolateDebugData> per_isolate_data_;
 

test/debugger/debug/wasm/breakpoints.js

+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Test setting and removing breakpoints in Wasm.
+// Similar tests exist as inspector tests already, but inspector tests are not
+// run concurrently in multiple isolates (see `run-tests.py --isolates`).
+
+load('test/mjsunit/wasm/wasm-module-builder.js');
+
+const builder = new WasmModuleBuilder();
+const body_a = [
+  kExprLocalGet, 0,  // local.get i0
+  kExprI32Const, 1,  // i32.const 1
+  kExprI32Add        // i32.add i0 1
+];
+const fun_a = builder.addFunction('a', kSig_i_i).addBody(body_a).exportFunc();
+const body_b = [
+  kExprLocalGet, 0,                // local.get i0
+  kExprCallFunction, fun_a.index,  // call a
+  kExprLocalGet, 1,                // local.get i1
+  kExprI32Sub,                     // i32.sub a(i0) i1
+  kExprCallFunction, fun_a.index,  // call a
+];
+const fun_b = builder.addFunction('b', kSig_i_ii).addBody(body_b).exportFunc();
+
+const instance = builder.instantiate();
+
+Debug = debug.Debug;
+
+const a_localget_offset = body_a.indexOf(kExprLocalGet);
+const a_const_offset = body_a.indexOf(kExprI32Const);
+const a_add_offset = body_a.indexOf(kExprI32Add);
+const b_sub_offset = body_b.indexOf(kExprI32Sub);
+
+const expected_breaks = [
+  `a:1:${fun_a.body_offset + a_add_offset}`,       // break in a at i32.add
+  `b:1:${fun_b.body_offset + b_sub_offset}`,       // break in b at i32.sub
+  `a:1:${fun_a.body_offset + a_localget_offset}`,  // break in a at local.get i0
+  `a:1:${fun_a.body_offset + a_const_offset}`      // break in a at i32.const 1
+];
+let error;
+function onBreak(event, exec_state, data) {
+  try {
+    if (event != Debug.DebugEvent.Break) return;
+    if (error) return;
+    const pos =
+        [data.functionName(), data.sourceLine(), data.sourceColumn()].join(':');
+    const loc = [pos, data.sourceLineText()].join(':');
+    print(`Break at ${loc}`);
+    assertTrue(expected_breaks.length > 0, 'expecting more breaks');
+    const expected_pos = expected_breaks.shift();
+    assertEquals(expected_pos, pos);
+    // When we stop in b, we add another breakpoint in a at offset 0 and remove
+    // the existing breakpoint.
+    if (data.functionName() == 'b') {
+      Debug.setBreakPoint(instance.exports.a, 0, a_localget_offset);
+      Debug.clearBreakPoint(breakpoint_a);
+    }
+    // When we stop at a at local.get, we set another breakpoint *in the same
+    // function*, one instruction later (at i32.const).
+    if (data.functionName() == 'a' &&
+        data.sourceColumn() == fun_a.body_offset) {
+      Debug.setBreakPoint(instance.exports.a, 0, a_const_offset);
+    }
+  } catch (e) {
+    if (!error) error = e;
+  }
+}
+
+Debug.setListener(onBreak);
+const breakpoint_a = Debug.setBreakPoint(instance.exports.a, 0, a_add_offset);
+const breakpoint_b = Debug.setBreakPoint(instance.exports.b, 0, b_sub_offset);
+print('Running b(11).');
+const result = instance.exports.b(11);
+print('Returned from wasm.');
+if (error) throw error;
+assertEquals(0, expected_breaks.length, 'all breaks were hit');