[dds/dap] Add support for breakpoint resolution
Previously we just always sent verified:true for all breakpoints and never handled resolution. This meant breakpoints always appeared verified in VS Code, and did not update to their correct locations when resolved.
This change sends verified:false initially, and then uses the BreakpointAdded/BreakpointResolved events to send verified:true and an updated location as the events arrive.
Fixes https://github.com/Dart-Code/Dart-Code/issues/764.
Change-Id: I5b008bef802bb1c31219175a03498f5015ec4a04
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/285909
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
diff --git a/pkg/dds/lib/src/dap/adapters/dart.dart b/pkg/dds/lib/src/dap/adapters/dart.dart
index e6fc711..5e57d09 100644
--- a/pkg/dds/lib/src/dap/adapters/dart.dart
+++ b/pkg/dds/lib/src/dap/adapters/dart.dart
@@ -1326,12 +1326,16 @@
final name = args.source.name;
final uri = path != null ? Uri.file(normalizePath(path)).toString() : name!;
- await _isolateManager.setBreakpoints(uri, breakpoints);
+ final clientBreakpoints = breakpoints.map(ClientBreakpoint.new).toList();
+ await _isolateManager.setBreakpoints(uri, clientBreakpoints);
- // TODO(dantup): Handle breakpoint resolution rather than pretending all
- // breakpoints are verified immediately.
sendResponse(SetBreakpointsResponseBody(
- breakpoints: breakpoints.map((e) => Breakpoint(verified: true)).toList(),
+ // Send breakpoints back as unverified and with our generated IDs so we
+ // can update them with a 'breakpoint' event when we get the
+ // 'BreakpointAdded'/'BreakpointResolved' events from the VM.
+ breakpoints: clientBreakpoints
+ .map((bp) => Breakpoint(id: bp.id, verified: false))
+ .toList(),
));
}
diff --git a/pkg/dds/lib/src/dap/isolate_manager.dart b/pkg/dds/lib/src/dap/isolate_manager.dart
index 36fb54e..eea5807 100644
--- a/pkg/dds/lib/src/dap/isolate_manager.dart
+++ b/pkg/dds/lib/src/dap/isolate_manager.dart
@@ -68,11 +68,11 @@
/// Tracks breakpoints last provided by the client so they can be sent to new
/// isolates that appear after initial breakpoints were sent.
- final Map<String, List<SourceBreakpoint>> _clientBreakpointsByUri = {};
+ final Map<String, List<ClientBreakpoint>> _clientBreakpointsByUri = {};
/// Tracks client breakpoints by the ID assigned by the VM so we can look up
/// conditions/logpoints when hitting breakpoints.
- final Map<String, SourceBreakpoint> _clientBreakpointsByVmId = {};
+ final Map<String, ClientBreakpoint> _clientBreakpointsByVmId = {};
/// Tracks breakpoints created in the VM so they can be removed when the
/// editor sends new breakpoints (currently the editor just sends a new list
@@ -180,6 +180,9 @@
_handleResumed(event);
} else if (eventKind == vm.EventKind.kInspect) {
_handleInspect(event);
+ } else if (eventKind == vm.EventKind.kBreakpointAdded ||
+ eventKind == vm.EventKind.kBreakpointResolved) {
+ _handleBreakpointAddedOrResolved(event);
}
}
@@ -292,7 +295,7 @@
/// before.
Future<void> setBreakpoints(
String uri,
- List<SourceBreakpoint> breakpoints,
+ List<ClientBreakpoint> breakpoints,
) async {
// Track the breakpoints to get sent to any new isolates that start.
_clientBreakpointsByUri[uri] = breakpoints;
@@ -482,7 +485,7 @@
// we hit. It's possible some of these may be missing because we could
// hit a breakpoint that was set before we were attached.
final clientBreakpoints = event.pauseBreakpoints!
- .map((bp) => _clientBreakpointsByVmId[bp.id!])
+ .map((bp) => _clientBreakpointsByVmId[bp.id!]?.breakpoint)
.toSet();
// Split into logpoints (which just print messages) and breakpoints.
@@ -556,6 +559,38 @@
}
}
+ /// Handles 'BreakpointAdded'/'BreakpointResolved' events from the VM,
+ /// informing the client of updated information about the breakpoint.
+ void _handleBreakpointAddedOrResolved(vm.Event event) {
+ final breakpoint = event.breakpoint!;
+ final breakpointId = breakpoint.id!;
+
+ final existingBreakpoint = _clientBreakpointsByVmId[breakpointId];
+ if (existingBreakpoint == null) {
+ // If we can't match this breakpoint up, we cannot get its ID or send
+ // events for it. This can happen if a breakpoint is being resolved just
+ // as a user changes breakpoints, so we have replaced our collection with
+ // a new set before we processed this event.
+ return;
+ }
+
+ // Location may be [SourceLocation] or [UnresolvedSourceLocaion] depending
+ // on whether this is an Added or Resolved event.
+ final location = breakpoint.location;
+ final resolvedLocation = location is vm.SourceLocation ? location : null;
+ final unresolvedLocation =
+ location is vm.UnresolvedSourceLocation ? location : null;
+ final updatedBreakpoint = Breakpoint(
+ id: existingBreakpoint.id,
+ line: resolvedLocation?.line ?? unresolvedLocation?.line,
+ column: resolvedLocation?.column ?? unresolvedLocation?.column,
+ verified: breakpoint.resolved ?? false,
+ );
+ _adapter.sendEvent(
+ BreakpointEventBody(breakpoint: updatedBreakpoint, reason: 'changed'),
+ );
+ }
+
/// Attempts to resolve [uris] to file:/// URIs via the VM Service.
///
/// This method calls the VM service directly. Most requests to resolve URIs
@@ -675,7 +710,7 @@
// Set new breakpoints.
final newBreakpoints = _clientBreakpointsByUri[uri] ?? const [];
- await Future.forEach<SourceBreakpoint>(newBreakpoints, (bp) async {
+ await Future.forEach<ClientBreakpoint>(newBreakpoints, (bp) async {
try {
// Some file URIs (like SDK sources) need to be converted to
// appropriate internal URIs to be able to set breakpoints.
@@ -688,8 +723,8 @@
}
final vmBp = await service.addBreakpointWithScriptUri(
- isolateId, vmUri.toString(), bp.line,
- column: bp.column);
+ isolateId, vmUri.toString(), bp.breakpoint.line,
+ column: bp.breakpoint.column);
existingBreakpointsForIsolateAndUri[vmBp.id!] = vmBp;
_clientBreakpointsByVmId[vmBp.id!] = bp;
} catch (e) {
@@ -1055,6 +1090,29 @@
}
}
+/// A wrapper over the client-provided [SourceBreakpoint] with a unique ID.
+///
+/// In order to tell clients about breakpoint changes (such as resolution) we
+/// must assign them an ID. If the VM does not have any running Isolates at the
+/// time initial breakpoints are set we cannot yet send the breakpoints (and
+/// therefore cannot get IDs from the VM). So we generate our own IDs and hold
+/// them with the breakpoint here. When we get a 'BreakpointResolved' event we
+/// can look up this [ClientBreakpoint] and use the ID to send an update to the
+/// client.
+class ClientBreakpoint {
+ /// The next number to use as a client ID for breakpoints.
+ ///
+ /// To slightly improve debugging, we start this at 100000 so it doesn't
+ /// initially overlap with VM-produced breakpoint numbers so it's more obvious
+ /// in log files which numbers are DAP-client and which are VM.
+ static int _nextId = 100000;
+
+ final SourceBreakpoint breakpoint;
+ final int id;
+
+ ClientBreakpoint(this.breakpoint) : id = _nextId++;
+}
+
class StoredData {
final ThreadInfo thread;
final Object data;
diff --git a/pkg/dds/test/dap/integration/debug_breakpoints_test.dart b/pkg/dds/test/dap/integration/debug_breakpoints_test.dart
index ceb6508..08bb84e 100644
--- a/pkg/dds/test/dap/integration/debug_breakpoints_test.dart
+++ b/pkg/dds/test/dap/integration/debug_breakpoints_test.dart
@@ -4,6 +4,7 @@
import 'dart:io';
+import 'package:dds/dap.dart';
import 'package:path/path.dart' as path;
import 'package:test/test.dart';
@@ -27,6 +28,42 @@
await client.hitBreakpoint(testFile, breakpointLine);
});
+ test('resolves and updates breakpoints', () async {
+ final client = dap.client;
+ final testFile = dap.createTestFile(simpleBreakpointResolutionProgram);
+ final setBreakpointLine = lineWith(testFile, breakpointMarker);
+ final expectedResolvedBreakpointLine = setBreakpointLine + 1;
+
+ // Collect any breakpoint changes during the run.
+ final breakpointChangesFuture = client.breakpointChangeEvents.toList();
+
+ Future<SetBreakpointsResponseBody> setBreakpointFuture;
+ await Future.wait([
+ client
+ .expectStop('breakpoint',
+ file: testFile, line: expectedResolvedBreakpointLine)
+ .then((_) => client.terminate()),
+ client.initialize(),
+ setBreakpointFuture = client.setBreakpoint(testFile, setBreakpointLine),
+ client.launch(testFile.path),
+ ], eagerError: true);
+
+ // The initial setBreakpointResponse should always return unverified
+ // because we verify using the BreakpointAdded/BreakpointResolved events.
+ final setBreakpointResponse = await setBreakpointFuture;
+ expect(setBreakpointResponse.breakpoints, hasLength(1));
+ final setBreakpoint = setBreakpointResponse.breakpoints.single;
+ expect(setBreakpoint.verified, isFalse);
+
+ // The last breakpoint change we had should be verified and also update
+ // the line to [expectedResolvedBreakpointLine] since the breakpoint was
+ // on a blank line.
+ final breakpointChanges = await breakpointChangesFuture;
+ final updatedBreakpoint = breakpointChanges.last.breakpoint;
+ expect(updatedBreakpoint.verified, isTrue);
+ expect(updatedBreakpoint.line, expectedResolvedBreakpointLine);
+ });
+
test('does not stop at a removed breakpoint', () async {
final testFile = dap.createTestFile('''
void main(List<String> args) async {
diff --git a/pkg/dds/test/dap/integration/test_client.dart b/pkg/dds/test/dap/integration/test_client.dart
index c5a3a28..fc2c616 100644
--- a/pkg/dds/test/dap/integration/test_client.dart
+++ b/pkg/dds/test/dap/integration/test_client.dart
@@ -98,6 +98,12 @@
events('dart.testNotification')
.map((e) => e.body as Map<String, Object?>);
+ /// Waits for a 'breakpoint' event that changes the breakpoint with [id].
+ Stream<BreakpointEventBody> get breakpointChangeEvents => events('breakpoint')
+ .map((event) =>
+ BreakpointEventBody.fromJson(event.body as Map<String, Object?>))
+ .where((body) => body.reason == 'changed');
+
/// Send an attachRequest to the server, asking it to attach to an existing
/// Dart program.
Future<Response> attach({
@@ -571,13 +577,18 @@
}
/// Sets a breakpoint at [line] in [file].
- Future<void> setBreakpoint(File file, int line, {String? condition}) async {
- await sendRequest(
+ Future<SetBreakpointsResponseBody> setBreakpoint(File file, int line,
+ {String? condition}) async {
+ final response = await sendRequest(
SetBreakpointsArguments(
source: Source(path: _normalizeBreakpointPath(file.path)),
breakpoints: [SourceBreakpoint(line: line, condition: condition)],
),
);
+
+ return SetBreakpointsResponseBody.fromJson(
+ response.body as Map<String, Object?>,
+ );
}
/// Sets breakpoints at [lines] in [file].
diff --git a/pkg/dds/test/dap/integration/test_scripts.dart b/pkg/dds/test/dap/integration/test_scripts.dart
index 5d155cb..4e521d7 100644
--- a/pkg/dds/test/dap/integration/test_scripts.dart
+++ b/pkg/dds/test/dap/integration/test_scripts.dart
@@ -127,6 +127,16 @@
}
''';
+/// A simple Dart script that should run with no errors and contains a comment
+/// marker '// BREAKPOINT' on a blank line where a breakpoint should be resolved
+/// to the next line.
+const simpleBreakpointResolutionProgram = '''
+ void main(List<String> args) async {
+ $breakpointMarker
+ print('Hello!');
+ }
+''';
+
/// A simple Dart script that has a blank line before its breakpoint, used to
/// ensure breakpoints that resolve to the same place are handled correctly.
const simpleBreakpointWithLeadingBlankLineProgram = '''
