Beginnings of a debugger wire protocol

Beginnings of a debugger wire protocol

The debugger wire handler is implemented similarly to the io event handler.
A dedicated thread monitors the debugger port for incoming connection
requests. When a debugger is connected, the VM sends events messages
over the wire and handles debugger requests.

To start the VM with a debugger connection, use the option
--debug:<portnumber>. The VM pauses at the beginning of main()
and waits for a debugger to connect.

Subsequent changes will implement debugger commands one by one.
With this change, the VM only understands "resume" commands. 
Committed: https://code.google.com/p/dart/source/detail?r=7330

[hausner, 2012-05-03 16:20:51]

There are some printf statements in the new code that I'd like to keep while I'm
still working on this code.

[siva, 2012-05-03 22:52:08]

LGTM with one comment regarding the busy while loop in the breakpoint handler.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection.cc
File runtime/bin/dbg_connection.cc (right):

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:49: bool connection_is_alive_;
The DISALLOW stuff ....

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:109: int bytes_read = Socket::Read(fd_, buf_ +
data_length_, max_read);
Socket:;Read could also return -1 on error conditions and that needs to be dealt
(maybe cleanup all the debugging stuff) and continue.
Maybe add a TODO here to deal with errors.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:110: if (bytes_read == 0) {
Socket::Read seems to have some weird code in there to return 0 on non blocking
sockets, I guess since you set the socket blocking you are ok but I did find
that weird.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:133: ASSERT(data_length_ < buf_length_);
This assert should be inside the loop where data_length_ is being incremented
and then buf_ accessed with that new length.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:148: FDUtils::WriteToBlocking(debugger_fd_,
msg.buf(), msg.length());
Need to check for return value from WriteToBlocking it could be -1.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:221: }
I am wondering if this loop instead of sitting here sleeping and waiting for
IsConnected to become true should instead wait for OOB messages here. The
listener thread then posts an OOB message when a debugger connection is made.

If you want to keep things simple initially then maybe this thread itself could
do the accept stuff waiting here for the connection to be established.
Especially since you are not dealing with the debugger interrupting a running
process I don't see the point of starting that other thread which will do a poll
and accept the connection. This thread could sit here on a blocking accept.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:243: }
What about the debugger state (breakpoints set etc.) Should they be retained
with a NOP breakpoint handler so that if a debugger subsequently attaches back
they still have the old state or the easier approach is to just delete all the
breakpoints etc. and clean up any other debugging state.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:251: listener_fd_ =
ServerSocket::CreateBindListen("127.0.0.1", port_number, 1);
instead of hard coding the ip address why not accept it as a command line
parameter similar to port number, (would be useful if a machine had more than
one nic and people wanted to use the other one).

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection.h
File runtime/bin/dbg_connection.h (right):

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.h:29: class DebuggerConnectionHandler {
extends AllStatic.

I guess we do not have AllStatic in the bin directory we could add one or you
could add

  DISALLOW_ALLOCATION();
  DISALLOW_IMPLICIT_CONSTRUCTORS(DebuggerConnectionHandler);

to this class.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/fdutils_linux.cc
File runtime/bin/fdutils_linux.cc (right):

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/fdutils_linux....
runtime/bin/fdutils_linux.cc:41: return true;
The code in SetNonBlocking and SetBlocking can be factored into a single helper
function:
bool SetBlockingState(intptr_t fd, intptr_t state);
SetBlocking would be SetBlockingState(fd, ~O_NONBLOCK);
SetNonBlocking would be SetBlockingState(fd, O_NONBLOCK);

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/fdutils_macos.cc
File runtime/bin/fdutils_macos.cc (right):

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/fdutils_macos....
runtime/bin/fdutils_macos.cc:41: return true;
Ditto comment regarding re factoring this.

I wonder why we are missing fdutils_win.cc

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/main.cc
File runtime/bin/main.cc (right):

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/main.cc#newcod...
runtime/bin/main.cc:107: "Use --debug[:<port number>]\n");
see comment regarding having this option be
--debug=[<address>:]<port_number>

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/main.cc#newcod...
runtime/bin/main.cc:110: breakpoint_at = "main";
Maybe later on this should also be a command line option i.e people may chose to
have the VM wait on main for a debugger to attach or just run and allow for a
debugger to attach later and use the OOB mechanism to interrupt the running
program.

[hausner, 2012-05-03 23:52:00]

Thank you. I fixed a few things and added TODOs for others.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection.cc
File runtime/bin/dbg_connection.cc (right):

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:49: bool connection_is_alive_;
On 2012/05/03 22:52:08, asiva wrote:
> The DISALLOW stuff ....

Done.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:109: int bytes_read = Socket::Read(fd_, buf_ +
data_length_, max_read);
On 2012/05/03 22:52:08, asiva wrote:
> Socket:;Read could also return -1 on error conditions and that needs to be
dealt
> (maybe cleanup all the debugging stuff) and continue.
> Maybe add a TODO here to deal with errors.

Done.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:110: if (bytes_read == 0) {
Yes I think on non-blocking sockets they treat the error "read would block" into
a successful read that has not read any data.

On 2012/05/03 22:52:08, asiva wrote:
> Socket::Read seems to have some weird code in there to return 0 on non
blocking
> sockets, I guess since you set the socket blocking you are ok but I did find
> that weird.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:133: ASSERT(data_length_ < buf_length_);
On 2012/05/03 22:52:08, asiva wrote:
> This assert should be inside the loop where data_length_ is being incremented
> and then buf_ accessed with that new length.

True, but rather than slowing things down too much I figured it's good enough to
assert at the end.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:148: FDUtils::WriteToBlocking(debugger_fd_,
msg.buf(), msg.length());
Added a TODO for error checking.

On 2012/05/03 22:52:08, asiva wrote:
> Need to check for return value from WriteToBlocking it could be -1.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:221: }
True, but fairly soon I plan to support the "pause" command which interrupts a
running dart isolate. Then I'll need the listener thread.

Adding a todo here to block until a debugger connection has been established.

On 2012/05/03 22:52:08, asiva wrote:
> I am wondering if this loop instead of sitting here sleeping and waiting for
> IsConnected to become true should instead wait for OOB messages here. The
> listener thread then posts an OOB message when a debugger connection is made.
> 
> If you want to keep things simple initially then maybe this thread itself
could
> do the accept stuff waiting here for the connection to be established.
> Especially since you are not dealing with the debugger interrupting a running
> process I don't see the point of starting that other thread which will do a
poll
> and accept the connection. This thread could sit here on a blocking accept.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:243: }
Good point. I was thinking of removing all breakpoints and allow the code
generator to use optimized code again. Added a todo.

On 2012/05/03 22:52:08, asiva wrote:
> What about the debugger state (breakpoints set etc.) Should they be retained
> with a NOP breakpoint handler so that if a debugger subsequently attaches back
> they still have the old state or the easier approach is to just delete all the
> breakpoints etc. and clean up any other debugging state.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.cc:251: listener_fd_ =
ServerSocket::CreateBindListen("127.0.0.1", port_number, 1);
I moved this default IP address to main.cc and added a TODO.

On 2012/05/03 22:52:08, asiva wrote:
> instead of hard coding the ip address why not accept it as a command line
> parameter similar to port number, (would be useful if a machine had more than
> one nic and people wanted to use the other one).

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection.h
File runtime/bin/dbg_connection.h (right):

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/dbg_connection...
runtime/bin/dbg_connection.h:29: class DebuggerConnectionHandler {
On 2012/05/03 22:52:08, asiva wrote:
> extends AllStatic.
> 
> I guess we do not have AllStatic in the bin directory we could add one or you
> could add
> 
>   DISALLOW_ALLOCATION();
>   DISALLOW_IMPLICIT_CONSTRUCTORS(DebuggerConnectionHandler);
> 
> to this class.

Done.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/fdutils_linux.cc
File runtime/bin/fdutils_linux.cc (right):

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/fdutils_linux....
runtime/bin/fdutils_linux.cc:41: return true;
Not quite. The bit operation is | in one case and & in the other.
But built a helper function along the same idea.

On 2012/05/03 22:52:08, asiva wrote:
> The code in SetNonBlocking and SetBlocking can be factored into a single
helper
> function:
> bool SetBlockingState(intptr_t fd, intptr_t state);
> SetBlocking would be SetBlockingState(fd, ~O_NONBLOCK);
> SetNonBlocking would be SetBlockingState(fd, O_NONBLOCK);

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/fdutils_macos.cc
File runtime/bin/fdutils_macos.cc (right):

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/fdutils_macos....
runtime/bin/fdutils_macos.cc:41: return true;
I was wondering too. They probably have a different API.

On 2012/05/03 22:52:08, asiva wrote:
> Ditto comment regarding re factoring this.
> 
> I wonder why we are missing fdutils_win.cc

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/main.cc
File runtime/bin/main.cc (right):

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/main.cc#newcod...
runtime/bin/main.cc:107: "Use --debug[:<port number>]\n");
On 2012/05/03 22:52:08, asiva wrote:
> see comment regarding having this option be
> --debug=[<address>:]<port_number>

Done.

http://codereview.chromium.org/10357003/diff/11003/runtime/bin/main.cc#newcod...
runtime/bin/main.cc:110: breakpoint_at = "main";
I agree. For now, I need this because I can't interrupt the running isolate.

On 2012/05/03 22:52:08, asiva wrote:
> Maybe later on this should also be a command line option i.e people may chose
to
> have the VM wait on main for a debugger to attach or just run and allow for a
> debugger to attach later and use the OOB mechanism to interrupt the running
> program.