LiveEdit: implement frame dropping

LiveEdit: implement frame dropping

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

[Peter Rybin, 2010-03-23 17:47:57]

Hi Soren

This is my approach to the taks.

It does not support try/catches yet and I haven't added stubs to all
architectures so far.

Peter

[Søren Thygesen Gjesse, 2010-03-25 13:13:51]

Here are some initial comments.

Just to make sure I understand, you are "restarting" the function on the top
frame after frames are dropped, right? Another option will of-cause be returning
from the last frame dropped, but of cause that would require some kind of
artificial return value.

http://codereview.chromium.org/1118007/diff/8001/9002
File src/builtins.h (right):

http://codereview.chromium.org/1118007/diff/8001/9002#newcode130
src/builtins.h:130: V(NoOp_LiveEditHelper,        BUILTIN, DEBUG_BREAK)         
\
The NoOp does do a return, how about calling it PlainReturn or something like
that?

Also change LiveEditHelper to just LiveEdit.

http://codereview.chromium.org/1118007/diff/8001/9004
File src/debug.cc (right):

http://codereview.chromium.org/1118007/diff/8001/9004#newcode900
src/debug.cc:900: Code* empty_code =
Builtins::builtin(Builtins::NoOp_LiveEditHelper);
empty_code -> plain_return_code?

http://codereview.chromium.org/1118007/diff/8001/9006
File src/frames.cc (right):

http://codereview.chromium.org/1118007/diff/8001/9006#newcode784
src/frames.cc:784: // Lined list element that helps building array of elements.
Lined -> Linked

http://codereview.chromium.org/1118007/diff/8001/9006#newcode792
src/frames.cc:792: #define DEFINE_WRAPPER(type, field) class field##_Wrapper :
public ZoneObject {\
Could you format this code to look a bit more like a normal class declaration?

http://codereview.chromium.org/1118007/diff/8001/9006#newcode827
src/frames.cc:827: 
Why do you build a linked list first and then add the elements to a list
afterwards? Why not just build the list in the first place?

http://codereview.chromium.org/1118007/diff/8001/9006#newcode829
src/frames.cc:829: for (current_marker = first_marker;
Indentation.

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

http://codereview.chromium.org/1118007/diff/8001/9010#newcode701
src/liveedit.cc:701: static const char* DropFrames(Vector<StackFrame*> frames,
Indentation of arguments.

http://codereview.chromium.org/1118007/diff/8001/9010#newcode737
src/liveedit.cc:737: for (Address a = unused_stack_top;
Allignment.

http://codereview.chromium.org/1118007/diff/8001/9010#newcode757
src/liveedit.cc:757: int top_frame_i = -1;
top_frame_i -> top_frame_index

http://codereview.chromium.org/1118007/diff/8001/9010#newcode770
src/liveedit.cc:770: 
Could you add some more comments to what all the different checks guards
against? Some of the variable names provide good indication to the check, but
e.g. interesting_activations_found leaves me somewhat in the dark.

[Peter Rybin, 2010-03-27 21:17:07]

Hi Søren

I think that's it from my side. I added unit-tests and I put stubs into non-ia32
architectures.

Here I have a small problem: on non-ia32 on of my tests fails (because of
stubs). Do you have an advice how I can handle this. I guess we don't like
failing tests in a codebase. Looks like I have to block my test on other
architectures somehow.

Peter

http://codereview.chromium.org/1118007/diff/8001/9002
File src/builtins.h (right):

http://codereview.chromium.org/1118007/diff/8001/9002#newcode130
src/builtins.h:130: V(NoOp_LiveEditHelper,        BUILTIN, DEBUG_BREAK)         
\
On 2010/03/25 13:13:51, Søren Gjesse wrote:
> The NoOp does do a return, how about calling it PlainReturn or something like
> that?
> 
> Also change LiveEditHelper to just LiveEdit.

Done.

http://codereview.chromium.org/1118007/diff/8001/9004
File src/debug.cc (right):

http://codereview.chromium.org/1118007/diff/8001/9004#newcode900
src/debug.cc:900: Code* empty_code =
Builtins::builtin(Builtins::NoOp_LiveEditHelper);
On 2010/03/25 13:13:51, Søren Gjesse wrote:
> empty_code -> plain_return_code?

Done.

http://codereview.chromium.org/1118007/diff/8001/9006
File src/frames.cc (right):

http://codereview.chromium.org/1118007/diff/8001/9006#newcode784
src/frames.cc:784: // Lined list element that helps building array of elements.
On 2010/03/25 13:13:51, Søren Gjesse wrote:
> Lined -> Linked

Done.

http://codereview.chromium.org/1118007/diff/8001/9006#newcode792
src/frames.cc:792: #define DEFINE_WRAPPER(type, field) class field##_Wrapper :
public ZoneObject {\
On 2010/03/25 13:13:51, Søren Gjesse wrote:
> Could you format this code to look a bit more like a normal class declaration?

Done.

http://codereview.chromium.org/1118007/diff/8001/9006#newcode827
src/frames.cc:827: 
On 2010/03/25 13:13:51, Søren Gjesse wrote:
> Why do you build a linked list first and then add the elements to a list
> afterwards? Why not just build the list in the first place?

Didn't want to take responsibility for specifying an initial list capacity :)

http://codereview.chromium.org/1118007/diff/8001/9006#newcode829
src/frames.cc:829: for (current_marker = first_marker;
On 2010/03/25 13:13:51, Søren Gjesse wrote:
> Indentation.

Done.

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

http://codereview.chromium.org/1118007/diff/8001/9010#newcode701
src/liveedit.cc:701: static const char* DropFrames(Vector<StackFrame*> frames,
On 2010/03/25 13:13:51, Søren Gjesse wrote:
> Indentation of arguments.

Done.

http://codereview.chromium.org/1118007/diff/8001/9010#newcode737
src/liveedit.cc:737: for (Address a = unused_stack_top;
On 2010/03/25 13:13:51, Søren Gjesse wrote:
> Allignment.

It doesn't fit in one line.
I think I saw exactly such alignment in our codebase.

http://codereview.chromium.org/1118007/diff/8001/9010#newcode757
src/liveedit.cc:757: int top_frame_i = -1;
On 2010/03/25 13:13:51, Søren Gjesse wrote:
> top_frame_i -> top_frame_index

Done.

http://codereview.chromium.org/1118007/diff/8001/9010#newcode770
src/liveedit.cc:770: 
On 2010/03/25 13:13:51, Søren Gjesse wrote:
> Could you add some more comments to what all the different checks guards
> against? Some of the variable names provide good indication to the check, but
> e.g. interesting_activations_found leaves me somewhat in the dark.

Done.

[Søren Thygesen Gjesse, 2010-04-06 09:41:36]

LGTM

You can use test/mjsunit/mjsunit.status to have architecture specific test
expectations.