Check that function being patched has no activations on any thread stack

Check that function being patched has no activations on any thread stack

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

[Søren Thygesen Gjesse, 2010-03-08 07:59:42]

LGTM, with some comments.

Regarding the stacks of other threads, see ThreadManager::Iterate in
v8threads.cc and Top::Iterate called from there, but leave that to a separate
change.

http://codereview.chromium.org/668246/diff/1001/13
File src/liveedit-delay.js (right):

http://codereview.chromium.org/668246/diff/1001/13#newcode257
src/liveedit-delay.js:257: // TODO: Need to check here that there are no
activations of the function
Ins't this comment obsolete?

http://codereview.chromium.org/668246/diff/1001/13#newcode330
src/liveedit-delay.js:330: // An object describing function compilation details.
Its index fields
Indentation of comment.

http://codereview.chromium.org/668246/diff/1001/13#newcode343
src/liveedit-delay.js:343: 
Indentation of comment.

http://codereview.chromium.org/668246/diff/1001/13#newcode412
src/liveedit-delay.js:412: 
Space after //.

http://codereview.chromium.org/668246/diff/1001/14
File src/runtime.cc (right):

http://codereview.chromium.org/668246/diff/1001/14#newcode8458
src/runtime.cc:8458: // For array of SharedFunctionInfo's (each wrapped in
JSValue)
I find the return stated for this function a bit strange. Both undefined and '0'
are considered 'false' values. Also the same code can be on the stack in
different threads. If you want to have some kind of enumeration use some
constants.

  static const int kLiveEditFunctionNotActivated = 0
  static const int kLiveEditFunctionActivatedOnThisThreadOnly = 1
  static const int kLiveEditFunctionActivatedOnOtherThreadsOnly = 2
  static const int ...

and add these to liveedit-delay.js

(like the kFrameDetailsXXX constants).

http://codereview.chromium.org/668246/diff/1001/16
File test/mjsunit/debug-liveedit-check-stack.js (right):

http://codereview.chromium.org/668246/diff/1001/16#newcode74
test/mjsunit/debug-liveedit-check-stack.js:74: assertEquals(true,
got_exception);
How about adding modifying the function when it is not activated any more to see
that it can now be changed.

[Peter Rybin, 2010-03-09 20:10:43]

http://codereview.chromium.org/668246/diff/1001/13
File src/liveedit-delay.js (right):

http://codereview.chromium.org/668246/diff/1001/13#newcode257
src/liveedit-delay.js:257: // TODO: Need to check here that there are no
activations of the function
On 2010/03/08 07:59:42, Søren Gjesse wrote:
> Ins't this comment obsolete?

Thanks
Done

http://codereview.chromium.org/668246/diff/1001/13#newcode330
src/liveedit-delay.js:330: // An object describing function compilation details.
Its index fields
On 2010/03/08 07:59:42, Søren Gjesse wrote:
> Indentation of comment.

Done.

http://codereview.chromium.org/668246/diff/1001/13#newcode343
src/liveedit-delay.js:343: 
On 2010/03/08 07:59:42, Søren Gjesse wrote:
> Indentation of comment.

Done.

http://codereview.chromium.org/668246/diff/1001/13#newcode412
src/liveedit-delay.js:412: 
On 2010/03/08 07:59:42, Søren Gjesse wrote:
> Space after //.

Done.

http://codereview.chromium.org/668246/diff/1001/14
File src/runtime.cc (right):

http://codereview.chromium.org/668246/diff/1001/14#newcode8458
src/runtime.cc:8458: // For array of SharedFunctionInfo's (each wrapped in
JSValue)
On 2010/03/08 07:59:42, Søren Gjesse wrote:
> I find the return stated for this function a bit strange. Both undefined and
'0'
> are considered 'false' values. Also the same code can be on the stack in
> different threads. If you want to have some kind of enumeration use some
> constants.
> 
>   static const int kLiveEditFunctionNotActivated = 0
>   static const int kLiveEditFunctionActivatedOnThisThreadOnly = 1
>   static const int kLiveEditFunctionActivatedOnOtherThreadsOnly = 2
>   static const int ...
> 
> and add these to liveedit-delay.js
> 
> (like the kFrameDetailsXXX constants).

I created explicit enum (in C++) and corresponding object in JS. Is it ok?

http://codereview.chromium.org/668246/diff/1001/16
File test/mjsunit/debug-liveedit-check-stack.js (right):

http://codereview.chromium.org/668246/diff/1001/16#newcode74
test/mjsunit/debug-liveedit-check-stack.js:74: assertEquals(true,
got_exception);
On 2010/03/08 07:59:42, Søren Gjesse wrote:
> How about adding modifying the function when it is not activated any more to
see
> that it can now be changed.

Oh, good idea.
Done.