Restore behavior of PrepareForBreakpoints which was broken by r21145

Restore behavior of PrepareForBreakpoints which was broken by r21145.

Rename Debug::MaybeRecompileFunctionForDebugging to
EnsureFunctionHasDebugBreakSlots and ensure that it does
nothing if the function is unoptimized code with debug
break slots, otherwise, if the shared code has no
debug break slots, it recompile that shared code and
sets the function code to that shared code.

Also removes two incorrect ASSERTs.

R=yangguo@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21201

[rmcilroy, 2014-05-08 14:18:00]

As discussed, PTAL.  Thanks!

[Yang, 2014-05-08 14:28:08]

Not LGTM just yet.

https://codereview.chromium.org/271873003/diff/1/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/271873003/diff/1/src/debug.cc#newcode2041
src/debug.cc:2041: if (function->code()->kind() != Code::FUNCTION ||
Like I said in the reland CL, this is incorrect. In this function we want to
make sure that the function code is unoptimized with debug break slots. If the
code type is optimized code or builtin, we want to replace that by recompiling
it.
We can only skip this if the code is already unoptimized with debug break slots.
So it should be

if (function->code()->kind() == Code::FUNCTION &&
function->code()->has_debug_break_slots())

[rmcilroy, 2014-05-08 14:53:59]

https://codereview.chromium.org/271873003/diff/1/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/271873003/diff/1/src/debug.cc#newcode2041
src/debug.cc:2041: if (function->code()->kind() != Code::FUNCTION ||
On 2014/05/08 14:28:09, Yang wrote:
> Like I said in the reland CL, this is incorrect. In this function we want to
> make sure that the function code is unoptimized with debug break slots. If the
> code type is optimized code or builtin, we want to replace that by recompiling
> it.
> We can only skip this if the code is already unoptimized with debug break
slots.
> So it should be
> 
> if (function->code()->kind() == Code::FUNCTION &&
> function->code()->has_debug_break_slots())

Sorry I missed that comment.  This doesn't work - I get the following ASSERT
failure with that change:

# Fatal error in ../src/compiler.cc, line 717
# CHECK(!old_code->has_debug_break_slots()) failed

==== C stack trace ===============================
v8::internal::Compiler::GetCodeForDebugging(v8::internal::Handle<v8::internal::JSFunction>)
 3:
v8::internal::Debug::MaybeRecompileFunctionForDebugging(v8::internal::Handle<v8::internal::JSFunction>)

Should it be:
if (function->shared()->code()->kind() == Code::FUNCTION && 
    function->shared()->code()->has_debug_break_slots()) return;

instead? That seems to work.

[Yang, 2014-05-08 15:12:38]

On 2014/05/08 14:53:59, rmcilroy wrote:
> https://codereview.chromium.org/271873003/diff/1/src/debug.cc
> File src/debug.cc (right):
> 
> https://codereview.chromium.org/271873003/diff/1/src/debug.cc#newcode2041
> src/debug.cc:2041: if (function->code()->kind() != Code::FUNCTION ||
> On 2014/05/08 14:28:09, Yang wrote:
> > Like I said in the reland CL, this is incorrect. In this function we want to
> > make sure that the function code is unoptimized with debug break slots. If
the
> > code type is optimized code or builtin, we want to replace that by
recompiling
> > it.
> > We can only skip this if the code is already unoptimized with debug break
> slots.
> > So it should be
> > 
> > if (function->code()->kind() == Code::FUNCTION &&
> > function->code()->has_debug_break_slots())
> 
> Sorry I missed that comment.  This doesn't work - I get the following ASSERT
> failure with that change:
> 
> # Fatal error in ../src/compiler.cc, line 717
> # CHECK(!old_code->has_debug_break_slots()) failed
> 
> ==== C stack trace ===============================
>
v8::internal::Compiler::GetCodeForDebugging(v8::internal::Handle<v8::internal::JSFunction>)
>  3:
>
v8::internal::Debug::MaybeRecompileFunctionForDebugging(v8::internal::Handle<v8::internal::JSFunction>)
> 
> Should it be:
> if (function->shared()->code()->kind() == Code::FUNCTION && 
>     function->shared()->code()->has_debug_break_slots()) return;
> 
> instead? That seems to work.

That's not it completely either. I was not entirely correct, we should simply
revert to the behavior in
https://codereview.chromium.org/260423002/diff/40001/src/debug.cc when
MaybeRecompileFunctionForDebugging was introduced:

if the code is already unoptimized with debug break slots, don't do anything,
skip to next function in the loop.
if the shared code has no debug break slots, recompile that shared code.
set the function code to that shared code, since that shared code now has debug
break slots in any case.

[rmcilroy, 2014-05-08 16:29:28]

On 2014/05/08 15:12:38, Yang wrote:
> On 2014/05/08 14:53:59, rmcilroy wrote:
> > https://codereview.chromium.org/271873003/diff/1/src/debug.cc
> > File src/debug.cc (right):
> > 
> > https://codereview.chromium.org/271873003/diff/1/src/debug.cc#newcode2041
> > src/debug.cc:2041: if (function->code()->kind() != Code::FUNCTION ||
> > On 2014/05/08 14:28:09, Yang wrote:
> > > Like I said in the reland CL, this is incorrect. In this function we want
to
> > > make sure that the function code is unoptimized with debug break slots. If
> the
> > > code type is optimized code or builtin, we want to replace that by
> recompiling
> > > it.
> > > We can only skip this if the code is already unoptimized with debug break
> > slots.
> > > So it should be
> > > 
> > > if (function->code()->kind() == Code::FUNCTION &&
> > > function->code()->has_debug_break_slots())
> > 
> > Sorry I missed that comment.  This doesn't work - I get the following ASSERT
> > failure with that change:
> > 
> > # Fatal error in ../src/compiler.cc, line 717
> > # CHECK(!old_code->has_debug_break_slots()) failed
> > 
> > ==== C stack trace ===============================
> >
>
v8::internal::Compiler::GetCodeForDebugging(v8::internal::Handle<v8::internal::JSFunction>)
> >  3:
> >
>
v8::internal::Debug::MaybeRecompileFunctionForDebugging(v8::internal::Handle<v8::internal::JSFunction>)
> > 
> > Should it be:
> > if (function->shared()->code()->kind() == Code::FUNCTION && 
> >     function->shared()->code()->has_debug_break_slots()) return;
> > 
> > instead? That seems to work.
> 
> That's not it completely either. I was not entirely correct, we should simply
> revert to the behavior in
> https://codereview.chromium.org/260423002/diff/40001/src/debug.cc when
> MaybeRecompileFunctionForDebugging was introduced:
> 
> if the code is already unoptimized with debug break slots, don't do anything,
> skip to next function in the loop.
> if the shared code has no debug break slots, recompile that shared code.
> set the function code to that shared code, since that shared code now has
debug
> break slots in any case.

Ok this makes sense.  Updated to do this (hopefully) and added some comments.

[Yang, 2014-05-08 16:36:48]

https://codereview.chromium.org/271873003/diff/40001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/271873003/diff/40001/src/debug.cc#newcode2057
src/debug.cc:2057: }
You need to replace the function code by the shared code in any case. Imagine a
function that has a code obj that has no debug break slots, but the shared code
has them. We want the function to use that shared code.

[rmcilroy, 2014-05-08 16:57:16]

https://codereview.chromium.org/271873003/diff/40001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/271873003/diff/40001/src/debug.cc#newcode2057
src/debug.cc:2057: }
On 2014/05/08 16:36:48, Yang wrote:
> You need to replace the function code by the shared code in any case. Imagine
a
> function that has a code obj that has no debug break slots, but the shared
code
> has them. We want the function to use that shared code.

Done.

[Yang, 2014-05-08 17:00:39]

On 2014/05/08 16:57:17, rmcilroy wrote:

Lgtm. But I think it's safer to use ReplaceCode instead of set_code.

[rmcilroy, 2014-05-08 17:00:45]

https://codereview.chromium.org/271873003/diff/60001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/271873003/diff/60001/src/debug.cc#newcode2067
src/debug.cc:2067: EnsureFunctionHasDebugBreakSlots(fun);
Should there also be checks here to just continue if function->shared() is
toplevel or !allow_lazy_compilation like lines 2223-2224 below, or is this not
possible for generators (I assume they can't be builtins)?

[Yang, 2014-05-08 17:03:49]

https://codereview.chromium.org/271873003/diff/60001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/271873003/diff/60001/src/debug.cc#newcode2067
src/debug.cc:2067: EnsureFunctionHasDebugBreakSlots(fun);
On 2014/05/08 17:00:45, rmcilroy wrote:
> Should there also be checks here to just continue if function->shared() is
> toplevel or !allow_lazy_compilation like lines 2223-2224 below, or is this not
> possible for generators (I assume they can't be builtins)?  

Not sure about lazy compilation. I'll do some research tomorrow.

[rmcilroy, 2014-05-08 17:45:37]

> Lgtm. But I think it's safer to use ReplaceCode instead of set_code.

Done

https://codereview.chromium.org/271873003/diff/60001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/271873003/diff/60001/src/debug.cc#newcode2067
src/debug.cc:2067: EnsureFunctionHasDebugBreakSlots(fun);
On 2014/05/08 17:03:49, Yang wrote:
> On 2014/05/08 17:00:45, rmcilroy wrote:
> > Should there also be checks here to just continue if function->shared() is
> > toplevel or !allow_lazy_compilation like lines 2223-2224 below, or is this
not
> > possible for generators (I assume they can't be builtins)?  
> 
> Not sure about lazy compilation. I'll do some research tomorrow.

OK, I'll land as-is now, and leave investigation of whether generators and
lazy_compilation interact to you.

[rmcilroy, 2014-05-08 18:00:34]

Committed patchset #5 manually as r21201 (tree was closed).

[wingo, 2014-05-12 07:54:30]

On 2014/05/08 16:36:48, Yang wrote:
> https://codereview.chromium.org/271873003/diff/40001/src/debug.cc
> File src/debug.cc (right):
> 
> https://codereview.chromium.org/271873003/diff/40001/src/debug.cc#newcode2057
> src/debug.cc:2057: }
> You need to replace the function code by the shared code in any case. Imagine
a
> function that has a code obj that has no debug break slots, but the shared
code
> has them. We want the function to use that shared code.

How could this be the case?  Why would the function's code not be the same as
the shared code?

[Yang, 2014-05-12 07:57:09]

On 2014/05/12 07:54:30, wingo wrote:
> On 2014/05/08 16:36:48, Yang wrote:
> > https://codereview.chromium.org/271873003/diff/40001/src/debug.cc
> > File src/debug.cc (right):
> > 
> >
https://codereview.chromium.org/271873003/diff/40001/src/debug.cc#newcode2057
> > src/debug.cc:2057: }
> > You need to replace the function code by the shared code in any case.
Imagine
> a
> > function that has a code obj that has no debug break slots, but the shared
> code
> > has them. We want the function to use that shared code.
> 
> How could this be the case?  Why would the function's code not be the same as
> the shared code?

If two closures share the same function literal, they have the same shared
function info, but are two different JSFunction objects. Now if both are on the
stack, we run into the situation where
- we look at the first closure. No debug break slots. Recompile so that both the
closure and the shared function info have the new debug code.
- we look at the second closure. No debug break slots. But the shared function
info already has it. No need to recompile.

[wingo, 2014-05-12 08:02:49]

https://codereview.chromium.org/271873003/diff/60001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/271873003/diff/60001/src/debug.cc#newcode2067
src/debug.cc:2067: EnsureFunctionHasDebugBreakSlots(fun);
On 2014/05/08 17:45:37, rmcilroy wrote:
> On 2014/05/08 17:03:49, Yang wrote:
> > On 2014/05/08 17:00:45, rmcilroy wrote:
> > > Should there also be checks here to just continue if function->shared() is
> > > toplevel or !allow_lazy_compilation like lines 2223-2224 below, or is this
> not
> > > possible for generators (I assume they can't be builtins)?  
> > 
> > Not sure about lazy compilation. I'll do some research tomorrow.
> 
> OK, I'll land as-is now, and leave investigation of whether generators and
> lazy_compilation interact to you.

Some test cases would have been nice here.  I don't think the condition
corresponding to this problem is exercised in the test suite.

Regarding generators -- it isn't possible to have a toplevel generator.  I think
however it is possible to have a generator that doesn't allow lazy compilation,
if I understand scopes.cc:AllowLazyCompilation() correctly.