Commit d1462a56 authored by Clemens Backes's avatar Clemens Backes Committed by Commit Bot

Revert "[wasm] Prevent breakpoints on nonbreakable positions"

This reverts commit 3c98a2a3.

Reason for revert: Fails on arm: https://ci.chromium.org/p/v8/builders/ci/V8%20Arm%20-%20debug/12134

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}

TBR=mstarzinger@chromium.org,clemensb@chromium.org,bmeurer@chromium.org,leese@chromium.org

Change-Id: I7468ea3b15fecccdea521308325cf4851e0a0396
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:1025184
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1926032Reviewed-by: 's avatarClemens Backes <clemensb@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65074}
parent 8c4fc5c2
......@@ -674,12 +674,9 @@ Handle<Code> WasmDebugInfo::GetCWasmEntry(Handle<WasmDebugInfo> debug_info,
namespace {
// 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) {
#ifdef DEBUG
bool IsBreakablePosition(wasm::NativeModule* native_module, int func_index,
int offset_in_func) {
AccountingAllocator alloc;
Zone tmp(&alloc, ZONE_NAME);
wasm::BodyLocalDecls locals(&tmp);
......@@ -690,12 +687,13 @@ int FindNextBreakablePosition(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)) return offset;
if (offset > static_cast<uint32_t>(offset_in_func)) break;
if (offset == static_cast<uint32_t>(offset_in_func)) return true;
}
return 0;
return false;
}
#endif // DEBUG
} // namespace
......@@ -711,10 +709,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();
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;
// 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));
// Insert new break point into break_positions of module object.
WasmScript::AddBreakpointToInfo(script, *position, break_point);
......@@ -731,7 +729,7 @@ bool WasmScript::SetBreakPoint(Handle<Script> script, int* position,
isolate);
Handle<WasmDebugInfo> debug_info =
WasmInstanceObject::GetOrCreateDebugInfo(instance);
WasmDebugInfo::SetBreakpoint(debug_info, func_index, breakable_offset);
WasmDebugInfo::SetBreakpoint(debug_info, func_index, offset_in_func);
}
}
......
......@@ -280,13 +280,6 @@ 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) {
......@@ -331,25 +324,10 @@ 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_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));
CHECK(!retval.is_null());
int result;
CHECK(retval.ToHandleChecked()->ToInt32(&result));
CHECK_EQ(14, result);
}
WASM_COMPILED_EXEC_TEST(WasmSimpleStepping) {
......@@ -373,7 +351,10 @@ 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_EQ(14, GetIntReturnValue(retval));
CHECK(!retval.is_null());
int result;
CHECK(retval.ToHandleChecked()->ToInt32(&result));
CHECK_EQ(14, result);
}
WASM_COMPILED_EXEC_TEST(WasmStepInAndOut) {
......@@ -489,7 +470,10 @@ 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_EQ(14, GetIntReturnValue(retval));
CHECK(!retval.is_null());
int result;
CHECK(retval.ToHandleChecked()->ToInt32(&result));
CHECK_EQ(14, result);
}
WASM_COMPILED_EXEC_TEST(WasmRemoveLastBreakPoint) {
......@@ -517,7 +501,10 @@ 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_EQ(14, GetIntReturnValue(retval));
CHECK(!retval.is_null());
int result;
CHECK(retval.ToHandleChecked()->ToInt32(&result));
CHECK_EQ(14, result);
}
WASM_COMPILED_EXEC_TEST(WasmRemoveAllBreakPoint) {
......@@ -547,7 +534,10 @@ 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_EQ(14, GetIntReturnValue(retval));
CHECK(!retval.is_null());
int result;
CHECK(retval.ToHandleChecked()->ToInt32(&result));
CHECK_EQ(14, result);
}
} // namespace wasm
......
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...
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...
Source retrieved without error: true
Setting breakpoint on offset 59 (should be propagated to 60, the offset of the call), url wasm://wasm/41f464ee
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}}}
{
columnNumber : 60
columnNumber : 54
lineNumber : 0
scriptId : <scriptId>
}
Paused at wasm://wasm/41f464ee/41f464ee-1:9:6
at wasm_B (9:6):
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):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":3}
stack: {"0":1024}
stack: {}
at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-0:1:2
Paused at wasm://wasm/18214bfe/18214bfe-0:1:2
at wasm_A (1:2):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1024}
stack: {}
at wasm_B (9:6):
at wasm_B (8:6):
- scope (global):
-- skipped
- scope (local):
......@@ -37,14 +48,13 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepOver called
Paused at wasm://wasm/41f464ee/41f464ee-0:2:2
Paused at wasm://wasm/18214bfe/18214bfe-0:2:2
at wasm_A (2:2):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1024}
stack: {}
at wasm_B (9:6):
at wasm_B (8:6):
- scope (global):
-- skipped
- scope (local):
......@@ -54,8 +64,8 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepOut called
Paused at wasm://wasm/41f464ee/41f464ee-1:10:6
at wasm_B (10:6):
Paused at wasm://wasm/18214bfe/18214bfe-1:9:6
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
......@@ -65,19 +75,19 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepOut called
Paused at wasm://wasm/41f464ee/41f464ee-1:9:6
at wasm_B (9:6):
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":1024}
locals: {"arg#0":3}
stack: {"0":2}
at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepOver called
Paused at wasm://wasm/41f464ee/41f464ee-1:10:6
at wasm_B (10:6):
Paused at wasm://wasm/18214bfe/18214bfe-1:8:6
at wasm_B (8:6):
- scope (global):
-- skipped
- scope (local):
......@@ -86,9 +96,9 @@ at wasm_B (10:6):
at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-1:1:2
at wasm_B (1:2):
Debugger.stepOver called
Paused at wasm://wasm/18214bfe/18214bfe-1:9:6
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
......@@ -98,25 +108,35 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.resume called
Paused at wasm://wasm/41f464ee/41f464ee-1:9:6
at wasm_B (9:6):
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):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1}
stack: {"0":1024}
stack: {}
at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-0:1:2
Paused at wasm://wasm/18214bfe/18214bfe-0:1:2
at wasm_A (1:2):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1024}
stack: {}
at wasm_B (9:6):
at wasm_B (8:6):
- scope (global):
-- skipped
- scope (local):
......@@ -126,8 +146,8 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepOut called
Paused at wasm://wasm/41f464ee/41f464ee-1:10:6
at wasm_B (10:6):
Paused at wasm://wasm/18214bfe/18214bfe-1:9:6
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
......@@ -137,7 +157,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-1:1:2
Paused at wasm://wasm/18214bfe/18214bfe-1:1:2
at wasm_B (1:2):
- scope (global):
-- skipped
......@@ -148,7 +168,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-1:2:4
Paused at wasm://wasm/18214bfe/18214bfe-1:2:4
at wasm_B (2:4):
- scope (global):
-- skipped
......@@ -159,7 +179,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-1:3:4
Paused at wasm://wasm/18214bfe/18214bfe-1:3:4
at wasm_B (3:4):
- scope (global):
-- skipped
......@@ -170,7 +190,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-1:4:6
Paused at wasm://wasm/18214bfe/18214bfe-1:4:6
at wasm_B (4:6):
- scope (global):
-- skipped
......@@ -181,7 +201,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-1:5:6
Paused at wasm://wasm/18214bfe/18214bfe-1:5:6
at wasm_B (5:6):
- scope (global):
-- skipped
......@@ -192,7 +212,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-1:6:6
Paused at wasm://wasm/18214bfe/18214bfe-1:6:6
at wasm_B (6:6):
- scope (global):
-- skipped
......@@ -203,7 +223,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-1:7:6
Paused at wasm://wasm/18214bfe/18214bfe-1:7:6
at wasm_B (7:6):
- scope (global):
-- skipped
......@@ -214,7 +234,7 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-1:8:6
Paused at wasm://wasm/18214bfe/18214bfe-1:8:6
at wasm_B (8:6):
- scope (global):
-- skipped
......@@ -225,25 +245,13 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
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
Paused at wasm://wasm/18214bfe/18214bfe-0:1:2
at wasm_A (1:2):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1024}
stack: {}
at wasm_B (9:6):
at wasm_B (8:6):
- scope (global):
-- skipped
- scope (local):
......@@ -253,14 +261,13 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-0:2:2
Paused at wasm://wasm/18214bfe/18214bfe-0:2:2
at wasm_A (2:2):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1024}
stack: {}
at wasm_B (9:6):
at wasm_B (8:6):
- scope (global):
-- skipped
- scope (local):
......@@ -270,14 +277,13 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-0:3:0
Paused at wasm://wasm/18214bfe/18214bfe-0:3:0
at wasm_A (3:0):
- scope (global):
-- skipped
- scope (local):
locals: {"arg#0":1024}
stack: {}
at wasm_B (9:6):
at wasm_B (8:6):
- scope (global):
-- skipped
- scope (local):
......@@ -287,8 +293,8 @@ at (anonymous) (0:17):
- scope (global):
-- skipped
Debugger.stepInto called
Paused at wasm://wasm/41f464ee/41f464ee-1:10:6
at wasm_B (10:6):
Paused at wasm://wasm/18214bfe/18214bfe-1:9:6
at wasm_B (9:6):
- scope (global):
-- skipped
- scope (local):
......
// Copyright 2019 the V8 project authors. All rights reserved.
// Copyright 2018 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 by byte offsets');
InspectorTest.start('Tests stepping through wasm scripts with source maps');
utils.load('test/mjsunit/wasm/wasm-module-builder.js');
var builder = new WasmModuleBuilder();
var func_a_idx =
builder.addFunction('wasm_A', kSig_v_i).addBody([kExprNop, kExprNop]).index;
builder.addFunction('wasm_A', kSig_v_v).addBody([kExprNop, kExprNop]).index;
// wasm_B calls wasm_A <param0> times.
builder.addFunction('wasm_B', kSig_v_i)
......@@ -23,7 +23,6 @@ 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, // -
......@@ -52,10 +51,9 @@ function instantiate(bytes) {
InspectorTest.logProtocolCommandCalls('Debugger.' + action);
await Protocol.Debugger.enable();
InspectorTest.log('Setting up global instance variable.');
InspectorTest.log('Installing code an global variable and instantiate.');
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);
......@@ -64,39 +62,39 @@ function instantiate(bytes) {
const msg =
await Protocol.Debugger.getScriptSource({scriptId: wasmScript.scriptId});
InspectorTest.log(`Source retrieved without error: ${!msg.error}`);
// TODO(leese): Add check that source text is empty but bytecode is present.
// TODO: 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 59 (should be propagated to 60, the ` +
`offset of the call), url ${wasmScript.url}`);
`Setting breakpoint on offset 54 (on the setlocal before the call), url ${wasmScript.url}`);
const bpmsg = await Protocol.Debugger.setBreakpoint({
location: {scriptId: wasmScript.scriptId, lineNumber: 0, columnNumber: 59}
location: {scriptId: wasmScript.scriptId, lineNumber: 0, columnNumber: 54}
});
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
await waitForPauseAndStep('stepOut'); // out of wasm_B, stop on breakpoint again
await waitForPauseAndStep('stepOver'); // to call
await waitForPauseAndStep('stepOver'); // over call
await waitForPauseAndStep('stepInto'); // == stepOver br
await waitForPauseAndStep('resume'); // to next breakpoint (3rd iteration)
await waitForPauseAndStep('resume'); // to next breakpoint (third iteration)
await waitForPauseAndStep('stepInto'); // to call
await waitForPauseAndStep('stepInto'); // into wasm_A
await waitForPauseAndStep('stepOut'); // out to wasm_B
// Now step 10 times, until we are in wasm_A again.
for (let i = 0; i < 10; ++i) await waitForPauseAndStep('stepInto');
// now step 9 times, until we are in wasm_A again.
for (let i = 0; i < 9; ++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!');
})().catch(reason => InspectorTest.log(`Failed: ${reason}`))
.finally(InspectorTest.completeTest);
//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