Reimplement stack manipulations for LiveEdit

Reimplement stack manipulations for LiveEdit

Overview:
When function gets updated with a new code, it may stay activated on stack and have to be restarted. The code of the function is additionally patched with a 'restarter' code snippet and PC is set there.

All scope analyzers must realize that there are no local variables in this state.

[Peter Rybin, 2010-07-13 00:45:19]

Hi Mads

Can I ask you to take a look at this CL. Soren is out of office same as Vitaly.

This change modifies ScopeInfo, but is should behave exactly the same until
liveedit is actually used.

I am disposing old stack manipulation code and replace it with a better one. It
remove 1+ frames, but the bottom (js) frame gets restarted. I add a small patch
to the code of function and reset the scope on stack. Hope that's enough.

Peter

[Mads Ager (chromium), 2010-07-15 13:09:04]

Initial set of comments. I don't understand the messing with the context of the
restarted function. Also, I don't like changing the scopeinfo for this. Comments
on these below.

http://codereview.chromium.org/2943002/diff/8001/9012
File src/ia32/debug-ia32.cc (right):

http://codereview.chromium.org/2943002/diff/8001/9012#newcode275
src/ia32/debug-ia32.cc:275: // However height the frame was, reset the stack
pointer
However height the frame was -> Whatever the height of the frame was

http://codereview.chromium.org/2943002/diff/8001/9013
File src/liveedit-debugger.js (right):

http://codereview.chromium.org/2943002/diff/8001/9013#newcode899
src/liveedit-debugger.js:899: // Returns StackInfoWrapper for a future stack
manipulations or
for a -> for

http://codereview.chromium.org/2943002/diff/8001/9014
File src/liveedit.cc (right):

http://codereview.chromium.org/2943002/diff/8001/9014#newcode594
src/liveedit.cc:594: static Handle<JSArray> CreateSuccess(int
bottom_function_index,
Please add an empty lines between these methods for readability.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode661
src/liveedit.cc:661: // TODO(LiveEdit): Move private method below.
Comment should be indented.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode856
src/liveedit.cc:856: SharedFunctionInfo* expected_function);
Indentation.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode916
src/liveedit.cc:916: Object* LiveEdit::ReplaceFunctionCode(Handle<JSArray>
new_compile_info_array,
Move this parameter to next line as well when using four space indent for the
arguments.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode1329
src/liveedit.cc:1329: // If function was in some nested scope (e.g. 'with'),
reset it back to
Why would you want to do that? That makes no sense to me. The function is still
in the same context.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode1357
src/liveedit.cc:1357: int top_frame_index,
Indentation.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode1443
src/liveedit.cc:1443: ResetFrameScope(bottom_js_frame);
I do not understand why you mess with the scope of the function here? Could you
please explain?

http://codereview.chromium.org/2943002/diff/8001/9014#newcode1456
src/liveedit.cc:1456: a < unused_stack_bottom;
Indentation.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode1542
src/liveedit.cc:1542: SharedFunctionInfo* expected_function) {
Indentation.

http://codereview.chromium.org/2943002/diff/8001/9015
File src/liveedit.h (right):

http://codereview.chromium.org/2943002/diff/8001/9015#newcode89
src/liveedit.h:89: // in result the function gets restarted.
in result -> as a result

http://codereview.chromium.org/2943002/diff/8001/9015#newcode175
src/liveedit.h:175: #endif /* V*_LIVEEDIT_H_ */
V* -> V8? ;)

http://codereview.chromium.org/2943002/diff/8001/9018
File src/runtime.cc (right):

http://codereview.chromium.org/2943002/diff/8001/9018#newcode8493
src/runtime.cc:8493: FunctionScopeState scope_state =
LiveEdit::IsAtFrameResetPatch(it.frame())
I don't understand why we need to deal with this. Can't we make sure that it is
impossible to have a break point in the restart patch? That way you don't have
to mess with the scope info because at all breakpoints you will have valid scope
info?

http://codereview.chromium.org/2943002/diff/8001/9018#newcode10052
src/runtime.cc:10052: stack_drop_data_temp, stack_drop_data_wrapped->value());
Could you use one line per argument or move 'stack_drop_data_temp' to previous
line?

http://codereview.chromium.org/2943002/diff/8001/9021
File src/scopeinfo.h (right):

http://codereview.chromium.org/2943002/diff/8001/9021#newcode49
src/scopeinfo.h:49: class ScopeInfoLiveEditHack {};
Where do you use this? Please remove.

I would prefer that live edit does not end up messing with scope info at all.
Can we make sure that the debugger cannot break until we are in a consistent
state after restarting?

[Peter Rybin, 2010-07-15 18:51:57]

http://codereview.chromium.org/2943002/diff/8001/9012
File src/ia32/debug-ia32.cc (right):

http://codereview.chromium.org/2943002/diff/8001/9012#newcode275
src/ia32/debug-ia32.cc:275: // However height the frame was, reset the stack
pointer
On 2010/07/15 13:09:04, Mads Ager wrote:
> However height the frame was -> Whatever the height of the frame was

Done.

http://codereview.chromium.org/2943002/diff/8001/9013
File src/liveedit-debugger.js (right):

http://codereview.chromium.org/2943002/diff/8001/9013#newcode899
src/liveedit-debugger.js:899: // Returns StackInfoWrapper for a future stack
manipulations or
On 2010/07/15 13:09:04, Mads Ager wrote:
> for a -> for

Done.

http://codereview.chromium.org/2943002/diff/8001/9014
File src/liveedit.cc (right):

http://codereview.chromium.org/2943002/diff/8001/9014#newcode594
src/liveedit.cc:594: static Handle<JSArray> CreateSuccess(int
bottom_function_index,
On 2010/07/15 13:09:04, Mads Ager wrote:
> Please add an empty lines between these methods for readability.

Done.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode661
src/liveedit.cc:661: // TODO(LiveEdit): Move private method below.
On 2010/07/15 13:09:04, Mads Ager wrote:
> Comment should be indented.

Done.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode856
src/liveedit.cc:856: SharedFunctionInfo* expected_function);
On 2010/07/15 13:09:04, Mads Ager wrote:
> Indentation.

Done.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode916
src/liveedit.cc:916: Object* LiveEdit::ReplaceFunctionCode(Handle<JSArray>
new_compile_info_array,
On 2010/07/15 13:09:04, Mads Ager wrote:
> Move this parameter to next line as well when using four space indent for the
> arguments.

Done.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode1329
src/liveedit.cc:1329: // If function was in some nested scope (e.g. 'with'),
reset it back to
On 2010/07/15 13:09:04, Mads Ager wrote:
> Why would you want to do that? That makes no sense to me. The function is
still
> in the same context.

As I understand that pointer to context (ebp - 1) gets temporary updated when
function enters info 'with' statement. Now that we are restarting the frame, we
need the original context both for running and (I think) for stack analyzer.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode1357
src/liveedit.cc:1357: int top_frame_index,
On 2010/07/15 13:09:04, Mads Ager wrote:
> Indentation.

Done.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode1443
src/liveedit.cc:1443: ResetFrameScope(bottom_js_frame);
On 2010/07/15 13:09:04, Mads Ager wrote:
> I do not understand why you mess with the scope of the function here? Could
you
> please explain?

See above.
When we are restarting function we literally make (deferred) jump to its entry.
We load context/function registers from stack. As I understand the stack slot
with context gets temporary updated by 'with' instruction.

We break 'with' forever. We revert its context change here (and we drop
invisible try/finally block of 'with' in the previous line).

http://codereview.chromium.org/2943002/diff/8001/9014#newcode1456
src/liveedit.cc:1456: a < unused_stack_bottom;
On 2010/07/15 13:09:04, Mads Ager wrote:
> Indentation.

Done.

http://codereview.chromium.org/2943002/diff/8001/9014#newcode1542
src/liveedit.cc:1542: SharedFunctionInfo* expected_function) {
On 2010/07/15 13:09:04, Mads Ager wrote:
> Indentation.

Done.

http://codereview.chromium.org/2943002/diff/8001/9015
File src/liveedit.h (right):

http://codereview.chromium.org/2943002/diff/8001/9015#newcode89
src/liveedit.h:89: // in result the function gets restarted.
On 2010/07/15 13:09:04, Mads Ager wrote:
> in result -> as a result

Done.

http://codereview.chromium.org/2943002/diff/8001/9015#newcode175
src/liveedit.h:175: #endif /* V*_LIVEEDIT_H_ */
On 2010/07/15 13:09:04, Mads Ager wrote:
> V* -> V8? ;)

Thanks! I had been doing '8' in caps. Could type it with 3rd try this time --
fingers think themselves :)

http://codereview.chromium.org/2943002/diff/8001/9018
File src/runtime.cc (right):

http://codereview.chromium.org/2943002/diff/8001/9018#newcode8493
src/runtime.cc:8493: FunctionScopeState scope_state =
LiveEdit::IsAtFrameResetPatch(it.frame())
On 2010/07/15 13:09:04, Mads Ager wrote:
> I don't understand why we need to deal with this. Can't we make sure that it
is
> impossible to have a break point in the restart patch? That way you don't have
> to mess with the scope info because at all breakpoints you will have valid
scope
> info?

In this design the restarted frame may stay on stack for a long time (user all
this time stays on break).

If this is a real pain there could be some alternatives but they seem to promise
much more hacks to debug framework.

http://codereview.chromium.org/2943002/diff/8001/9018#newcode10052
src/runtime.cc:10052: stack_drop_data_temp, stack_drop_data_wrapped->value());
On 2010/07/15 13:09:04, Mads Ager wrote:
> Could you use one line per argument or move 'stack_drop_data_temp' to previous
> line?

Sure :)
That was bad code evolution.

http://codereview.chromium.org/2943002/diff/8001/9021
File src/scopeinfo.h (right):

http://codereview.chromium.org/2943002/diff/8001/9021#newcode49
src/scopeinfo.h:49: class ScopeInfoLiveEditHack {};
> Where do you use this? Please remove.

Sorry for forgetting it here (it was a marker type I used to track all ScopeInfo
usages).

> I would prefer that live edit does not end up messing with scope info at all.
> Can we make sure that the debugger cannot break until we are in a consistent
> state after restarting?

I'm sorry that LiveEdit got that deep.
This way I am trying to make 'restarted function' as a semi-consistent/legal
state. The changes seems very light and the checks in runtime.cc are very cheap.

I'm not sure I do like the alternative: temporary return to user JavaScript and
making sure control gets back. I think it promises much more hacks.

[Mads Ager (chromium), 2010-07-16 09:32:53]

Thanks for your explanations! :)

I still don't like the debugger ever seeing this temporary state. We should be
able to generate some sort of one-shot breakpoint in the beginning of the
function and then restart the function. When we hit the one-shot breakpoint in
the beginning of the function we have all the scope information and can remove
the one-shot breakpoint. I think this will be a better experience for the user
as well. The stack will be changed and the function restarted but he will be
able to see everything as expected. 

I don't like changing the scope info to deal with this artificial state (not
worried about performance, but we really shouldn't have to look at scope info in
this state at all). We should get out of that state immediately instead and find
a way to have the debugger break immediately on entry to the restarted function.

http://codereview.chromium.org/2943002/diff/8001/9014
File src/liveedit.cc (right):

http://codereview.chromium.org/2943002/diff/8001/9014#newcode1329
src/liveedit.cc:1329: // If function was in some nested scope (e.g. 'with'),
reset it back to
On 2010/07/15 18:51:57, Peter Rybin wrote:
> On 2010/07/15 13:09:04, Mads Ager wrote:
> > Why would you want to do that? That makes no sense to me. The function is
> still
> > in the same context.
> 
> As I understand that pointer to context (ebp - 1) gets temporary updated when
> function enters info 'with' statement. Now that we are restarting the frame,
we
> need the original context both for running and (I think) for stack analyzer.

Ah! Sorry about that, I misread this as messing with the context of a function.
Not with the context in the frame. I understand what you are doing now, thanks.