Add V8 debugger agent....

Add V8 debugger agent.

The debugger agent listens on a TCP/IP port for a remote debugger connection. When such a connection is established the debuger JSON protocol is communicated between the agent the the remote debugger. The messages containing the JSON protocol has a RFC-822 like header with a Content-Length field and with the body containing the JSON in UTF-8 encoding.

The D8 shell has option --debugger-agent to start the debugger agent.

Committed: http://code.google.com/p/v8/source/detail?r=1405

[Mads Ager (chromium), 2009-03-03 11:04:30]

LGTM

We need to update the XCode project as well.

http://codereview.chromium.org/27355/diff/17/23
File src/SConscript (right):

http://codereview.chromium.org/27355/diff/17/23#newcode43
Line 43: 'flags.cc', 'frames.cc', 'global-handles.cc', 'handles.cc',
'hashmap.cc',
Something is fishy about the indentation here.

http://codereview.chromium.org/27355/diff/17/26
File src/debug-agent.cc (right):

http://codereview.chromium.org/27355/diff/17/26#newcode50
Line 50: // Listen for new connections
Add periods to the end of these comments?

http://codereview.chromium.org/27355/diff/17/26#newcode72
Line 72: client->Send("Remote debugging session already active\r\n", 41);
Create a const char* and use strlen?

Also, why is the string terminated by \r\n instead of just \n?

http://codereview.chromium.org/27355/diff/17/26#newcode132
Line 132: v8::Debug::SendCommand((const uint16_t *) temp, len);
Use C++ style cast.

http://codereview.chromium.org/27355/diff/17/26#newcode174
Line 174: header_buffer[header_buffer_position] = '\0';
Don't you need an if here instead?  Can't you end up writing a '\0' right after
the allocated buffer?

http://codereview.chromium.org/27355/diff/17/26#newcode186
Line 186: // Check that key is Content-Length.
Where are you checking that the key is Content-Length?

http://codereview.chromium.org/27355/diff/17/26#newcode189
Line 189: content_length = 10 * content_length + (value[i] - '0');
Can there be no overflow here?

http://codereview.chromium.org/27355/diff/17/27
File src/debug-agent.h (right):

http://codereview.chromium.org/27355/diff/17/27#newcode41
Line 41: // Debugger agent which starts a  socket listener on the debugger port
and
a  socket -> a socket

two spaces instead of one.

http://codereview.chromium.org/27355/diff/17/27#newcode48
Line 48: virtual ~DebuggerAgent() {}
Do you need the virtual destructor?  The DebuggerAgentSession class does not
have one.

http://codereview.chromium.org/27355/diff/17/27#newcode72
Line 72: // Debugger agent session. The session received requests from the
remote
received -> receives

http://codereview.chromium.org/27355/diff/17/22
File src/flag-definitions.h (right):

http://codereview.chromium.org/27355/diff/17/22#newcode204
Line 204: DEFINE_bool(regexp_native, true, "use native code regexp
implementation (IA32"
I would break this differently:

DEFINE_bool(regexp_native, true,
            "use native ...")

?

[Søren Thygesen Gjesse, 2009-03-03 11:56:14]

http://codereview.chromium.org/27355/diff/17/23
File src/SConscript (right):

http://codereview.chromium.org/27355/diff/17/23#newcode43
Line 43: 'flags.cc', 'frames.cc', 'global-handles.cc', 'handles.cc',
'hashmap.cc',
On 2009/03/03 11:04:31, Mads Ager wrote:
> Something is fishy about the indentation here.

Fixed (some tabs sneaked in).

http://codereview.chromium.org/27355/diff/17/26
File src/debug-agent.cc (right):

http://codereview.chromium.org/27355/diff/17/26#newcode50
Line 50: // Listen for new connections
On 2009/03/03 11:04:31, Mads Ager wrote:
> Add periods to the end of these comments?

Done.

http://codereview.chromium.org/27355/diff/17/26#newcode72
Line 72: client->Send("Remote debugging session already active\r\n", 41);
On 2009/03/03 11:04:31, Mads Ager wrote:
> Create a const char* and use strlen?
> 
> Also, why is the string terminated by \r\n instead of just \n?

Added static const char* message = ... and removed \r.

http://codereview.chromium.org/27355/diff/17/26#newcode132
Line 132: v8::Debug::SendCommand((const uint16_t *) temp, len);
On 2009/03/03 11:04:31, Mads Ager wrote:
> Use C++ style cast.

Done.

http://codereview.chromium.org/27355/diff/17/26#newcode174
Line 174: header_buffer[header_buffer_position] = '\0';
On 2009/03/03 11:04:31, Mads Ager wrote:
> Don't you need an if here instead?  Can't you end up writing a '\0' right
after
> the allocated buffer?

Added an if and a '- 1' where filling the buffer to always leave room for the
'\0'. Kept the ASSERT.

http://codereview.chromium.org/27355/diff/17/26#newcode186
Line 186: // Check that key is Content-Length.
On 2009/03/03 11:04:31, Mads Ager wrote:
> Where are you checking that the key is Content-Length?

I was not - added this check.

http://codereview.chromium.org/27355/diff/17/26#newcode189
Line 189: content_length = 10 * content_length + (value[i] - '0');
On 2009/03/03 11:04:31, Mads Ager wrote:
> Can there be no overflow here?

Added range checking and bailout for non numeric data.

http://codereview.chromium.org/27355/diff/17/27
File src/debug-agent.h (right):

http://codereview.chromium.org/27355/diff/17/27#newcode41
Line 41: // Debugger agent which starts a  socket listener on the debugger port
and
On 2009/03/03 11:04:31, Mads Ager wrote:
> a  socket -> a socket
> 
> two spaces instead of one.

Done.

http://codereview.chromium.org/27355/diff/17/27#newcode48
Line 48: virtual ~DebuggerAgent() {}
On 2009/03/03 11:04:31, Mads Ager wrote:
> Do you need the virtual destructor?  The DebuggerAgentSession class does not
> have one.

No, removed.

http://codereview.chromium.org/27355/diff/17/27#newcode72
Line 72: // Debugger agent session. The session received requests from the
remote
On 2009/03/03 11:04:31, Mads Ager wrote:
> received -> receives

Done.

http://codereview.chromium.org/27355/diff/17/22
File src/flag-definitions.h (right):

http://codereview.chromium.org/27355/diff/17/22#newcode204
Line 204: DEFINE_bool(regexp_native, true, "use native code regexp
implementation (IA32"
On 2009/03/03 11:04:31, Mads Ager wrote:
> I would break this differently:
> 
> DEFINE_bool(regexp_native, true,
>             "use native ...")
> 
> ?
>         

So would I - done.

[Søren Thygesen Gjesse, 2009-03-03 12:21:42]

Updated the xcode project using xcodebodge.py from the Chromium project - hope
it works.

On 2009/03/03 11:56:14, Søren Gjesse wrote:
> http://codereview.chromium.org/27355/diff/17/23
> File src/SConscript (right):
> 
> http://codereview.chromium.org/27355/diff/17/23#newcode43
> Line 43: 'flags.cc', 'frames.cc', 'global-handles.cc', 'handles.cc',
> 'hashmap.cc',
> On 2009/03/03 11:04:31, Mads Ager wrote:
> > Something is fishy about the indentation here.
> 
> Fixed (some tabs sneaked in).
> 
> http://codereview.chromium.org/27355/diff/17/26
> File src/debug-agent.cc (right):
> 
> http://codereview.chromium.org/27355/diff/17/26#newcode50
> Line 50: // Listen for new connections
> On 2009/03/03 11:04:31, Mads Ager wrote:
> > Add periods to the end of these comments?
> 
> Done.
> 
> http://codereview.chromium.org/27355/diff/17/26#newcode72
> Line 72: client->Send("Remote debugging session already active\r\n", 41);
> On 2009/03/03 11:04:31, Mads Ager wrote:
> > Create a const char* and use strlen?
> > 
> > Also, why is the string terminated by \r\n instead of just \n?
> 
> Added static const char* message = ... and removed \r.
> 
> http://codereview.chromium.org/27355/diff/17/26#newcode132
> Line 132: v8::Debug::SendCommand((const uint16_t *) temp, len);
> On 2009/03/03 11:04:31, Mads Ager wrote:
> > Use C++ style cast.
> 
> Done.
> 
> http://codereview.chromium.org/27355/diff/17/26#newcode174
> Line 174: header_buffer[header_buffer_position] = '\0';
> On 2009/03/03 11:04:31, Mads Ager wrote:
> > Don't you need an if here instead?  Can't you end up writing a '\0' right
> after
> > the allocated buffer?
> 
> Added an if and a '- 1' where filling the buffer to always leave room for the
> '\0'. Kept the ASSERT.
> 
> http://codereview.chromium.org/27355/diff/17/26#newcode186
> Line 186: // Check that key is Content-Length.
> On 2009/03/03 11:04:31, Mads Ager wrote:
> > Where are you checking that the key is Content-Length?
> 
> I was not - added this check.
> 
> http://codereview.chromium.org/27355/diff/17/26#newcode189
> Line 189: content_length = 10 * content_length + (value[i] - '0');
> On 2009/03/03 11:04:31, Mads Ager wrote:
> > Can there be no overflow here?
> 
> Added range checking and bailout for non numeric data.
> 
> http://codereview.chromium.org/27355/diff/17/27
> File src/debug-agent.h (right):
> 
> http://codereview.chromium.org/27355/diff/17/27#newcode41
> Line 41: // Debugger agent which starts a  socket listener on the debugger
port
> and
> On 2009/03/03 11:04:31, Mads Ager wrote:
> > a  socket -> a socket
> > 
> > two spaces instead of one.
> 
> Done.
> 
> http://codereview.chromium.org/27355/diff/17/27#newcode48
> Line 48: virtual ~DebuggerAgent() {}
> On 2009/03/03 11:04:31, Mads Ager wrote:
> > Do you need the virtual destructor?  The DebuggerAgentSession class does not
> > have one.
> 
> No, removed.
> 
> http://codereview.chromium.org/27355/diff/17/27#newcode72
> Line 72: // Debugger agent session. The session received requests from the
> remote
> On 2009/03/03 11:04:31, Mads Ager wrote:
> > received -> receives
> 
> Done.
> 
> http://codereview.chromium.org/27355/diff/17/22
> File src/flag-definitions.h (right):
> 
> http://codereview.chromium.org/27355/diff/17/22#newcode204
> Line 204: DEFINE_bool(regexp_native, true, "use native code regexp
> implementation (IA32"
> On 2009/03/03 11:04:31, Mads Ager wrote:
> > I would break this differently:
> > 
> > DEFINE_bool(regexp_native, true,
> >             "use native ...")
> > 
> > ?
> >         
> 
> So would I - done.