Debugger: prepare code for debugging on a per-function basis.

Debugger: prepare code for debugging on a per-function basis.

Prior to this patch, we enter a global debug mode whenever a break point
is set. By entering this mode, all code is deoptimized and activated
frames are recompiled and redirected to newly compiled debug code.

After this patch, we only deoptimize/redirect for functions we want to
debug. Trigger for this is Debug::EnsureDebugInfo, and having DebugInfo
object attached to the SFI prevents optimization/inlining.

The result is that we can have optimized code for functions without break
points alongside functions that do have break points, which are not
optimized.

R=mstarzinger@chromium.org, ulan@chromium.org
BUG=v8:4132
LOG=Y

Committed: https://crrev.com/35c28ce0a742e58346d2dea009428cacd442040d
Cr-Commit-Position: refs/heads/master@{#29758}

[Yang, 2015-07-20 10:47:59]

Michael, please review the changes to compiler.cc, crankshaft and turbofan (wrt
inlining).

Ulan, please review the changes to the debugger and code flushing.

[Yang, 2015-07-20 10:48:50]

The CQ bit was checked by yangguo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-20 10:48:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233073005/1

[commit-bot: I haz the power, 2015-07-20 11:13:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-20 11:13:58]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2015-07-20 11:29:40]

Looking good, just minor comments.

https://codereview.chromium.org/1233073005/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1233073005/diff/1/src/compiler.cc#newcode84
src/compiler.cc:84: explicit CompilationInfoWithZone(Handle<SharedFunctionInfo>
shared)
See comment below.

https://codereview.chromium.org/1233073005/diff/1/src/compiler.cc#newcode1001
src/compiler.cc:1001: CompilationInfoWithZone info(shared);
There is a constant back-and-forth refactoring around CompilationInfoWithZone.
Can we make the following lines explicit here to avoid adding a new constructor:

    Zone zone;
    ParseInfo parse_info(&zone, shared);
    CompilationInfo info(&parse_info);

https://codereview.chromium.org/1233073005/diff/1/src/compiler.cc#newcode1394
src/compiler.cc:1394: allow_lazy &= !LiveEditFunctionTracker::IsActive(isolate);
nit: Can we combine this into one big conjunction instead?

https://codereview.chromium.org/1233073005/diff/1/src/compiler.cc#newcode1464
src/compiler.cc:1464: DCHECK(info.is_debug() ? !existing->HasDebugCode()
nit: Not sure exactly what this assertion is guarding against, I would be fine
with dropping it entirely.

https://codereview.chromium.org/1233073005/diff/1/src/compiler.cc#newcode1560
src/compiler.cc:1560: DCHECK(!shared->HasDebugInfo());
What happens of a concurrent compile job for a function has been enqueued and
the debugger sets a breakpoint inside that function while it's on the queue.
Does the debugger flush the queue somehow? Or is there another mechanism in
place to prevent this?

https://codereview.chromium.org/1233073005/diff/1/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1233073005/diff/1/src/compiler.h#newcode630
src/compiler.h:630: MUST_USE_RESULT static MaybeHandle<Code>
CompileForDebugging(
What do you think about sticking with GetDebugCode as the name for both of these
function, to keep it in sync with the rest of the API methods?

https://codereview.chromium.org/1233073005/diff/1/src/compiler/js-inlining.cc
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/1233073005/diff/1/src/compiler/js-inlining.cc...
src/compiler/js-inlining.cc:254: return NoChange();
Please add a tracing line ...

TRACE("Not inlining %s into %s because callee contains breakpoints\n", ...)

[Michael Starzinger, 2015-07-20 11:33:41]

https://codereview.chromium.org/1233073005/diff/1/src/heap/objects-visiting-i...
File src/heap/objects-visiting-inl.h (right):

https://codereview.chromium.org/1233073005/diff/1/src/heap/objects-visiting-i...
src/heap/objects-visiting-inl.h:663: if (shared_info->HasDebugInfo()) {
Ouch, this is scary! Is this a hard requirement? If it is then we need to make
double-sure that flushing decisions are revisited whenever this predicate
changes. Otherwise the SharedFunctionInfo might be enqueued as a flushing
candidate already when the debugger adds breakpoints.

[Michael Starzinger, 2015-07-20 11:46:09]

https://codereview.chromium.org/1233073005/diff/1/src/debug.cc
File src/debug.cc (left):

https://codereview.chromium.org/1233073005/diff/1/src/debug.cc#oldcode1626
src/debug.cc:1626: heap->CollectAllGarbage(Heap::kMakeHeapIterableMask,
As discussed offline: We might need to still abort incremental marking
explicitly, to disable code flushing.

[Yang, 2015-07-20 12:13:47]

https://codereview.chromium.org/1233073005/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1233073005/diff/1/src/compiler.cc#newcode84
src/compiler.cc:84: explicit CompilationInfoWithZone(Handle<SharedFunctionInfo>
shared)
On 2015/07/20 11:29:40, Michael Starzinger wrote:
> See comment below.

Acknowledged.

https://codereview.chromium.org/1233073005/diff/1/src/compiler.cc#newcode1001
src/compiler.cc:1001: CompilationInfoWithZone info(shared);
On 2015/07/20 11:29:39, Michael Starzinger wrote:
> There is a constant back-and-forth refactoring around CompilationInfoWithZone.
> Can we make the following lines explicit here to avoid adding a new
constructor:
> 
>     Zone zone;
>     ParseInfo parse_info(&zone, shared);
>     CompilationInfo info(&parse_info);

Done.

https://codereview.chromium.org/1233073005/diff/1/src/compiler.cc#newcode1394
src/compiler.cc:1394: allow_lazy &= !LiveEditFunctionTracker::IsActive(isolate);
On 2015/07/20 11:29:39, Michael Starzinger wrote:
> nit: Can we combine this into one big conjunction instead?

Done.

https://codereview.chromium.org/1233073005/diff/1/src/compiler.cc#newcode1464
src/compiler.cc:1464: DCHECK(info.is_debug() ? !existing->HasDebugCode()
On 2015/07/20 11:29:39, Michael Starzinger wrote:
> nit: Not sure exactly what this assertion is guarding against, I would be fine
> with dropping it entirely.

This is supposed to guard against replacing existing debug code with a new copy.
In that case we would lose break points, as the new code is not patched.

https://codereview.chromium.org/1233073005/diff/1/src/compiler.cc#newcode1560
src/compiler.cc:1560: DCHECK(!shared->HasDebugInfo());
On 2015/07/20 11:29:39, Michael Starzinger wrote:
> What happens of a concurrent compile job for a function has been enqueued and
> the debugger sets a breakpoint inside that function while it's on the queue.
> Does the debugger flush the queue somehow? Or is there another mechanism in
> place to prevent this?

Done. We flush the queue when preparing the function for break points.

https://codereview.chromium.org/1233073005/diff/1/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1233073005/diff/1/src/compiler.h#newcode630
src/compiler.h:630: MUST_USE_RESULT static MaybeHandle<Code>
CompileForDebugging(
On 2015/07/20 11:29:40, Michael Starzinger wrote:
> What do you think about sticking with GetDebugCode as the name for both of
these
> function, to keep it in sync with the rest of the API methods?

Done.

https://codereview.chromium.org/1233073005/diff/1/src/compiler/js-inlining.cc
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/1233073005/diff/1/src/compiler/js-inlining.cc...
src/compiler/js-inlining.cc:254: return NoChange();
On 2015/07/20 11:29:40, Michael Starzinger wrote:
> Please add a tracing line ...
> 
> TRACE("Not inlining %s into %s because callee contains breakpoints\n", ...)

Done.

https://codereview.chromium.org/1233073005/diff/1/src/debug.cc
File src/debug.cc (left):

https://codereview.chromium.org/1233073005/diff/1/src/debug.cc#oldcode1626
src/debug.cc:1626: heap->CollectAllGarbage(Heap::kMakeHeapIterableMask,
On 2015/07/20 11:46:09, Michael Starzinger wrote:
> As discussed offline: We might need to still abort incremental marking
> explicitly, to disable code flushing.

Done.

https://codereview.chromium.org/1233073005/diff/1/src/heap/objects-visiting-i...
File src/heap/objects-visiting-inl.h (right):

https://codereview.chromium.org/1233073005/diff/1/src/heap/objects-visiting-i...
src/heap/objects-visiting-inl.h:663: if (shared_info->HasDebugInfo()) {
On 2015/07/20 11:33:41, Michael Starzinger wrote:
> Ouch, this is scary! Is this a hard requirement? If it is then we need to make
> double-sure that flushing decisions are revisited whenever this predicate
> changes. Otherwise the SharedFunctionInfo might be enqueued as a flushing
> candidate already when the debugger adds breakpoints.

Done.

[Michael Starzinger, 2015-07-20 12:29:28]

LGTM on compiler and code flushing. Didn't look at the rest.

[ulan, 2015-07-20 13:22:47]

debugger parts lgtm

https://codereview.chromium.org/1233073005/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (left):

https://codereview.chromium.org/1233073005/diff/20001/test/cctest/test-api.cc...
test/cctest/test-api.cc:20990:
TEST(StreamingWithDebuggingDoesNotProduceParserCache) {
Why is this removed?

[Yang, 2015-07-20 14:25:00]

https://codereview.chromium.org/1233073005/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (left):

https://codereview.chromium.org/1233073005/diff/20001/test/cctest/test-api.cc...
test/cctest/test-api.cc:20990:
TEST(StreamingWithDebuggingDoesNotProduceParserCache) {
On 2015/07/20 13:22:47, ulan wrote:
> Why is this removed?

I thought about this. It seems like I have to restore the old logic here and
also this test.

The current logic is still a bit wrong: if we parse first, then enable the
debugger, then we would have parse data that does not work for compiling for
debugging. But that's an existing issue.

I'll fix that in a follow-up CL.

[Yang, 2015-07-20 14:25:11]

The CQ bit was checked by yangguo@chromium.org

[Yang, 2015-07-20 14:25:12]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1233073005/#ps40001
(title: "addressed comments.")

[commit-bot: I haz the power, 2015-07-20 14:25:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233073005/40001

[commit-bot: I haz the power, 2015-07-20 14:53:33]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-20 14:53:42]

Patchset 3 (id:??) landed as
https://crrev.com/35c28ce0a742e58346d2dea009428cacd442040d
Cr-Commit-Position: refs/heads/master@{#29758}