[debug] Untangle BreakLocation and DebugInfo

[debug] Untangle BreakLocation and DebugInfo

... because we want to use BreakLocation also for wasm, but we don't
instantiate a DebugInfo for wasm.

R=yangguo@chromium.org, titzer@chromium.org
BUG=v8:5822
Review-Url: https://codereview.chromium.org/2621953002
Cr-Commit-Position: refs/heads/master@{#42283}
Committed: https://chromium.googlesource.com/v8/v8/+/166ed9690144565367600c6b008a61c82aad36c6

[Clemens Hammacher, 2017-01-10 15:19:22]

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

[commit-bot: I haz the power, 2017-01-10 15:19:32]

Dry run: 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-10 15:26:42]

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

[commit-bot: I haz the power, 2017-01-10 15:26:43]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)

[Clemens Hammacher, 2017-01-10 17:24:49]

The red bots are already merge conflicts of earlier CLs. Will rebase everything
before committing.

I think I will stop uploading more CLs until the ones we have now have landed.
Is that fine for you?

[Clemens Hammacher, 2017-01-12 11:22:17]

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

[commit-bot: I haz the power, 2017-01-12 11:22:22]

Dry run: 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-12 11:54:46]

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

[commit-bot: I haz the power, 2017-01-12 11:54:46]

Dry run: This issue passed the CQ dry run.

[Yang, 2017-01-12 12:50:21]

lgtm.

https://codereview.chromium.org/2621953002/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2621953002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:516: JavaScriptFrame* js_frame =
JavaScriptFrame::cast(frame);
Currently frame has the type JavaScriptFrame*. I assume this is in preparation
of changing it to StandardFrame*?

Since wasm code doesn't have debug info, wouldn't it make more sense to have
this code (including EnsureDebugInfo) inside BreakLocation::FromFrame for the JS
frame case?

https://codereview.chromium.org/2621953002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:1012: auto summary =
FrameSummary::GetFirst(frame).AsJavaScript();
These few lines could be dropped if we already have them in
BreakLocation::FromFrame.

[Clemens Hammacher, 2017-01-12 14:11:38]

https://codereview.chromium.org/2621953002/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2621953002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:516: JavaScriptFrame* js_frame =
JavaScriptFrame::cast(frame);
On 2017/01/12 at 12:50:21, Yang wrote:
> Currently frame has the type JavaScriptFrame*. I assume this is in preparation
of changing it to StandardFrame*?

That's right. This will change in one of the following CLs.

> Since wasm code doesn't have debug info, wouldn't it make more sense to have
this code (including EnsureDebugInfo) inside BreakLocation::FromFrame for the JS
frame case?

That's not so easy, since BreakLocation::FromFrame returns a BreakLocation. We
would need to have a BreakLocation representing the error case.
Currently, we check for the error before, and don't even call
BreakLocation::FromFrame.

https://codereview.chromium.org/2621953002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:1012: auto summary =
FrameSummary::GetFirst(frame).AsJavaScript();
On 2017/01/12 at 12:50:21, Yang wrote:
> These few lines could be dropped if we already have them in
BreakLocation::FromFrame.

Same problem.

[Yang, 2017-01-12 14:18:19]

https://codereview.chromium.org/2621953002/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2621953002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:516: JavaScriptFrame* js_frame =
JavaScriptFrame::cast(frame);
On 2017/01/12 14:11:38, Clemens Hammacher wrote:
> On 2017/01/12 at 12:50:21, Yang wrote:
> > Currently frame has the type JavaScriptFrame*. I assume this is in
preparation
> of changing it to StandardFrame*?
> 
> That's right. This will change in one of the following CLs.
> 
> > Since wasm code doesn't have debug info, wouldn't it make more sense to have
> this code (including EnsureDebugInfo) inside BreakLocation::FromFrame for the
JS
> frame case?
> 
> That's not so easy, since BreakLocation::FromFrame returns a BreakLocation. We
> would need to have a BreakLocation representing the error case.
> Currently, we check for the error before, and don't even call
> BreakLocation::FromFrame.

right... but currently we do the check for is_java_script here and will do it
later in FromFrame as well. Might as well introduce a flag on BreakLocation to
indicate failure.

But I don't feel strongly about this so either way is fine.

[Clemens Hammacher, 2017-01-12 15:19:01]

On 2017/01/12 at 14:18:19, yangguo wrote:
> https://codereview.chromium.org/2621953002/diff/20001/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/2621953002/diff/20001/src/debug/debug.cc#newc...
> src/debug/debug.cc:516: JavaScriptFrame* js_frame =
JavaScriptFrame::cast(frame);
> On 2017/01/12 14:11:38, Clemens Hammacher wrote:
> > On 2017/01/12 at 12:50:21, Yang wrote:
> > > Currently frame has the type JavaScriptFrame*. I assume this is in
preparation
> > of changing it to StandardFrame*?
> > 
> > That's right. This will change in one of the following CLs.
> > 
> > > Since wasm code doesn't have debug info, wouldn't it make more sense to
have
> > this code (including EnsureDebugInfo) inside BreakLocation::FromFrame for
the JS
> > frame case?
> > 
> > That's not so easy, since BreakLocation::FromFrame returns a BreakLocation.
We
> > would need to have a BreakLocation representing the error case.
> > Currently, we check for the error before, and don't even call
> > BreakLocation::FromFrame.
> 
> right... but currently we do the check for is_java_script here and will do it
later in FromFrame as well. Might as well introduce a flag on BreakLocation to
indicate failure.
> 
> But I don't feel strongly about this so either way is fine.

I see the point. Having to check for "invalid" break locations, or passing a
bool* to the FromFrame methods feels even worse to me, so I will keep it this
way for now.

[Clemens Hammacher, 2017-01-12 18:15:25]

The CQ bit was checked by clemensh@chromium.org

[commit-bot: I haz the power, 2017-01-12 18:15:36]

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-12 18:31:29]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484244925598390,
"parent_rev": "ddbfbefc0b6482399121ac4afa41d8784d868332", "commit_rev":
"166ed9690144565367600c6b008a61c82aad36c6"}

[commit-bot: I haz the power, 2017-01-12 18:31:34]

Description was changed from

==========
[debug] Untangle BreakLocation and DebugInfo

... because we want to use BreakLocation also for wasm, but we don't
instantiate a DebugInfo for wasm.

R=yangguo@chromium.org, titzer@chromium.org
BUG=v8:5822
==========

to

==========
[debug] Untangle BreakLocation and DebugInfo

... because we want to use BreakLocation also for wasm, but we don't
instantiate a DebugInfo for wasm.

R=yangguo@chromium.org, titzer@chromium.org
BUG=v8:5822

Review-Url: https://codereview.chromium.org/2621953002
Cr-Commit-Position: refs/heads/master@{#42283}
Committed:
https://chromium.googlesource.com/v8/v8/+/166ed9690144565367600c6b008a61c82aa...
==========

[commit-bot: I haz the power, 2017-01-12 18:31:35]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/166ed9690144565367600c6b008a61c82aa...