Debugger: introduce parametrized debug break.

Debugger: introduce parametrized debug break, the parameter is passed
back to EventListener to be able to dynamically specify behavior
on asynchronously enforced VM breakouts.

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

[yurys, 2010-07-12 10:38:19]

http://codereview.chromium.org/2962007/diff/1/2
File include/v8-debug.h (right):

http://codereview.chromium.org/2962007/diff/1/2#newcode258
include/v8-debug.h:258: static void DebugBreak(void* data = NULL);
It becomes non-deterministic, last caller of the method will override previous
value. The value may leak. Also with this implementation we won't be able to
distinguish between execution interrupts that were meant to be script pauses and
others that were scheduled to perform an action with v8 on the thread where JS
is executing. I think we need some sort of queue to serve what we need in Chrome
of this method.

http://codereview.chromium.org/2962007/diff/1/6
File src/execution.cc (right):

http://codereview.chromium.org/2962007/diff/1/6#newcode303
src/execution.cc:303: void* StackGuard::GetDebugBreakData() {
consider renaming it to TakeDebugBreakData as it clears
thread_local_.debug_break_data_

http://codereview.chromium.org/2962007/diff/1/7
File src/execution.h (right):

http://codereview.chromium.org/2962007/diff/1/7#newcode217
src/execution.h:217: if (should_postpone_interrupts(lock)) return;
is this really needed here?

[mnaganov (inactive), 2010-07-13 09:38:20]

I reworked the code to reuse CommandQueue

[yurys, 2010-07-13 09:50:42]

http://codereview.chromium.org/2962007/diff/7001/9
File include/v8-debug.h (right):

http://codereview.chromium.org/2962007/diff/7001/9#newcode263
include/v8-debug.h:263: static void DebugBreakForCommand(ClientData* data =
NULL);
You will end up processing a series of indistinguishable BreakForCommand events
if you call DebugBreakForCommand several times in row with NULL argument. But
probably that's fine.

http://codereview.chromium.org/2962007/diff/7001/11
File src/debug.cc (right):

http://codereview.chromium.org/2962007/diff/7001/11#newcode2234
src/debug.cc:2234: if (event == v8::Break) {
This command will be processed in NotifyMessageHandler above if there is a
message_handler_. I'd recommend you use separate queue that would store
DebugBreak requests.

[mnaganov (inactive), 2010-07-13 10:07:32]

http://codereview.chromium.org/2962007/diff/7001/9
File include/v8-debug.h (right):

http://codereview.chromium.org/2962007/diff/7001/9#newcode263
include/v8-debug.h:263: static void DebugBreakForCommand(ClientData* data =
NULL);
On 2010/07/13 09:50:42, Yury Semikhatsky wrote:
> You will end up processing a series of indistinguishable BreakForCommand
events
> if you call DebugBreakForCommand several times in row with NULL argument. But
> probably that's fine.

That may have sense to a client, so why not?

http://codereview.chromium.org/2962007/diff/7001/11
File src/debug.cc (right):

http://codereview.chromium.org/2962007/diff/7001/11#newcode2234
src/debug.cc:2234: if (event == v8::Break) {
On 2010/07/13 09:50:42, Yury Semikhatsky wrote:
> This command will be processed in NotifyMessageHandler above if there is a
> message_handler_. I'd recommend you use separate queue that would store
> DebugBreak requests.

OK, introduced a separate queue.

[yurys, 2010-07-13 10:38:22]

http://codereview.chromium.org/2962007/diff/12001/13003
File src/debug.cc (right):

http://codereview.chromium.org/2962007/diff/12001/13003#newcode2237
src/debug.cc:2237: !event_listener_.is_null() && event_listener_->IsProxy();
I don't see why you should ignore event_listener_ if it is not a proxy.

[mnaganov (inactive), 2010-07-13 12:52:27]

http://codereview.chromium.org/2962007/diff/12001/13003
File src/debug.cc (right):

http://codereview.chromium.org/2962007/diff/12001/13003#newcode2237
src/debug.cc:2237: !event_listener_.is_null() && event_listener_->IsProxy();
On 2010/07/13 10:38:22, Yury Semikhatsky wrote:
> I don't see why you should ignore event_listener_ if it is not a proxy.

I don't see a point in calling them, because passing a raw pointer to JS code
doesn't make much sense, and is cumbersome (for 64-bit case). But if you are
insisting on being consistent, here it is.

[yurys, 2010-07-13 13:03:26]

http://codereview.chromium.org/2962007/diff/17001/18003
File src/debug.cc (right):

http://codereview.chromium.org/2962007/diff/17001/18003#newcode2275
src/debug.cc:2275: Handle<Object> result = Execution::TryCall(fun,
Top::global(),
Please remove unused parameter.

[yurys, 2010-07-13 13:07:27]

I am not fond of DebugBreakForCommand implementation, I don't see how to
implement it in a more elegant way though.

[mnaganov (inactive), 2010-07-13 13:08:12]

http://codereview.chromium.org/2962007/diff/17001/18003
File src/debug.cc (right):

http://codereview.chromium.org/2962007/diff/17001/18003#newcode2275
src/debug.cc:2275: Handle<Object> result = Execution::TryCall(fun,
Top::global(),
On 2010/07/13 13:03:26, Yury Semikhatsky wrote:
> Please remove unused parameter.

Done.

[yurys, 2010-07-13 14:19:37]

Mads, can you take a look at this change?

[Mads Ager (chromium), 2010-07-13 17:07:31]

LGTM

http://codereview.chromium.org/2962007/diff/21001/22003
File src/debug.cc (right):

http://codereview.chromium.org/2962007/diff/21001/22003#newcode2250
src/debug.cc:2250: v8::Debug::EventCallback2 callback =
Hmm, is this a transition name? Are we getting close to getting rid of it?

[yurys, 2010-07-14 07:01:03]

http://codereview.chromium.org/2962007/diff/21001/22003
File src/debug.cc (right):

http://codereview.chromium.org/2962007/diff/21001/22003#newcode2250
src/debug.cc:2250: v8::Debug::EventCallback2 callback =
On 2010/07/13 17:07:31, Mads Ager wrote:
> Hmm, is this a transition name? Are we getting close to getting rid of it?

EventCallback2 and MessageHandler2 were meant to be transition names. But then
we were hesitant to remove them because it might break existing clients. We
probably should get rid of them now.