[wasm] Support remove breakpoint

In setting breakpoint in wasm, we can find wasm script from location but
in removing a breakpoint, only breakpoint id is provided. For wasm, we
have a list of all BreakPointInfo objects attached to the Script. From
breakpoint id, we iterates all scripts to find the targeted breakpoint
and remove it.

Bug: chromium:837572
Change-Id: Ia5d0fb7d804fb98270b2103232bc10eb5d4f93a3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1959749
Commit-Queue: Z Nguyen-Huu <duongn@microsoft.com>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65505}

src/api/api.cc

@@ -9656,6 +9656,12 @@ bool debug::Script::SetBreakpointOnScriptEntry(BreakpointId* id) const {
   return false;
 }
 
+void debug::Script::RemoveWasmBreakpoint(debug::BreakpointId id) {
+  i::Handle<i::Script> script = Utils::OpenHandle(this);
+  i::Isolate* isolate = script->GetIsolate();
+  isolate->debug()->RemoveBreakpointForWasmScript(script, id);
+}
+
 void debug::RemoveBreakpoint(Isolate* v8_isolate, BreakpointId id) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
   i::HandleScope handle_scope(isolate);

src/debug/debug-interface.h

@@ -157,6 +157,7 @@ class V8_EXPORT_PRIVATE Script {
                        LiveEditResult* result) const;
   bool SetBreakpoint(v8::Local<v8::String> condition, debug::Location* location,
                      BreakpointId* id) const;
+  void RemoveWasmBreakpoint(BreakpointId id);
   bool SetBreakpointOnScriptEntry(BreakpointId* id) const;
 };
 

src/debug/debug.cc

@@ -760,6 +760,12 @@ void Debug::RemoveBreakpoint(int id) {
   ClearBreakPoint(breakpoint);
 }
 
+void Debug::RemoveBreakpointForWasmScript(Handle<Script> script, int id) {
+  if (script->type() == Script::TYPE_WASM) {
+    WasmScript::ClearBreakPointById(script, id);
+  }
+}
+
 // Clear out all the debug break code.
 void Debug::ClearAllBreakPoints() {
   ClearAllDebugInfos([=](Handle<DebugInfo> info) {

src/debug/debug.h

@@ -243,6 +243,7 @@ class V8_EXPORT_PRIVATE Debug {
   bool SetBreakpointForFunction(Handle<SharedFunctionInfo> shared,
                                 Handle<String> condition, int* id);
   void RemoveBreakpoint(int id);
+  void RemoveBreakpointForWasmScript(Handle<Script> script, int id);
 
   // Find breakpoints from the debug info and the break location and check
   // whether they are hit. Return an empty handle if not, or a FixedArray with

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

@@ -460,8 +460,10 @@ static bool matches(V8InspectorImpl* inspector, const V8DebuggerScript& script,
       V8Regex regex(inspector, selector, true);
       return regex.match(script.sourceURL()) != -1;
     }
+    case BreakpointType::kByScriptId: {
+      return script.scriptId() == selector;
+    }
     default:
-      UNREACHABLE();
       return false;
   }
 }
@@ -646,11 +648,24 @@ Response V8DebuggerAgentImpl::removeBreakpoint(const String16& breakpointId) {
   protocol::DictionaryValue* breakpointHints =
       m_state->getObject(DebuggerAgentState::breakpointHints);
   if (breakpointHints) breakpointHints->remove(breakpointId);
-  removeBreakpointImpl(breakpointId);
+
+  // Get a list of scripts to remove breakpoints.
+  // TODO(duongn): we can do better here if from breakpoint id we can tell it is
+  // not Wasm breakpoint.
+  std::vector<V8DebuggerScript*> scripts;
+  for (const auto& scriptIter : m_scripts) {
+    if (!matches(m_inspector, *scriptIter.second, type, selector)) continue;
+    V8DebuggerScript* script = scriptIter.second.get();
+    scripts.push_back(script);
+  }
+  removeBreakpointImpl(breakpointId, scripts);
+
   return Response::OK();
 }
 
-void V8DebuggerAgentImpl::removeBreakpointImpl(const String16& breakpointId) {
+void V8DebuggerAgentImpl::removeBreakpointImpl(
+    const String16& breakpointId,
+    const std::vector<V8DebuggerScript*>& scripts) {
   DCHECK(enabled());
   BreakpointIdToDebuggerBreakpointIdsMap::iterator
       debuggerBreakpointIdsIterator =
@@ -660,6 +675,9 @@ void V8DebuggerAgentImpl::removeBreakpointImpl(const String16& breakpointId) {
     return;
   }
   for (const auto& id : debuggerBreakpointIdsIterator->second) {
+    for (auto& script : scripts) {
+      script->removeWasmBreakpoint(id);
+    }
     v8::debug::RemoveBreakpoint(m_isolate, id);
     m_debuggerBreakpointIdToBreakpointId.erase(id);
   }
@@ -1706,7 +1724,8 @@ void V8DebuggerAgentImpl::removeBreakpointFor(v8::Local<v8::Function> function,
       source == DebugCommandBreakpointSource ? BreakpointType::kDebugCommand
                                              : BreakpointType::kMonitorCommand,
       function);
-  removeBreakpointImpl(breakpointId);
+  std::vector<V8DebuggerScript*> scripts;
+  removeBreakpointImpl(breakpointId, scripts);
 }
 
 void V8DebuggerAgentImpl::reset() {

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

@@ -177,7 +177,8 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
   void setBreakpointImpl(const String16& breakpointId,
                          v8::Local<v8::Function> function,
                          v8::Local<v8::String> condition);
-  void removeBreakpointImpl(const String16& breakpointId);
+  void removeBreakpointImpl(const String16& breakpointId,
+                            const std::vector<V8DebuggerScript*>& scripts);
   void clearBreakDetails();
 
   void internalSetAsyncCallStackDepth(int);

src/inspector/v8-debugger-script.cc

@@ -502,4 +502,9 @@ bool V8DebuggerScript::setBreakpoint(const String16& condition,
   return script()->SetBreakpoint(toV8String(m_isolate, condition), loc, id);
 }
 
+void V8DebuggerScript::removeWasmBreakpoint(int id) {
+  v8::HandleScope scope(m_isolate);
+  script()->RemoveWasmBreakpoint(id);
+}
+
 }  // namespace v8_inspector

src/inspector/v8-debugger-script.h

@@ -92,6 +92,7 @@ class V8DebuggerScript {
 
   virtual bool setBreakpoint(const String16& condition,
                              v8::debug::Location* location, int* id) const = 0;
+  void removeWasmBreakpoint(int id);
   virtual void MakeWeak() = 0;
   virtual bool setBreakpointOnRun(int* id) const = 0;
 

src/objects/debug-objects.cc

@@ -321,6 +321,32 @@ bool BreakPointInfo::HasBreakPoint(Isolate* isolate,
   return false;
 }
 
+MaybeHandle<BreakPoint> BreakPointInfo::GetBreakPointById(
+    Isolate* isolate, Handle<BreakPointInfo> break_point_info,
+    int breakpoint_id) {
+  // No break point.
+  if (break_point_info->break_points().IsUndefined(isolate)) {
+    return MaybeHandle<BreakPoint>();
+  }
+  // Single break point.
+  if (!break_point_info->break_points().IsFixedArray()) {
+    BreakPoint breakpoint = BreakPoint::cast(break_point_info->break_points());
+    if (breakpoint.id() == breakpoint_id) {
+      return handle(breakpoint, isolate);
+    }
+  } else {
+    // Multiple break points.
+    FixedArray array = FixedArray::cast(break_point_info->break_points());
+    for (int i = 0; i < array.length(); i++) {
+      BreakPoint breakpoint = BreakPoint::cast(array.get(i));
+      if (breakpoint.id() == breakpoint_id) {
+        return handle(breakpoint, isolate);
+      }
+    }
+  }
+  return MaybeHandle<BreakPoint>();
+}
+
 // Get the number of break points.
 int BreakPointInfo::GetBreakPointCount(Isolate* isolate) {
   // No break point.

src/objects/debug-objects.h

@@ -178,6 +178,10 @@ class BreakPointInfo : public Tuple2 {
   // Check if break point info has this break point.
   static bool HasBreakPoint(Isolate* isolate, Handle<BreakPointInfo> info,
                             Handle<BreakPoint> break_point);
+  // Check if break point info has break point with this id.
+  static MaybeHandle<BreakPoint> GetBreakPointById(Isolate* isolate,
+                                                   Handle<BreakPointInfo> info,
+                                                   int breakpoint_id);
   // Get the number of break points for this code offset.
   int GetBreakPointCount(Isolate* isolate);
 

src/wasm/wasm-debug.cc

@@ -802,6 +802,34 @@ bool WasmScript::ClearBreakPoint(Handle<Script> script, int position,
   return true;
 }
 
+// static
+bool WasmScript::ClearBreakPointById(Handle<Script> script, int breakpoint_id) {
+  if (!script->has_wasm_breakpoint_infos()) {
+    return false;
+  }
+  Isolate* isolate = script->GetIsolate();
+  Handle<FixedArray> breakpoint_infos(script->wasm_breakpoint_infos(), isolate);
+  // If the array exists, it should not be empty.
+  DCHECK_LT(0, breakpoint_infos->length());
+
+  for (int i = 0, e = breakpoint_infos->length(); i < e; ++i) {
+    Handle<Object> obj(breakpoint_infos->get(i), isolate);
+    if (obj->IsUndefined(isolate)) {
+      continue;
+    }
+    Handle<BreakPointInfo> breakpoint_info = Handle<BreakPointInfo>::cast(obj);
+    Handle<BreakPoint> breakpoint;
+    if (BreakPointInfo::GetBreakPointById(isolate, breakpoint_info,
+                                          breakpoint_id)
+            .ToHandle(&breakpoint)) {
+      DCHECK(breakpoint->id() == breakpoint_id);
+      return WasmScript::ClearBreakPoint(
+          script, breakpoint_info->source_position(), breakpoint);
+    }
+  }
+  return false;
+}
+
 namespace {
 
 int GetBreakpointPos(Isolate* isolate, Object break_point_info_or_undef) {

src/wasm/wasm-objects.h

@@ -905,6 +905,11 @@ class WasmScript : public AllStatic {
   V8_EXPORT_PRIVATE static bool ClearBreakPoint(Handle<Script>, int position,
                                                 Handle<BreakPoint> break_point);
 
+  // Remove a previously set breakpoint by id. If this breakpoint is not found,
+  // returns false.
+  V8_EXPORT_PRIVATE static bool ClearBreakPointById(Handle<Script>,
+                                                    int breakpoint_id);
+
   static void SetBreakpointsOnNewInstance(Handle<Script>,
                                           Handle<WasmInstanceObject>);
 

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

@@ -0,0 +1,23 @@
+Tests remove breakpoint from wasm scripts.
+Installing code and global variable.
+Calling instantiate function.
+Waiting for wasm scripts to be parsed.
+Ignoring script with url v8://test/callInstantiate
+Ignoring script with url wasm://wasm/fa045c1e
+Got wasm script: wasm://wasm/fa045c1e/fa045c1e-0
+Setting breakpoint on line 3 of wasm function
+{
+    columnNumber : 2
+    lineNumber : 3
+    scriptId : <scriptId>
+}
+BreakpointId: 4:3:0:6-0
+paused No 1
+  i32.const 1
+  #i32.sub
+end
+
+Remove breakpoint with breakpointId: 4:3:0:6-0
+Debugger.resume
+exports.main returned!
+Finished!

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

@@ -0,0 +1,107 @@
+// 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 remove breakpoint from wasm scripts.');
+session.setupScriptMap();
+
+utils.load('test/mjsunit/wasm/wasm-module-builder.js');
+
+let builder = new WasmModuleBuilder();
+
+// wasm_A
+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');
+
+let module_bytes = builder.toArray();
+
+function instantiate(bytes) {
+  let buffer = new ArrayBuffer(bytes.length);
+  let view = new Uint8Array(buffer);
+  for (let i = 0; i < bytes.length; ++i) {
+    view[i] = bytes[i] | 0;
+  }
+
+  let module = new WebAssembly.Module(buffer);
+  // Set global variable.
+  instance = new WebAssembly.Instance(module);
+}
+
+let evalWithUrl = (code, url) => Protocol.Runtime.evaluate(
+    {'expression': code + '\n//# sourceURL=v8://test/' + url});
+
+let breakCount = 0;
+let breakpointId = 0;
+
+Protocol.Debugger.onPaused(async message => {
+  breakCount++;
+  InspectorTest.log("paused No " + breakCount);
+  var frames = message.params.callFrames;
+  await session.logSourceLocation(frames[0].location);
+  if (breakCount == 1) {
+    InspectorTest.log('Remove breakpoint with breakpointId: ' + breakpointId);
+    await Protocol.Debugger.removeBreakpoint({breakpointId});
+  }
+  let action= 'resume';
+  InspectorTest.log('Debugger.' + action)
+  await Protocol.Debugger[action]();
+})
+
+contextGroup.addScript(`
+function test() {
+  instance.exports.main(1);
+  instance.exports.main(1);
+}
+//# sourceURL=test.js`);
+
+(async function Test() {
+  await Protocol.Debugger.enable();
+  InspectorTest.log('Installing code and global variable.');
+  await evalWithUrl('var instance;\n' + instantiate.toString(), 'setup');
+  InspectorTest.log('Calling instantiate function.');
+  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': 3}});
+  printFailure(msg);
+  InspectorTest.logMessage(msg.result.actualLocation);
+  InspectorTest.logMessage('BreakpointId: '+ msg.result.breakpointId);
+  breakpointId = msg.result.breakpointId;
+  await Protocol.Runtime.evaluate({ expression: 'test()' });
+  await Protocol.Runtime.evaluate({ expression: 'test()' });
+  InspectorTest.log('exports.main returned!');
+  InspectorTest.log('Finished!');
+  InspectorTest.completeTest();
+})();
+
+function printFailure(message) {
+  if (!message.result) {
+    InspectorTest.logMessage(message);
+  }
+  return message;
+}
+
+async function waitForWasmScript() {
+  InspectorTest.log('Waiting for wasm scripts to be parsed.');
+  while (true) {
+    let msg = await Protocol.Debugger.onceScriptParsed();
+    let url = msg.params.url;
+    if (!url.startsWith('wasm://') || url.split('/').length != 5) {
+      InspectorTest.log('Ignoring script with url ' + url);
+      continue;
+    }
+    let scriptId = msg.params.scriptId;
+    InspectorTest.log('Got wasm script: ' + url);
+    return scriptId;
+  }
+}