From 9743479196fc5cdb930f516d093ed525e3f19200 Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Thu, 28 May 2020 15:55:40 +0200 Subject: [PATCH] [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/+/2214838 Reviewed-by: Thibaud Michaud Commit-Queue: Clemens Backes Cr-Commit-Position: refs/heads/master@{#68045} --- src/runtime/runtime-wasm.cc | 4 +- src/wasm/wasm-debug.cc | 100 +++++++++++++------ src/wasm/wasm-debug.h | 4 +- src/wasm/wasm-engine.cc | 4 + test/debugger/debug/wasm/stepping-from-js.js | 68 +++++++++++++ 5 files changed, 145 insertions(+), 35 deletions(-) create mode 100644 test/debugger/debug/wasm/stepping-from-js.js diff --git a/src/runtime/runtime-wasm.cc b/src/runtime/runtime-wasm.cc index f842805afa..72b98fc6f2 100644 --- a/src/runtime/runtime-wasm.cc +++ b/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 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. diff --git a/src/wasm/wasm-debug.cc b/src/wasm/wasm-debug.cc index f31d654927..64f99ce9c9 100644 --- a/src/wasm/wasm-debug.cc +++ b/src/wasm/wasm-debug.cc @@ -485,10 +485,6 @@ class DebugInfoImpl { WasmCode* RecompileLiftoffWithBreakpoints( int func_index, Vector offsets, Vector 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 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 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 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 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 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(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> 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 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::New(Handle instance) { diff --git a/src/wasm/wasm-debug.h b/src/wasm/wasm-debug.h index 7bd78235e0..6050cb3a58 100644 --- a/src/wasm/wasm-debug.h +++ b/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 impl_; }; diff --git a/src/wasm/wasm-engine.cc b/src/wasm/wasm-engine.cc index 3d44c79d55..402467b5de 100644 --- a/src/wasm/wasm-engine.cc +++ b/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(); diff --git a/test/debugger/debug/wasm/stepping-from-js.js b/test/debugger/debug/wasm/stepping-from-js.js new file mode 100644 index 0000000000..6d2d5e4ef4 --- /dev/null +++ b/test/debugger/debug/wasm/stepping-from-js.js @@ -0,0 +1,68 @@ +// 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);