From 788bffd532b47d13c60437fda3995873aa85986f Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Fri, 27 Mar 2020 17:54:28 +0100 Subject: [PATCH] [liftoff][debug] Fix step in from JS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When stepping in from JS, the stepping frame ID will not be set. Instead of ensuring to set it properly, we can just skip the check for the frame ID. It was needed before, when we didn't properly reset stepping information. Now, it's redundant anyway. Also, ensure that we don't redirect to the interpreter if the --debug-in-liftoff flag is set. Drive-by: Fix and clang-format some parts of the test (no semantic change). R=thibaudm@chromium.org, szuend@chromium.org Bug: v8:10351 Change-Id: I58a3cd68937006c2d6b755a4465e793abcf8a20c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2124317 Reviewed-by: Simon Zünd Reviewed-by: Thibaud Michaud Commit-Queue: Clemens Backes Cr-Commit-Position: refs/heads/master@{#66904} --- src/debug/debug.cc | 8 +++-- src/wasm/wasm-debug.cc | 11 +++--- src/wasm/wasm-debug.h | 2 +- .../wasm-stepping-in-from-js-expected.txt | 2 +- .../debugger/wasm-stepping-in-from-js.js | 36 ++++++++++--------- test/inspector/inspector.status | 1 - 6 files changed, 32 insertions(+), 28 deletions(-) diff --git a/src/debug/debug.cc b/src/debug/debug.cc index 0beb2bb621..a7a3d2fd81 100644 --- a/src/debug/debug.cc +++ b/src/debug/debug.cc @@ -872,8 +872,9 @@ void Debug::PrepareStepIn(Handle function) { if (in_debug_scope()) return; if (break_disabled()) return; Handle shared(function->shared(), isolate_); - // If stepping from JS into Wasm, prepare for it. - if (shared->HasWasmExportedFunctionData()) { + // If stepping from JS into Wasm, and we are using the wasm interpreter for + // debugging, prepare the interpreter for step in. + if (shared->HasWasmExportedFunctionData() && !FLAG_debug_in_liftoff) { auto imported_function = Handle::cast(function); Handle wasm_instance(imported_function->instance(), isolate_); @@ -1053,7 +1054,8 @@ void Debug::PrepareStep(StepAction step_action) { wasm::WasmCodeRefScope code_ref_scope; wasm::WasmCode* code = wasm_frame->wasm_code(); if (code->is_liftoff()) { - wasm_frame->native_module()->GetDebugInfo()->PrepareStep(isolate_); + wasm_frame->native_module()->GetDebugInfo()->PrepareStep(isolate_, + frame_id); } // In case the wasm code returns, prepare the next frame (if JS) to break. step_action = StepOut; diff --git a/src/wasm/wasm-debug.cc b/src/wasm/wasm-debug.cc index 78cd30d53f..3cad919689 100644 --- a/src/wasm/wasm-debug.cc +++ b/src/wasm/wasm-debug.cc @@ -754,8 +754,8 @@ class DebugInfoImpl { current_isolate); } - void PrepareStep(Isolate* isolate) { - StackTraceFrameIterator it(isolate); + void PrepareStep(Isolate* isolate, StackFrameId break_frame_id) { + StackTraceFrameIterator it(isolate, break_frame_id); DCHECK(!it.done()); DCHECK(it.frame()->is_wasm_compiled()); WasmCompiledFrame* frame = WasmCompiledFrame::cast(it.frame()); @@ -787,8 +787,7 @@ class DebugInfoImpl { bool IsStepping(WasmCompiledFrame* 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 && stepping_frame_ != NO_ID); + return stepping_frame_ == frame->id() || last_step_action == StepIn; } void RemoveBreakpoint(int func_index, int position, @@ -998,7 +997,9 @@ void DebugInfo::SetBreakpoint(int func_index, int offset, impl_->SetBreakpoint(func_index, offset, current_isolate); } -void DebugInfo::PrepareStep(Isolate* isolate) { impl_->PrepareStep(isolate); } +void DebugInfo::PrepareStep(Isolate* isolate, StackFrameId break_frame_id) { + impl_->PrepareStep(isolate, break_frame_id); +} void DebugInfo::ClearStepping() { impl_->ClearStepping(); } diff --git a/src/wasm/wasm-debug.h b/src/wasm/wasm-debug.h index b28bc0264f..2611b7facc 100644 --- a/src/wasm/wasm-debug.h +++ b/src/wasm/wasm-debug.h @@ -153,7 +153,7 @@ class DebugInfo { void SetBreakpoint(int func_index, int offset, Isolate* current_isolate); - void PrepareStep(Isolate*); + void PrepareStep(Isolate*, StackFrameId); void ClearStepping(); diff --git a/test/inspector/debugger/wasm-stepping-in-from-js-expected.txt b/test/inspector/debugger/wasm-stepping-in-from-js-expected.txt index 841c688db1..89d64b9513 100644 --- a/test/inspector/debugger/wasm-stepping-in-from-js-expected.txt +++ b/test/inspector/debugger/wasm-stepping-in-from-js-expected.txt @@ -4,7 +4,7 @@ Calling instantiate function. Waiting for wasm scripts to be parsed. Ignoring script with url v8://test/callInstantiate Got wasm script: wasm://wasm/7d022e0e -Setting breakpoint on line 3 of wasm function +Setting breakpoint on i32.const { columnNumber : 37 lineNumber : 0 diff --git a/test/inspector/debugger/wasm-stepping-in-from-js.js b/test/inspector/debugger/wasm-stepping-in-from-js.js index f8c0b9dbbb..510c1c0073 100644 --- a/test/inspector/debugger/wasm-stepping-in-from-js.js +++ b/test/inspector/debugger/wasm-stepping-in-from-js.js @@ -2,7 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -let {session, contextGroup, Protocol} = InspectorTest.start('Tests stepping from javascript into wasm'); +let {session, contextGroup, Protocol} = + InspectorTest.start('Tests stepping from javascript into wasm'); session.setupScriptMap(); utils.load('test/mjsunit/wasm/wasm-module-builder.js'); @@ -11,14 +12,12 @@ let builder = new WasmModuleBuilder(); // wasm_A let func = builder.addFunction('wasm_A', kSig_i_i) - .addBody([ - // clang-format off - kExprLocalGet, 0, // Line 1: get input - kExprI32Const, 1, // Line 2: get constant 1 - kExprI32Sub // Line 3: decrease - // clang-format on - ]) - .exportAs('main'); + .addBody([ + kExprLocalGet, 0, // push param 0 + kExprI32Const, 1, // push constant 1 + kExprI32Sub // subtract + ]) + .exportAs('main'); let module_bytes = builder.toArray(); @@ -38,7 +37,7 @@ let evalWithUrl = (code, url) => Protocol.Runtime.evaluate( {'expression': code + '\n//# sourceURL=v8://test/' + url}); Protocol.Debugger.onPaused(async message => { - InspectorTest.log("paused"); + InspectorTest.log('paused'); var frames = message.params.callFrames; await session.logSourceLocation(frames[0].location); let action = step_actions.shift() || 'resume'; @@ -50,8 +49,7 @@ let step_actions = [ 'stepInto', // # debugger 'stepInto', // step into instance.exports.main(1) 'resume', // move to breakpoint - // then just resume. - 'resume', + 'resume', // then just resume. ]; contextGroup.addScript(` @@ -69,13 +67,17 @@ function test() { evalWithUrl( 'instantiate(' + JSON.stringify(module_bytes) + ')', 'callInstantiate'); const scriptId = await waitForWasmScript(); - InspectorTest.log( - 'Setting breakpoint on line 3 of wasm function'); - let msg = await Protocol.Debugger.setBreakpoint( - {'location': {'scriptId': scriptId, 'lineNumber': 0, 'columnNumber': 2 + func.body_offset}}); + InspectorTest.log('Setting breakpoint on i32.const'); + let msg = await Protocol.Debugger.setBreakpoint({ + 'location': { + 'scriptId': scriptId, + 'lineNumber': 0, + 'columnNumber': 2 + func.body_offset + } + }); printFailure(msg); InspectorTest.logMessage(msg.result.actualLocation); - await Protocol.Runtime.evaluate({ expression: 'test()' }); + await Protocol.Runtime.evaluate({expression: 'test()'}); InspectorTest.log('exports.main returned!'); InspectorTest.log('Finished!'); InspectorTest.completeTest(); diff --git a/test/inspector/inspector.status b/test/inspector/inspector.status index 603a117df0..aa74ca3dc4 100644 --- a/test/inspector/inspector.status +++ b/test/inspector/inspector.status @@ -23,7 +23,6 @@ # differences to the old behaviour (in particular, anyref is not # implemented in Liftoff yet). # TODO(clemensb/thibaudm): Get this list to zero and remove this block. - 'debugger/wasm-stepping-in-from-js': [FAIL], 'debugger/wasm-anyref-global': [FAIL], }],