Support for precise stepping in functions compiled before debugging was started (step 2)

Support for precise stepping in functions compiled before debugging was started (step 2)

This change will ensure that full code with debug break slots is compiled and activated for all functions which already have activation frames.

This additional handling is only for functions which have activations on the stack, and that activation is of the full code compiled without debug break slots. In that case the full code is recompiled with debug break slots. It is ensured that the full code is compiled generating the exact same instructions - except for the additional debug break slots - as before. The return address on the stack is then patched to continue execution in the new code.

Also fixed SortedListBSearch to actually use the passed comparision function.

R=svenpanne@chromium.org, kmillikin@chromium.org

BUG=
TEST=

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

[Søren Thygesen Gjesse, 2011-09-29 08:19:59]

Friendly ping...

[Kevin Millikin (Chromium), 2011-09-29 10:47:29]

LGTM with a couple of suggestions.

http://codereview.chromium.org/8050010/diff/2001/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/8050010/diff/2001/src/compiler.cc#newcode314
src/compiler.cc:314: if (info->CompilingForDebugging()) {
You could piggyback on the existing test so the control flow through here
doesn't get too complicated conditions don't get too complicated:

return V8::UseCrankshaft() && !info->CompilingForDebugging()
    ? MakeCrankshaftCode(info)
    : FullCodeGenerator::MakeCode(info);

or 

return info->CompilingForDebugging() || !V8::UseCrankshaft()
    ? FullCodeGenerator::MakeCode(info)
    : MakeCrankshaftCode(info);

http://codereview.chromium.org/8050010/diff/2001/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/8050010/diff/2001/src/compiler.h#newcode250
src/compiler.h:250: bool compiling_for_debugging_;
There's a bit field "flags_" in this class.  I think this bit (as well as
supports_deoptimization_) should be packed in there.

Conveniently, the flags all default to false.

http://codereview.chromium.org/8050010/diff/2001/src/debug.cc
File src/debug.cc (right):

http://codereview.chromium.org/8050010/diff/2001/src/debug.cc#newcode1734
src/debug.cc:1734: static int HandleObjectPointerCompare(const Handle<T>* a,
const Handle<T>* b) {
Hmm.  There are a couple of other Compare functions in utils.h.  I don't think
this can go there, because of include dependencies, but maybe it should go in
handles.h with a comment in utils.h that it's in handles.h, and a comment in
handles.h that it's like the ones in utils.h but moved because of include
dependencies.

I'm worried someone else will just define the same function, because debug.cc is
a strange place to look for it.  Or maybe that's not a big deal.  What do you
think?

http://codereview.chromium.org/8050010/diff/2001/src/debug.cc#newcode1782
src/debug.cc:1782: Handle<Code> lazy_compile(
Handle<Code> lazy_compile = isolate_->builtins()->LazyCompile();

http://codereview.chromium.org/8050010/diff/2001/src/debug.cc#newcode1813
src/debug.cc:1813: while (((obj = iterator.next()) != NULL)) {
Maybe it doesn't matter, but should you avoid scanning map, code, and data
spaces?

[Søren Thygesen Gjesse, 2011-09-30 08:33:22]

http://codereview.chromium.org/8050010/diff/2001/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/8050010/diff/2001/src/compiler.cc#newcode314
src/compiler.cc:314: if (info->CompilingForDebugging()) {
On 2011/09/29 10:47:35, Kevin Millikin wrote:
> You could piggyback on the existing test so the control flow through here
> doesn't get too complicated conditions don't get too complicated:
> 
> return V8::UseCrankshaft() && !info->CompilingForDebugging()
>     ? MakeCrankshaftCode(info)
>     : FullCodeGenerator::MakeCode(info);
> 
> or 
> 
> return info->CompilingForDebugging() || !V8::UseCrankshaft()
>     ? FullCodeGenerator::MakeCode(info)
>     : MakeCrankshaftCode(info);

Done.

http://codereview.chromium.org/8050010/diff/2001/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/8050010/diff/2001/src/compiler.h#newcode250
src/compiler.h:250: bool compiling_for_debugging_;
On 2011/09/29 10:47:35, Kevin Millikin wrote:
> There's a bit field "flags_" in this class.  I think this bit (as well as
> supports_deoptimization_) should be packed in there.
> 
> Conveniently, the flags all default to false.

Good point - changed.

http://codereview.chromium.org/8050010/diff/2001/src/debug.cc
File src/debug.cc (right):

http://codereview.chromium.org/8050010/diff/2001/src/debug.cc#newcode1734
src/debug.cc:1734: static int HandleObjectPointerCompare(const Handle<T>* a,
const Handle<T>* b) {
On 2011/09/29 10:47:35, Kevin Millikin wrote:
> Hmm.  There are a couple of other Compare functions in utils.h.  I don't think
> this can go there, because of include dependencies, but maybe it should go in
> handles.h with a comment in utils.h that it's in handles.h, and a comment in
> handles.h that it's like the ones in utils.h but moved because of include
> dependencies.
> 
> I'm worried someone else will just define the same function, because debug.cc
is
> a strange place to look for it.  Or maybe that's not a big deal.  What do you
> think?

Good point. I moved it to utils.h together with a forward declaration of Handle.

http://codereview.chromium.org/8050010/diff/2001/src/debug.cc#newcode1782
src/debug.cc:1782: Handle<Code> lazy_compile(
On 2011/09/29 10:47:35, Kevin Millikin wrote:
> Handle<Code> lazy_compile = isolate_->builtins()->LazyCompile();

Done, actually

Handle<Code> lazy_compile = Handle<Code>(isolate_->builtins()->LazyCompile());

http://codereview.chromium.org/8050010/diff/2001/src/debug.cc#newcode1813
src/debug.cc:1813: while (((obj = iterator.next()) != NULL)) {
On 2011/09/29 10:47:35, Kevin Millikin wrote:
> Maybe it doesn't matter, but should you avoid scanning map, code, and data
> spaces?

Currently there is no space filtering in the heap iterator. I will prepare one
as a separate change.