[inspector] Expose GetPossibleBreakpoints for wasm

[inspector] Expose GetPossibleBreakpoints for wasm

and add an inspector test for it. Also test that a breakpoint can be
set on each reported position, and that it is also hit during
execution.
This CL also fixes two little things which were uncovered by the new
test.

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

[Clemens Hammacher, 2017-01-24 20:36:32]

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

[commit-bot: I haz the power, 2017-01-24 20:36:43]

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-24 21:17:56]

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

[commit-bot: I haz the power, 2017-01-24 21:17:57]

Dry run: This issue passed the CQ dry run.

[Yang, 2017-01-25 10:46:33]

On 2017/01/24 21:19:24, Clemens Hammacher wrote:

lgtm, but I haven't given the inspector part close attention :)

[Clemens Hammacher, 2017-01-25 12:09:24]

On 2017/01/25 at 10:46:33, yangguo wrote:
> On 2017/01/24 21:19:24, Clemens Hammacher wrote:
> 
> lgtm, but I haven't given the inspector part close attention :)

Alexey, can you PTAL?

[kozy, 2017-01-25 18:34:19]

https://codereview.chromium.org/2655653003/diff/1/src/inspector/wasm-translat...
File src/inspector/wasm-translation.cc (right):

https://codereview.chromium.org/2655653003/diff/1/src/inspector/wasm-translat...
src/inspector/wasm-translation.cc:268: new TranslatorImpl::RawTranslator(this,
script);
Who will free this memory?

https://codereview.chromium.org/2655653003/diff/1/src/inspector/wasm-translat...
src/inspector/wasm-translation.cc:274: default:
It's redundant and will silence compiler warning about not covered enum values.

https://codereview.chromium.org/2655653003/diff/1/test/inspector/debugger/was...
File test/inspector/debugger/wasm-get-breakable-locations.js (right):

https://codereview.chromium.org/2655653003/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-get-breakable-locations.js:105: if (!message.result
|| !message.result.scriptSource) {
Why do you need this method? If you have only valid scriptIds then this method
always should return valid result that you can just use as msg =>
msg.result.scriptSource.

https://codereview.chromium.org/2655653003/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-get-breakable-locations.js:134: .then(() =>
Protocol.Debugger.getPossibleBreakpoints({
Could we add following test:
getPossibleBreakpoints(wasmScript)
execute(wasmScript)
and check that all possible paused occurred?

[Clemens Hammacher, 2017-01-25 20:22:05]

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

[commit-bot: I haz the power, 2017-01-25 20:22:13]

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

[Clemens Hammacher, 2017-01-25 20:23:29]

Description was changed from

==========
[inspector] Expose GetPossibleBreakpoints for wasm

and add an inspector test for it.

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

to

==========
[inspector] Expose GetPossibleBreakpoints for wasm

and add an inspector test for it. Also test that a breakpoint can be
set on each reported position, and that it is also hit during
execution.
This CL also fixes two little things which were uncovered by the new
test.

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

[Clemens Hammacher, 2017-01-25 20:23:54]

https://codereview.chromium.org/2655653003/diff/1/src/inspector/wasm-translat...
File src/inspector/wasm-translation.cc (right):

https://codereview.chromium.org/2655653003/diff/1/src/inspector/wasm-translat...
src/inspector/wasm-translation.cc:268: new TranslatorImpl::RawTranslator(this,
script);
On 2017/01/25 at 18:34:19, kozy wrote:
> Who will free this memory?

As documented above: the WasmTranslation takes ownership by putting them in
wasm_translators_.
The problem is that the constructor already triggers generation of the fake
scripts, which in turn triggers setting breakpoints. So the TranslatorImpl needs
to be registered right at the beginning of the constructor.
Alternatively, we could split it into a constructor and a init function, which
then creates the fake scripts. I thought about both options and decided for the
one implemented.

https://codereview.chromium.org/2655653003/diff/1/src/inspector/wasm-translat...
src/inspector/wasm-translation.cc:274: default:
On 2017/01/25 at 18:34:19, kozy wrote:
> It's redundant and will silence compiler warning about not covered enum
values.

Fixed.

https://codereview.chromium.org/2655653003/diff/1/test/inspector/debugger/was...
File test/inspector/debugger/wasm-get-breakable-locations.js (right):

https://codereview.chromium.org/2655653003/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-get-breakable-locations.js:105: if (!message.result
|| !message.result.scriptSource) {
On 2017/01/25 at 18:34:19, kozy wrote:
> Why do you need this method? If you have only valid scriptIds then this method
always should return valid result that you can just use as msg =>
msg.result.scriptSource.

I thought it helps debugging in the error case. Removed it.

https://codereview.chromium.org/2655653003/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-get-breakable-locations.js:134: .then(() =>
Protocol.Debugger.getPossibleBreakpoints({
On 2017/01/25 at 18:34:19, kozy wrote:
> Could we add following test:
> getPossibleBreakpoints(wasmScript)
> execute(wasmScript)
> and check that all possible paused occurred?

Fair enough :)
Added this test, moving over two fixes from the other CL
http://crrev.com/2651043004.

[commit-bot: I haz the power, 2017-01-25 21:12:45]

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

[commit-bot: I haz the power, 2017-01-25 21:12:47]

Dry run: This issue passed the CQ dry run.

[dgozman, 2017-01-26 06:24:34]

dgozman@chromium.org changed reviewers:
+ dgozman@chromium.org

[dgozman, 2017-01-26 06:24:34]

https://codereview.chromium.org/2655653003/diff/20001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2655653003/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:483: translatedBreakpoint,
&translatedBreakpoint.line_number,
drive-by: it doesn't look safe to pass a part of input parameter as an output
parameter.

https://codereview.chromium.org/2655653003/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:488:
m_debugger->wasmTranslation()->TranslateWasmScriptLocationToProtocolLocation(
drive-by: why don't we pass ScriptBreakpoint* around so these methods can modify
it directly?

[Clemens Hammacher, 2017-01-26 09:33:13]

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

[commit-bot: I haz the power, 2017-01-26 09:33:22]

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

[Clemens Hammacher, 2017-01-26 09:57:48]

Alexey or Dmitry, can one of you please review the inspector changes again?

https://codereview.chromium.org/2655653003/diff/20001/src/inspector/v8-debugg...
File src/inspector/v8-debugger-agent-impl.cc (right):

https://codereview.chromium.org/2655653003/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:483: translatedBreakpoint,
&translatedBreakpoint.line_number,
On 2017/01/26 at 06:24:34, dgozman wrote:
> drive-by: it doesn't look safe to pass a part of input parameter as an output
parameter.

Yes, it looks weird. I checked that it works, but anyway undid this change now.
Two stack variables don't cost us too much ;)

https://codereview.chromium.org/2655653003/diff/20001/src/inspector/v8-debugg...
src/inspector/v8-debugger-agent-impl.cc:488:
m_debugger->wasmTranslation()->TranslateWasmScriptLocationToProtocolLocation(
On 2017/01/26 at 06:24:34, dgozman wrote:
> drive-by: why don't we pass ScriptBreakpoint* around so these methods can
modify it directly?

It's not only ScriptBreakpoint object which need to be translated. It's also
Locations and StackFrames. All of them have a script id, line and column though,
that's why the interface is modeled this way.

[commit-bot: I haz the power, 2017-01-26 10:09:55]

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

[commit-bot: I haz the power, 2017-01-26 10:09:56]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-01-26 20:58:16]

lgtm % comment

https://codereview.chromium.org/2655653003/diff/40001/src/inspector/wasm-tran...
File src/inspector/wasm-translation.cc (right):

https://codereview.chromium.org/2655653003/diff/40001/src/inspector/wasm-tran...
src/inspector/wasm-translation.cc:265: // the fake scripts, which triggers
setting breakpoints.
When base class manage memory of derived class by putting them into third class
it doesn't sound right to me..

Could we just put std::unique_ptr<TranslatorImpl> intro base class as member and
create required by checking mode?

[Clemens Hammacher, 2017-01-26 22:07:37]

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

[commit-bot: I haz the power, 2017-01-26 22:07:42]

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

[Clemens Hammacher, 2017-01-26 22:12:07]

Thanks Alexey!

https://codereview.chromium.org/2655653003/diff/40001/src/inspector/wasm-tran...
File src/inspector/wasm-translation.cc (right):

https://codereview.chromium.org/2655653003/diff/40001/src/inspector/wasm-tran...
src/inspector/wasm-translation.cc:265: // the fake scripts, which triggers
setting breakpoints.
On 2017/01/26 at 20:58:16, kozy wrote:
> When base class manage memory of derived class by putting them into third
class it doesn't sound right to me..
> 
> Could we just put std::unique_ptr<TranslatorImpl> intro base class as member
and create required by checking mode?

I reintroduced the std::unique_ptr from before, and now just call a (new) Init
method right *after* inserting the TranslatorImpl into wasm_translators_.

[commit-bot: I haz the power, 2017-01-26 22:43:46]

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

[commit-bot: I haz the power, 2017-01-26 22:43:47]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-01-27 07:16:26]

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

[commit-bot: I haz the power, 2017-01-27 07:16:29]

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-27 07:17:59]

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

[commit-bot: I haz the power, 2017-01-27 07:18:00]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-01-27 07:18:25]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2017-01-27 07:18:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, kozyatinskiy@chromium.org
Link to the patchset: https://codereview.chromium.org/2655653003/#ps60001
(title: "Introduce Init method and clang-format wasm-translation.cc")

[commit-bot: I haz the power, 2017-01-27 07:18:30]

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-27 07:20:11]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485501505593840,
"parent_rev": "fec83a2a5a289584df98bd19ab9051f41b062e70", "commit_rev":
"c6663ef1c1c95e506e5baa22dae301c5af172e27"}

[commit-bot: I haz the power, 2017-01-27 07:20:16]

Description was changed from

==========
[inspector] Expose GetPossibleBreakpoints for wasm

and add an inspector test for it. Also test that a breakpoint can be
set on each reported position, and that it is also hit during
execution.
This CL also fixes two little things which were uncovered by the new
test.

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

to

==========
[inspector] Expose GetPossibleBreakpoints for wasm

and add an inspector test for it. Also test that a breakpoint can be
set on each reported position, and that it is also hit during
execution.
This CL also fixes two little things which were uncovered by the new
test.

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

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

[commit-bot: I haz the power, 2017-01-27 07:20:18]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/c6663ef1c1c95e506e5baa22dae301c5af1...