[wasm][debug] Implement instrumentation breakpoints

This CL adds support for instrumentation breakpoints in wasm. The
request for "break on entry" is set on the script, and we need to keep
it stored there because there might not be any instances of that wasm
module yet. Once instances get created, the flag value is transferred to
all instances. The flag stored there is then checked in the function
prologue in Liftoff debugging code. This ensures that we will stop at
the first valid break position in any function within that module.
Hitting that instrumentation breakpoint will then clear the flag from
the script and from all other live instances (in the same isolate).

A first basic test is contained in this CL. More tests will be added
later.

R=thibaudm@chromium.org, bmeurer@chromium.org

Bug: chromium:1151211
Change-Id: I5442d4044934988269becececc03699b850d51d7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2690588Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72701}

src/debug/debug-interface.cc

@@ -479,6 +479,11 @@ bool Script::SetBreakpoint(Local<String> condition, Location* location,
 bool Script::SetBreakpointOnScriptEntry(BreakpointId* id) const {
   i::Handle<i::Script> script = Utils::OpenHandle(this);
   i::Isolate* isolate = script->GetIsolate();
+  if (script->type() == i::Script::TYPE_WASM) {
+    int position = i::WasmScript::kOnEntryBreakpointPosition;
+    return isolate->debug()->SetBreakPointForScript(
+        script, isolate->factory()->empty_string(), &position, id);
+  }
   i::SharedFunctionInfo::ScriptIterator it(isolate, *script);
   for (i::SharedFunctionInfo sfi = it.Next(); !sfi.is_null(); sfi = it.Next()) {
     if (sfi.is_toplevel()) {

src/objects/script-inl.h

@@ -107,6 +107,12 @@ void Script::set_is_repl_mode(bool value) {
   set_flags(IsReplModeBit::update(flags(), value));
 }
 
+bool Script::break_on_entry() const { return BreakOnEntryBit::decode(flags()); }
+
+void Script::set_break_on_entry(bool value) {
+  set_flags(BreakOnEntryBit::update(flags(), value));
+}
+
 ScriptOriginOptions Script::origin_options() {
   return ScriptOriginOptions(OriginOptionsBits::decode(flags()));
 }

src/objects/script.h

@@ -107,6 +107,13 @@ class Script : public TorqueGeneratedScript<Script, Struct> {
   inline bool is_repl_mode() const;
   inline void set_is_repl_mode(bool value);
 
+  // [break_on_entry] (wasm only): whether an instrumentation breakpoint is set
+  // for this script; this information will be transferred to existing and
+  // future instances to make sure that we stop before executing any code in
+  // this wasm module.
+  inline bool break_on_entry() const;
+  inline void set_break_on_entry(bool value);
+
   // [origin_options]: optional attributes set by the embedder via ScriptOrigin,
   // and used by the embedder to make decisions about the script. V8 just passes
   // this through. Encoded in the 'flags' field.

src/objects/script.tq

@@ -10,6 +10,8 @@ bitfield struct ScriptFlags extends uint31 {
   compilation_state: CompilationState: 1 bit;
   is_repl_mode: bool: 1 bit;
   origin_options: int32: 4 bit;
+  // Whether an instrumentation breakpoint is set for this script (wasm only).
+  break_on_entry: bool: 1 bit;
 }
 
 @generateCppClass

src/runtime/runtime-wasm.cc

@@ -558,8 +558,11 @@ RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
   FrameFinder<WasmFrame, StackFrame::EXIT, StackFrame::WASM_DEBUG_BREAK>
       frame_finder(isolate);
   auto instance = handle(frame_finder.frame()->wasm_instance(), isolate);
-  int position = frame_finder.frame()->position();
-  auto frame_id = frame_finder.frame()->id();
+  auto script = handle(instance->module_object().script(), isolate);
+  WasmFrame* frame = frame_finder.frame();
+  int position = frame->position();
+  auto frame_id = frame->id();
+  auto* debug_info = frame->native_module()->GetDebugInfo();
   isolate->set_context(instance->native_context());
 
   // Stepping can repeatedly create code, and code GC requires stack guards to
@@ -570,28 +573,52 @@ RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
   // Enter the debugger.
   DebugScope debug_scope(isolate->debug());
 
-  WasmFrame* frame = frame_finder.frame();
-  auto* debug_info = frame->native_module()->GetDebugInfo();
+  // Check for instrumentation breakpoint.
+  DCHECK_EQ(script->break_on_entry(), instance->break_on_entry());
+  if (script->break_on_entry()) {
+    MaybeHandle<FixedArray> maybe_on_entry_breakpoints =
+        WasmScript::CheckBreakPoints(
+            isolate, script, WasmScript::kOnEntryBreakpointPosition, frame_id);
+    script->set_break_on_entry(false);
+    // Update the "break_on_entry" flag on all live instances.
+    i::WeakArrayList weak_instance_list = script->wasm_weak_instance_list();
+    for (int i = 0; i < weak_instance_list.length(); ++i) {
+      if (weak_instance_list.Get(i)->IsCleared()) continue;
+      i::WasmInstanceObject instance = i::WasmInstanceObject::cast(
+          weak_instance_list.Get(i)->GetHeapObject());
+      instance.set_break_on_entry(false);
+    }
+    DCHECK(!instance->break_on_entry());
+    Handle<FixedArray> on_entry_breakpoints;
+    if (maybe_on_entry_breakpoints.ToHandle(&on_entry_breakpoints)) {
+      debug_info->ClearStepping(isolate);
+      StepAction step_action = isolate->debug()->last_step_action();
+      isolate->debug()->ClearStepping();
+      isolate->debug()->OnDebugBreak(on_entry_breakpoints, step_action);
+      // Don't process regular breakpoints.
+      return ReadOnlyRoots(isolate).undefined_value();
+    }
+  }
+
   if (debug_info->IsStepping(frame)) {
     debug_info->ClearStepping(isolate);
-    StepAction stepAction = isolate->debug()->last_step_action();
+    StepAction step_action = isolate->debug()->last_step_action();
     isolate->debug()->ClearStepping();
     isolate->debug()->OnDebugBreak(isolate->factory()->empty_fixed_array(),
-                                   stepAction);
+                                   step_action);
     return ReadOnlyRoots(isolate).undefined_value();
   }
 
   // Check whether we hit a breakpoint.
-  Handle<Script> script(instance->module_object().script(), isolate);
   Handle<FixedArray> breakpoints;
   if (WasmScript::CheckBreakPoints(isolate, script, position, frame_id)
           .ToHandle(&breakpoints)) {
     debug_info->ClearStepping(isolate);
-    StepAction stepAction = isolate->debug()->last_step_action();
+    StepAction step_action = isolate->debug()->last_step_action();
     isolate->debug()->ClearStepping();
     if (isolate->debug()->break_points_active()) {
       // We hit one or several breakpoints. Notify the debug listeners.
-      isolate->debug()->OnDebugBreak(breakpoints, stepAction);
+      isolate->debug()->OnDebugBreak(breakpoints, step_action);
     }
   }
 

src/wasm/baseline/arm/liftoff-assembler-arm.h

@@ -576,9 +576,18 @@ void LiftoffAssembler::LoadConstant(LiftoffRegister reg, WasmValue value,
 
 void LiftoffAssembler::LoadFromInstance(Register dst, int offset, int size) {
   DCHECK_LE(0, offset);
-  DCHECK_EQ(4, size);
   ldr(dst, liftoff::GetInstanceOperand());
-  ldr(dst, MemOperand(dst, offset));
+  MemOperand src{dst, offset};
+  switch (size) {
+    case 1:
+      ldrb(dst, src);
+      break;
+    case 4:
+      ldr(dst, src);
+      break;
+    default:
+      UNIMPLEMENTED();
+  }
 }
 
 void LiftoffAssembler::LoadTaggedPointerFromInstance(Register dst, int offset) {

src/wasm/baseline/arm64/liftoff-assembler-arm64.h

@@ -395,11 +395,19 @@ void LiftoffAssembler::LoadConstant(LiftoffRegister reg, WasmValue value,
 void LiftoffAssembler::LoadFromInstance(Register dst, int offset, int size) {
   DCHECK_LE(0, offset);
   Ldr(dst, liftoff::GetInstanceOperand());
-  DCHECK(size == 4 || size == 8);
-  if (size == 4) {
-    Ldr(dst.W(), MemOperand(dst, offset));
-  } else {
-    Ldr(dst, MemOperand(dst, offset));
+  MemOperand src{dst, offset};
+  switch (size) {
+    case 1:
+      Ldrb(dst.W(), src);
+      break;
+    case 4:
+      Ldr(dst.W(), src);
+      break;
+    case 8:
+      Ldr(dst, src);
+      break;
+    default:
+      UNIMPLEMENTED();
   }
 }
 

src/wasm/baseline/ia32/liftoff-assembler-ia32.h

@@ -305,8 +305,17 @@ void LiftoffAssembler::LoadConstant(LiftoffRegister reg, WasmValue value,
 void LiftoffAssembler::LoadFromInstance(Register dst, int offset, int size) {
   DCHECK_LE(0, offset);
   mov(dst, liftoff::GetInstanceOperand());
-  DCHECK_EQ(4, size);
-  mov(dst, Operand(dst, offset));
+  Operand src{dst, offset};
+  switch (size) {
+    case 1:
+      movzx_b(dst, src);
+      break;
+    case 4:
+      mov(dst, src);
+      break;
+    default:
+      UNIMPLEMENTED();
+  }
 }
 
 void LiftoffAssembler::LoadTaggedPointerFromInstance(Register dst, int offset) {

src/wasm/baseline/liftoff-compiler.cc

@@ -954,19 +954,28 @@ class LiftoffCompiler {
     }
     if (has_breakpoint) {
       EmitBreakpoint(decoder);
-      // Once we emitted a breakpoint, we don't need to check the "hook on
-      // function call" any more.
-      checked_hook_on_function_call_ = true;
-    } else if (!checked_hook_on_function_call_) {
-      checked_hook_on_function_call_ = true;
-      // Check the "hook on function call" flag. If set, trigger a break.
-      DEBUG_CODE_COMMENT("check hook on function call");
+      // Once we emitted an unconditional breakpoint, we don't need to check
+      // function entry breaks any more.
+      did_function_entry_break_checks_ = true;
+    } else if (!did_function_entry_break_checks_) {
+      did_function_entry_break_checks_ = true;
+      DEBUG_CODE_COMMENT("check function entry break");
+      Label do_break;
+      Label no_break;
       Register flag = __ GetUnusedRegister(kGpReg, {}).gp();
+
+      // Check the "hook on function call" flag. If set, trigger a break.
       LOAD_INSTANCE_FIELD(flag, HookOnFunctionCallAddress, kSystemPointerSize);
-      Label no_break;
       __ Load(LiftoffRegister{flag}, flag, no_reg, 0, LoadType::kI32Load8U, {});
+      // Unary "unequal" means "not equals zero".
+      __ emit_cond_jump(kUnequal, &do_break, kWasmI32, flag);
+
+      // Check if we should stop on "script entry".
+      LOAD_INSTANCE_FIELD(flag, BreakOnEntry, kUInt8Size);
       // Unary "equal" means "equals zero".
       __ emit_cond_jump(kEqual, &no_break, kWasmI32, flag);
+
+      __ bind(&do_break);
       EmitBreakpoint(decoder);
       __ bind(&no_break);
     } else if (dead_breakpoint_ == decoder->position()) {
@@ -5347,10 +5356,10 @@ class LiftoffCompiler {
   // address in OSR is correct.
   int dead_breakpoint_ = 0;
 
-  // Remember whether the "hook on function call" has already been checked.
-  // This happens at the first breakable opcode in the function (if compiling
-  // for debugging).
-  bool checked_hook_on_function_call_ = false;
+  // Remember whether the did function-entry break checks (for "hook on function
+  // call" and "break on entry" a.k.a. instrumentation breakpoint). This happens
+  // at the first breakable opcode in the function (if compiling for debugging).
+  bool did_function_entry_break_checks_ = false;
 
   bool has_outstanding_op() const {
     return outstanding_op_ != kNoOutstandingOp;

src/wasm/baseline/x64/liftoff-assembler-x64.h

@@ -282,12 +282,20 @@ void LiftoffAssembler::LoadConstant(LiftoffRegister reg, WasmValue value,
 
 void LiftoffAssembler::LoadFromInstance(Register dst, int offset, int size) {
   DCHECK_LE(0, offset);
-  DCHECK(size == 4 || size == 8);
   movq(dst, liftoff::GetInstanceOperand());
-  if (size == 4) {
-    movl(dst, Operand(dst, offset));
-  } else {
-    movq(dst, Operand(dst, offset));
+  Operand src{dst, offset};
+  switch (size) {
+    case 1:
+      movzxbl(dst, src);
+      break;
+    case 4:
+      movl(dst, src);
+      break;
+    case 8:
+      movq(dst, src);
+      break;
+    default:
+      UNIMPLEMENTED();
   }
 }
 

src/wasm/wasm-debug.cc

@@ -774,6 +774,22 @@ int FindNextBreakablePosition(wasm::NativeModule* native_module, int func_index,
 // static
 bool WasmScript::SetBreakPoint(Handle<Script> script, int* position,
                                Handle<BreakPoint> break_point) {
+  // Special handling for on-entry breakpoints.
+  if (*position == kOnEntryBreakpointPosition) {
+    AddBreakpointToInfo(script, *position, break_point);
+    script->set_break_on_entry(true);
+
+    // Update the "break_on_entry" flag on all live instances.
+    i::WeakArrayList weak_instance_list = script->wasm_weak_instance_list();
+    for (int i = 0; i < weak_instance_list.length(); ++i) {
+      if (weak_instance_list.Get(i)->IsCleared()) continue;
+      i::WasmInstanceObject instance = i::WasmInstanceObject::cast(
+          weak_instance_list.Get(i)->GetHeapObject());
+      instance.set_break_on_entry(true);
+    }
+    return true;
+  }
+
   // Find the function for this breakpoint.
   const wasm::WasmModule* module = script->wasm_native_module()->module();
   int func_index = GetContainingWasmFunction(module, *position);
@@ -818,8 +834,7 @@ bool WasmScript::SetBreakPointForFunction(Handle<Script> script, int func_index,
   const wasm::WasmFunction& func = module->functions[func_index];
 
   // Insert new break point into {wasm_breakpoint_infos} of the script.
-  WasmScript::AddBreakpointToInfo(script, func.code.offset() + offset,
-                                  break_point);
+  AddBreakpointToInfo(script, func.code.offset() + offset, break_point);
 
   native_module->GetDebugInfo()->SetBreakpoint(func_index, offset, isolate);
 
@@ -837,8 +852,9 @@ int FindBreakpointInfoInsertPos(Isolate* isolate,
                                 Handle<FixedArray> breakpoint_infos,
                                 int position) {
   // Find insert location via binary search, taking care of undefined values on
-  // the right. Position is always greater than zero.
-  DCHECK_LT(0, position);
+  // the right. {position} is either {kOnEntryBreakpointPosition} (which is -1),
+  // or positive.
+  DCHECK(position == WasmScript::kOnEntryBreakpointPosition || position > 0);
 
   int left = 0;                            // inclusive
   int right = breakpoint_infos->length();  // exclusive

src/wasm/wasm-objects-inl.h

@@ -239,6 +239,8 @@ PRIMITIVE_ACCESSORS(WasmInstanceObject, hook_on_function_call_address, Address,
                     kHookOnFunctionCallAddressOffset)
 PRIMITIVE_ACCESSORS(WasmInstanceObject, num_liftoff_function_calls_array,
                     uint32_t*, kNumLiftoffFunctionCallsArrayOffset)
+PRIMITIVE_ACCESSORS(WasmInstanceObject, break_on_entry, uint8_t,
+                    kBreakOnEntryOffset)
 
 ACCESSORS(WasmInstanceObject, module_object, WasmModuleObject,
           kModuleObjectOffset)

src/wasm/wasm-objects.cc

@@ -1298,6 +1298,7 @@ Handle<WasmInstanceObject> WasmInstanceObject::New(
   instance->set_managed_object_maps(*isolate->factory()->empty_fixed_array());
   instance->set_num_liftoff_function_calls_array(
       module_object->native_module()->num_liftoff_function_calls_array());
+  instance->set_break_on_entry(module_object->script().break_on_entry());
 
   // Insert the new instance into the scripts weak list of instances. This list
   // is used for breakpoints affecting all instances belonging to the script.

src/wasm/wasm-objects.h

@@ -418,6 +418,7 @@ class V8_EXPORT_PRIVATE WasmInstanceObject : public JSObject {
   DECL_PRIMITIVE_ACCESSORS(dropped_elem_segments, byte*)
   DECL_PRIMITIVE_ACCESSORS(hook_on_function_call_address, Address)
   DECL_PRIMITIVE_ACCESSORS(num_liftoff_function_calls_array, uint32_t*)
+  DECL_PRIMITIVE_ACCESSORS(break_on_entry, uint8_t)
 
   // Clear uninitialized padding space. This ensures that the snapshot content
   // is deterministic. Depending on the V8 build mode there could be no padding.
@@ -466,6 +467,9 @@ class V8_EXPORT_PRIVATE WasmInstanceObject : public JSObject {
   V(kDroppedElemSegmentsOffset, kSystemPointerSize)                       \
   V(kHookOnFunctionCallAddressOffset, kSystemPointerSize)                 \
   V(kNumLiftoffFunctionCallsArrayOffset, kSystemPointerSize)              \
+  V(kBreakOnEntryOffset, kUInt8Size)                                      \
+  /* More padding to make the header pointer-size aligned */              \
+  V(kHeaderPaddingOffset, POINTER_SIZE_PADDING(kHeaderPaddingOffset))     \
   V(kHeaderSize, 0)
 
   DEFINE_FIELD_OFFSET_CONSTANTS(JSObject::kHeaderSize,
@@ -795,6 +799,10 @@ class WasmJSFunctionData : public Struct {
 
 class WasmScript : public AllStatic {
  public:
+  // Position used for storing "on entry" breakpoints (a.k.a. instrumentation
+  // breakpoints). This would be an illegal position for any other breakpoint.
+  static constexpr int kOnEntryBreakpointPosition = -1;
+
   // Set a breakpoint on the given byte position inside the given module.
   // This will affect all live and future instances of the module.
   // The passed position might be modified to point to the next breakable

test/inspector/debugger/wasm-instrumentation-breakpoint-expected.txt

+Test instrumentation breakpoints in wasm.
+
+Running test: testBreakInStartFunction
+Setting instrumentation breakpoint
+{
+    id : <messageId>
+    result : {
+        breakpointId : <breakpointId>
+    }
+}
+Compiling wasm module.
+Paused at v8://test/compile_module with reason "instrumentation".
+Instantiating module.
+Paused at v8://test/instantiate with reason "instrumentation".
+Paused at wasm://wasm/20da547a with reason "instrumentation".
+Script wasm://wasm/20da547a byte offset 26: Wasm opcode 0x01 (kExprNop)
+Instantiating a second time (should trigger no breakpoint).
+Paused at v8://test/instantiate2 with reason "instrumentation".
+Done.
+
+Running test: testBreakInStartFunctionCompileTwice
+Setting instrumentation breakpoint
+{
+    id : <messageId>
+    result : {
+        breakpointId : <breakpointId>
+    }
+}
+Instantiating module.
+Paused at v8://test/instantiate with reason "instrumentation".
+Paused at wasm://wasm/20da547a with reason "instrumentation".
+Script wasm://wasm/20da547a byte offset 26: Wasm opcode 0x01 (kExprNop)
+Instantiating a second time (should trigger another breakpoint).
+Paused at v8://test/instantiate with reason "instrumentation".
+Paused at wasm://wasm/20da547a with reason "instrumentation".
+Script wasm://wasm/20da547a byte offset 26: Wasm opcode 0x01 (kExprNop)
+Done.

test/inspector/debugger/wasm-instrumentation-breakpoint.js

+// Copyright 2021 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.
+
+utils.load('test/inspector/wasm-inspector-test.js');
+
+const {session, contextGroup, Protocol} =
+    InspectorTest.start('Test instrumentation breakpoints in wasm.');
+session.setupScriptMap();
+
+Protocol.Debugger.onPaused(async msg => {
+  let top_frame = msg.params.callFrames[0];
+  let reason = msg.params.reason;
+  InspectorTest.log(`Paused at ${top_frame.url} with reason "${reason}".`);
+  if (!top_frame.url.startsWith('v8://test/')) {
+    await session.logSourceLocation(top_frame.location);
+  }
+  Protocol.Debugger.resume();
+});
+
+// TODO(clemensb): Add test for 'beforeScriptWithSourceMapExecution'.
+// TODO(clemensb): Add test for module without start function.
+
+InspectorTest.runAsyncTestSuite([
+  async function testBreakInStartFunction() {
+    const builder = new WasmModuleBuilder();
+    const start_fn = builder.addFunction('start', kSig_v_v).addBody([kExprNop]);
+    builder.addStart(start_fn.index);
+
+    await Protocol.Debugger.enable();
+    InspectorTest.log('Setting instrumentation breakpoint');
+    InspectorTest.logMessage(
+        await Protocol.Debugger.setInstrumentationBreakpoint(
+            {instrumentation: 'beforeScriptExecution'}));
+    InspectorTest.log('Compiling wasm module.');
+    await WasmInspectorTest.compile(builder.toArray());
+    InspectorTest.log('Instantiating module.');
+    await WasmInspectorTest.evalWithUrl(
+        'new WebAssembly.Instance(module)', 'instantiate');
+    InspectorTest.log(
+        'Instantiating a second time (should trigger no breakpoint).');
+    await WasmInspectorTest.evalWithUrl(
+        'new WebAssembly.Instance(module)', 'instantiate2');
+    InspectorTest.log('Done.');
+    await Protocol.Debugger.disable();
+  },
+
+  // If we compile twice, we get two instrumentation breakpoints (which might or
+  // might not be expected, but it's the current behaviour).
+  async function testBreakInStartFunctionCompileTwice() {
+    const builder = new WasmModuleBuilder();
+    const start_fn = builder.addFunction('start', kSig_v_v).addBody([kExprNop]);
+    builder.addStart(start_fn.index);
+
+    await Protocol.Debugger.enable();
+    InspectorTest.log('Setting instrumentation breakpoint');
+    InspectorTest.logMessage(
+        await Protocol.Debugger.setInstrumentationBreakpoint(
+            {instrumentation: 'beforeScriptExecution'}));
+    InspectorTest.log('Instantiating module.');
+    await WasmInspectorTest.instantiate(builder.toArray());
+    InspectorTest.log(
+        'Instantiating a second time (should trigger another breakpoint).');
+    await WasmInspectorTest.instantiate(builder.toArray());
+    InspectorTest.log('Done.');
+    await Protocol.Debugger.disable();
+  }
+]);

test/inspector/wasm-inspector-test.js

@@ -13,14 +13,26 @@ WasmInspectorTest.evalWithUrl = async function(code, url) {
       .then(printIfFailure);
 };
 
-WasmInspectorTest.instantiateFromBuffer = function(bytes, imports) {
+WasmInspectorTest.compileFromBuffer = (function(bytes) {
   var buffer = new ArrayBuffer(bytes.length);
   var view = new Uint8Array(buffer);
   for (var i = 0; i < bytes.length; ++i) {
     view[i] = bytes[i] | 0;
   }
-  const module = new WebAssembly.Module(buffer);
-  return new WebAssembly.Instance(module, imports);
+  return new WebAssembly.Module(buffer);
+}).toString();
+
+WasmInspectorTest.instantiateFromBuffer =
+    (function(bytes, imports) {
+      return new WebAssembly.Instance(compileFromBuffer(bytes), imports);
+    })
+        .toString()
+        .replace('compileFromBuffer', WasmInspectorTest.compileFromBuffer);
+
+WasmInspectorTest.compile = async function(bytes, module_name = 'module') {
+  const compile_code = `var ${module_name} = (${
+      WasmInspectorTest.compileFromBuffer})(${JSON.stringify(bytes)});`;
+  await WasmInspectorTest.evalWithUrl(compile_code, 'compile_module');
 };
 
 WasmInspectorTest.instantiate =