Redo "running" field in debug-delay.js and support "suspend" command

Redo "running" field in debug-delay.js and support "suspend" command

It also fixes "backtrace" command so that it didn't give away random stack if we are running

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

[yurys, 2009-09-28 18:08:15]

Please add a test for that change and update wiki page with the protocol
description.

http://codereview.chromium.org/242034/diff/1/2
File src/debug-delay.js (right):

http://codereview.chromium.org/242034/diff/1/2#newcode1110
Line 1110: // Handler may set this field to contol debugger state.
typo: contol->control

http://codereview.chromium.org/242034/diff/1/2#newcode1260
Line 1260: if (response.running === true || response.running === false) {
use if (!IS_UNDEFINED(response.running)) as in other places in this file

http://codereview.chromium.org/242034/diff/1/2#newcode1263
Line 1263: response.running = this.running_;  // Response gets current running
state.
long line

[Peter Rybin, 2009-09-28 18:20:36]

http://codereview.chromium.org/242034/diff/1/2
File src/debug-delay.js (right):

http://codereview.chromium.org/242034/diff/1/2#newcode1110
Line 1110: // Handler may set this field to contol debugger state.
On 2009/09/28 18:08:15, Yury Semikhatsky wrote:
> typo: contol->control

Done.

http://codereview.chromium.org/242034/diff/1/2#newcode1260
Line 1260: if (response.running === true || response.running === false) {
On 2009/09/28 18:08:15, Yury Semikhatsky wrote:
> use if (!IS_UNDEFINED(response.running)) as in other places in this file

Done.

http://codereview.chromium.org/242034/diff/1/2#newcode1263
Line 1263: response.running = this.running_;  // Response gets current running
state.
On 2009/09/28 18:08:15, Yury Semikhatsky wrote:
> long line

Done.

[Søren Thygesen Gjesse, 2009-09-29 11:30:54]

Maybe renaming "running" to "continue" in debug-delay.js would make it more
clear whats is happening. When inside the debugger V8 is not running, but it
might be expecting to continue running (or not continue running) after
processing the command.

http://codereview.chromium.org/242034/diff/1002/2004
File src/debug-delay.js (right):

http://codereview.chromium.org/242034/diff/1002/2004#newcode1260
Line 1260: if (!IS_UNDEFINED(response.new_running_value)) {
Is the new_running_state required, can't running just be assigned directly?

http://codereview.chromium.org/242034/diff/1002/2004#newcode1538
Line 1538: if (this.isRunning()) {
Why is this check for backtrace required? If it is required for backtrace then
it should be required for other requests like frame and scopes/scope.

http://codereview.chromium.org/242034/diff/1002/2004#newcode1918
Line 1918: // TODO(peter.rybin): probably we need some body field here.
Please remove this TODO and file a bug instead.

[Peter Rybin, 2009-09-29 11:51:32]

Hi Soren

Thanks for looking at this CL.

I'm trying to refactor our code a bit so that each command handler might
optionally change the running state of VM.

However, not only I need "continue", but also a "suspend". That's why
new_running_value has 3 "position": true, false and undefined.

Suspend command is a regular JSON command that does not have any special QoS and
relies on --debugger-auto-break=true

Peter

http://codereview.chromium.org/242034/diff/1002/2004
File src/debug-delay.js (right):

http://codereview.chromium.org/242034/diff/1002/2004#newcode1260
Line 1260: if (!IS_UNDEFINED(response.new_running_value)) {
On 2009/09/29 11:30:54, Søren Gjesse wrote:
> Is the new_running_state required, can't running just be assigned directly?

I wouldn't say it was "required"; however I tried to avoid variables with a
complex lifecycle to keep the code managable.
"new_running_value" is optionally set by handlers and is read here
"running" is always set here and goes to JSON.
I can merge this 2 variables in one, but it might make it less obvious who
expects what.
Anyway, I don't mean to bring my own style here. So please advise, do thing I
should merge them to keep the code shorter?

http://codereview.chromium.org/242034/diff/1002/2004#newcode1538
Line 1538: if (this.isRunning()) {
On 2009/09/29 11:30:54, Søren Gjesse wrote:
> Why is this check for backtrace required? If it is required for backtrace then
> it should be required for other requests like frame and scopes/scope.

Sure. I just haven't finished it.
I don't see a reason to give away a random stack trace.
However I have a secret agenda here: I plan to use this call to tell whether V8
is running or paused; I need it when I am attaching to VM via V8 DebuggerAgent.

http://codereview.chromium.org/242034/diff/1002/2004#newcode1918
Line 1918: // TODO(peter.rybin): probably we need some body field here.
On 2009/09/29 11:30:54, Søren Gjesse wrote:
> Please remove this TODO and file a bug instead.

Will do.
btw Do you think we really need a body for this response? For now I have nothing
to put in this body.

[Peter Rybin, 2009-09-29 20:41:53]

I changed debug.cc (moved running out of the cycle to preserve its value if
failed to read it from debug command processor).

I also added a simple test for suspend.
And fixed others test to pass after my change and made them somewhat more
verbose.

What do you think?

Peter

[Søren Thygesen Gjesse, 2009-09-30 09:19:15]

If you need some way of checking whether the VM is running you should add a
command for that and not rely on a command like backtrace returning an empty
response. When connecting to the debugger agent the VM should never be in a
break state as all debugger events are ignored when no debugger is connected.

http://codereview.chromium.org/242034/diff/2009/2010
File src/debug-delay.js (right):

http://codereview.chromium.org/242034/diff/2009/2010#newcode798
Line 798: ExecutionState.prototype.debugCommandProcessor =
function(is_running_initial) {
How about changing the name of the argument to auto_continue as it indicates
whether an inplicit continue command is in effect. Also consider making it
optional (just prefixing it with opt_).

http://codereview.chromium.org/242034/diff/2009/2010#newcode1086
Line 1086: this.running_ = is_running_initial;
Consider making this optional and default to false.

  this.running_ = opt_auto_continue || false;

http://codereview.chromium.org/242034/diff/2009/2010#newcode1319
Line 1319: response.new_running_value = true;
I still cannot see why just assigning response.running here will not work.

http://codereview.chromium.org/242034/diff/2009/2010#newcode1538
Line 1538: if (this.isRunning()) {
This should be removed. I see no reason for not allowing backtrace requests at
all times. Even with this check the command sequence suspend, backtrace,
continue will provide the same information.

http://codereview.chromium.org/242034/diff/2009/2010#newcode1539
Line 1539: // No stacktrace when running
Even if we want to disallow some commands in some situations the response should
have success=false and a message explaining the cause of the failure.

http://codereview.chromium.org/242034/diff/2009/2010#newcode1918
Line 1918: // TODO(peter.rybin): probably we need some body field here.
I think you should just remove this comment. A response to the suspend command
containing success=true and running=false should be all the information required
for the debugger client.

http://codereview.chromium.org/242034/diff/2009/2010#newcode1919
Line 1919: response.new_running_value = false;
Also here, I cannot see why just assigning response.running will not work.

http://codereview.chromium.org/242034/diff/2009/2012
File test/mjsunit/debug-backtrace.js (right):

http://codereview.chromium.org/242034/diff/2009/2012#newcode101
Line 101: dcp = exec_state.debugCommandProcessor(false);
Consider argument default to false.

http://codereview.chromium.org/242034/diff/2009/2013
File test/mjsunit/debug-changebreakpoint.js (right):

http://codereview.chromium.org/242034/diff/2009/2013#newcode62
Line 62: var dcp =
exec_state.debugCommandProcessor("unspecified_running_state");
Passing other values than true or false does not make any sense. Please use true
or false in all the tests.

http://codereview.chromium.org/242034/diff/2009/2017
File test/mjsunit/debug-evaluate-recursive.js (right):

http://codereview.chromium.org/242034/diff/2009/2017#newcode151
Line 151: assertTrue(listenerComplete, "msg5");
msg5?

http://codereview.chromium.org/242034/diff/2009/2017#newcode166
Line 166: assertTrue(listenerComplete, "msg1");
msg1?

http://codereview.chromium.org/242034/diff/2009/2025
File test/mjsunit/debug-suspend.js (right):

http://codereview.chromium.org/242034/diff/2009/2025#newcode58
Line 58: var response = safeEval(dcp.processDebugJSONRequest(request));
This does not really test that the suspend command changes the running state, as
the running state is false before as this code is running in a debug event
listener and not a message handler with auto break enabled. To test suspend you
could issue a continue command before the suspend command to see running go from
false to true to false.

http://codereview.chromium.org/242034/diff/2009/2025#newcode61
Line 61: } else {
Empty else part.

http://codereview.chromium.org/242034/diff/2009/2025#newcode67
Line 67: if (event == Debug.DebugEvent.Break) {
Please indent the try code.

[Peter Rybin, 2009-10-05 15:29:26]

Hi Soren

Sorry for not replying earlier -- I was on vacation.

On 2009/09/30 09:19:15, Søren Gjesse wrote:
> If you need some way of checking whether the VM is running you should add a
> command for that and not rely on a command like backtrace returning an empty
> response.

Actually my plan was to rely on "running" field of backtrace response. I don't
need "body" at all. I just wanted to be accurate and additionally block
random-time stacktraces; I'm find with leaving them.
I'm going to fix "running" field in responses by this CL. Currently "running"
field is not accurate at all.

Do you really thing I should introduce additional command instead of using
"running"?


> When connecting to the debugger agent the VM should never be in a
> break state as all debugger events are ignored when no debugger is connected.

I propose we separate this statement from a previous one. Currently I'm working
from assumption that VM may be found in a break state (there are some reasons
for this).

[Peter Rybin, 2009-10-05 16:26:22]

http://codereview.chromium.org/242034/diff/2009/2010
File src/debug-delay.js (right):

http://codereview.chromium.org/242034/diff/2009/2010#newcode798
Line 798: ExecutionState.prototype.debugCommandProcessor =
function(is_running_initial) {
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> How about changing the name of the argument to auto_continue as it indicates
> whether an inplicit continue command is in effect. Also consider making it
> optional (just prefixing it with opt_).

I'm not sure it's perfect.
All around here we deal with "running" variables. Here we do not actually care
what it means. We do not run a cycle in debug-delay.js, so we do not know what
"auto_continue" means here. I would prefer to keep "auto_continue" in debug.cc
code and here to work with "running".

What do you think?

http://codereview.chromium.org/242034/diff/2009/2010#newcode1086
Line 1086: this.running_ = is_running_initial;
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> Consider making this optional and default to false.
> 
>   this.running_ = opt_auto_continue || false;
> 

Done.

http://codereview.chromium.org/242034/diff/2009/2010#newcode1319
Line 1319: response.new_running_value = true;
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> I still cannot see why just assigning response.running here will not work.

It will surely work for computer but I'm not sure well enough for human.
It gets set and read in a unobvious sequence. Having a single "running" variable
is like a variable reuse. I tried to have each variable set only once.

Done

http://codereview.chromium.org/242034/diff/2009/2010#newcode1538
Line 1538: if (this.isRunning()) {
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> This should be removed. I see no reason for not allowing backtrace requests at
> all times. Even with this check the command sequence suspend, backtrace,
> continue will provide the same information.

I wasn't sure we want random-time backtrace commands.

Done

http://codereview.chromium.org/242034/diff/2009/2010#newcode1539
Line 1539: // No stacktrace when running
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> Even if we want to disallow some commands in some situations the response
should
> have success=false and a message explaining the cause of the failure.

I like it.
However, I guess we think we shouldn't do this.

http://codereview.chromium.org/242034/diff/2009/2010#newcode1918
Line 1918: // TODO(peter.rybin): probably we need some body field here.
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> I think you should just remove this comment. A response to the suspend command
> containing success=true and running=false should be all the information
required
> for the debugger client.

Thanks
Done

http://codereview.chromium.org/242034/diff/2009/2010#newcode1919
Line 1919: response.new_running_value = false;
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> Also here, I cannot see why just assigning response.running will not work.

Done.

http://codereview.chromium.org/242034/diff/2009/2012
File test/mjsunit/debug-backtrace.js (right):

http://codereview.chromium.org/242034/diff/2009/2012#newcode101
Line 101: dcp = exec_state.debugCommandProcessor(false);
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> Consider argument default to false.

I did it optional. However, I think explicit is better...

http://codereview.chromium.org/242034/diff/2009/2013
File test/mjsunit/debug-changebreakpoint.js (right):

http://codereview.chromium.org/242034/diff/2009/2013#newcode62
Line 62: var dcp =
exec_state.debugCommandProcessor("unspecified_running_state");
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> Passing other values than true or false does not make any sense. Please use
true or false in all the tests.

I'm not sure here.
It does make sense as a "unique" value. I check that handler doesn't alter
"running" field whether to "true" or "false", because it shouldn't.

I think that debug command processor should not necessarily have boolean
"running", because it doesn't use it.

Do you think I should document it?

http://codereview.chromium.org/242034/diff/2009/2017
File test/mjsunit/debug-evaluate-recursive.js (right):

http://codereview.chromium.org/242034/diff/2009/2017#newcode151
Line 151: assertTrue(listenerComplete, "msg5");
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> msg5?

We don't have messages here and we don't have stacktrace. I thought this mark-up
helps tracing error and it's better than nothing.

Done.

http://codereview.chromium.org/242034/diff/2009/2017#newcode166
Line 166: assertTrue(listenerComplete, "msg1");
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> msg1?

Done.

http://codereview.chromium.org/242034/diff/2009/2025
File test/mjsunit/debug-suspend.js (right):

http://codereview.chromium.org/242034/diff/2009/2025#newcode58
Line 58: var response = safeEval(dcp.processDebugJSONRequest(request));
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> This does not really test that the suspend command changes the running state,
as
> the running state is false before as this code is running in a debug event
> listener and not a message handler with auto break enabled. To test suspend
you
> could issue a continue command before the suspend command to see running go
from
> false to true to false.

Done

http://codereview.chromium.org/242034/diff/2009/2025#newcode61
Line 61: } else {
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> Empty else part.

Done.

http://codereview.chromium.org/242034/diff/2009/2025#newcode67
Line 67: if (event == Debug.DebugEvent.Break) {
On 2009/09/30 09:19:15, Søren Gjesse wrote:
> Please indent the try code.

Done.

[Peter Rybin, 2009-10-14 11:44:22]

ping

[yurys, 2009-10-14 12:01:28]

LGTM with comments

http://codereview.chromium.org/242034/diff/4005/11003
File test/mjsunit/debug-backtrace.js (right):

http://codereview.chromium.org/242034/diff/4005/11003#newcode69
Line 69: return null;
you can instead use assertFalse(!!response.body()... below

http://codereview.chromium.org/242034/diff/4005/11008
File test/mjsunit/debug-evaluate-recursive.js (right):

http://codereview.chromium.org/242034/diff/4005/11008#newcode89
Line 89: testRequest(exec_state, '{"expression":"f()","disable_break":true}',
true, 1);
long lines: 89,90,92,93

http://codereview.chromium.org/242034/diff/4005/11012
File test/mjsunit/debug-references.js (right):

http://codereview.chromium.org/242034/diff/4005/11012#newcode69
Line 69: assertEquals(response.running, dcp.isRunning(), request + ' -> expected
not running');
long line

http://codereview.chromium.org/242034/diff/4005/11016
File test/mjsunit/debug-suspend.js (right):

http://codereview.chromium.org/242034/diff/4005/11016#newcode48
Line 48: function testArguments(exec_state) {
please inline or rename this method