Issue 117133005: Change how debugger handles source breakpoints

Issue 117133005: Change how debugger handles source breakpoints (Closed)

Created:
7 years ago by hausner

Modified:
6 years, 12 months ago

Reviewers:
turnidge

CC:
reviews_dartlang.org, vm-dev_dartlang.org

Base URL:
http://dart.googlecode.com/svn/branches/bleeding_edge/dart/

Visibility:
Public.

More Reviews

Description

Change how debugger handles source breakpoints - Identify a source breakpoint by a script and a token position rather than a function object. - When a new breakpoint is set, iterate over all compiled functions to see whether code breakpoints must be set. This change fixes issues with functions that get compiled several times, for example as implicit closures or a copies of mixin functions or local closures of mixing functions. We did not detect all duplicates of functions. Eliminated recently introduced const modifiers in SourceBreakpoint class. They tend to be in the way because several accessor methods of SourceBreakpoint compute data lazily and modify the object as a side effect. Committed: https://code.google.com/p/dart/source/detail?r=31372

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+444 lines, -246 lines)			Patch
M	runtime/tests/vm/vm.status	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	runtime/vm/debugger.h	View	1 2 3	3 chunks	+22 lines, -15 lines	0 comments	Download
M	runtime/vm/debugger.cc	View	1 2 3	13 chunks	+359 lines, -218 lines	0 comments	Download
M	runtime/vm/debugger_test.cc	View	1 2 3	1 chunk	+2 lines, -2 lines	0 comments	Download
M	runtime/vm/json_stream.h	View	1 2 3	2 chunks	+2 lines, -2 lines	0 comments	Download
M	runtime/vm/json_stream.cc	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	runtime/vm/service_test.cc	View	1 2 3	2 chunks	+2 lines, -2 lines	0 comments	Download
M	tests/standalone/debugger/breakpoint_resolved_test.dart	View	1 2 3	2 chunks	+4 lines, -5 lines	0 comments	Download
M	tests/standalone/debugger/debug_lib.dart	View	1 2 3	2 chunks	+4 lines, -1 line	0 comments	Download
A	tests/standalone/debugger/local_function_test.dart	View	1	1 chunk	+46 lines, -0 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

turnidge

Hi Matthias - I see you just uploaded a new draft. All of my comments ...

7 years ago (2013-12-19 22:31:58 UTC) #3

hausner

PTAL I eliminated the concept of a token range for unresolved source breakpoints. They now ...

7 years ago (2013-12-21 00:08:32 UTC) #4

PTAL

I eliminated the concept of a token range for unresolved source breakpoints.
They now just carry a requested token position, which gets adjusted to an actual
token position when the breakpoint is resolved.

The Debug_StepInto test now fails, but not due to this change. It was passing
for the wrong reason before. Will tackle that as a separate change.

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:66: ASSERT((range_start_ >= 0) && (range_start_ <=
range_end_));
On 2013/12/19 22:31:59, turnidge wrote:
> Should this <= be a <?  It depends whether the range is meant to include the
> limit or not.
> 
> You use a < in RangeOverlaps below... should that be a <= or not?
obsolete

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:82: void SourceBreakpoint::SetResolved(const Function&
func, intptr_t token_pos) {
On 2013/12/19 22:31:59, turnidge wrote:
> What do you think of renaming "SetResolved" to "Resolve"?

I like SetResolved better. The actual resolving (computing the token position of
the safe point) happens in a different function. Here we just update the token
position and mark the breakpoint as resolved. Does this make sense?

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:239: if (sbpt->IsResolved() && (func.raw() ==
sbpt->function())) {
On 2013/12/19 22:31:59, turnidge wrote:
> So this function has no code, has not been compiled, but the breakpoints have
> been resolved?  The breakpoint is both "resolved" and "latent" (from comment)?
> 
> I don't know the states here too well, so just making sure this makes sense. 
I
> would have thought that a resolved breakpoint wouldn't be latent and would
have
> code.
Yes that seems fishy. Changed so it returns true if the breakpoint falls within
the function's token range.

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:1359: void Debugger::FindAllFunctions(const Script&
script,
On 2013/12/19 22:31:59, turnidge wrote:
> I find the name confusing -- it seems to be trying to find *equivalent*
> functions to some other function.  Would it work to change the name to
something
> like FindEquivalentFunctions or FindMatchingFunctions?

Name changed.

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:1362: GrowableObjectArray* function_list) {
On 2013/12/19 22:31:59, turnidge wrote:
> This function belongs lower in the file, nearer to FindBestFit and
> SetBreakpoint.

Done.

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:1481: return false;
On 2013/12/19 22:31:59, turnidge wrote:
> This cl contains a lot of range math that could have minor errors at the
> endpoints, etc.  It looks fine to me but it is a good candidate for unit
> testing.
> 
> If I were testing, I would write some small unit tests of SelectBestFit,
> RangeOverlaps (building block functions) to cover edge cases.  I would then
> write tests to make sure that FindBestFit works and that FindAllFunctions (or
> FindEquivalentFunctions) really finds the full equivalence class of functions.

> Then I would write a few higher level tests of SetBreakpoint and make sure
that
> the token pos gets resolved to what I expected.
> 
> These tests could also help document why you made the heuristic choices that
you
> have made for future coders.
> 
> I realize this will slow you down a bit, sorry.

Range math simplified and test added (not unit test though).

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:1484: 
On 2013/12/19 22:31:59, turnidge wrote:
> Could use a comment saying what it means for one function to be a better fit,
> and why.

I think the comments you suggest are inside the function, no?

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:1487: intptr_t range_end) {
On 2013/12/19 22:31:59, turnidge wrote:
> Out of curiousity, could this function be static?

Yes, but then I could not use the debugger object's isolate_ field to speed up
the handle allocation.

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:1521: SelectBestFit(&best_fit, &function);
On 2013/12/19 22:31:59, turnidge wrote:
> If we were to change SelectBestFit to return the RawFunction* of the best fit,
> the code would look like this:
> 
> best_fit = BestFit(best_fit, function);
> 
> Which would get rid of the & operators and make the code slightly more obvious
> to me.
> 
> Your call.

I wanted to reuse the handles so that SelectBestFit does not have to allocate
any handles...

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:1575: bpt->Enable();
On 2013/12/19 22:31:59, turnidge wrote:
> If we move the call to Enable before we make the code breakpoints, does that
> work too?  The code breakpoints would then be enabled as they are created
> instead of being enabled later.

Yes but the call to Enable() has the side effect of going over all the code
breakpoints anyway. So if we do it here all the function objects that contain
this breakpoint will be instrumented at the same time.

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:1633: target_function.token_pos() + 1);
On 2013/12/19 22:31:59, turnidge wrote:
> If the range end is included in the range, do you need the +1 here?  Would it
> work to use token_pos() as both the start and the end?

obsolete.

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:2063: intptr_t range_end) {
On 2013/12/19 22:31:59, turnidge wrote:
> Could this be static?
Yes but I wanted to used debugger->isolate_ to speed up handle allocation.

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:2070: // in mixin classes whose source code is contained
in a differenct
On 2013/12/19 22:31:59, turnidge wrote:
> typo "differenct"

Done.

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:2105: Function& inner_function =
Function::Handle(isolate_);
On 2013/12/19 22:31:59, turnidge wrote:
> Since the function name doesn't say "containing the range", that should be
said
> in a comment here.

obsolete

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:2156: MakeCodeBreakpointsAt(func, bpt);
On 2013/12/19 22:31:59, turnidge wrote:
> In SetBreakpoint, you call SignalBpResolved *after* you call
> MakeCodeBreakpointsAt.  Here you call SIgnalBpResolved *before* you call
> MakeCodeBreakpointsAt.  Does this matter?  Is there any circumstance where the
> embedder's breakpoint resolved callback might *want* to hit the newly resolved
> breakpoint?

I don't think it matters. The isolate is not paused when this code runs so the
resolved event gets queued before it's sent over the wire. If you think we
should send it after all code breakpoints are made, we have to keep a list of
all BPs that got resolved as a result of this compilation and signal the events
after the list is entirely traversed.

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:2255: if ((bpt->script_ == script.raw()) &&
(bpt->range_start_ == range_start)) {
On 2013/12/19 22:31:59, turnidge wrote:
> SetResolved does not clear the range_start_, does it?  Isn't it possible for
> this function to return a resolved breakpoint?  What do you think of checking
> IsResolved in GetUnresolvedBreakpoint and GetResolvedBreakpoint?

obsolete

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/117133005/diff/1/runtime/vm/debugger.h#newcode29
runtime/vm/debugger.h:29: public:
On 2013/12/19 22:31:59, turnidge wrote:
> Maybe comment whether the range includes end or not.

obsolete

hausner

Committed patchset #4 manually as r31372 (presubmit successful).

6 years, 12 months ago (2013-12-26 20:54:05 UTC) #5

hausner

6 years, 12 months ago (2013-12-26 20:54:53 UTC) #6

Message was sent while issue was closed.

TBR=turnidge

I'll address review comments in a separate checkin if necessary.

Expand Messages | Collapse Messages