[inspector] ignores debugger; in blackboxed code

[inspector] ignores debugger; in blackboxed code

Current state and proposed changes for blackboxing: https://docs.google.com/document/d/1hnzaXPAN8_QC5ENxIgxgMNDbXLraM_OXT73rAyijTF8/edit?usp=sharing

BUG=v8:5842, chromium:583193
R=yangguo@chromium.org,dgozman@chromium.org,alph@chromium.org

Review-Url: https://codereview.chromium.org/2648353006
Cr-Commit-Position: refs/heads/master@{#42668}
Committed: https://chromium.googlesource.com/v8/v8/+/c8691efb38242c5481428d63bc16fddee73879ac

[kozy, 2017-01-25 03:10:30]

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

[commit-bot: I haz the power, 2017-01-25 03:10:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kozy, 2017-01-25 03:11:03]

Please take a look!

[commit-bot: I haz the power, 2017-01-25 04:08:55]

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

[commit-bot: I haz the power, 2017-01-25 04:08:56]

Dry run: This issue passed the CQ dry run.

[Yang, 2017-01-25 06:03:52]

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

https://codereview.chromium.org/2648353006/diff/1/src/debug/debug.cc#newcode2142
src/debug/debug.cc:2142: if (IsBlackboxed(JSFunction::cast(fun)->shared())) {
Why do we need to deoptimize the function? We don't do this for regular
optimized functions that have a debugger statement. Simply returning should be
fine imo.

[Yang, 2017-01-25 06:05:02]

On 2017/01/25 06:03:52, Yang wrote:
> https://codereview.chromium.org/2648353006/diff/1/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/2648353006/diff/1/src/debug/debug.cc#newcode2142
> src/debug/debug.cc:2142: if (IsBlackboxed(JSFunction::cast(fun)->shared())) {
> Why do we need to deoptimize the function? We don't do this for regular
> optimized functions that have a debugger statement. Simply returning should be
> fine imo.

Just to clarify: we do need to deoptimize for stepping and setting break points,
since we cannot do that in optimized code. But debugger statement is fine.

[kozy, 2017-01-25 06:10:44]

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

https://codereview.chromium.org/2648353006/diff/1/src/debug/debug.cc#newcode2142
src/debug/debug.cc:2142: if (IsBlackboxed(JSFunction::cast(fun)->shared())) {
On 2017/01/25 06:03:52, Yang wrote:
> Why do we need to deoptimize the function? We don't do this for regular
> optimized functions that have a debugger statement. Simply returning should be
> fine imo.

I think when we schedule pause on next statement for async break points, we can
enter to this function on function call and we would like to deoptimize this
function if it's blackboxed to correctly break on enter to inner function.
As proof: one of inspector test about async breakpoints are flaky without this
code.

[kozy, 2017-01-25 07:02:51]

Patchset #2 (id:20001) has been deleted

[kozy, 2017-01-25 07:03:13]

Added a comment and todo after discussion.

[Yang, 2017-01-25 07:29:05]

On 2017/01/25 07:03:13, kozy wrote:
> Added a comment and todo after discussion.

lgtm.

[kozy, 2017-01-25 07:31:22]

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

[commit-bot: I haz the power, 2017-01-25 07:31:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yang, 2017-01-25 08:32:34]

lgtm

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

https://codereview.chromium.org/2648353006/diff/1/src/debug/debug.cc#newcode2142
src/debug/debug.cc:2142: if (IsBlackboxed(JSFunction::cast(fun)->shared())) {
On 2017/01/25 06:10:44, kozy wrote:
> On 2017/01/25 06:03:52, Yang wrote:
> > Why do we need to deoptimize the function? We don't do this for regular
> > optimized functions that have a debugger statement. Simply returning should
be
> > fine imo.
> 
> I think when we schedule pause on next statement for async break points, we
can
> enter to this function on function call and we would like to deoptimize this
> function if it's blackboxed to correctly break on enter to inner function.
> As proof: one of inspector test about async breakpoints are flaky without this
> code.

As discussed, please also deopt the function for the case of not subject to
debugging. And add a comment for why this is necessary, with a TODO to remove
once we have break-on-entry.

[Yang, 2017-01-25 08:32:49]

On 2017/01/25 08:32:34, Yang wrote:
> lgtm
> 
> https://codereview.chromium.org/2648353006/diff/1/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/2648353006/diff/1/src/debug/debug.cc#newcode2142
> src/debug/debug.cc:2142: if (IsBlackboxed(JSFunction::cast(fun)->shared())) {
> On 2017/01/25 06:10:44, kozy wrote:
> > On 2017/01/25 06:03:52, Yang wrote:
> > > Why do we need to deoptimize the function? We don't do this for regular
> > > optimized functions that have a debugger statement. Simply returning
should
> be
> > > fine imo.
> > 
> > I think when we schedule pause on next statement for async break points, we
> can
> > enter to this function on function call and we would like to deoptimize this
> > function if it's blackboxed to correctly break on enter to inner function.
> > As proof: one of inspector test about async breakpoints are flaky without
this
> > code.
> 
> As discussed, please also deopt the function for the case of not subject to
> debugging. And add a comment for why this is necessary, with a TODO to remove
> once we have break-on-entry.

ignore last comment. :)

[commit-bot: I haz the power, 2017-01-25 08:37:02]

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

[commit-bot: I haz the power, 2017-01-25 08:37:03]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-01-25 23:28:20]

The CQ bit was checked by kozyatinskiy@chromium.org

[commit-bot: I haz the power, 2017-01-25 23:28:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 23:30:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485386900169180,
"parent_rev": "6d42c4504a2522e235975a0189c0bd90b6fde47f", "commit_rev":
"c8691efb38242c5481428d63bc16fddee73879ac"}

[commit-bot: I haz the power, 2017-01-25 23:30:45]

Description was changed from

==========
[inspector] ignores debugger; in blackboxed code

Current state and proposed changes for blackboxing:
https://docs.google.com/document/d/1hnzaXPAN8_QC5ENxIgxgMNDbXLraM_OXT73rAyijT...

BUG=v8:5842,chromium:583193
R=yangguo@chromium.org,dgozman@chromium.org,alph@chromium.org
==========

to

==========
[inspector] ignores debugger; in blackboxed code

Current state and proposed changes for blackboxing:
https://docs.google.com/document/d/1hnzaXPAN8_QC5ENxIgxgMNDbXLraM_OXT73rAyijT...

BUG=v8:5842,chromium:583193
R=yangguo@chromium.org,dgozman@chromium.org,alph@chromium.org

Review-Url: https://codereview.chromium.org/2648353006
Cr-Commit-Position: refs/heads/master@{#42668}
Committed:
https://chromium.googlesource.com/v8/v8/+/c8691efb38242c5481428d63bc16fddee73...
==========

[commit-bot: I haz the power, 2017-01-25 23:30:47]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/c8691efb38242c5481428d63bc16fddee73...