Very first steps for a debugger

Very first steps for a debugger

- Introduce Debugger class.
- Each isolate has a debugger object.
- Introduce stubs that can be put in place of static or dynamic
  Dart function calls.
- Very rudimentary command line option to place a breakpoint
  at the beginning of a function.
Committed: https://code.google.com/p/dart/source/detail?r=1928

[srdjan, 2011-11-29 22:26:00]

DBC

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/code_generator.cc...
runtime/vm/code_generator.cc:479: printf(">>> Breakpoint at 0x%08x\n",
frame->pc());
Why printf instead of OS::Print ? (also below)

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/code_patcher.h
File runtime/vm/code_patcher.h (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/code_patcher.h#ne...
runtime/vm/code_patcher.h:25: // Patch static or instance call to the new
target.
I would use two comments instead of one.

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

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:26: Breakpoint(const Function& func, int pc_desc_index,
uword pc)
intptr_t pc_dec_index

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:43: private:
DISALLOW_....

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:45: int pc_desc_index_;
intptr_t

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:60:
Library::Handle(isolate->object_store()->root_library());
4 spaces indent.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:100: function_name.ToCString(), class_name.ToCString());
OS::Print

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:112: printf("patching dynamic call at %p\n", desc.PC(i));
ditto

Maybe add a tracing flag and hide the printing with it?

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:121: AddBreakpoint(new Breakpoint(func, i, desc.PC(i)));
Who deletes breakpoints? Do we need a ~Debugger() to delete them?

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.h#newcode21
runtime/vm/debugger.h:21: void SetBreakpoint(const String& class_name, const
String& function_name);
Should this be SetBreakpointAtEntry?

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.h#newcode25
runtime/vm/debugger.h:25: Breakpoint* GetBreakpoint(uword breakpoint_address);
Maybe comment that it returns NULL if breakpoint is not found.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.h#newcode27
runtime/vm/debugger.h:27: private:
DISALLOW_.....

[siva, 2011-11-30 00:00:47]

LGTM as the first step, most of my comments can be addressed as this evolves.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/code_generator.h
File runtime/vm/code_generator.h (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/code_generator.h#...
runtime/vm/code_generator.h:30: DECLARE_RUNTIME_ENTRY(Breakpoint);
Why not call it BreakpointHandler because it is essentially takes care of
handling stuff after a breakpoint is hit.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1567: isolate->debugger()->Initialize(isolate);
Why do we have to wait until InvokeStatic is called to initialize the debugger,
why not do it as part of isolate initialization.

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

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:21: DEFINE_FLAG(charp, bpt, NULL, "Debug breakpoint at
<func>");
I hope all these flags are just temporary stuff because the debug API should
drive this functionality.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:46: uword pc_;
Don't you also need to store the original contents of the patched location so
that you can restore it when the breakpoint is deleted.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:105: }
If the function already has code and it is running optimized code shouldn't we
deoptimize it here before proceeding.

Also once breakpoints are set we need a mechanism to disable the optimizing
compiler for this function until all breakpoints are removed.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:115: AddBreakpoint(new Breakpoint(func, i, desc.PC(i)));
you probably need a return here too, otherwise there will be multiple
breakpoints set.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:123: }
Why don't we want to set breakpoints on locations which have calls to the
runtime or stubs?

In the current scenario we won't be able to set breakpoints on leaf dart
functions.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:153: String& fname = String::Handle();
We would need a library name too over here, may not always be the root library.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.h#newcode21
runtime/vm/debugger.h:21: void SetBreakpoint(const String& class_name, const
String& function_name);
I am presuming this will evolve into two functions
SetBreakpointInFunction(library_name, class_name, function_name);
SetBreakpointAtLine(library_name, class_name, token_index);

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:300: 
// Visit objects in the debugger.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/stub_code.h
File runtime/vm/stub_code.h (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/stub_code.h#newco...
runtime/vm/stub_code.h:48: V(BreakpointStatic)                                  
                       \
Does breakpoint static stub need to be generated on a per isolate basis? Can it
not be in the VM Isolate?

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/stub_code_ia32.cc
File runtime/vm/stub_code_ia32.cc (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/stub_code_ia32.cc...
runtime/vm/stub_code_ia32.cc:1549: //  ECX: Function object
Missing period after comment in all the comment lines in these two stubs.

[hausner, 2011-11-30 01:17:04]

Thanks for the reviews and suggestions. This is clearly work in progress.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/code_generator.cc...
runtime/vm/code_generator.cc:479: printf(">>> Breakpoint at 0x%08x\n",
frame->pc());
On 2011/11/29 22:26:00, srdjan wrote:
> Why printf instead of OS::Print ? (also below)

All the printf calls will go away as the code becomes more useful. I just didn't
want to remove them for the checkin. Changed to OS::Print.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/code_generator.h
File runtime/vm/code_generator.h (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/code_generator.h#...
runtime/vm/code_generator.h:30: DECLARE_RUNTIME_ENTRY(Breakpoint);
On 2011/11/30 00:00:47, asiva wrote:
> Why not call it BreakpointHandler because it is essentially takes care of
> handling stuff after a breakpoint is hit.

Done.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/code_patcher.h
File runtime/vm/code_patcher.h (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/code_patcher.h#ne...
runtime/vm/code_patcher.h:25: // Patch static or instance call to the new
target.
On 2011/11/29 22:26:00, srdjan wrote:
> I would use two comments instead of one.

Done.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:1567: isolate->debugger()->Initialize(isolate);
On 2011/11/30 00:00:47, asiva wrote:
> Why do we have to wait until InvokeStatic is called to initialize the
debugger,
> why not do it as part of isolate initialization.

Because I need to wait until all the code is loaded in the isolate before I can
set a breakpoint. I could initialize the debugger sooner, but processing the
command line option to set the breakpoint would still have to be done at this
point.

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

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:21: DEFINE_FLAG(charp, bpt, NULL, "Debug breakpoint at
<func>");
On 2011/11/30 00:00:47, asiva wrote:
> I hope all these flags are just temporary stuff because the debug API should
> drive this functionality.
Yes, only temporary.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:26: Breakpoint(const Function& func, int pc_desc_index,
uword pc)
On 2011/11/29 22:26:00, srdjan wrote:
> intptr_t pc_dec_index

Done.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:43: private:
On 2011/11/29 22:26:00, srdjan wrote:
> DISALLOW_....

Done.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:45: int pc_desc_index_;
On 2011/11/29 22:26:00, srdjan wrote:
> intptr_t

Done, but over the long run I would change this to a smaller integer. We don't
want to burn 4 or 8 bytes for a value that rarely is higher than 10 or 100.

(How do you create a memory hog? One byte at a time... :)

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:46: uword pc_;
On 2011/11/30 00:00:47, asiva wrote:
> Don't you also need to store the original contents of the patched location so
> that you can restore it when the breakpoint is deleted.

At this point I don't need to. To remove a BP I just have to restore the call to
the static or dynamic call stub. It may well make sense to save the overwritten
call target in the BP though. I'll do that when the need comes.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:60:
Library::Handle(isolate->object_store()->root_library());
On 2011/11/29 22:26:00, srdjan wrote:
> 4 spaces indent.

Done.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:100: function_name.ToCString(), class_name.ToCString());
On 2011/11/29 22:26:00, srdjan wrote:
> OS::Print

Done.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:105: }
On 2011/11/30 00:00:47, asiva wrote:
> If the function already has code and it is running optimized code shouldn't we
> deoptimize it here before proceeding.
> 
> Also once breakpoints are set we need a mechanism to disable the optimizing
> compiler for this function until all breakpoints are removed.

Yes. For now I just assume the optimizing compiler is never invoked. I should
probably disable optimization implicitly when the --debug flag is present.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:112: printf("patching dynamic call at %p\n", desc.PC(i));
On 2011/11/29 22:26:00, srdjan wrote:
> ditto
> 
> Maybe add a tracing flag and hide the printing with it?

As mentioned above, all these prints will go away once the code matures. Is that
good enough?

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:115: AddBreakpoint(new Breakpoint(func, i, desc.PC(i)));
On 2011/11/30 00:00:47, asiva wrote:
> you probably need a return here too, otherwise there will be multiple
> breakpoints set.

Good catch.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:121: AddBreakpoint(new Breakpoint(func, i, desc.PC(i)));
On 2011/11/29 22:26:00, srdjan wrote:
> Who deletes breakpoints? Do we need a ~Debugger() to delete them?

Nobody yet. I think it's ok to leave this for future checkins.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:123: }
On 2011/11/30 00:00:47, asiva wrote:
> Why don't we want to set breakpoints on locations which have calls to the
> runtime or stubs?
> 
> In the current scenario we won't be able to set breakpoints on leaf dart
> functions.

I had to leave some functionality for version 2 :)

Agree, your suggestion makes sense.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.cc#newco...
runtime/vm/debugger.cc:153: String& fname = String::Handle();
On 2011/11/30 00:00:47, asiva wrote:
> We would need a library name too over here, may not always be the root
library.

Very true. To keep things simple for the beginning, I just assume the root
library implicitly.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.h#newcode21
runtime/vm/debugger.h:21: void SetBreakpoint(const String& class_name, const
String& function_name);
On 2011/11/29 22:26:00, srdjan wrote:
> Should this be SetBreakpointAtEntry?

Done.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.h#newcode21
runtime/vm/debugger.h:21: void SetBreakpoint(const String& class_name, const
String& function_name);
On 2011/11/30 00:00:47, asiva wrote:
> I am presuming this will evolve into two functions
> SetBreakpointInFunction(library_name, class_name, function_name);
> SetBreakpointAtLine(library_name, class_name, token_index);

Yes, there will eventually be several methods, eg. setting a breakpoint at a
given script and line number. This is just one simple case to get something
going,

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.h#newcode25
runtime/vm/debugger.h:25: Breakpoint* GetBreakpoint(uword breakpoint_address);
On 2011/11/29 22:26:00, srdjan wrote:
> Maybe comment that it returns NULL if breakpoint is not found.

Done.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/debugger.h#newcode27
runtime/vm/debugger.h:27: private:
On 2011/11/29 22:26:00, srdjan wrote:
> DISALLOW_.....

Done.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/isolate.cc#newcod...
runtime/vm/isolate.cc:300: 
On 2011/11/30 00:00:47, asiva wrote:
> // Visit objects in the debugger.

Done.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/stub_code.h
File runtime/vm/stub_code.h (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/stub_code.h#newco...
runtime/vm/stub_code.h:48: V(BreakpointStatic)                                  
                       \
On 2011/11/30 00:00:47, asiva wrote:
> Does breakpoint static stub need to be generated on a per isolate basis? Can
it
> not be in the VM Isolate?

Done.

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/stub_code_ia32.cc
File runtime/vm/stub_code_ia32.cc (right):

http://codereview.chromium.org/8687037/diff/6001/runtime/vm/stub_code_ia32.cc...
runtime/vm/stub_code_ia32.cc:1549: //  ECX: Function object
On 2011/11/30 00:00:47, asiva wrote:
> Missing period after comment in all the comment lines in these two stubs.

Done.