[VM/Debugger] Fix behaviour of setting breakpoints
My older CL that aimed to fix inline breakpoints was not quite correct.
It introduced some desirable behaviour, but also some undesirable
behaviour like https://github.com/dart-lang/sdk/issues/54100. This CL
reverts most of the changes in
https://dart-review.googlesource.com/c/sdk/+/316440 and then builds on
top of that.
TEST=pkg/vm_service/test/column_breakpoint_test.dart,
pkg/vm_service/test/column_breakpoint_test.dart, other debugger tests
Fixes: https://github.com/dart-lang/sdk/issues/54100
Change-Id: I2f5fdd55070aea9412a0b37e83c930cd24382adf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/338040
Commit-Queue: Derek Xu <derekx@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
diff --git a/pkg/vm_service/test/add_breakpoint_rpc_kernel_test.dart b/pkg/vm_service/test/add_breakpoint_rpc_kernel_test.dart
index 1c11d2f..d9200af 100644
--- a/pkg/vm_service/test/add_breakpoint_rpc_kernel_test.dart
+++ b/pkg/vm_service/test/add_breakpoint_rpc_kernel_test.dart
@@ -144,12 +144,14 @@
bpt.location!.tokenPos!,
)!;
print('$LINE_A:$col -> $resolvedLine:$resolvedCol');
- if (col <= 12) {
+ if (col < 12) {
+ // The second 'incValue' begins at column 12.
expect(resolvedLine, LINE_A);
expect(bpt.location!.line, LINE_A);
expect(resolvedCol, 3);
expect(bpt.location!.column, 3);
} else if (col <= 36) {
+ // The newline character at the end of LINE_A is at column 36.
expect(resolvedLine, LINE_A);
expect(bpt.location!.line, LINE_A);
expect(resolvedCol, 12);
diff --git a/pkg/vm_service/test/break_on_function_declared_inside_map_test.dart b/pkg/vm_service/test/break_on_function_declared_inside_map_test.dart
new file mode 100644
index 0000000..68c55904
--- /dev/null
+++ b/pkg/vm_service/test/break_on_function_declared_inside_map_test.dart
@@ -0,0 +1,56 @@
+// 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 'common/service_test_common.dart';
+import 'common/test_helper.dart';
+
+// AUTOGENERATED START
+//
+// Update these constants by running:
+//
+// dart pkg/vm_service/test/update_line_numbers.dart <test.dart>
+//
+const LINE_A = 22;
+const LINE_B = 25;
+// AUTOGENERATED END
+
+const String shortFile = 'break_on_function_declared_inside_map_test.dart';
+
+void testMain() {
+ final funcs = {
+ /* LINE_A */ 'a': () {
+ print('a');
+ },
+ /* LINE_B */ 'b': () {
+ print('b');
+ },
+ };
+
+ funcs['a']!();
+ funcs['b']!();
+}
+
+final stops = <String>[];
+
+const expected = <String>[
+ '$shortFile:$LINE_A:23', // on '(' of '()'
+ '$shortFile:$LINE_B:23', // on '(' of '()'
+];
+
+final tests = <IsolateTest>[
+ hasPausedAtStart,
+ setBreakpointAtLineColumn(LINE_A, 24), // on ')' of '()'
+ setBreakpointAtLine(LINE_B),
+ resumeProgramRecordingStops(stops, false),
+ checkRecordedStops(stops, expected),
+];
+
+Future<void> main([args = const <String>[]]) => runIsolateTests(
+ args,
+ tests,
+ 'break_on_function_declared_inside_map_test.dart',
+ testeeConcurrent: testMain,
+ pauseOnStart: true,
+ pauseOnExit: true,
+ );
diff --git a/pkg/vm_service/test/column_breakpoint_test.dart b/pkg/vm_service/test/column_breakpoint_test.dart
index 3ceb6fe..ea1c187 100644
--- a/pkg/vm_service/test/column_breakpoint_test.dart
+++ b/pkg/vm_service/test/column_breakpoint_test.dart
@@ -5,27 +5,35 @@
import 'common/service_test_common.dart';
import 'common/test_helper.dart';
-void testMain() {
- final b = [1, 2].map((i) => i == 0).toList();
- print(b.length);
-}
+// AUTOGENERATED START
+//
+// Update these constants by running:
+//
+// dart pkg/vm_service/test/update_line_numbers.dart <test.dart>
+//
+const LINE_A = 21;
+const LINE_B = 22;
+// AUTOGENERATED END
-const int LINE = 9;
-const int COLUMN = 29;
const String shortFile = 'column_breakpoint_test.dart';
+void testMain() {
+ final b = [1, 2].map((i) => i == 0).toList(); // LINE_A
+ print(b.length); // LINE_B
+}
+
final stops = <String>[];
const expected = <String>[
- '$shortFile:${LINE + 0}:33', // on 'i == 0'
- '$shortFile:${LINE + 0}:33', // iterate twice
- '$shortFile:${LINE + 1}:11', //on 'b.length'
+ '$shortFile:$LINE_A:33', // on first '=' of 'i == 0'
+ '$shortFile:$LINE_A:33', // iterate twice
+ '$shortFile:$LINE_B:11', // on 'l' of 'b.length'
];
final tests = <IsolateTest>[
hasPausedAtStart,
- setBreakpointAtLineColumn(LINE, COLUMN), // on 'i == 0'
- setBreakpointAtLineColumn(LINE + 1, 9), // on 'b.length'
+ setBreakpointAtLineColumn(LINE_A, 34), // on second '=' of 'i == 0'
+ setBreakpointAtLineColumn(LINE_B, 13), // on 'n' of 'b.length'
resumeProgramRecordingStops(stops, false),
checkRecordedStops(stops, expected),
];
diff --git a/runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart b/runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart
index 96ae4b5..67fa8ba 100644
--- a/runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart
+++ b/runtime/observatory/tests/service/add_breakpoint_rpc_kernel_test.dart
@@ -88,7 +88,7 @@
int resolvedLine = await bpt.location!.getLine() as int;
int resolvedCol = await bpt.location!.getColumn() as int;
print('$LINE_A:${col} -> ${resolvedLine}:${resolvedCol}');
- if (col <= 12) {
+ if (col < 12) {
expect(resolvedLine, equals(LINE_A));
expect(resolvedCol, equals(3));
} else if (col <= 36) {
diff --git a/runtime/observatory/tests/service/column_breakpoint_test.dart b/runtime/observatory/tests/service/column_breakpoint_test.dart
index 8d78440..7f4da75 100644
--- a/runtime/observatory/tests/service/column_breakpoint_test.dart
+++ b/runtime/observatory/tests/service/column_breakpoint_test.dart
@@ -6,12 +6,12 @@
import 'service_test_common.dart';
void testMain() {
- var b = [1, 2].map((i) => i == 0).toList();
+ final b = [1, 2].map((i) => i == 0).toList();
print(b.length);
}
-const int LINE = 9;
-const int COLUMN = 29;
+const LINE_A = 9;
+const LINE_B = 10;
const String shortFile = "column_breakpoint_test.dart";
const String breakpointFile =
"package:observatory_test_package/column_breakpoint_test.dart";
@@ -19,15 +19,15 @@
List<String> stops = [];
const List<String> expected = [
- "$shortFile:${LINE + 0}:29", // on 'i == 0'
- "$shortFile:${LINE + 0}:29", // iterate twice
- "$shortFile:${LINE + 1}:11" //on 'b.length'
+ '$shortFile:$LINE_A:33', // on first '=' of 'i == 0'
+ '$shortFile:$LINE_A:33', // iterate twice
+ '$shortFile:$LINE_B:11', // on 'l' of 'b.length'
];
final tests = <IsolateTest>[
hasPausedAtStart,
- setBreakpointAtLineColumn(LINE, COLUMN), // on 'i == 0'
- setBreakpointAtLineColumn(LINE + 1, 9), // on 'b.length'
+ setBreakpointAtLineColumn(LINE_A, 34), // on second '=' of 'i == 0'
+ setBreakpointAtLineColumn(LINE_B, 13), // on 'n' of 'b.length'
resumeProgramRecordingStops(stops, false),
checkRecordedStops(stops, expected)
];
diff --git a/runtime/vm/debugger.cc b/runtime/vm/debugger.cc
index 779992b..d730e00 100644
--- a/runtime/vm/debugger.cc
+++ b/runtime/vm/debugger.cc
@@ -1895,7 +1895,12 @@
ClearCachedStackTraces();
}
-// Helper to refine the resolved token pos.
+// Helper that refines the resolved token pos.
+//
+// If |requested_column| is specified, then |exact_token_pos| must be the exact
+// position where the user requested a column breakpoint to be set. If
+// |requested_column| is |-1|, |exact_token_pos| must be
+// |TokenPosition::kNoSource|.
static void RefineBreakpointPos(const Script& script,
TokenPosition pos,
TokenPosition next_closest_token_position,
@@ -1907,6 +1912,10 @@
intptr_t* best_column,
intptr_t* best_line,
TokenPosition* best_token_pos) {
+ ASSERT(requested_column == -1 &&
+ exact_token_pos == TokenPosition::kNoSource ||
+ requested_column > -1 && exact_token_pos != TokenPosition::kNoSource);
+
intptr_t token_start_column = -1;
intptr_t token_line = -1;
if (requested_column >= 0) {
@@ -1914,14 +1923,19 @@
TokenPosition end_of_line_pos = TokenPosition::kNoSource;
script.GetTokenLocation(pos, &token_line, &token_start_column);
script.TokenRangeAtLine(token_line, &ignored, &end_of_line_pos);
- TokenPosition token_end_pos =
- TokenPosition::Min(next_closest_token_position, end_of_line_pos);
+
+ TokenPosition token_end_pos = TokenPosition::Min(
+ TokenPosition::Deserialize(next_closest_token_position.Pos() - 1),
+ end_of_line_pos);
if ((token_end_pos.IsReal() && exact_token_pos.IsReal() &&
(token_end_pos < exact_token_pos)) ||
(token_start_column > *best_column)) {
- // Prefer the token with the lowest column number compatible
- // with the requested column.
+ // We prefer the token with the lowest column number compatible with the
+ // requested column. The current token under consideration either ends
+ // before the requested column, and is thus incompatible with it, or
+ // has a higher column number than the best token we've found so far, so
+ // we reject the current token under consideration.
return;
}
}
@@ -1946,6 +1960,11 @@
// represents one line of the program text, but can represent a larger
// range on recursive calls.
//
+// If |requested_column| is specified, then |exact_token_pos| must be the exact
+// position where the user requested a column breakpoint to be set. If
+// |requested_column| is |-1|, |exact_token_pos| must be
+// |TokenPosition::kNoSource|.
+//
// The best fit is found in two passes.
//
// The first pass finds a candidate token which:
@@ -2001,6 +2020,9 @@
intptr_t requested_column,
TokenPosition exact_token_pos) {
ASSERT(!func.HasOptimizedCode());
+ ASSERT(requested_column == -1 &&
+ exact_token_pos == TokenPosition::kNoSource ||
+ requested_column > -1 && exact_token_pos != TokenPosition::kNoSource);
requested_token_pos =
TokenPosition::Max(requested_token_pos, func.token_pos());
@@ -2432,6 +2454,10 @@
return false;
}
+// If |requested_column| is specified, then |exact_token_pos| must be the exact
+// position where the user requested a column breakpoint to be set. If
+// |requested_column| is |-1|, |exact_token_pos| must be
+// |TokenPosition::kNoSource|.
BreakpointLocation* Debugger::SetCodeBreakpoints(
const GrowableHandlePtrArray<const Script>& scripts,
TokenPosition token_pos,
@@ -2440,6 +2466,10 @@
intptr_t requested_column,
TokenPosition exact_token_pos,
const GrowableObjectArray& functions) {
+ ASSERT(requested_column == -1 &&
+ exact_token_pos == TokenPosition::kNoSource ||
+ requested_column > -1 && exact_token_pos != TokenPosition::kNoSource);
+
Function& function = Function::Handle();
function ^= functions.At(0);
TokenPosition breakpoint_pos = ResolveBreakpointPos(
@@ -2526,14 +2556,6 @@
}
}
- TokenPosition exact_token_pos = token_pos;
-#if !defined(DART_PRECOMPILED_RUNTIME)
- if (token_pos != last_token_pos && requested_column >= 0) {
- exact_token_pos =
- FindExactTokenPosition(script, token_pos, requested_column);
- }
-#endif // !defined(DART_PRECOMPILED_RUNTIME)
-
if (!func.IsNull()) {
// There may be more than one function object for a given function
// in source code. There may be implicit closure functions, and
@@ -2550,6 +2572,13 @@
// have already been compiled. We can resolve the breakpoint now.
// If requested_column is larger than zero, [token_pos, last_token_pos]
// governs one single line of code.
+ TokenPosition exact_token_pos = TokenPosition::kNoSource;
+#if !defined(DART_PRECOMPILED_RUNTIME)
+ if (token_pos != last_token_pos && requested_column >= 0) {
+ exact_token_pos =
+ FindExactTokenPosition(script, token_pos, requested_column);
+ }
+#endif // !defined(DART_PRECOMPILED_RUNTIME)
DeoptimizeWorld();
BreakpointLocation* loc =
SetCodeBreakpoints(scripts, token_pos, last_token_pos, requested_line,
@@ -2565,7 +2594,7 @@
if (FLAG_verbose_debug) {
intptr_t line_number = -1;
intptr_t column_number = -1;
- script.GetTokenLocation(exact_token_pos, &line_number, &column_number);
+ script.GetTokenLocation(token_pos, &line_number, &column_number);
if (func.IsNull()) {
OS::PrintErr(
"Registering pending breakpoint for "
@@ -2580,11 +2609,10 @@
}
const String& script_url = String::Handle(script.url());
BreakpointLocation* loc = GetBreakpointLocation(
- script_url, exact_token_pos, requested_line, requested_column);
+ script_url, token_pos, requested_line, requested_column);
if (loc == nullptr) {
- loc =
- new BreakpointLocation(this, scripts, exact_token_pos, exact_token_pos,
- requested_line, requested_column);
+ loc = new BreakpointLocation(this, scripts, token_pos, last_token_pos,
+ requested_line, requested_column);
RegisterBreakpointLocation(loc);
}
return loc;
@@ -2890,7 +2918,16 @@
location->debugger()->isolate());
// Ensure the location is resolved for the original function.
- location->EnsureIsResolved(function, location->token_pos());
+ TokenPosition exact_token_pos = TokenPosition::kNoSource;
+#if !defined(DART_PRECOMPILED_RUNTIME)
+ if (location->token_pos() != location->end_token_pos() &&
+ location->requested_column_number() >= 0) {
+ exact_token_pos = FindExactTokenPosition(
+ Script::Handle(location->script()), location->token_pos(),
+ location->requested_column_number());
+ }
+#endif // !defined(DART_PRECOMPILED_RUNTIME)
+ location->EnsureIsResolved(function, exact_token_pos);
if (FLAG_verbose_debug) {
Breakpoint* bpt = location->breakpoints();
while (bpt != nullptr) {
