Teach debugger to ignore breakpoints

Teach debugger to ignore breakpoints

Reentrant debugger calls to fetch values may execute dart code as a
side effect. Teach the debugger to ignore any breakpoints it might
hit while doing so.
Committed: https://code.google.com/p/dart/source/detail?r=5274

[hausner, 2012-03-09 23:24:13]

The actual functional change is tiny.

[siva, 2012-03-10 00:11:40]

LGTM.

There is also the case of ignoring breakpoints when stepping over a recursive
function call that needs to be implemented.

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

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger.cc#newcode687
runtime/vm/debugger.cc:687: ignore_breakpoints_ = saved_ignore_flag;
Why save and restore the flag instead of just setting it to
false here?
Do you expect recursive calls to this? I thought the goal was to avoid recursive
calls to this.

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
File runtime/vm/debugger_api_impl_test.cc (right):

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
runtime/vm/debugger_api_impl_test.cc:379: PrintStackTrace(trace);
This ensures that the ignore breakpoint feature is tested when accessing
instance fields, what about also testing it when accessing static fields.

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
runtime/vm/debugger_api_impl_test.cc:383: Dart_SetStepInto();
Why is it necessary to single step after this?

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
runtime/vm/debugger_api_impl_test.cc:412: breakpoint_hit_counter = 0;
What is the significance of breakpoint_hit_counter in this test as you are not
checking the value with any EXPECT clause.

Maybe there should be a EXPECT_EQ(1, breakpoint_hit_counter);

[hausner, 2012-03-10 00:41:56]

Thank you.

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

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger.cc#newcode687
runtime/vm/debugger.cc:687: ignore_breakpoints_ = saved_ignore_flag;
On 2012/03/10 00:11:40, asiva wrote:
> Why save and restore the flag instead of just setting it to
> false here?
> Do you expect recursive calls to this? I thought the goal was to avoid
recursive
> calls to this.

I thought it's more future-proof to allow for recursion. On the other hand,
there is probably a lot more state that would need to be saved and restored to
make the debugger truly reentrant.

For example, we might want to call the debugger when an exception is thrown,
which could happen during the evaluation of a field.

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
File runtime/vm/debugger_api_impl_test.cc (right):

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
runtime/vm/debugger_api_impl_test.cc:379: PrintStackTrace(trace);
On 2012/03/10 00:11:40, asiva wrote:
> This ensures that the ignore breakpoint feature is tested when accessing
> instance fields, what about also testing it when accessing static fields.
Added a static field to the sample and to the stack dump.

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
runtime/vm/debugger_api_impl_test.cc:383: Dart_SetStepInto();
On 2012/03/10 00:11:40, asiva wrote:
> Why is it necessary to single step after this?
It's just the easiest way to make sure we get into every function.

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
runtime/vm/debugger_api_impl_test.cc:412: breakpoint_hit_counter = 0;
It's just boilerplate that I copied from other test cases. I could eliminate it,
but then, there's no harm in leaving it.

On 2012/03/10 00:11:40, asiva wrote:
> What is the significance of breakpoint_hit_counter in this test as you are not
> checking the value with any EXPECT clause.
> 
> Maybe there should be a EXPECT_EQ(1, breakpoint_hit_counter);

[siva, 2012-03-12 22:19:14]

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

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger.cc#newcode687
runtime/vm/debugger.cc:687: ignore_breakpoints_ = saved_ignore_flag;
Sounds like premature optimization :-)

On 2012/03/10 00:41:56, hausner wrote:
> On 2012/03/10 00:11:40, asiva wrote:
> > Why save and restore the flag instead of just setting it to
> > false here?
> > Do you expect recursive calls to this? I thought the goal was to avoid
> recursive
> > calls to this.
> 
> I thought it's more future-proof to allow for recursion. On the other hand,
> there is probably a lot more state that would need to be saved and restored to
> make the debugger truly reentrant.
> 
> For example, we might want to call the debugger when an exception is thrown,
> which could happen during the evaluation of a field.
>

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
File runtime/vm/debugger_api_impl_test.cc (right):

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
runtime/vm/debugger_api_impl_test.cc:383: Dart_SetStepInto();
Not sure I understand what you mean by get into every function.

On 2012/03/10 00:41:56, hausner wrote:
> On 2012/03/10 00:11:40, asiva wrote:
> > Why is it necessary to single step after this?
> It's just the easiest way to make sure we get into every function.

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
runtime/vm/debugger_api_impl_test.cc:412: breakpoint_hit_counter = 0;
If you are retaining it then why not add the check
EXPECT_EQ(1, breakpoint_hit_counter);

On 2012/03/10 00:41:56, hausner wrote:
> It's just boilerplate that I copied from other test cases. I could eliminate
it,
> but then, there's no harm in leaving it.
> 
> On 2012/03/10 00:11:40, asiva wrote:
> > What is the significance of breakpoint_hit_counter in this test as you are
not
> > checking the value with any EXPECT clause.
> > 
> > Maybe there should be a EXPECT_EQ(1, breakpoint_hit_counter);
>

[hausner, 2012-03-12 22:36:44]

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
File runtime/vm/debugger_api_impl_test.cc (right):

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
runtime/vm/debugger_api_impl_test.cc:383: Dart_SetStepInto();
The relevant case that we test here happens when we step into the getter
function get:i of class B. The easiest way to get a breakpoint inside the getter
is to stop at main, and then step into every function call.

On 2012/03/12 22:19:14, asiva wrote:
> Not sure I understand what you mean by get into every function.
> 
> On 2012/03/10 00:41:56, hausner wrote:
> > On 2012/03/10 00:11:40, asiva wrote:
> > > Why is it necessary to single step after this?
> > It's just the easiest way to make sure we get into every function.
>

http://codereview.chromium.org/9668036/diff/1/runtime/vm/debugger_api_impl_te...
runtime/vm/debugger_api_impl_test.cc:412: breakpoint_hit_counter = 0;
This test hits many breakpoints (including the internal ones that are the result
of stepping):

Running test: Debug_IgnoreBP
>>> Breakpoint nr. 0 in function main (dart:test-lib:9) <<<
    function main
      local var x = null
>>> Breakpoint nr. 1 in function B.B. (dart:test-lib:1) <<<
    function B.B.
      local var this = object of type 'Library:'dart:test-lib' Class: B'
        field i = 100
        field d = 3.14
        field s = "Dr Seuss"
        static field z = 0
    function main
      local var x = null
>>> Breakpoint nr. 2 in function Object.Object. (bootstrap:668) <<<
    function Object.Object.
      local var this = object of type 'Library:'dart:test-lib' Class: B'
        field i = 100
        field d = 3.14
        field s = "Dr Seuss"
        static field z = 0
    function B.B.
      local var this = object of type 'Library:'dart:test-lib' Class: B'
        field i = 100
        field d = 3.14
        field s = "Dr Seuss"
        static field z = 0
    function main
      local var x = null
>>> Breakpoint nr. 3 in function B.B. (dart:test-lib:1) <<<
    function B.B.
      local var this = object of type 'Library:'dart:test-lib' Class: B'
        field i = 100
        field d = 3.14
        field s = "Dr Seuss"
        static field z = 0
    function main
      local var x = null
>>> Breakpoint nr. 4 in function main (dart:test-lib:10) <<<
    function main
      local var x = object of type 'Library:'dart:test-lib' Class: B'
        field i = 100
        field d = 3.14
        field s = "Dr Seuss"
        static field z = 0
>>> Breakpoint nr. 5 in function B.get:i (dart:test-lib:3) <<<
    function B.get:i
      local var this = object of type 'Library:'dart:test-lib' Class: B'
        field i = 100
        field d = 3.14
        field s = "Dr Seuss"
        static field z = 0
    function main
      local var x = object of type 'Library:'dart:test-lib' Class: B'
        field i = 100
        field d = 3.14
        field s = "Dr Seuss"
        static field z = 0
>>> Breakpoint nr. 6 in function main (dart:test-lib:10) <<<
    function main
      local var x = object of type 'Library:'dart:test-lib' Class: B'
        field i = 100
        field d = 3.14
        field s = "Dr Seuss"
        static field z = 0
>>> Breakpoint nr. 7 in function IntegerImplementation.+ (bootstrap_impl:1441)
<<<
    function IntegerImplementation.+
      local var this = 100
      local var other = 1
    function main
      local var x = object of type 'Library:'dart:test-lib' Class: B'
        field i = 100
        field d = 3.14
        field s = "Dr Seuss"
        static field z = 0
>>> Breakpoint nr. 8 in function IntegerImplementation.addFromInteger
(bootstrap_impl:1483) <<<
    function IntegerImplementation.addFromInteger
      local var this = 1
      local var other = 100
    function IntegerImplementation.+
      local var this = 100
      local var other = 1
    function main
      local var x = object of type 'Library:'dart:test-lib' Class: B'
        field i = 100
        field d = 3.14
        field s = "Dr Seuss"
        static field z = 0
>>> Breakpoint nr. 9 in function IntegerImplementation.+ (bootstrap_impl:1441)
<<<
    function IntegerImplementation.+
      local var this = 100
      local var other = 1
    function main
      local var x = object of type 'Library:'dart:test-lib' Class: B'
        field i = 100
        field d = 3.14
        field s = "Dr Seuss"
        static field z = 0
>>> Breakpoint nr. 10 in function main (dart:test-lib:10) <<<
    function main
      local var x = object of type 'Library:'dart:test-lib' Class: B'
        field i = 100
        field d = 3.14
        field s = "Dr Seuss"
        static field z = 0
Done: Debug_IgnoreBP


On 2012/03/12 22:19:14, asiva wrote:
> If you are retaining it then why not add the check
> EXPECT_EQ(1, breakpoint_hit_counter);
> 
> On 2012/03/10 00:41:56, hausner wrote:
> > It's just boilerplate that I copied from other test cases. I could eliminate
> it,
> > but then, there's no harm in leaving it.
> > 
> > On 2012/03/10 00:11:40, asiva wrote:
> > > What is the significance of breakpoint_hit_counter in this test as you are
> not
> > > checking the value with any EXPECT clause.
> > > 
> > > Maybe there should be a EXPECT_EQ(1, breakpoint_hit_counter);
> > 
>