Handle thread preemption in debugger

All preemption requests are now ignored while in the debugger. This ensures that no change of V8 thread happenes  while in the debugger. The only thing that happens is that a flag is set to indicate that preemption happened. When the debugger is left preemption is requested if it occourred while in the debugger.

Moved the debugger related global variables from Top to thread local in Debug.

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

[Søren Thygesen Gjesse, 2009-03-04 15:36:31]

Updated to handle this entirely in the V8 thread being debugged as discussed
off-line.

[Søren Thygesen Gjesse, 2009-03-05 07:32:34]

Moved the debugger entry and preemption pending flag from a global variable to a
thread local.

[Erik Corry, 2009-03-05 09:42:51]

LGTM

http://codereview.chromium.org/39124/diff/19/1003
File src/debug.h (right):

http://codereview.chromium.org/39124/diff/19/1003#newcode361
Line 361: // Preemption happened while debugging
full stop

http://codereview.chromium.org/39124/diff/19/1003#newcode553
Line 553: // Make sure that premption does not occour while handling the break
point.
Spolling in too places.

http://codereview.chromium.org/39124/diff/19/1003#newcode559
Line 559: }
Seems like this part is not needed.  If a preemption is pending then it will hit
immediately JS is resumed and it will set the preemption pending flag in
execution.cc.

[Søren Thygesen Gjesse, 2009-03-05 16:11:23]

http://codereview.chromium.org/39124/diff/19/1003
File src/debug.h (right):

http://codereview.chromium.org/39124/diff/19/1003#newcode361
Line 361: // Preemption happened while debugging
On 2009/03/05 09:42:51, Erik Corry wrote:
> full stop

Done.

http://codereview.chromium.org/39124/diff/19/1003#newcode553
Line 553: // Make sure that premption does not occour while handling the break
point.
On 2009/03/05 09:42:51, Erik Corry wrote:
> Spolling in too places.

Done.

http://codereview.chromium.org/39124/diff/19/1003#newcode559
Line 559: }
On 2009/03/05 09:42:51, Erik Corry wrote:
> Seems like this part is not needed.  If a preemption is pending then it will
hit
> immediately JS is resumed and it will set the preemption pending flag in
> execution.cc.

Without this I have seen some severe starvation when having two threads running
with preemption and one of them having a break point

[Søren Thygesen Gjesse, 2009-03-05 16:19:48]

Updated the CL to move the debugger related global variables from Top to thread
local in Debug.

Ivan, please take another look.

On 2009/03/05 16:11:23, Søren Gjesse wrote:
> http://codereview.chromium.org/39124/diff/19/1003
> File src/debug.h (right):
> 
> http://codereview.chromium.org/39124/diff/19/1003#newcode361
> Line 361: // Preemption happened while debugging
> On 2009/03/05 09:42:51, Erik Corry wrote:
> > full stop
> 
> Done.
> 
> http://codereview.chromium.org/39124/diff/19/1003#newcode553
> Line 553: // Make sure that premption does not occour while handling the break
> point.
> On 2009/03/05 09:42:51, Erik Corry wrote:
> > Spolling in too places.
> 
> Done.
> 
> http://codereview.chromium.org/39124/diff/19/1003#newcode559
> Line 559: }
> On 2009/03/05 09:42:51, Erik Corry wrote:
> > Seems like this part is not needed.  If a preemption is pending then it will
> hit
> > immediately JS is resumed and it will set the preemption pending flag in
> > execution.cc.
> 
> Without this I have seen some severe starvation when having two threads
running
> with preemption and one of them having a break point

[iposva, 2009-03-06 09:25:35]

At the moment I cannot find any threading related issues with the code anymore.
Some small comments below, otherwise LGTM.

-Ivan

http://codereview.chromium.org/39124/diff/1015/30
File src/debug.h (right):

http://codereview.chromium.org/39124/diff/1015/30#newcode574
Line 574: // If a preemption is pending clear it and store this information.
Comment does not match with the code.
Why do you need to test if prev_ is NULL?
You are not recording the pending preemption in this case. Did you miss to
uncomment your setting of the preemption bit after testing? Otherwise if you do
not intend to set it, then you should remove the commented code.

http://codereview.chromium.org/39124/diff/1015/30#newcode607
Line 607: if (prev_ == NULL && Debug::preemption_pending()) {
Please add a comment why you test for prev_ being NULL. For example:

// Request preemption when leaving the last debugger entry and a preemption had
been recorded while debugging.

http://codereview.chromium.org/39124/diff/1015/32
File src/execution.cc (right):

http://codereview.chromium.org/39124/diff/1015/32#newcode527
Line 527: if (!Debug::InDebugger()) {
I would probably turn this around to not get the NOT in the test for readability
and then the comment also is closer to the described action.

http://codereview.chromium.org/39124/diff/1015/33
File test/cctest/test-debug.cc (right):

http://codereview.chromium.org/39124/diff/1015/33#newcode53
Line 53: using ::v8::internal::DeleteArray;
Why did you have to add these?

[Søren Thygesen Gjesse, 2009-03-06 09:52:36]

http://codereview.chromium.org/39124/diff/1015/30
File src/debug.h (right):

http://codereview.chromium.org/39124/diff/1015/30#newcode574
Line 574: // If a preemption is pending clear it and store this information.
On 2009/03/06 09:25:35, iposva wrote:
> Comment does not match with the code.
> Why do you need to test if prev_ is NULL?
> You are not recording the pending preemption in this case. Did you miss to
> uncomment your setting of the preemption bit after testing? Otherwise if you
do
> not intend to set it, then you should remove the commented code.
> 
> 

Sorry, I forgot to un-comment code after doing some testing. Without storing the
preemption state when the debugger is first entered and restoring it when it is
left I can get starvation in some debugging scenarios. Extended the comment with
this information.

http://codereview.chromium.org/39124/diff/1015/30#newcode607
Line 607: if (prev_ == NULL && Debug::preemption_pending()) {
On 2009/03/06 09:25:35, iposva wrote:
> Please add a comment why you test for prev_ being NULL. For example:
> 
> // Request preemption when leaving the last debugger entry and a preemption
had
> been recorded while debugging. 

Clarified commet.

http://codereview.chromium.org/39124/diff/1015/32
File src/execution.cc (right):

http://codereview.chromium.org/39124/diff/1015/32#newcode527
Line 527: if (!Debug::InDebugger()) {
On 2009/03/06 09:25:35, iposva wrote:
> I would probably turn this around to not get the NOT in the test for
readability
> and then the comment also is closer to the described action.

Done.

http://codereview.chromium.org/39124/diff/1015/33
File test/cctest/test-debug.cc (right):

http://codereview.chromium.org/39124/diff/1015/33#newcode53
Line 53: using ::v8::internal::DeleteArray;
On 2009/03/06 09:25:35, iposva wrote:
> Why did you have to add these?

Removed - left over from temporary test cases.