[wasm][debug] Support multi-threaded stepping

Instead of keeping a single {stepping_frame_} per native module, we now
keep one frame id per isolate. Hence, each isolate can step through a
different frame, independent of other isolates.
The on-stack-replacement of the stepping frame already works on a
per-isolate basis, since we only replace the return address of a single
frame, part of the isolate that requested stepping.

The new test (which also executes in a variant with two concurrent
isolates) revealed some more data races to fix.

R=thibaudm@chromium.org

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

src/runtime/runtime-wasm.cc

@@ -450,7 +450,7 @@ RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
   WasmFrame* frame = frame_finder.frame();
   auto* debug_info = frame->native_module()->GetDebugInfo();
   if (debug_info->IsStepping(frame)) {
-    debug_info->ClearStepping();
+    debug_info->ClearStepping(isolate);
     isolate->debug()->ClearStepping();
     isolate->debug()->OnDebugBreak(isolate->factory()->empty_fixed_array());
     return undefined;
@@ -461,7 +461,7 @@ RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
   Handle<FixedArray> breakpoints;
   if (WasmScript::CheckBreakPoints(isolate, script, position)
           .ToHandle(&breakpoints)) {
-    debug_info->ClearStepping();
+    debug_info->ClearStepping(isolate);
     isolate->debug()->ClearStepping();
     if (isolate->debug()->break_points_active()) {
       // We hit one or several breakpoints. Notify the debug listeners.

src/wasm/wasm-debug.cc

@@ -485,10 +485,6 @@ class DebugInfoImpl {
 
   WasmCode* RecompileLiftoffWithBreakpoints(
       int func_index, Vector<int> offsets, Vector<int> extra_source_positions) {
-    // During compilation, we cannot hold the lock, since compilation takes the
-    // {NativeModule} lock, which could lead to deadlocks.
-    mutex_.AssertUnheld();
-
     // Recompile the function with Liftoff, setting the new breakpoints.
     // Not thread-safe. The caller is responsible for locking {mutex_}.
     CompilationEnv env = native_module_->CreateCompilationEnv();
@@ -514,16 +510,18 @@ class DebugInfoImpl {
         native_module_->AddCompiledCode(std::move(result)));
 
     DCHECK(new_code->is_inspectable());
-    bool added =
-        debug_side_tables_.emplace(new_code, std::move(debug_sidetable)).second;
-    DCHECK(added);
-    USE(added);
+    {
+      base::MutexGuard guard(&mutex_);
+      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* current_isolate) {
+  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
@@ -544,26 +542,28 @@ class DebugInfoImpl {
       }
       breakpoints.insert(insertion_point, offset);
       breakpoints_copy = breakpoints;
+
+      stepping_frame = per_isolate_data_[isolate].stepping_frame;
     }
 
-    UpdateBreakpoints(func_index, VectorOf(breakpoints_copy), current_isolate);
+    UpdateBreakpoints(func_index, VectorOf(breakpoints_copy), isolate,
+                      stepping_frame);
   }
 
   void UpdateBreakpoints(int func_index, Vector<int> breakpoints,
-                         Isolate* current_isolate) {
+                         Isolate* isolate, StackFrameId stepping_frame) {
     // 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);
+        StackFramePositions(func_index, isolate);
 
     WasmCodeRefScope wasm_code_ref_scope;
     WasmCode* new_code = RecompileLiftoffWithBreakpoints(
         func_index, breakpoints, VectorOf(stack_frame_positions));
-    UpdateReturnAddresses(current_isolate, new_code);
+    UpdateReturnAddresses(isolate, new_code, stepping_frame);
   }
 
-  void FloodWithBreakpoints(WasmFrame* frame, Isolate* current_isolate,
-                            ReturnLocation return_location) {
+  void FloodWithBreakpoints(WasmFrame* frame, ReturnLocation return_location) {
     // 0 is an invalid offset used to indicate flooding.
     int offset = 0;
     WasmCodeRefScope wasm_code_ref_scope;
@@ -596,21 +596,30 @@ class DebugInfoImpl {
       return_location = kAfterWasmCall;
     }
 
-    FloodWithBreakpoints(frame, isolate, return_location);
-    stepping_frame_ = frame->id();
+    FloodWithBreakpoints(frame, return_location);
+
+    base::MutexGuard guard(&mutex_);
+    per_isolate_data_[isolate].stepping_frame = frame->id();
   }
 
-  void ClearStepping() { stepping_frame_ = NO_ID; }
+  void ClearStepping(Isolate* isolate) {
+    base::MutexGuard guard(&mutex_);
+    auto it = per_isolate_data_.find(isolate);
+    if (it != per_isolate_data_.end()) it->second.stepping_frame = NO_ID;
+  }
 
   bool IsStepping(WasmFrame* frame) {
     Isolate* isolate = frame->wasm_instance().GetIsolate();
-    StepAction last_step_action = isolate->debug()->last_step_action();
-    return stepping_frame_ == frame->id() || last_step_action == StepIn;
+    if (isolate->debug()->last_step_action() == StepIn) return true;
+    base::MutexGuard guard(&mutex_);
+    auto it = per_isolate_data_.find(isolate);
+    return it != per_isolate_data_.end() &&
+           it->second.stepping_frame == frame->id();
   }
 
-  void RemoveBreakpoint(int func_index, int position,
-                        Isolate* current_isolate) {
+  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];
@@ -624,9 +633,12 @@ class DebugInfoImpl {
       if (*insertion_point != offset) return;
       breakpoints.erase(insertion_point);
       breakpoints_copy = breakpoints;
+
+      stepping_frame = per_isolate_data_[isolate].stepping_frame;
     }
 
-    UpdateBreakpoints(func_index, VectorOf(breakpoints_copy), current_isolate);
+    UpdateBreakpoints(func_index, VectorOf(breakpoints_copy), isolate,
+                      stepping_frame);
   }
 
   void RemoveDebugSideTables(Vector<WasmCode* const> codes) {
@@ -642,6 +654,11 @@ class DebugInfoImpl {
     return it == debug_side_tables_.end() ? nullptr : it->second.get();
   }
 
+  void RemoveIsolate(Isolate* isolate) {
+    base::MutexGuard guard(&mutex_);
+    per_isolate_data_.erase(isolate);
+  }
+
  private:
   struct FrameInspectionScope {
     FrameInspectionScope(DebugInfoImpl* debug_info, Address pc)
@@ -691,10 +708,13 @@ class DebugInfoImpl {
         GenerateLiftoffDebugSideTable(allocator, &env, func_body);
     DebugSideTable* ret = debug_side_table.get();
 
-    // Install into cache and return.
+    // Check cache again, maybe another thread concurrently generated a debug
+    // side table already.
     {
       base::MutexGuard guard(&mutex_);
-      debug_side_tables_[code] = std::move(debug_side_table);
+      auto& slot = debug_side_tables_[code];
+      if (slot != nullptr) return slot.get();
+      slot = std::move(debug_side_table);
     }
 
     // Print the code together with the debug table, if requested.
@@ -768,15 +788,15 @@ class DebugInfoImpl {
   // After installing a Liftoff code object with a different set of breakpoints,
   // update return addresses on the stack so that execution resumes in the new
   // code. The frame layout itself should be independent of breakpoints.
-  // TODO(thibaudm): update other threads as well.
-  void UpdateReturnAddresses(Isolate* isolate, WasmCode* new_code) {
+  void UpdateReturnAddresses(Isolate* isolate, WasmCode* new_code,
+                             StackFrameId stepping_frame) {
     // The first return location is after the breakpoint, others are after wasm
     // calls.
     ReturnLocation return_location = kAfterBreakpoint;
     for (StackTraceFrameIterator it(isolate); !it.done();
          it.Advance(), return_location = kAfterWasmCall) {
       // We still need the flooded function for stepping.
-      if (it.frame()->id() == stepping_frame_) continue;
+      if (it.frame()->id() == stepping_frame) continue;
       if (!it.is_wasm()) continue;
       WasmFrame* frame = WasmFrame::cast(it.frame());
       if (frame->native_module() != new_code->native_module()) continue;
@@ -815,6 +835,16 @@ class DebugInfoImpl {
     return static_cast<size_t>(position) == code.end_offset() - 1;
   }
 
+  // Isolate-specific data, for debugging modules that are shared by multiple
+  // isolates.
+  struct PerIsolateDebugData {
+    // 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_;
 
   // {mutex_} protects all fields below.
@@ -829,11 +859,11 @@ class DebugInfoImpl {
 
   // 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_;
 
-  // Store the frame ID when stepping, to avoid overwriting that frame when
-  // setting or removing a breakpoint.
-  StackFrameId stepping_frame_ = NO_ID;
+  // Isolate-specific data.
+  std::unordered_map<Isolate*, PerIsolateDebugData> per_isolate_data_;
 
   DISALLOW_COPY_AND_ASSIGN(DebugInfoImpl);
 };
@@ -882,7 +912,9 @@ void DebugInfo::PrepareStep(Isolate* isolate, StackFrameId break_frame_id) {
   impl_->PrepareStep(isolate, break_frame_id);
 }
 
-void DebugInfo::ClearStepping() { impl_->ClearStepping(); }
+void DebugInfo::ClearStepping(Isolate* isolate) {
+  impl_->ClearStepping(isolate);
+}
 
 bool DebugInfo::IsStepping(WasmFrame* frame) {
   return impl_->IsStepping(frame);
@@ -902,6 +934,10 @@ DebugSideTable* DebugInfo::GetDebugSideTableIfExists(
   return impl_->GetDebugSideTableIfExists(code);
 }
 
+void DebugInfo::RemoveIsolate(Isolate* isolate) {
+  return impl_->RemoveIsolate(isolate);
+}
+
 }  // namespace wasm
 
 Handle<WasmDebugInfo> WasmDebugInfo::New(Handle<WasmInstanceObject> instance) {

src/wasm/wasm-debug.h

@@ -168,7 +168,7 @@ class V8_EXPORT_PRIVATE DebugInfo {
 
   void PrepareStep(Isolate*, StackFrameId);
 
-  void ClearStepping();
+  void ClearStepping(Isolate*);
 
   bool IsStepping(WasmFrame*);
 
@@ -180,6 +180,8 @@ class V8_EXPORT_PRIVATE DebugInfo {
   // already been created. This will never trigger generation of the table.
   DebugSideTable* GetDebugSideTableIfExists(const WasmCode*) const;
 
+  void RemoveIsolate(Isolate*);
+
  private:
   std::unique_ptr<DebugInfoImpl> impl_;
 };

src/wasm/wasm-engine.cc

@@ -22,6 +22,7 @@
 #include "src/wasm/module-decoder.h"
 #include "src/wasm/module-instantiate.h"
 #include "src/wasm/streaming-decoder.h"
+#include "src/wasm/wasm-debug.h"
 #include "src/wasm/wasm-limits.h"
 #include "src/wasm/wasm-objects-inl.h"
 
@@ -924,6 +925,9 @@ void WasmEngine::RemoveIsolate(Isolate* isolate) {
         current_gc_info_->dead_code.erase(code);
       }
     }
+    if (native_module->HasDebugInfo()) {
+      native_module->GetDebugInfo()->RemoveIsolate(isolate);
+    }
   }
   if (current_gc_info_) {
     if (RemoveIsolateFromCurrentGC(isolate)) PotentiallyFinishCurrentGC();

test/debugger/debug/wasm/stepping-from-js.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 stepping from JS through Wasm.
+// A similar test exists as an inspector test 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 imp_fun = builder.addImport('imp', 'ort', kSig_i_v);
+const sub_fun = builder.addFunction('sub', kSig_i_ii).addBody([
+  kExprLocalGet, 0,  // local.get i0
+  kExprLocalGet, 1,  // local.get i1
+  kExprI32Sub        // i32.sub i0 i1
+]);
+builder.addFunction('main', kSig_i_v)
+    .addBody([
+      kExprCallFunction, imp_fun,       // call import
+      kExprI32Const, 3,                 // i32.const 3
+      kExprCallFunction, sub_fun.index  // call 'sub'
+    ])
+    .exportFunc();
+// Compute the line number of the 'imported' function (to avoid hard-coding it).
+const import_line_nr = parseInt((new Error()).stack.match(/:([0-9]+):/)[1]) + 1;
+function imported() {
+  debugger;
+  return 7;
+}
+const instance = builder.instantiate({imp: {ort: imported}});
+
+Debug = debug.Debug;
+
+const expected_breaks = [
+  `imported:${import_line_nr + 1}:2`,   // debugger;
+  `imported:${import_line_nr + 2}:2`,   // return 7;
+  `imported:${import_line_nr + 2}:11`,  // return 7;
+  'sub:1:58', 'sub:1:60', 'sub:1:62', 'sub:1:63', 'main:1:72'
+];
+let error;
+function onBreak(event, exec_state, data) {
+  try {
+    if (event != Debug.DebugEvent.Break) return;
+    if (error) return;
+    if (data.sourceLineText().indexOf('Main returned.') >= 0) {
+      assertEquals(0, expected_breaks.length, 'hit all expected breaks');
+      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);
+    exec_state.prepareStep(Debug.StepAction.StepIn);
+  } catch (e) {
+    if (!error) error = e;
+  }
+}
+
+Debug.setListener(onBreak);
+print('Running main.');
+const result = instance.exports.main();
+print('Main returned.');
+if (error) throw error;
+assertEquals(4, result);