Commit 5cf61684 authored by Clemens Backes's avatar Clemens Backes Committed by Commit Bot

Reland "[wasm] Prevent breakpoints on nonbreakable positions"

This is an unmodified reland of 3c98a2a3.
The actual issue was fixed in https://crrev.com/c/1926769.

Original change's description:
> [wasm] Prevent breakpoints on nonbreakable positions
>
> If a breakpoint is set on a non-breakable position, the wasm interpreter
> just stores the value 0xFF (kInternalBreakpoint) in the function body
> (actually, a copy of the function body). This might overwrite immediates
> and cause subsequent failures in the wasm interpreter.
>
> In JavaScript, breakpoints are just forwarded to the next breakable
> position. This CL implements the same for WebAssembly.
> A cctest tests this behavior, and the existing
> wasm-stepping-byte-offsets.js inspector test is extended to also set the
> breakpoint within an i32 constant immediate.
>
> R=leese@chromium.org, mstarzinger@chromium.org
> CC=​bmeurer@chromium.org
>
> Bug: chromium:1025184
> Change-Id: Ia2706f8f1c3d686cbbe8e1e7339d9ee86247bb4a
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1925152
> Commit-Queue: Clemens Backes <clemensb@chromium.org>
> Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
> Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#65070}

Bug: chromium:1025184
Change-Id: I5e16df645bbacf039b7a5e55a0c2a64cdb4c6a32
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1926152
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: 's avatarBenedikt Meurer <bmeurer@chromium.org>
Reviewed-by: 's avatarMichael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65093}
parent dbfb14bf
......@@ -674,9 +674,12 @@ Handle<Code> WasmDebugInfo::GetCWasmEntry(Handle<WasmDebugInfo> debug_info,
namespace {
#ifdef DEBUG
bool IsBreakablePosition(wasm::NativeModule* native_module, int func_index,
int offset_in_func) {
// Return the next breakable position after {offset_in_func} in function
// {func_index}, or 0 if there is none.
// Note that 0 is never a breakable position in wasm, since the first byte
// contains the locals count for the function.
int FindNextBreakablePosition(wasm::NativeModule* native_module, int func_index,
int offset_in_func) {
AccountingAllocator alloc;
Zone tmp(&alloc, ZONE_NAME);
wasm::BodyLocalDecls locals(&tmp);
......@@ -687,13 +690,12 @@ bool IsBreakablePosition(wasm::NativeModule* native_module, int func_index,
module_start + func.code.end_offset(),
&locals);
DCHECK_LT(0, locals.encoded_size);
if (offset_in_func < 0) return 0;
for (uint32_t offset : iterator.offsets()) {
if (offset > static_cast<uint32_t>(offset_in_func)) break;
if (offset == static_cast<uint32_t>(offset_in_func)) return true;
if (offset >= static_cast<uint32_t>(offset_in_func)) return offset;
}
return false;
return 0;
}
#endif // DEBUG
} // namespace
......@@ -709,10 +711,10 @@ bool WasmScript::SetBreakPoint(Handle<Script> script, int* position,
const wasm::WasmFunction& func = module->functions[func_index];
int offset_in_func = *position - func.code.offset();
// According to the current design, we should only be called with valid
// breakable positions.
DCHECK(IsBreakablePosition(script->wasm_native_module(), func_index,
offset_in_func));
int breakable_offset = FindNextBreakablePosition(script->wasm_native_module(),
func_index, offset_in_func);
if (breakable_offset == 0) return false;
*position = func.code.offset() + breakable_offset;
// Insert new break point into break_positions of module object.
WasmScript::AddBreakpointToInfo(script, *position, break_point);
......@@ -729,7 +731,7 @@ bool WasmScript::SetBreakPoint(Handle<Script> script, int* position,
isolate);
Handle<WasmDebugInfo> debug_info =
WasmInstanceObject::GetOrCreateDebugInfo(instance);
WasmDebugInfo::SetBreakpoint(debug_info, func_index, offset_in_func);
WasmDebugInfo::SetBreakpoint(debug_info, func_index, breakable_offset);
}
}
......
......@@ -280,6 +280,13 @@ std::vector<WasmValue> wasmVec(Args... args) {
return std::vector<WasmValue>{arr.begin(), arr.end()};
}
int GetIntReturnValue(MaybeHandle<Object> retval) {
CHECK(!retval.is_null());
int result;
CHECK(retval.ToHandleChecked()->ToInt32(&result));
return result;
}
} // namespace
WASM_COMPILED_EXEC_TEST(WasmCollectPossibleBreakpoints) {
......@@ -324,10 +331,25 @@ WASM_COMPILED_EXEC_TEST(WasmSimpleBreak) {
Handle<Object> global(isolate->context().global_object(), isolate);
MaybeHandle<Object> retval =
Execution::Call(isolate, main_fun_wrapper, global, 0, nullptr);
CHECK(!retval.is_null());
int result;
CHECK(retval.ToHandleChecked()->ToInt32(&result));
CHECK_EQ(14, result);
CHECK_EQ(14, GetIntReturnValue(retval));
}
WASM_COMPILED_EXEC_TEST(WasmNonBreakablePosition) {
WasmRunner<int> runner(execution_tier);
Isolate* isolate = runner.main_isolate();
BUILD(runner, WASM_RETURN1(WASM_I32V_2(1024)));
Handle<JSFunction> main_fun_wrapper =
runner.builder().WrapCode(runner.function_index());
SetBreakpoint(&runner, runner.function_index(), 2, 4);
BreakHandler count_breaks(isolate, {{4, BreakHandler::Continue}});
Handle<Object> global(isolate->context().global_object(), isolate);
MaybeHandle<Object> retval =
Execution::Call(isolate, main_fun_wrapper, global, 0, nullptr);
CHECK_EQ(1024, GetIntReturnValue(retval));
}
WASM_COMPILED_EXEC_TEST(WasmSimpleStepping) {
......@@ -351,10 +373,7 @@ WASM_COMPILED_EXEC_TEST(WasmSimpleStepping) {
Handle<Object> global(isolate->context().global_object(), isolate);
MaybeHandle<Object> retval =
Execution::Call(isolate, main_fun_wrapper, global, 0, nullptr);
CHECK(!retval.is_null());
int result;
CHECK(retval.ToHandleChecked()->ToInt32(&result));
CHECK_EQ(14, result);
CHECK_EQ(14, GetIntReturnValue(retval));
}
WASM_COMPILED_EXEC_TEST(WasmStepInAndOut) {
......@@ -470,10 +489,7 @@ WASM_COMPILED_EXEC_TEST(WasmRemoveBreakPoint) {
Handle<Object> global(isolate->context().global_object(), isolate);
MaybeHandle<Object> retval =
Execution::Call(isolate, main_fun_wrapper, global, 0, nullptr);
CHECK(!retval.is_null());
int result;
CHECK(retval.ToHandleChecked()->ToInt32(&result));
CHECK_EQ(14, result);
CHECK_EQ(14, GetIntReturnValue(retval));
}
WASM_COMPILED_EXEC_TEST(WasmRemoveLastBreakPoint) {
......@@ -501,10 +517,7 @@ WASM_COMPILED_EXEC_TEST(WasmRemoveLastBreakPoint) {
Handle<Object> global(isolate->context().global_object(), isolate);
MaybeHandle<Object> retval =
Execution::Call(isolate, main_fun_wrapper, global, 0, nullptr);
CHECK(!retval.is_null());
int result;
CHECK(retval.ToHandleChecked()->ToInt32(&result));
CHECK_EQ(14, result);
CHECK_EQ(14, GetIntReturnValue(retval));
}
WASM_COMPILED_EXEC_TEST(WasmRemoveAllBreakPoint) {
......@@ -534,10 +547,7 @@ WASM_COMPILED_EXEC_TEST(WasmRemoveAllBreakPoint) {
Handle<Object> global(isolate->context().global_object(), isolate);
MaybeHandle<Object> retval =
Execution::Call(isolate, main_fun_wrapper, global, 0, nullptr);
CHECK(!retval.is_null());
int result;
CHECK(retval.ToHandleChecked()->ToInt32(&result));
CHECK_EQ(14, result);
CHECK_EQ(14, GetIntReturnValue(retval));
}
} // namespace wasm
......
Tests stepping through wasm scripts with source maps
Installing code an global variable and instantiate.
Got wasm script: wasm://wasm/18214bfe
Requesting source for wasm://wasm/18214bfe...
Tests stepping through wasm scripts by byte offsets
Setting up global instance variable.
Got wasm script: wasm://wasm/41f464ee
Requesting source for wasm://wasm/41f464ee...
Source retrieved without error: true
Setting breakpoint on offset 54 (on the setlocal before the call), url wasm://wasm/18214bfe
Result: {"id":4,"result":{"breakpointId":"4:0:54:4","actualLocation":{"scriptId":"4","lineNumber":0,"columnNumber":54}}}
Setting breakpoint on offset 59 (should be propagated to 60, the offset of the call), url wasm://wasm/41f464ee
{
columnNumber : 54
columnNumber : 60
lineNumber : 0
scriptId : <scriptId>
}
Paused at wasm://wasm/18214bfe/18214bfe-1:7:6
at wasm_B (7:6):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":4}
stack: {"0":3}
at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-1:8:6
at wasm_B (8:6):
Paused at wasm://wasm/41f464ee/41f464ee-1:9:6
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":3}
stack: {}
stack: {"0":1024}
at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-0:1:2
Paused at wasm://wasm/41f464ee/41f464ee-0:1:2
at wasm_A (1:2):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1024}
stack: {}
at wasm_B (8:6):
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
......@@ -48,13 +37,14 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepOver called
Paused at wasm://wasm/18214bfe/18214bfe-0:2:2
Paused at wasm://wasm/41f464ee/41f464ee-0:2:2
at wasm_A (2:2):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1024}
stack: {}
at wasm_B (8:6):
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
......@@ -64,8 +54,8 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepOut called
Paused at wasm://wasm/18214bfe/18214bfe-1:9:6
at wasm_B (9:6):
Paused at wasm://wasm/41f464ee/41f464ee-1:10:6
at wasm_B (10:6):
- scope (global):
-- skipped
- scope (local):
......@@ -75,19 +65,19 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepOut called
Paused at wasm://wasm/18214bfe/18214bfe-1:7:6
at wasm_B (7:6):
Paused at wasm://wasm/41f464ee/41f464ee-1:9:6
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":3}
stack: {"0":2}
locals: {"arg#0":2}
stack: {"0":1024}
at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepOver called
Paused at wasm://wasm/18214bfe/18214bfe-1:8:6
at wasm_B (8:6):
Paused at wasm://wasm/41f464ee/41f464ee-1:10:6
at wasm_B (10:6):
- scope (global):
-- skipped
- scope (local):
......@@ -96,9 +86,9 @@ at wasm_B (8:6):
at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepOver called
Paused at wasm://wasm/18214bfe/18214bfe-1:9:6
at wasm_B (9:6):
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-1:1:2
at wasm_B (1:2):
- scope (global):
-- skipped
- scope (local):
......@@ -108,35 +98,25 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.resume called
Paused at wasm://wasm/18214bfe/18214bfe-1:7:6
at wasm_B (7:6):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":2}
stack: {"0":1}
at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-1:8:6
at wasm_B (8:6):
Paused at wasm://wasm/41f464ee/41f464ee-1:9:6
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1}
stack: {}
stack: {"0":1024}
at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-0:1:2
Paused at wasm://wasm/41f464ee/41f464ee-0:1:2
at wasm_A (1:2):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1024}
stack: {}
at wasm_B (8:6):
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
......@@ -146,8 +126,8 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepOut called
Paused at wasm://wasm/18214bfe/18214bfe-1:9:6
at wasm_B (9:6):
Paused at wasm://wasm/41f464ee/41f464ee-1:10:6
at wasm_B (10:6):
- scope (global):
-- skipped
- scope (local):
......@@ -157,7 +137,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-1:1:2
Paused at wasm://wasm/41f464ee/41f464ee-1:1:2
at wasm_B (1:2):
- scope (global):
-- skipped
......@@ -168,7 +148,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-1:2:4
Paused at wasm://wasm/41f464ee/41f464ee-1:2:4
at wasm_B (2:4):
- scope (global):
-- skipped
......@@ -179,7 +159,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-1:3:4
Paused at wasm://wasm/41f464ee/41f464ee-1:3:4
at wasm_B (3:4):
- scope (global):
-- skipped
......@@ -190,7 +170,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-1:4:6
Paused at wasm://wasm/41f464ee/41f464ee-1:4:6
at wasm_B (4:6):
- scope (global):
-- skipped
......@@ -201,7 +181,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-1:5:6
Paused at wasm://wasm/41f464ee/41f464ee-1:5:6
at wasm_B (5:6):
- scope (global):
-- skipped
......@@ -212,7 +192,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-1:6:6
Paused at wasm://wasm/41f464ee/41f464ee-1:6:6
at wasm_B (6:6):
- scope (global):
-- skipped
......@@ -223,7 +203,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-1:7:6
Paused at wasm://wasm/41f464ee/41f464ee-1:7:6
at wasm_B (7:6):
- scope (global):
-- skipped
......@@ -234,7 +214,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-1:8:6
Paused at wasm://wasm/41f464ee/41f464ee-1:8:6
at wasm_B (8:6):
- scope (global):
-- skipped
......@@ -245,13 +225,25 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-0:1:2
Paused at wasm://wasm/41f464ee/41f464ee-1:9:6
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":0}
stack: {"0":1024}
at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-0:1:2
at wasm_A (1:2):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1024}
stack: {}
at wasm_B (8:6):
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
......@@ -261,13 +253,14 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-0:2:2
Paused at wasm://wasm/41f464ee/41f464ee-0:2:2
at wasm_A (2:2):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1024}
stack: {}
at wasm_B (8:6):
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
......@@ -277,13 +270,14 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-0:3:0
Paused at wasm://wasm/41f464ee/41f464ee-0:3:0
at wasm_A (3:0):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1024}
stack: {}
at wasm_B (8:6):
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
......@@ -293,8 +287,8 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/18214bfe/18214bfe-1:9:6
at wasm_B (9:6):
Paused at wasm://wasm/41f464ee/41f464ee-1:10:6
at wasm_B (10:6):
- scope (global):
-- skipped
- scope (local):
......
// Copyright 2018 the V8 project authors. All rights reserved.
// Copyright 2019 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.
let {session, contextGroup, Protocol} =
InspectorTest.start('Tests stepping through wasm scripts with source maps');
InspectorTest.start('Tests stepping through wasm scripts by byte offsets');
utils.load('test/mjsunit/wasm/wasm-module-builder.js');
var builder = new WasmModuleBuilder();
var func_a_idx =
builder.addFunction('wasm_A', kSig_v_v).addBody([kExprNop, kExprNop]).index;
builder.addFunction('wasm_A', kSig_v_i).addBody([kExprNop, kExprNop]).index;
// wasm_B calls wasm_A <param0> times.
builder.addFunction('wasm_B', kSig_v_i)
......@@ -23,6 +23,7 @@ builder.addFunction('wasm_B', kSig_v_i)
kExprI32Const, 1, // -
kExprI32Sub, // -
kExprLocalSet, 0, // decrease <param0>
...wasmI32Const(1024), // some longer i32 const (2 byte imm)
kExprCallFunction, func_a_idx, // -
kExprBr, 1, // continue
kExprEnd, // -
......@@ -51,9 +52,10 @@ function instantiate(bytes) {
InspectorTest.logProtocolCommandCalls('Debugger.' + action);
await Protocol.Debugger.enable();
InspectorTest.log('Installing code an global variable and instantiate.');
InspectorTest.log('Setting up global instance variable.');
Protocol.Runtime.evaluate({
expression: `var instance;(${instantiate.toString()})(${JSON.stringify(module_bytes)})`
expression: `var instance;` +
`(${instantiate.toString()})(${JSON.stringify(module_bytes)})`
});
const [, {params: wasmScript}, ,] = await Protocol.Debugger.onceScriptParsed(4);
......@@ -62,39 +64,39 @@ function instantiate(bytes) {
const msg =
await Protocol.Debugger.getScriptSource({scriptId: wasmScript.scriptId});
InspectorTest.log(`Source retrieved without error: ${!msg.error}`);
// TODO: Add check that source text is empty but bytecode is present.
// TODO(leese): Add check that source text is empty but bytecode is present.
// Set the breakpoint on a non-breakable position. This should resolve to the
// next instruction.
InspectorTest.log(
`Setting breakpoint on offset 54 (on the setlocal before the call), url ${wasmScript.url}`);
`Setting breakpoint on offset 59 (should be propagated to 60, the ` +
`offset of the call), url ${wasmScript.url}`);
const bpmsg = await Protocol.Debugger.setBreakpoint({
location: {scriptId: wasmScript.scriptId, lineNumber: 0, columnNumber: 54}
location: {scriptId: wasmScript.scriptId, lineNumber: 0, columnNumber: 59}
});
InspectorTest.log(`Result: ${JSON.stringify(bpmsg)}`);
const actualLocation = bpmsg.result.actualLocation;
InspectorTest.logMessage(actualLocation);
Protocol.Runtime.evaluate({ expression: 'instance.exports.main(4)' });
await waitForPauseAndStep('stepInto'); // == stepOver, to call instruction
await waitForPauseAndStep('stepInto'); // into call to wasm_A
await waitForPauseAndStep('stepOver'); // over first nop
await waitForPauseAndStep('stepOut'); // out of wasm_A
await waitForPauseAndStep('stepOut'); // out of wasm_B, stop on breakpoint again
await waitForPauseAndStep('stepOver'); // to call
await waitForPauseAndStep('stepOut'); // out of wasm_B, stop on breakpoint
await waitForPauseAndStep('stepOver'); // over call
await waitForPauseAndStep('resume'); // to next breakpoint (third iteration)
await waitForPauseAndStep('stepInto'); // to call
await waitForPauseAndStep('stepInto'); // == stepOver br
await waitForPauseAndStep('resume'); // to next breakpoint (3rd iteration)
await waitForPauseAndStep('stepInto'); // into wasm_A
await waitForPauseAndStep('stepOut'); // out to wasm_B
// now step 9 times, until we are in wasm_A again.
for (let i = 0; i < 9; ++i) await waitForPauseAndStep('stepInto');
// Now step 10 times, until we are in wasm_A again.
for (let i = 0; i < 10; ++i) await waitForPauseAndStep('stepInto');
// 3 more times, back to wasm_B.
for (let i = 0; i < 3; ++i) await waitForPauseAndStep('stepInto');
// then just resume.
// Then just resume.
await waitForPauseAndStep('resume');
InspectorTest.log('exports.main returned!');
InspectorTest.log('Finished!');
//InspectorTest.completeTest();
})().catch(reason => InspectorTest.log(`Failed: ${reason}`)).finally(InspectorTest.completeTest);
})().catch(reason => InspectorTest.log(`Failed: ${reason}`))
.finally(InspectorTest.completeTest);
async function waitForPauseAndStep(stepAction) {
const {params: {callFrames}} = await Protocol.Debugger.oncePaused();
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment