From ec30968b54fa33aefa3f1a33ea038e63ec8cfbb3 Mon Sep 17 00:00:00 2001 From: Derek Xu Date: Mon, 27 Nov 2023 18:20:18 +0000 Subject: [PATCH] [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 Reviewed-by: Ben Konyi --- .../test/add_breakpoint_rpc_kernel_test.dart | 4 +- ..._on_function_declared_inside_map_test.dart | 56 ++++++++++++++ .../test/column_breakpoint_test.dart | 30 +++++--- .../add_breakpoint_rpc_kernel_test.dart | 2 +- .../tests/service/column_breakpoint_test.dart | 16 ++-- runtime/vm/debugger.cc | 75 ++++++++++++++----- 6 files changed, 143 insertions(+), 40 deletions(-) create mode 100644 pkg/vm_service/test/break_on_function_declared_inside_map_test.dart 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 1c11d2fe2ec..d9200af2a57 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 @@ final tests = [ 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 00000000000..68c55904b00 --- /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 +// +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 = []; + +const expected = [ + '$shortFile:$LINE_A:23', // on '(' of '()' + '$shortFile:$LINE_B:23', // on '(' of '()' +]; + +final tests = [ + hasPausedAtStart, + setBreakpointAtLineColumn(LINE_A, 24), // on ')' of '()' + setBreakpointAtLine(LINE_B), + resumeProgramRecordingStops(stops, false), + checkRecordedStops(stops, expected), +]; + +Future main([args = const []]) => 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 3ceb6fe5aee..ea1c1879419 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 +// +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 = []; const expected = [ - '$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 = [ 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 96ae4b5783a..67fa8babee3 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 @@ var tests = [ 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 8d78440c256..7f4da75e483 100644 --- a/runtime/observatory/tests/service/column_breakpoint_test.dart +++ b/runtime/observatory/tests/service/column_breakpoint_test.dart @@ -6,12 +6,12 @@ import 'test_helper.dart'; 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 @@ const String breakpointFile = List stops = []; const List 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 = [ 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 779992ba5fc..d730e000fa8 100644 --- a/runtime/vm/debugger.cc +++ b/runtime/vm/debugger.cc @@ -1895,7 +1895,12 @@ void Debugger::PauseException(const Instance& exc) { 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 @@ static void RefineBreakpointPos(const Script& script, 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 @@ static void RefineBreakpointPos(const Script& script, 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 @@ static void RefineBreakpointPos(const Script& script, // 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 @@ static TokenPosition ResolveBreakpointPos(const Function& func, 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 @@ bool Debugger::FindBestFit(const Script& script, 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& scripts, TokenPosition token_pos, @@ -2440,6 +2466,10 @@ BreakpointLocation* Debugger::SetCodeBreakpoints( 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 @@ BreakpointLocation* Debugger::SetBreakpoint( } } - 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 @@ BreakpointLocation* Debugger::SetBreakpoint( // 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 @@ BreakpointLocation* Debugger::SetBreakpoint( 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 @@ BreakpointLocation* Debugger::SetBreakpoint( } 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 @@ void GroupDebugger::NotifyCompilation(const Function& function) { 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) {