Hi Mads
Thank you very much for review. Maybe I should have explained the idea
better before.
User has edited a function while standing on break. The function was
activated on stack in frames #11 and #15 (in both frames function entered
'with' statement). Those activations must be cancelled, i.e. frames #js_top
- #14 should be zeroed out. Frame #15 should become 'restarted': manually
set up as if the function was stopped on break in the very beginning.
It is important that when this stack manipulation finished, debugger remains
in the same break state (with updated backtrace of course) and user sees the
cursor in the first line of his function.
I cannot restart the function to its actual breakpoint (seems too
complicated, I'm setting up the stack frame manually), so I add a new state
'function restarted' and a supporting code patch, that only needs 2 words on
stack -- pointers to function and context. The rest of the stack may be
zeroed out. However, the context pointer on stack doesn't necessarily point
to a function context: 'with' instruction may have updated it. For
restarting function we need the function context back.
The main thing is that it should be a consistent state for debugger. User
stays on break as long as he wants, exploring the stack, evaluating
expressions and doing more live editing, and all this time restarted
function is on stack.
*I think* that the only thing we have to do is to make sure that stack
analyzer understands that restarted function cannot have locals. To make it
certain I patched ScopeInfo itself, making up the alternative state of JS
function scope: "not started" ('state' is an allusion to scope depending on
function execution point (state) in other languages). Though in many points
you have to deal with it (locals reading, evaluation etc), it is very cheap
-- to check whether the frame is 'restarted' costs comparing 2 pointers.
This design I chose over several others, I thought it the most elegant. I
don't know how faulty or naive it is in real.
I thought that ScopeInfo patch may be not too expensive. However, I can try
to find some alternative approach if needed.
Peter
2010/7/15 <[email protected]>
> 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?
>
>
> http://codereview.chromium.org/2943002/show
>
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
