From 2d8764a5575091bd501a7c1d97cb2707da190a5b Mon Sep 17 00:00:00 2001 From: Danny Tuppeny Date: Wed, 25 Aug 2021 16:37:52 +0000 Subject: [PATCH] [dds] Don't automatically resume when attaching to processes via DAP Change-Id: I8b0fd4b36a9dd229226046a74e031de005908417 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/210920 Reviewed-by: Ben Konyi Commit-Queue: Ben Konyi --- pkg/dds/lib/src/dap/adapters/dart.dart | 27 +++++++--- pkg/dds/lib/src/dap/adapters/dart_cli.dart | 4 +- pkg/dds/lib/src/dap/isolate_manager.dart | 50 ++++++++++++++----- .../dap/integration/debug_attach_test.dart | 2 + pkg/dds/test/dap/integration/test_client.dart | 41 ++++++++++----- 5 files changed, 92 insertions(+), 32 deletions(-) diff --git a/pkg/dds/lib/src/dap/adapters/dart.dart b/pkg/dds/lib/src/dap/adapters/dart.dart index b2ace331f68..5099eefb695 100644 --- a/pkg/dds/lib/src/dap/adapters/dart.dart +++ b/pkg/dds/lib/src/dap/adapters/dart.dart @@ -467,11 +467,16 @@ abstract class DartDebugAdapter connectDebugger(Uri uri) async { + /// + /// If [resumeIfStarting] is true, isolates waiting to start will + /// automatically be resumed. This is usually desired in launch requests, but + /// not when attaching. + Future connectDebugger( + Uri uri, { + required bool resumeIfStarting, + }) async { // Start up a DDS instance for this VM. if (enableDds) { - // TODO(dantup): Do we need to worry about there already being one connected - // if this URL came from another service that may have started one? logger?.call('Starting a DDS instance for $uri'); try { final dds = await DartDevelopmentService.startDartDevelopmentService( @@ -552,7 +557,8 @@ abstract class DartDebugAdapter connectDebugger(uri))); + .then((uri) => connectDebugger(uri, resumeIfStarting: true))); } final vmArgs = [ @@ -230,7 +230,7 @@ class DartCliDebugAdapter extends DartDebugAdapter _threadsByThreadId[threadId]; - /// Handles Isolate and Debug events - Future handleEvent(vm.Event event) async { + /// Handles Isolate and Debug events. + /// + /// If [resumeIfStarting] is `true`, PauseStart/PausePostStart events will be + /// automatically resumed from. + Future handleEvent( + vm.Event event, { + bool resumeIfStarting = true, + }) async { final isolateId = event.isolate?.id; if (isolateId == null) { return; @@ -151,7 +157,7 @@ class IsolateManager { if (eventKind == vm.EventKind.kIsolateExit) { _handleExit(event); } else if (eventKind?.startsWith('Pause') ?? false) { - await _handlePause(event); + await _handlePause(event, resumeIfStarting: resumeIfStarting); } else if (eventKind == vm.EventKind.kResume) { _handleResumed(event); } @@ -163,7 +169,7 @@ class IsolateManager { /// New isolates will be configured with the correct pause-exception behaviour, /// libraries will be marked as debuggable if appropriate, and breakpoints /// sent. - Future registerIsolate( + Future registerIsolate( vm.IsolateRef isolate, String eventKind, ) async { @@ -193,6 +199,8 @@ class IsolateManager { await _configureIsolate(isolate); registrationCompleter.complete(); } + + return info; } Future resumeIsolate(vm.IsolateRef isolateRef, @@ -245,6 +253,11 @@ class IsolateManager { } } + /// Sends an event informing the client that a thread is stopped at entry. + void sendStoppedOnEntryEvent(int threadId) { + _adapter.sendEvent(StoppedEventBody(reason: 'entry', threadId: threadId)); + } + /// Records breakpoints for [uri]. /// /// [breakpoints] represents the new set and entirely replaces anything given @@ -368,18 +381,23 @@ class IsolateManager { /// Handles a pause event. /// - /// For [vm.EventKind.kPausePostRequest] which occurs after a restart, the isolate - /// will be re-configured (pause-exception behaviour, debuggable libraries, - /// breakpoints) and then resumed. + /// For [vm.EventKind.kPausePostRequest] which occurs after a restart, the + /// isolate will be re-configured (pause-exception behaviour, debuggable + /// libraries, breakpoints) and then (if [resumeIfStarting] is `true`) + /// resumed. /// - /// For [vm.EventKind.kPauseStart], the isolate will be resumed. + /// For [vm.EventKind.kPauseStart] and [resumeIfStarting] is `true`, the + /// isolate will be resumed. /// /// For breakpoints with conditions that are not met and for logpoints, the /// isolate will be automatically resumed. /// /// For all other pause types, the isolate will remain paused and a /// corresponding "Stopped" event sent to the editor. - Future _handlePause(vm.Event event) async { + Future _handlePause( + vm.Event event, { + bool resumeIfStarting = true, + }) async { final eventKind = event.kind; final isolate = event.isolate!; final thread = _threadsByIsolateId[isolate.id!]; @@ -396,13 +414,21 @@ class IsolateManager { // after a hot restart. if (eventKind == vm.EventKind.kPausePostRequest) { await _configureIsolate(isolate); - await resumeThread(thread.threadId); + if (resumeIfStarting) { + await resumeThread(thread.threadId); + } } else if (eventKind == vm.EventKind.kPauseStart) { // Don't resume from a PauseStart if this has already happened (see // comments on [thread.hasBeenStarted]). if (!thread.hasBeenStarted) { - thread.hasBeenStarted = true; - await resumeThread(thread.threadId); + // If requested, automatically resume. Otherwise send a Stopped event to + // inform the client UI the thread is paused. + if (resumeIfStarting) { + thread.hasBeenStarted = true; + await resumeThread(thread.threadId); + } else { + sendStoppedOnEntryEvent(thread.threadId); + } } } else { // PauseExit, PauseBreakpoint, PauseInterrupted, PauseException diff --git a/pkg/dds/test/dap/integration/debug_attach_test.dart b/pkg/dds/test/dap/integration/debug_attach_test.dart index f0e3f14fdc1..3df0fc9f4bd 100644 --- a/pkg/dds/test/dap/integration/debug_attach_test.dart +++ b/pkg/dds/test/dap/integration/debug_attach_test.dart @@ -31,6 +31,7 @@ main() { final outputEvents = await dap.client.collectOutput( launch: () => dap.client.attach( vmServiceUri: vmServiceUri.toString(), + autoResume: true, cwd: dap.testAppDir.path, ), ); @@ -81,6 +82,7 @@ main() { final outputEvents = await dap.client.collectOutput( launch: () => dap.client.attach( vmServiceInfoFile: vmServiceInfoFilePath, + autoResume: true, cwd: dap.testAppDir.path, ), ); diff --git a/pkg/dds/test/dap/integration/test_client.dart b/pkg/dds/test/dap/integration/test_client.dart index 08d7e8f76c6..77ddc1ae68f 100644 --- a/pkg/dds/test/dap/integration/test_client.dart +++ b/pkg/dds/test/dap/integration/test_client.dart @@ -62,6 +62,7 @@ class DapTestClient { /// Send an attachRequest to the server, asking it to attach to an existing /// Dart program. Future attach({ + required bool autoResume, String? vmServiceUri, String? vmServiceInfoFile, String? cwd, @@ -70,12 +71,20 @@ class DapTestClient { bool? debugExternalPackageLibraries, bool? evaluateGettersInDebugViews, bool? evaluateToStringInDebugViews, - }) { + }) async { assert( (vmServiceUri == null) != (vmServiceInfoFile == null), 'Provide exactly one of vmServiceUri/vmServiceInfoFile', ); - return sendRequest( + + // When attaching, the paused VM will not be automatically unpaused, but + // instead send a Stopped(reason: 'entry') event. Respond to this by + // resuming. + final resumeFuture = autoResume + ? expectStop('entry').then((event) => continue_(event.threadId!)) + : null; + + final attachResponse = sendRequest( DartAttachRequestArguments( vmServiceUri: vmServiceUri, vmServiceInfoFile: vmServiceInfoFile, @@ -94,6 +103,11 @@ class DapTestClient { // (DartAttachRequestArguments). overrideCommand: 'attach', ); + + // If we were expecting a pause and to resume, ensure that happens. + await resumeFuture; + + return attachResponse; } /// Sends a continue request for the given thread. @@ -553,17 +567,20 @@ extension DapTestClientExtension on DapTestClient { final result = await getValidStack(stop.threadId!, startFrame: 0, numFrames: 1); - expect(result.stackFrames, hasLength(1)); - final frame = result.stackFrames[0]; - if (file != null) { - expect(frame.source?.path, equals(file.path)); - } - if (sourceName != null) { - expect(frame.source?.name, equals(sourceName)); - } - if (line != null) { - expect(frame.line, equals(line)); + if (file != null || line != null || sourceName != null) { + expect(result.stackFrames, hasLength(1)); + final frame = result.stackFrames[0]; + + if (file != null) { + expect(frame.source?.path, equals(file.path)); + } + if (sourceName != null) { + expect(frame.source?.name, equals(sourceName)); + } + if (line != null) { + expect(frame.line, equals(line)); + } } return stop;