Misc debugger enhancements and bug fixes....

Misc debugger enhancements and bug fixes.

1. Added gdb style debugger commands (and their shortcuts) for d8.
   These include:
   - s[tep] : step into the current statement.
   - s[tep]i[n]: step into the current statement with the minimum step.
   - n[ext] : step to the next statement.
   - fin[ish] : step out of the current function.
   - cond : setting conditions on breakpoints.
   - d[elete] : deletes breakpoints.
   - en[able]|dis[able]: enables/disables breakpoints including
                         exception breakpoints.
   - ignore : ignores a breakpoint for a specified period.
   - inf[o] ar[gs] : info on arguments of the current function.
   - inf[o] lo[cals] : info on local vars of the current function.
   - inf[o] br[eakpoints] : info on breakpoints.
   - l[ist] : similar to source, but allows the user to continually
              dump subsequent lines of source code either in the
              forward or backward direction.
   - quit / exit / disconnect : terminates the remote debugger
                                session.

     NOTE: Active breakpoints will automatically be disabled when
     the remote debugger detaches.  This allows v8 to continue to
     run without worrying about a loss of a debugger session.

2. Added support for breaking the debugger by simply typing ENTER.
   The break command is now optional.

3. Once the debugger is broken, the user can now just type ENTER
   to repeat the last command.  This is useful to functionality that
   needs to be invoked repeatedly e.g. step, list.

4. Added more verbose descriptions in d8's help.

5. Fixed a line and column number offset bug in the listing of breakpoint
   line and column numbers.

6. Added a gc command to allow GCs to be requested from the debugger
   interface.  The plumbing for requesting different types of GCs is
   there, but the underlying implementation currently only triggers a
   full mark-compact GC.  The command also returns the before and after
   sizes of the heap.

7. Added trace json, and flags commands that are not published in help.
   trace json is used for tracing the debugger packets send from and
   received by d8.  flags is for setting v8 flags.  These are useful for
   people debugging v8 itself, but not necessarily users of v8.

8. Added the ability to enable and disable break on all / uncaught
   exceptions in to d8.

9. Added a fix to prevent the Debugger Agent from being re-instantiated
   if one already exists.

10. Added the ability to filter results of the script command by matching
    text or numbers on the results.

11. Added v8 flags to enable/disable the sending of debugger BeforeCompile,
    AfterCompile, and ScriptCollected events.

12. Fixed some undefined value bugs that resulted in v8 or the debugger
    failing.

13. Added a few minor WEBOS__ customizations (analogous to ANDROID
    customizations).

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

Reverted: http://code.google.com/p/v8/source/detail?r=6183

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

[marklam, 2010-12-24 01:05:13]

Hi Søren,

Here are most of the debugger changes I had said that I wanted to contribute. 
Among other things, these contains some of the ground work laid for the heap
profiler enhancements that I will do in another contribution later after we
square away this set of changes.  

If you wish, I can go through the review and make comments on why I made some of
the changes.  Just let me know if that is needed.

Thanks.

Some details: I resolved all the lint issues that seem to make sense.  I
couldn't get the JS files to be lint'ed by the review site.  If there are
issues, please let me know.  This set of changes has been run against the
test.py testsuite from bleeding edge rev 6117 for a release ia32 build with 0
failures.

[marklam, 2010-12-27 00:47:34]

Søren,

I forgot to mention one detail: I think most of these changes are probably
transparent to the Chromium debugger as they don't change the json protocol in
an incompatible way.  The only one change that I think is a departure from how
things used to work is in d8 itself where the "step" command now defaults to
"step in" (just like how gdb does it) as opposed to "step next".  I hope that
that's an acceptable change of behavior.  Thanks.

Mark

[Søren Thygesen Gjesse, 2011-01-03 08:56:07]

http://codereview.chromium.org/5980006/diff/10002/src/d8.js
File src/d8.js (right):

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode302
src/d8.js:302: cmd_line = 'break'; // If not in debugger mode, break with a
frame request.
Long line. Some more below.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode608
src/d8.js:608: var stepcount = Number(args[0]);
Tab character sneaked in.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode788
src/d8.js:788: if (Debug.State.displaySourceEndLine == -1) {
Comment indented more that source line (This comment and the next). Also I am
not sure I fully understand these two comments as one says "If we list forwards"
and the other "If we list backwards", but they are not inside different "if"
clauses.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode791
src/d8.js:791: // If we list backwards, start from 1 lines before the current
location.
This sets displaySourceStartLine if displaySourceEndLine is -1 - can't that
cause displaySourceStartLine to stay as -1 - is that intended?

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode978
src/d8.js:978: // Check for exception breaks first:
Why do you want both

  en exc all

and

  en all exc

?

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1227
src/d8.js:1227: print('trace compile');
Code in comment. PLease just use an "ordinary" for information on the hidden
command.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1313
src/d8.js:1313: function refObjectToString_(protocolPackage, handle) {
Please use 2-char indentation.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1418
src/d8.js:1418: result += body.enabled ? 'enabled':'disabled';
Please add spaces on both sides of ':'.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1468
src/d8.js:1468: }
Tabs.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode2096
src/d8.js:2096: if (property_value === null) {
Tab.

http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc
File src/debug-agent.cc (right):

http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc#newcode173
src/debug-agent.cc:173: const char *msg = *message;
Please avoid mixed case variables, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names.

http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc#newcode182
src/debug-agent.cc:182: // Check if we're getting a break request:
These checks seems pretty fragile, and depends on the debugger client formatting
of the JSON. There is now a separate JSON parser in C++ (see Runtime_ParseJson
where it is used), and maybe that can be used instead, but that might require
more refactoring as we don't want to parse the request twice.

We can probably have it like this for now.

http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc#newcode199
src/debug-agent.cc:199: if (isBreakRequest && !Debug::InDebugger()) {
Is this required? Sending the command using v8::Debug::SendCommand should
trigger a break of the DEBUGCOMMAND - is that not sufficient?

http://codereview.chromium.org/5980006/diff/10002/src/debug-debugger.js
File src/debug-debugger.js (right):

http://codereview.chromium.org/5980006/diff/10002/src/debug-debugger.js#newco...
src/debug-debugger.js:1729: DebugCommandProcessor.prototype.disconnectRequest_ =
function(request, response) {
Long line.

http://codereview.chromium.org/5980006/diff/10002/src/debug-debugger.js#newco...
src/debug-debugger.js:1735:
DebugCommandProcessor.prototype.setExceptionBreakRequest_ = function(request,
response) {
Ditto.

http://codereview.chromium.org/5980006/diff/10002/src/debug.cc
File src/debug.cc (right):

http://codereview.chromium.org/5980006/diff/10002/src/debug.cc#newcode625
src/debug.cc:625: #if defined(WEBOS__)
I think this change of the default is OK for all platforms.

http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc#newcode10730
src/runtime.cc:10730: #ifdef ENABLE_DEBUGGER_SUPPORT
I think there is a long section above guarded with #ifdef
ENABLE_DEBUGGER_SUPPORT/#endif - please move this up there.

http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc#newcode10732
src/runtime.cc:10732: static MaybeObject* Runtime_SetFlag(Arguments args) {
Please change name to plural.

http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc#newcode10743
src/runtime.cc:10743: static MaybeObject* Runtime_DoGC(Arguments args) {
How about CollectGarbage (or just GC) instead of DoGC?

[marklam, 2011-01-04 20:12:13]

I've made all the necessary changes except for breaking up long lines which are
purely due to long strings in d8.js.  I had a question regarding that in my
comments below.  Please let me know what the preferred practice is there and
I'll make the remaining change.  Thanks.

FYI, I also svn up to the latest to make sure I have no conflicts.  As a result,
some new diffs showed up that are not due to this change set.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js
File src/d8.js (right):

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode302
src/d8.js:302: cmd_line = 'break'; // If not in debugger mode, break with a
frame request.
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Long line. Some more below.

Done.  Shortened the comment.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode386
src/d8.js:386: this.request_ = this.frameCommandToJSONRequest_('' +
(Debug.State.currentFrame + 1));
broke up this line to fit in 80 columns.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode391
src/d8.js:391: this.request_ = this.frameCommandToJSONRequest_('' +
(Debug.State.currentFrame - 1));
broke up this line to fit in 80 columns.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode452
src/d8.js:452: this.request_ = this.changeBreakpointCommandToJSONRequest_(args,
'enable');
changed to fit into 80 columns.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode457
src/d8.js:457: this.request_ = this.changeBreakpointCommandToJSONRequest_(args,
'disable');
changed to fit into 80 columns.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode461
src/d8.js:461: this.request_ = this.changeBreakpointCommandToJSONRequest_(args,
'ignore');
changed to fit into 80 columns.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode608
src/d8.js:608: var stepcount = Number(args[0]);
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Tab character sneaked in.

Done.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode788
src/d8.js:788: if (Debug.State.displaySourceEndLine == -1) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Comment indented more that source line (This comment and the next). Also I am
> not sure I fully understand these two comments as one says "If we list
forwards"
> and the other "If we list backwards", but they are not inside different "if"
> clauses.

There was some accidental tabs in here.  Fixed.  The comments were unclear. 
I've clarified it a little.  It now reads:

    // If we list forwards, we will start listing after the last source end
    // line.  Set it to start from 5 lines before the current location.

    // If we list backwards, we will start listing backwards from the last
    // source start line.  Set it to start from 1 lines before the current
    // location.

Hence, no "if" clauses are needed here.  I'm just setting up the requisite
initial conditions for the "if" clauses that follow below.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode791
src/d8.js:791: // If we list backwards, start from 1 lines before the current
location.
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> This sets displaySourceStartLine if displaySourceEndLine is -1 - can't that
> cause displaySourceStartLine to stay as -1 - is that intended?

displaySourceStartLine and displaySourceEndLine are always set as a pair. 
Hence, I only needed to check that one is uninitialized and initialize both at
the same time.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode975
src/d8.js:975: DebugRequest.prototype.changeBreakpointCommandToJSONRequest_ =
function(args, command) {
broke up this line to fit in 80 columns.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode978
src/d8.js:978: // Check for exception breaks first:
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Why do you want both
> 
>   en exc all
> 
> and
> 
>   en all exc
> 
> ?

"en exc all" when we think in terms of types: enable exceptions of all types.
"en all exc" when we think in terms how one would say it in English: enable all
exceptions.

I had it as "en exc all" initially but found that in usage, a human would
sometimes think the other way.  I just added support for both to make it easier
to use.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode983
src/d8.js:983: if ((command == 'enable' || command == 'disable') && args &&
args.length > 1) {
broke up this line to fit in 80 columns.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode993
src/d8.js:993: var arg2 = (nextPos > 0) ? args.substring(nextPos + 1,
args.length) : 'all';
broke up this line to fit in 80 columns.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1006
src/d8.js:1006: var arg2 = (nextPos > 0) ? args.substring(nextPos + 1,
args.length) : null;
broke up this line to fit in 80 columns.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1176
src/d8.js:1176: print('Note: [] denotes optional parts of commands, or optional
options / arguments.');
How strict do you want to be on long strings like these exceeding the 80
columns?  If you want me to break it up, should I use ' ' + ' ' notation to join
2 strings?  The reason I ask is because unlike C++ code, I presume we actually
have a runtime performance impact here (though it is insignificant in this
case).  Just thought I'd ask to understand what is considered the preferred
practice here.  Thanks.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1227
src/d8.js:1227: print('trace compile');
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Code in comment. PLease just use an "ordinary" for information on the hidden
> command.

I presume you mean to use an ordinary comment and not commented out code here. 
I've applied that.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1313
src/d8.js:1313: function refObjectToString_(protocolPackage, handle) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Please use 2-char indentation.

Sorry.  Some tabs snuck in.  Fixed.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1418
src/d8.js:1418: result += body.enabled ? 'enabled':'disabled';
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Please add spaces on both sides of ':'.

Done.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode1468
src/d8.js:1468: }
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Tabs.

Done.

http://codereview.chromium.org/5980006/diff/10002/src/d8.js#newcode2096
src/d8.js:2096: if (property_value === null) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Tab.

Done.

http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc
File src/debug-agent.cc (right):

http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc#newcode173
src/debug-agent.cc:173: const char *msg = *message;
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Please avoid mixed case variables, see
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names.

Done.

http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc#newcode182
src/debug-agent.cc:182: // Check if we're getting a break request:
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> These checks seems pretty fragile, and depends on the debugger client
formatting
> of the JSON. There is now a separate JSON parser in C++ (see Runtime_ParseJson
> where it is used), and maybe that can be used instead, but that might require
> more refactoring as we don't want to parse the request twice.
> 
> We can probably have it like this for now.

I see what you mean.  But yes, at least as a first pass, I would rather just
leave this as is for now.  This can be re-implemented later if needed.

http://codereview.chromium.org/5980006/diff/10002/src/debug-agent.cc#newcode199
src/debug-agent.cc:199: if (isBreakRequest && !Debug::InDebugger()) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Is this required? Sending the command using v8::Debug::SendCommand should
> trigger a break of the DEBUGCOMMAND - is that not sufficient?

Hmmm.  This used to be needed, but I just re-tested without it and things seem
to be working fine now.  Perhaps the v8 source has mutated to make this
unnecessary since the time I added it in the early days.  I've removed this if
statement along with all references to isBreakRequest above.

http://codereview.chromium.org/5980006/diff/10002/src/debug-debugger.js
File src/debug-debugger.js (right):

http://codereview.chromium.org/5980006/diff/10002/src/debug-debugger.js#newco...
src/debug-debugger.js:1729: DebugCommandProcessor.prototype.disconnectRequest_ =
function(request, response) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Long line.

Done.

http://codereview.chromium.org/5980006/diff/10002/src/debug-debugger.js#newco...
src/debug-debugger.js:1735:
DebugCommandProcessor.prototype.setExceptionBreakRequest_ = function(request,
response) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/5980006/diff/10002/src/debug.cc
File src/debug.cc (right):

http://codereview.chromium.org/5980006/diff/10002/src/debug.cc#newcode625
src/debug.cc:625: #if defined(WEBOS__)
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> I think this change of the default is OK for all platforms.

Done.  Made it unconditional.

http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc#newcode10730
src/runtime.cc:10730: #ifdef ENABLE_DEBUGGER_SUPPORT
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> I think there is a long section above guarded with #ifdef
> ENABLE_DEBUGGER_SUPPORT/#endif - please move this up there.

Done.

http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc#newcode10732
src/runtime.cc:10732: static MaybeObject* Runtime_SetFlag(Arguments args) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> Please change name to plural.

Done.

http://codereview.chromium.org/5980006/diff/10002/src/runtime.cc#newcode10743
src/runtime.cc:10743: static MaybeObject* Runtime_DoGC(Arguments args) {
On 2011/01/03 08:56:07, Søren Gjesse wrote:
> How about CollectGarbage (or just GC) instead of DoGC?

Done.

[Søren Thygesen Gjesse, 2011-01-05 13:57:32]

LGTM

We are not linting the line lengths of the JS files, so I accepted the long
lines in d8.js.

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

Thank you for the patch!

It would be nice if you could follow up with some tests of the new debug
protocol messages like we already have for the current messages in
test/mjsumit/debug-*

We also need to update http://code.google.com/p/v8/wiki/DebuggerProtocol with
information on the new messages and changes to the existing ones. Actually you
will not be able to edit http://code.google.com/p/v8/wiki/DebuggerProtocol
online, but you can send a update and I will put it in.