[dds/dap] Don't call setLibraryDebuggable unnecessarily

Fixes https://github.com/Dart-Code/Dart-Code/issues/4464.

TEST=DAP tests included, VM changes are only a comment
Change-Id: I5ee44fda5b954d371bd00345815c24515eb14f61
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/291282
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
This commit is contained in:
Danny Tuppeny 2023-03-29 15:54:02 +00:00 committed by Commit Queue
parent ed09412b96
commit 11129423b9
5 changed files with 262 additions and 47 deletions

View file

@ -317,7 +317,8 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
/// Manages VM Isolates and their events, including fanning out any requests
/// to set breakpoints etc. from the client to all Isolates.
late IsolateManager _isolateManager;
@visibleForTesting
late IsolateManager isolateManager;
/// A helper that handlers converting to/from DAP and VM Service types.
late ProtocolConverter _converter;
@ -459,11 +460,11 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
Future<void> preventBreakingAndResume() async {
// Remove anything that may cause us to pause again.
await Future.wait([
_isolateManager.clearAllBreakpoints(),
_isolateManager.setExceptionPauseMode('None'),
isolateManager.clearAllBreakpoints(),
isolateManager.setExceptionPauseMode('None'),
]);
// Once those have completed, it's safe to resume anything paused.
await _isolateManager.resumeAll();
await isolateManager.resumeAll();
}
DartDebugAdapter(
@ -480,7 +481,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
dartSdkRoot = path.dirname(path.dirname(vmPath));
orgDartlangSdkMappings[dartSdkRoot] = Uri.parse('org-dartlang-sdk:///sdk');
_isolateManager = IsolateManager(this);
isolateManager = IsolateManager(this);
_converter = ProtocolConverter(this);
}
@ -542,7 +543,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
// When attaching to a process, suppress auto-resuming isolates until the
// first time the user resumes anything.
_isolateManager.autoResumeStartingIsolates = false;
isolateManager.autoResumeStartingIsolates = false;
// Common setup.
await _prepareForLaunchOrAttach(null);
@ -745,7 +746,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
? vm.EventKind.kIsolateRunnable
: vm.EventKind.kIsolateStart;
final thread =
await _isolateManager.registerIsolate(isolate, pauseEventKind);
await isolateManager.registerIsolate(isolate, pauseEventKind);
// If the Isolate already has a Pause event we can give it to the
// IsolateManager to handle (if it's PausePostStart it will re-configure
@ -753,16 +754,16 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
// runnable - otherwise we'll handle this when it becomes runnable in an
// event later).
if (isolate.pauseEvent?.kind?.startsWith('Pause') ?? false) {
await _isolateManager.handleEvent(
await isolateManager.handleEvent(
isolate.pauseEvent!,
);
} else if (isolate.runnable == true) {
// If requested, automatically resume. Otherwise send a Stopped event to
// inform the client UI the thread is paused.
if (_isolateManager.autoResumeStartingIsolates) {
await _isolateManager.resumeIsolate(isolate);
if (isolateManager.autoResumeStartingIsolates) {
await isolateManager.resumeIsolate(isolate);
} else {
_isolateManager.sendStoppedOnEntryEvent(thread.threadId);
isolateManager.sendStoppedOnEntryEvent(thread.threadId);
}
}
}));
@ -776,7 +777,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
ContinueArguments args,
void Function(ContinueResponseBody) sendResponse,
) async {
await _isolateManager.resumeThread(args.threadId);
await isolateManager.resumeThread(args.threadId);
sendResponse(ContinueResponseBody(allThreadsContinued: false));
}
@ -839,7 +840,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
// through the run daemon) as it needs to perform additional work to
// rebuild widgets afterwards.
case 'hotReload':
await _isolateManager.reloadSources();
await isolateManager.reloadSources();
sendResponse(_noResult);
break;
@ -919,7 +920,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
ThreadInfo? thread;
int? frameIndex;
if (frameId != null) {
final data = _isolateManager.getStoredData(frameId);
final data = isolateManager.getStoredData(frameId);
if (data != null) {
thread = data.thread;
frameIndex = (data.data as vm.Frame).index;
@ -1182,9 +1183,9 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
/// by the `updateDebugOptions` custom request.
Future<bool> libraryIsDebuggable(ThreadInfo thread, Uri uri) async {
if (isSdkLibrary(uri)) {
return _isolateManager.debugSdkLibraries;
return isolateManager.debugSdkLibraries;
} else if (await isExternalPackageLibrary(thread, uri)) {
return _isolateManager.debugExternalPackageLibraries;
return isolateManager.debugExternalPackageLibraries;
} else {
return true;
}
@ -1198,7 +1199,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
NextArguments args,
void Function() sendResponse,
) async {
await _isolateManager.resumeThread(args.threadId, vm.StepOption.kOver);
await isolateManager.resumeThread(args.threadId, vm.StepOption.kOver);
sendResponse();
}
@ -1227,7 +1228,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
ScopesArguments args,
void Function(ScopesResponseBody) sendResponse,
) async {
final storedData = _isolateManager.getStoredData(args.frameId);
final storedData = isolateManager.getStoredData(args.frameId);
final thread = storedData?.thread;
final data = storedData?.data;
final frameData = data is vm.Frame ? data : null;
@ -1237,7 +1238,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
scopes.add(Scope(
name: 'Locals',
presentationHint: 'locals',
variablesReference: _isolateManager.storeData(
variablesReference: isolateManager.storeData(
thread,
FrameScopeData(frameData, FrameScopeDataKind.locals),
),
@ -1247,7 +1248,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
scopes.add(Scope(
name: 'Globals',
presentationHint: 'globals',
variablesReference: _isolateManager.storeData(
variablesReference: isolateManager.storeData(
thread,
FrameScopeData(frameData, FrameScopeDataKind.globals),
),
@ -1325,7 +1326,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
/// the file URI in [args.source.path].
///
/// The VM requires breakpoints to be set per-isolate so these will be passed
/// to [_isolateManager] that will fan them out to each isolate.
/// to [isolateManager] that will fan them out to each isolate.
///
/// When new isolates are registered, it is [isolateManager]'s responsibility
/// to ensure all breakpoints are given to them (and like at startup, this
@ -1343,7 +1344,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
final uri = path != null ? Uri.file(normalizePath(path)).toString() : name!;
final clientBreakpoints = breakpoints.map(ClientBreakpoint.new).toList();
await _isolateManager.setBreakpoints(uri, clientBreakpoints);
await isolateManager.setBreakpoints(uri, clientBreakpoints);
sendResponse(SetBreakpointsResponseBody(
// Send breakpoints back as unverified and with our generated IDs so we
@ -1361,7 +1362,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
/// the app is running).
///
/// The VM requires exception modes to be set per-isolate so these will be
/// passed to [_isolateManager] that will fan them out to each isolate.
/// passed to [isolateManager] that will fan them out to each isolate.
///
/// When new isolates are registered, it is [isolateManager]'s responsibility
/// to ensure the pause mode is given to them (and like at startup, this
@ -1378,7 +1379,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
? 'Unhandled'
: 'None';
await _isolateManager.setExceptionPauseMode(mode);
await isolateManager.setExceptionPauseMode(mode);
sendResponse(SetExceptionBreakpointsResponseBody());
}
@ -1477,7 +1478,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
SourceArguments args,
void Function(SourceResponseBody) sendResponse,
) async {
final storedData = _isolateManager.getStoredData(
final storedData = isolateManager.getStoredData(
args.source?.sourceReference ?? args.sourceReference,
);
if (storedData == null) {
@ -1528,7 +1529,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
const stackFrameBatchSize = 20;
final threadId = args.threadId;
final thread = _isolateManager.getThread(threadId);
final thread = isolateManager.getThread(threadId);
final topFrame = thread?.pauseEvent?.topFrame;
final startFrame = args.startFrame ?? 0;
final numFrames = args.levels ?? 0;
@ -1625,7 +1626,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
StepInArguments args,
void Function() sendResponse,
) async {
await _isolateManager.resumeThread(args.threadId, vm.StepOption.kInto);
await isolateManager.resumeThread(args.threadId, vm.StepOption.kInto);
sendResponse();
}
@ -1636,7 +1637,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
StepOutArguments args,
void Function() sendResponse,
) async {
await _isolateManager.resumeThread(args.threadId, vm.StepOption.kOut);
await isolateManager.resumeThread(args.threadId, vm.StepOption.kOut);
sendResponse();
}
@ -1687,7 +1688,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
void Function(ThreadsResponseBody) sendResponse,
) async {
final threads = [
for (final thread in _isolateManager.threads)
for (final thread in isolateManager.threads)
Thread(
id: thread.threadId,
name: thread.isolate.name ?? '<unnamed isolate>',
@ -1716,7 +1717,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
) async {
final childStart = args.start;
final childCount = args.count;
final storedData = _isolateManager.getStoredData(args.variablesReference);
final storedData = isolateManager.getStoredData(args.variablesReference);
if (storedData == null) {
throw StateError('variablesReference is no longer valid');
}
@ -1819,7 +1820,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
]);
}
} else if (data is vm.ObjRef) {
final object = await _isolateManager.getObject(
final object = await isolateManager.getObject(
storedData.thread.isolate,
data,
offset: childStart,
@ -1957,7 +1958,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
// isolate printed an error to stderr, we just have to use the first one and
// hope the packages are available. If one is not available (which should
// never be the case), we will just skip resolution.
final thread = _isolateManager.threads.firstOrNull;
final thread = isolateManager.threads.firstOrNull;
// Send a batch request. This will cache the results so we can easily use
// them in the loop below by calling the method again.
@ -2028,7 +2029,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
String expression,
ThreadInfo thread,
) async {
final exception = _isolateManager.getStoredData(exceptionReference)?.data
final exception = isolateManager.getStoredData(exceptionReference)?.data
as vm.InstanceRef?;
if (exception == null) {
@ -2060,7 +2061,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
// it's doing is own initialization that this may interfere with.
await debuggerInitialized;
await _isolateManager.handleEvent(event);
await isolateManager.handleEvent(event);
final eventKind = event.kind;
final isolate = event.isolate;
@ -2071,7 +2072,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
eventKind == vm.EventKind.kPauseExit &&
isolate != null) {
await _waitForPendingOutputEvents();
await _isolateManager.resumeIsolate(isolate);
await isolateManager.resumeIsolate(isolate);
}
}
@ -2093,7 +2094,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
await debuggerInitialized;
// Allow IsolateManager to handle any state-related events.
await _isolateManager.handleEvent(event);
await isolateManager.handleEvent(event);
switch (event.kind) {
// Pass any Service Extension events on to the client so they can enable
@ -2143,7 +2144,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
@mustCallSuper
Future<void> handleLoggingEvent(vm.Event event) async {
final record = event.logRecord;
final thread = _isolateManager.threadForIsolate(event.isolate);
final thread = isolateManager.threadForIsolate(event.isolate);
if (record == null || thread == null) {
return;
}
@ -2196,7 +2197,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
Map<String, Object?> data,
String field,
) async {
final thread = _isolateManager.threadForIsolate(isolate);
final thread = isolateManager.threadForIsolate(isolate);
if (thread == null) {
return;
}
@ -2283,9 +2284,9 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
// Notify IsolateManager if we'll be debugging so it knows whether to set
// up breakpoints etc. when isolates are registered.
final debug = !(noDebug ?? false);
_isolateManager.debug = debug;
_isolateManager.debugSdkLibraries = args.debugSdkLibraries ?? true;
_isolateManager.debugExternalPackageLibraries =
isolateManager.debug = debug;
isolateManager.debugSdkLibraries = args.debugSdkLibraries ?? true;
isolateManager.debugExternalPackageLibraries =
args.debugExternalPackageLibraries ?? true;
}
@ -2328,13 +2329,13 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
/// in the map will not be updated by this method.
Future<void> _updateDebugOptions(Map<String, Object?> args) async {
if (args.containsKey('debugSdkLibraries')) {
_isolateManager.debugSdkLibraries = args['debugSdkLibraries'] as bool;
isolateManager.debugSdkLibraries = args['debugSdkLibraries'] as bool;
}
if (args.containsKey('debugExternalPackageLibraries')) {
_isolateManager.debugExternalPackageLibraries =
isolateManager.debugExternalPackageLibraries =
args['debugExternalPackageLibraries'] as bool;
}
await _isolateManager.applyDebugOptions();
await isolateManager.applyDebugOptions();
}
/// A wrapper around the same name function from package:vm_service that

View file

@ -777,12 +777,18 @@ class IsolateManager {
await Future.wait(libraries.map((library) async {
final libraryUri = library.uri;
final isDebuggable = libraryUri != null
final isDebuggableNew = libraryUri != null
? await _adapter.libraryIsDebuggable(thread, Uri.parse(libraryUri))
: false;
final isDebuggableCurrent =
thread.getIsLibraryCurrentlyDebuggable(library);
thread.setIsLibraryCurrentlyDebuggable(library, isDebuggableNew);
if (isDebuggableNew == isDebuggableCurrent) {
return;
}
try {
await service.setLibraryDebuggable(
isolateId, library.id!, isDebuggable);
isolateId, library.id!, isDebuggableNew);
} on vm.RPCError catch (e) {
// DWDS does not currently support `setLibraryDebuggable` so instead of
// failing (because this code runs in a VM event handler where there's
@ -999,6 +1005,46 @@ class ThreadInfo {
return Future.wait(futures);
}
/// Returns whether [library] is currently debuggable according to the VM
/// (or there is a request in-flight to set it).
bool getIsLibraryCurrentlyDebuggable(vm.LibraryRef library) {
return _libraryIsDebuggableById[library.id!] ??
_getIsLibraryDebuggableByDefault(library);
}
/// Records whether [library] is currently debuggable for this isolate.
///
/// This should be called whenever a `setLibraryDebuggable` request is made
/// to the VM.
void setIsLibraryCurrentlyDebuggable(
vm.LibraryRef library,
bool isDebuggable,
) {
if (isDebuggable == _getIsLibraryDebuggableByDefault(library)) {
_libraryIsDebuggableById.remove(library.id!);
} else {
_libraryIsDebuggableById[library.id!] = isDebuggable;
}
}
/// Returns whether [library] is debuggable by default.
///
/// This value is _assumed_ to avoid having to fetch each library for each
/// isolate.
bool _getIsLibraryDebuggableByDefault(vm.LibraryRef library) {
final isSdkLibrary = library.uri?.startsWith('dart:') ?? false;
return !isSdkLibrary;
}
/// Tracks whether libraries are currently marked as debuggable in the VM.
///
/// If a library ID is not in the map, it is set to the default (which is
/// debuggable for non-SDK sources, and not-debuggable for SDK sources).
///
/// This can be used to avoid calling setLibraryDebuggable where the value
/// would not be changed.
final _libraryIsDebuggableById = <String, bool>{};
/// Resolves a source URI to a file path for the lib folder of its package.
///
/// package:foo/a/b/c/d.dart -> /code/packages/foo/lib

View file

@ -0,0 +1,103 @@
// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
import 'package:test/test.dart';
import 'package:vm_service/vm_service.dart';
import 'mocks.dart';
main() {
group('IsolateManager', () {
final adapter = MockDartCliDebugAdapter();
final isolateManager = adapter.isolateManager;
setUp(() async {
isolateManager.debug = true;
isolateManager.debugSdkLibraries = false;
isolateManager.debugExternalPackageLibraries = true;
await isolateManager.registerIsolate(
adapter.mockService.isolate,
EventKind.kIsolateStart,
);
});
test('sends only changes to SDK libraries debuggable flag', () async {
// Default is false, so should not send anything.
isolateManager.debugSdkLibraries = false;
await isolateManager.applyDebugOptions();
expect(
adapter.mockService.requests,
isNot(
contains(startsWith('setLibraryDebuggable(isolate1, libSdk,')),
),
);
// Changing to non-default should send a request.
isolateManager.debugSdkLibraries = true;
await isolateManager.applyDebugOptions();
expect(
adapter.mockService.requests,
contains('setLibraryDebuggable(isolate1, libSdk, true)'),
);
// Setting back to default should now send.
isolateManager.debugSdkLibraries = false;
await isolateManager.applyDebugOptions();
expect(
adapter.mockService.requests,
contains('setLibraryDebuggable(isolate1, libSdk, false)'),
);
});
test('sends only changes to external package libraries debuggable flag',
() async {
// Default is true, so should not send anything.
isolateManager.debugExternalPackageLibraries = true;
await isolateManager.applyDebugOptions();
expect(
adapter.mockService.requests,
isNot(
contains(
startsWith('setLibraryDebuggable(isolate1, libPkgExternal,')),
),
);
// Changing to non-default should send a request.
isolateManager.debugExternalPackageLibraries = false;
await isolateManager.applyDebugOptions();
expect(
adapter.mockService.requests,
contains('setLibraryDebuggable(isolate1, libPkgExternal, false)'),
);
// Setting back to default should now send.
isolateManager.debugExternalPackageLibraries = true;
await isolateManager.applyDebugOptions();
expect(
adapter.mockService.requests,
contains('setLibraryDebuggable(isolate1, libPkgExternal, true)'),
);
});
/// Local packages are always debuggable, in the VM and because we have no
/// settings. No changes should ever cause requests for them.
test('never sends values for local package libraries debuggable flag',
() async {
isolateManager.debugSdkLibraries = true;
isolateManager.debugExternalPackageLibraries = true;
await isolateManager.applyDebugOptions();
isolateManager.debugSdkLibraries = false;
isolateManager.debugExternalPackageLibraries = false;
await isolateManager.applyDebugOptions();
expect(
adapter.mockService.requests,
isNot(
contains(startsWith('setLibraryDebuggable(isolate1, libPkgLocal,')),
),
);
});
});
}

View file

@ -7,6 +7,8 @@ import 'dart:async';
import 'package:dds/dap.dart';
import 'package:dds/src/dap/adapters/dart_cli_adapter.dart';
import 'package:dds/src/dap/adapters/dart_test_adapter.dart';
import 'package:dds/src/dap/isolate_manager.dart';
import 'package:vm_service/vm_service.dart';
/// A [DartCliDebugAdapter] that captures information about the process that
/// will be launched.
@ -14,6 +16,8 @@ class MockDartCliDebugAdapter extends DartCliDebugAdapter {
final StreamSink<List<int>> stdin;
final Stream<List<int>> stdout;
final mockService = MockVmService();
late bool launchedInTerminal;
late String executable;
late List<String> processArgs;
@ -32,7 +36,14 @@ class MockDartCliDebugAdapter extends DartCliDebugAdapter {
MockDartCliDebugAdapter.withStreams(
this.stdin, this.stdout, ByteStreamServerChannel channel)
: super(channel);
: super(channel) {
vmService = mockService;
}
@override
Future<bool> isExternalPackageLibrary(ThreadInfo thread, Uri uri) async {
return uri.isScheme('package') && uri.path.startsWith('external');
}
@override
Future<void> launchAsProcess(
@ -116,3 +127,53 @@ class MockRequest extends Request {
'seq': _requestId++,
});
}
class MockVmService implements VmServiceInterface {
@override
dynamic noSuchMethod(Invocation invocation) => super.noSuchMethod(invocation);
final isolate = IsolateRef(id: 'isolate1');
final sdkLibrary = LibraryRef(id: 'libSdk', uri: 'dart:core');
final externalPackageLibrary =
LibraryRef(id: 'libPkgExternal', uri: 'package:external/foo.dart');
final localPackageLibrary =
LibraryRef(id: 'libPkgLocal', uri: 'package:local/foo.dart');
/// A human-readable string for each request made, useful for verifying in
/// tests.
final List<String> requests = [];
@override
Future<Success> setLibraryDebuggable(
String isolateId,
String libraryId,
bool isDebuggable,
) async {
requests.add('setLibraryDebuggable($isolateId, $libraryId, $isDebuggable)');
return Success();
}
@override
Future<Isolate> getIsolate(String isolateId) async {
return Isolate(
id: isolate.id,
libraries: [sdkLibrary, externalPackageLibrary, localPackageLibrary],
);
}
@override
Future<UriList> lookupResolvedPackageUris(
String isolateId,
List<String> uris, {
bool? local,
}) async {
return UriList(uris: uris.map((e) => null).toList());
}
@override
Future<VM> getVM() async {
return VM(
isolates: [isolate],
);
}
}

View file

@ -13996,6 +13996,10 @@ LibraryPtr Library::NewLibraryHelper(const String& url, bool import_core_lib) {
result.set_flags(0);
result.set_is_in_fullsnapshot(false);
result.set_is_nnbd(false);
// This logic is also in the DAP debug adapter in DDS to avoid needing
// to call setLibraryDebuggable for every library for every isolate.
// If these defaults change, the same should be done there in
// dap/IsolateManager._getIsLibraryDebuggableByDefault.
if (dart_scheme) {
// Only debug dart: libraries if we have been requested to show invisible
// frames.