StepOver, StepInto, StepOut

StepOver, StepInto, StepOut

Implement single stepping in the debugger.
- Add PC descriptors for function return
- functions to set temporary breakpoints on all
  locations in a function.
- patching/restoring of function return code pattern
- determine call target of instance calls

[hausner, 2012-02-24 19:00:04]

Sorry, it's a bigger review. I'll buy you coffee this afternoon.

[hausner, 2012-02-24 19:06:42]

+Anton, +Pavel

[regis, 2012-02-24 23:32:37]

LGTM

http://codereview.chromium.org/9460012/diff/4002/runtime/include/dart_debugge...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/9460012/diff/4002/runtime/include/dart_debugge...
runtime/include/dart_debugger_api.h:114: * break after at the beginning of the
next function call.
What if the next statement is not a function call?

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/code_generator_x6...
File runtime/vm/code_generator_x64.cc (right):

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/code_generator_x6...
runtime/vm/code_generator_x64.cc:800: __ nop(1);
Why not __ nop(8)?
Disassembly would be more compact.

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

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:298: CodePatcher::PatchStaticCallAt(pc_,
pc_ on next line as above?

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:727: InstrumentForStepping(callee);
Setting a breakpoint at the entry of the callee should be sufficient.

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:733: InstrumentForStepping(callee);
ditto

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_ia32.cc
File runtime/vm/debugger_ia32.cc (right):

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_ia32.cc#...
runtime/vm/debugger_ia32.cc:24: *reinterpret_cast<uword*>(var_address));
strange indentation

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_ia32.cc#...
runtime/vm/debugger_ia32.cc:29: intptr_t num_actual_args) {
strange indentation

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_ia32.cc#...
runtime/vm/debugger_ia32.cc:34: *reinterpret_cast<uword*>(receiver_addr));
ditto

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_x64.cc
File runtime/vm/debugger_x64.cc (right):

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_x64.cc#n...
runtime/vm/debugger_x64.cc:27: *reinterpret_cast<uword*>(receiver_addr));
indentation

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_x64.cc#n...
runtime/vm/debugger_x64.cc:37: // Next 8 bytes are nop instructions
If you use nop(8), this would have to change. Since the nop's are not executed,
I guess it is fine to leave them as is.

[hausner, 2012-02-27 22:03:57]

I have trouble with the svn client. I committed this changelist in a new set:

https://code.google.com/p/dart/source/detail?r=4639

http://codereview.chromium.org/9460012/diff/4002/runtime/include/dart_debugge...
File runtime/include/dart_debugger_api.h (right):

http://codereview.chromium.org/9460012/diff/4002/runtime/include/dart_debugge...
runtime/include/dart_debugger_api.h:114: * break after at the beginning of the
next function call.
On 2012/02/24 23:32:37, regis wrote:
> What if the next statement is not a function call?

You mean a function call at the dart level? Currently I still step into the
code. Everything in Dart is a function, even the operators like +, * etc.
Eventually we probably want to be smart and not step into built-in functions
like integer add, list add etc.

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/code_generator_x6...
File runtime/vm/code_generator_x64.cc (right):

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/code_generator_x6...
runtime/vm/code_generator_x64.cc:800: __ nop(1);
On 2012/02/24 23:32:37, regis wrote:
> Why not __ nop(8)?
> Disassembly would be more compact.
It's simpler to recognize the code pattern when I patch the code to install a
breakpoint.

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

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:298: CodePatcher::PatchStaticCallAt(pc_,
On 2012/02/24 23:32:37, regis wrote:
> pc_ on next line as above?

Done.

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:727: InstrumentForStepping(callee);
On 2012/02/24 23:32:37, regis wrote:
> Setting a breakpoint at the entry of the callee should be sufficient.
True. Added a todo to optimize later.

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:733: InstrumentForStepping(callee);
On 2012/02/24 23:32:37, regis wrote:
> ditto
ditto.

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_ia32.cc
File runtime/vm/debugger_ia32.cc (right):

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_ia32.cc#...
runtime/vm/debugger_ia32.cc:24: *reinterpret_cast<uword*>(var_address));
On 2012/02/24 23:32:37, regis wrote:
> strange indentation

Done.

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_ia32.cc#...
runtime/vm/debugger_ia32.cc:29: intptr_t num_actual_args) {
On 2012/02/24 23:32:37, regis wrote:
> strange indentation

4 character indent from the function name is strange? Moved it a bit more to the
right.

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_ia32.cc#...
runtime/vm/debugger_ia32.cc:34: *reinterpret_cast<uword*>(receiver_addr));
On 2012/02/24 23:32:37, regis wrote:
> ditto

Done.

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_x64.cc
File runtime/vm/debugger_x64.cc (right):

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_x64.cc#n...
runtime/vm/debugger_x64.cc:27: *reinterpret_cast<uword*>(receiver_addr));
On 2012/02/24 23:32:37, regis wrote:
> indentation

Done.

http://codereview.chromium.org/9460012/diff/4002/runtime/vm/debugger_x64.cc#n...
runtime/vm/debugger_x64.cc:37: // Next 8 bytes are nop instructions
On 2012/02/24 23:32:37, regis wrote:
> If you use nop(8), this would have to change. Since the nop's are not
executed,
> I guess it is fine to leave them as is.
Thanks.

[Anton Muhin, 2012-04-03 20:55:19]

should this issue get closed?

[hausner, 2012-04-03 21:08:28]

Yes, closed.