[debug] Allow removal of instrumentation breakpoints in wasm

This CL adds the functionality to remove instrumentation breakpoints
in wasm.

Bug: chromium:1133307
Change-Id: I05ec7f8ac634267d95744ef4565d81cd0c372a2d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3460407Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79088}

src/inspector/v8-debugger-agent-impl.cc

@@ -717,9 +717,15 @@ Response V8DebuggerAgentImpl::removeBreakpoint(const String16& breakpointId) {
   // not Wasm breakpoint.
   std::vector<V8DebuggerScript*> scripts;
   for (const auto& scriptIter : m_scripts) {
-    if (!matches(m_inspector, *scriptIter.second, type, selector)) continue;
+    const bool scriptSelectorMatch =
+        matches(m_inspector, *scriptIter.second, type, selector);
+    const bool isInstrumentation =
+        type == BreakpointType::kInstrumentationBreakpoint;
+    if (!scriptSelectorMatch && !isInstrumentation) continue;
     V8DebuggerScript* script = scriptIter.second.get();
-    scripts.push_back(script);
+    if (script->getLanguage() == V8DebuggerScript::Language::WebAssembly) {
+      scripts.push_back(script);
+    }
   }
   removeBreakpointImpl(breakpointId, scripts);
 

src/wasm/wasm-debug.cc

@@ -897,6 +897,19 @@ int FindNextBreakablePosition(wasm::NativeModule* native_module, int func_index,
   return 0;
 }
 
+void SetBreakOnEntryFlag(Script script, bool enabled) {
+  if (script.break_on_entry() == enabled) return;
+
+  script.set_break_on_entry(enabled);
+  // 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(enabled);
+  }
+}
 }  // namespace
 
 // static
@@ -925,16 +938,9 @@ void WasmScript::SetInstrumentationBreakpoint(Handle<Script> script,
                                               Handle<BreakPoint> break_point) {
   // Special handling for on-entry breakpoints.
   AddBreakpointToInfo(script, kOnEntryBreakpointPosition, 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);
-  }
+  SetBreakOnEntryFlag(*script, true);
 }
 
 // static
@@ -1034,12 +1040,17 @@ bool WasmScript::ClearBreakPoint(Handle<Script> script, int position,
     breakpoint_infos->set_undefined(breakpoint_infos->length() - 1);
   }
 
-  // Remove the breakpoint from DebugInfo and recompile.
-  wasm::NativeModule* native_module = script->wasm_native_module();
-  const wasm::WasmModule* module = native_module->module();
-  int func_index = GetContainingWasmFunction(module, position);
-  native_module->GetDebugInfo()->RemoveBreakpoint(func_index, position,
-                                                  isolate);
+  if (break_point->id() == v8::internal::Debug::kInstrumentationId) {
+    // Special handling for instrumentation breakpoints.
+    SetBreakOnEntryFlag(*script, false);
+  } else {
+    // Remove the breakpoint from DebugInfo and recompile.
+    wasm::NativeModule* native_module = script->wasm_native_module();
+    const wasm::WasmModule* module = native_module->module();
+    int func_index = GetContainingWasmFunction(module, position);
+    native_module->GetDebugInfo()->RemoveBreakpoint(func_index, position,
+                                                    isolate);
+  }
 
   return true;
 }
@@ -1076,6 +1087,7 @@ bool WasmScript::ClearBreakPointById(Handle<Script> script, int breakpoint_id) {
 void WasmScript::ClearAllBreakpoints(Script script) {
   script.set_wasm_breakpoint_infos(
       ReadOnlyRoots(script.GetIsolate()).empty_fixed_array());
+  SetBreakOnEntryFlag(script, false);
 }
 
 // static

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

@@ -82,3 +82,69 @@ Paused at wasm://wasm/c8e3a856 with reason "instrumentation".
 Script wasm://wasm/c8e3a856 byte offset 33: Wasm opcode 0x01 (kExprNop)
 Hit breakpoints: []
 Done.
+
+Running test: testRemoveBeforeCompile
+Setting instrumentation breakpoint
+{
+    id : <messageId>
+    result : {
+        breakpointId : <breakpointId>
+    }
+}
+Remove instrumentation breakpoint..
+Compiling wasm module.
+Instantiating module should not trigger a break.
+Done.
+
+Running test: testRemoveBeforeInstantiate
+Setting instrumentation breakpoint
+{
+    id : <messageId>
+    result : {
+        breakpointId : <breakpointId>
+    }
+}
+Compiling wasm module.
+Paused at v8://test/compile_module with reason "instrumentation".
+Hit breakpoints: []
+Remove instrumentation breakpoint..
+Instantiating module should not trigger a break.
+Done.
+
+Running test: testRemoveAfterOnePause
+Setting instrumentation breakpoint
+{
+    id : <messageId>
+    result : {
+        breakpointId : <breakpointId>
+    }
+}
+Compiling wasm module.
+Paused at v8://test/compile_module with reason "instrumentation".
+Hit breakpoints: []
+Instantiating module should trigger a break.
+Paused at v8://test/instantiate with reason "instrumentation".
+Hit breakpoints: []
+Paused at wasm://wasm/20da547a with reason "instrumentation".
+Script wasm://wasm/20da547a byte offset 26: Wasm opcode 0x01 (kExprNop)
+Hit breakpoints: []
+Remove instrumentation breakpoint..
+Compiling another wasm module.
+Instantiating module should not trigger a break.
+Done.
+
+Running test: testDisableEnable
+Setting instrumentation breakpoint
+{
+    id : <messageId>
+    result : {
+        breakpointId : <breakpointId>
+    }
+}
+Compiling wasm module.
+Paused at v8://test/compile_module with reason "instrumentation".
+Hit breakpoints: []
+Disable debugger..
+Enable debugger
+Instantiating module should not trigger a break.
+Done.

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

@@ -75,8 +75,7 @@ InspectorTest.runAsyncTestSuite([
 
   async function testBreakInExportedFunction() {
     const builder = new WasmModuleBuilder();
-    const func =
-        builder.addFunction('func', kSig_v_v).addBody([kExprNop]).exportFunc();
+    builder.addFunction('func', kSig_v_v).addBody([kExprNop]).exportFunc();
 
     await Protocol.Runtime.enable();
     await Protocol.Debugger.enable();
@@ -100,8 +99,7 @@ InspectorTest.runAsyncTestSuite([
 
   async function testBreakOnlyWithSourceMap() {
     const builder = new WasmModuleBuilder();
-    const func =
-        builder.addFunction('func', kSig_v_v).addBody([kExprNop]).exportFunc();
+    builder.addFunction('func', kSig_v_v).addBody([kExprNop]).exportFunc();
     const bytes_no_source_map = builder.toArray();
     builder.addCustomSection('sourceMappingURL', [3, 97, 98, 99]);
     const bytes_with_source_map = builder.toArray();
@@ -130,4 +128,108 @@ InspectorTest.runAsyncTestSuite([
     await Protocol.Runtime.disable();
   },
 
+  async function testRemoveBeforeCompile() {
+    const builder = new WasmModuleBuilder();
+    const start_fn = builder.addFunction('start', kSig_v_v).addBody([kExprNop]);
+    builder.addStart(start_fn.index);
+
+    await Protocol.Runtime.enable();
+    await Protocol.Debugger.enable();
+    InspectorTest.log('Setting instrumentation breakpoint');
+    const addMsg = await Protocol.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'})
+    InspectorTest.logMessage(addMsg);
+    InspectorTest.log('Remove instrumentation breakpoint..');
+    await Protocol.Debugger.removeBreakpoint(
+        {breakpointId: addMsg.result.breakpointId});
+    InspectorTest.log('Compiling wasm module.');
+    await WasmInspectorTest.compile(builder.toArray());
+    InspectorTest.log('Instantiating module should not trigger a break.');
+    await WasmInspectorTest.evalWithUrl(
+        'new WebAssembly.Instance(module)', 'instantiate');
+    InspectorTest.log('Done.');
+    await Protocol.Debugger.disable();
+    await Protocol.Runtime.disable();
+  },
+
+  async function testRemoveBeforeInstantiate() {
+    const builder = new WasmModuleBuilder();
+    const start_fn = builder.addFunction('start', kSig_v_v).addBody([kExprNop]);
+    builder.addStart(start_fn.index);
+
+    await Protocol.Runtime.enable();
+    await Protocol.Debugger.enable();
+    InspectorTest.log('Setting instrumentation breakpoint');
+    const addMsg = await Protocol.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'})
+    InspectorTest.logMessage(addMsg);
+    InspectorTest.log('Compiling wasm module.');
+    await WasmInspectorTest.compile(builder.toArray());
+    InspectorTest.log('Remove instrumentation breakpoint..');
+    await Protocol.Debugger.removeBreakpoint(
+        {breakpointId: addMsg.result.breakpointId});
+    InspectorTest.log('Instantiating module should not trigger a break.');
+    await WasmInspectorTest.evalWithUrl(
+        'new WebAssembly.Instance(module)', 'instantiate');
+    InspectorTest.log('Done.');
+    await Protocol.Debugger.disable();
+    await Protocol.Runtime.disable();
+  },
+
+  async function testRemoveAfterOnePause() {
+    const builder = new WasmModuleBuilder();
+    const start_fn = builder.addFunction('start', kSig_v_v).addBody([kExprNop]);
+    builder.addStart(start_fn.index);
+
+    await Protocol.Runtime.enable();
+    await Protocol.Debugger.enable();
+    InspectorTest.log('Setting instrumentation breakpoint');
+    const addMsg = await Protocol.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'})
+    InspectorTest.logMessage(addMsg);
+    InspectorTest.log('Compiling wasm module.');
+    await WasmInspectorTest.compile(builder.toArray());
+    InspectorTest.log('Instantiating module should trigger a break.');
+    await WasmInspectorTest.evalWithUrl(
+        'new WebAssembly.Instance(module)', 'instantiate');
+    InspectorTest.log('Remove instrumentation breakpoint..');
+    await Protocol.Debugger.removeBreakpoint(
+        {breakpointId: addMsg.result.breakpointId});
+
+    InspectorTest.log('Compiling another wasm module.');
+    builder.addFunction('end', kSig_v_v).addBody([kExprNop]);
+    await WasmInspectorTest.compile(builder.toArray());
+    InspectorTest.log('Instantiating module should not trigger a break.');
+    await WasmInspectorTest.evalWithUrl(
+        'new WebAssembly.Instance(module)', 'instantiate');
+    InspectorTest.log('Done.');
+    await Protocol.Debugger.disable();
+    await Protocol.Runtime.disable();
+  },
+
+  async function testDisableEnable() {
+    const builder = new WasmModuleBuilder();
+    const start_fn = builder.addFunction('start', kSig_v_v).addBody([kExprNop]);
+    builder.addStart(start_fn.index);
+
+    await Protocol.Runtime.enable();
+    await Protocol.Debugger.enable();
+    InspectorTest.log('Setting instrumentation breakpoint');
+    const addMsg = await Protocol.Debugger.setInstrumentationBreakpoint(
+        {instrumentation: 'beforeScriptExecution'})
+    InspectorTest.logMessage(addMsg);
+    InspectorTest.log('Compiling wasm module.');
+    await WasmInspectorTest.compile(builder.toArray());
+    InspectorTest.log('Disable debugger..');
+    await Protocol.Debugger.disable();
+    InspectorTest.log('Enable debugger');
+    await Protocol.Debugger.enable();
+    InspectorTest.log('Instantiating module should not trigger a break.');
+    await WasmInspectorTest.evalWithUrl(
+        'new WebAssembly.Instance(module)', 'instantiate');
+    InspectorTest.log('Done.');
+    await Protocol.Debugger.disable();
+    await Protocol.Runtime.disable();
+  },
+
 ]);