[wasm] Fix importing wasm functions which are being debugged

[wasm] Fix importing wasm functions which are being debugged

If the imported wasm function is being debugged (i.e. redirects to the
interpreter), call it via the JS_TO_WASM stub, such that we can disable
the breakpoint later by patching the exported function.

This also contains a drive-by fix in wasm-translation.cc (for the case
that all known positions are bigger than the requested one).

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

[Clemens Hammacher, 2017-02-27 16:18:53]

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

[commit-bot: I haz the power, 2017-02-27 16:19:05]

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-02-27 16:46:16]

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

[commit-bot: I haz the power, 2017-02-27 16:46:17]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-02-28 13:33:23]

Description was changed from

==========
[wasm] Fix importing wasm functions which are being debugged

If the imported wasm function is being debugged (i.e. redirects to the
interpreter), call it via the JS_TO_WASM stub, such that we can disable
the breakpoint later by patching the exported function.

R=titzer@chromium.org
BUG=5971
==========

to

==========
[wasm] Fix importing wasm functions which are being debugged

If the imported wasm function is being debugged (i.e. redirects to the
interpreter), call it via the JS_TO_WASM stub, such that we can disable
the breakpoint later by patching the exported function.

R=titzer@chromium.org, kozyatinskiy@chromium.org
BUG=5971
==========

[Clemens Hammacher, 2017-02-28 13:33:23]

clemensh@chromium.org changed reviewers:
+ kozyatinskiy@chromium.org

[Clemens Hammacher, 2017-02-28 13:35:07]

+kozyatinskiy for the inspector test.
Or is there a better/easier way for a JS test involving breakpoints?

[kozy, 2017-02-28 16:47:02]

bunch of inspector-test comments.

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
File test/inspector/debugger/wasm-imports.js (right):

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:59: .then(() =>
Protocol.Debugger.setBreakpoint({
Just wondering, is setBreakpointByUrl supported for wasm modules?

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:60: 'location':
quotes are optional: 'location' -> location

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:64: .then(msg =>
InspectorTest.logMessage(msg.result.actualLocation))
You can use here too, but please change argument type from callFrame to
location, more details about this method below.
InspectorTest.logCallFrameSourceLocation(msg.result.actualLocation)

Then you don't actually need to dump source on parsed event and simplify
waitForWasmScript a little more then below.

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:74: .then(() =>
evalWithUrl('instances[1].exports.main()', 'runWasm'))
To handle asynchronous call in onPaused handler you can have separate chain of
promises for it instead of listener:
Protocol.Debugger.oncePaused()
  .then(handlePaused)
  .then(() => Protocol.Debugger.resume())

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:86: function waitForWasmScript() {
Less symbols with more promise magic:

function waitForWasmScript(msg) {
  if (!msg || !msg.params.url.startsWith('wasm://')) {
    return Protocol.Debugger.onceScriptParsed().then(waitForWasmScript);
  }
  InspectorTest.log('Got wasm script!');
  script_a_id = msg.params.scriptId;
  InspectorTest.log('Source:');
  return Protocol.Debugger.getScriptSource({scriptId: script_a_id})
      .then(msg => InspectorTest.log(msg.result.scriptSource));
}

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:97: InspectorTest.log('Ignoring script
with url ' + url);
Could potentially add flakiness if we compile somewhere for internal purpose,
let's drop this output.

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:115: InspectorTest.log(
You can use InspectorTest.logCallFrames([msg.params.callFrames[0]]); to log call
frames - all of them or only top if needed.

And you can use:
InspectorTest.logCallFrameSourceLocation(msg.params.callFrames[0]);
then it will dump following text:
func $func
  #nop
end
where # - position of actual break - I found it's very informative. To use this
function before Debugger.enable you need to call:
InspectorTest.setupScriptMap().
Example:
https://cs.chromium.org/chromium/src/v8/test/inspector/debugger/step-into.js?...

warning - this command dump output asynchronously (first call per scriptId) -
return promise.

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:118: // TODO(clemensh): The interpreted
frame should also show up on this stack
Sounds like big TODO, how stepping works in this case? Will we make step from
top interpreter frame or from top that we show to user?

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:120: evalWithUrl('new Error().stack',
'getStack')
This command is asynchronous and it works currently because we run commands in
FIFO order but adding any new command somewhere in between could break this
test.

[kozy, 2017-02-28 19:28:26]

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
File test/inspector/debugger/wasm-imports.js (right):

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:120: evalWithUrl('new Error().stack',
'getStack')
On 2017/02/28 16:47:01, kozy wrote:
> This command is asynchronous and it works currently because we run commands in
> FIFO order but adding any new command somewhere in between could break this
> test.

Oops, please disregard this comment, just change last .then to:
.then(Protocol.Debugger.resume)
otherwise you call resume synchronously.

[Clemens Hammacher, 2017-03-02 13:36:00]

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

[commit-bot: I haz the power, 2017-03-02 13:36:02]

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-03-02 14:01:25]

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

[commit-bot: I haz the power, 2017-03-02 14:01:32]

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-03-02 14:02:35]

Thanks a lot for the detailed comments, Alexey. All fixed, and it's truly
beautiful now :)

Ben, PTAL at wasm-module.cc changes. Thanks!

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
File test/inspector/debugger/wasm-imports.js (right):

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:59: .then(() =>
Protocol.Debugger.setBreakpoint({
On 2017/02/28 at 16:47:02, kozy wrote:
> Just wondering, is setBreakpointByUrl supported for wasm modules?

Yes, it is. In order to also cover it, I just changed this test to use
setBreakpointByUrl instead of setBreakpoint. That uncovered a minor bug in
wasm-translation.cc which this CL now also fixes.

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:60: 'location':
On 2017/02/28 at 16:47:01, kozy wrote:
> quotes are optional: 'location' -> location

Done.

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:64: .then(msg =>
InspectorTest.logMessage(msg.result.actualLocation))
On 2017/02/28 at 16:47:02, kozy wrote:
> You can use here too, but please change argument type from callFrame to
location, more details about this method below.
> InspectorTest.logCallFrameSourceLocation(msg.result.actualLocation)
> 
> Then you don't actually need to dump source on parsed event and simplify
waitForWasmScript a little more then below.

Done, but it required some refactoring.
logCallFrameSourceLocation accepted a callFrame, not a location. I renamed the
method to logSourceLocation, and added another method logSourceLocations to log
a full array.

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:74: .then(() =>
evalWithUrl('instances[1].exports.main()', 'runWasm'))
On 2017/02/28 at 16:47:02, kozy wrote:
> To handle asynchronous call in onPaused handler you can have separate chain of
promises for it instead of listener:
> Protocol.Debugger.oncePaused()
>   .then(handlePaused)
>   .then(() => Protocol.Debugger.resume())

Done, also inlined the handlePaused method, which was basically just another
promise chain.

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:86: function waitForWasmScript() {
On 2017/02/28 at 16:47:02, kozy wrote:
> Less symbols with more promise magic:
> 
> function waitForWasmScript(msg) {
>   if (!msg || !msg.params.url.startsWith('wasm://')) {
>     return Protocol.Debugger.onceScriptParsed().then(waitForWasmScript);
>   }
>   InspectorTest.log('Got wasm script!');
>   script_a_id = msg.params.scriptId;
>   InspectorTest.log('Source:');
>   return Protocol.Debugger.getScriptSource({scriptId: script_a_id})
>       .then(msg => InspectorTest.log(msg.result.scriptSource));
> }

Wow, that's awesome. I now even return the url from this method instead of
storing it to a global variable.

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:97: InspectorTest.log('Ignoring script
with url ' + url);
On 2017/02/28 at 16:47:01, kozy wrote:
> Could potentially add flakiness if we compile somewhere for internal purpose,
let's drop this output.

Done.

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:115: InspectorTest.log(
On 2017/02/28 at 16:47:02, kozy wrote:
> You can use InspectorTest.logCallFrames([msg.params.callFrames[0]]); to log
call frames - all of them or only top if needed.
> 
> And you can use:
> InspectorTest.logCallFrameSourceLocation(msg.params.callFrames[0]);
> then it will dump following text:
> func $func
>   #nop
> end
> where # - position of actual break - I found it's very informative. To use
this function before Debugger.enable you need to call:
> InspectorTest.setupScriptMap().
> Example:
https://cs.chromium.org/chromium/src/v8/test/inspector/debugger/step-into.js?...
> 
> warning - this command dump output asynchronously (first call per scriptId) -
return promise.

Neverending awesomeness. Done!

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:118: // TODO(clemensh): The interpreted
frame should also show up on this stack
On 2017/02/28 at 16:47:02, kozy wrote:
> Sounds like big TODO, how stepping works in this case? Will we make step from
top interpreter frame or from top that we show to user?

Stepping works correctly (on the top interpreter frame). This TODO is referring
to another TODO in Isolate::CaptureSimpleStackTrace, where
WASM_INTERPRETER_ENTRY frames are just skipped currently.

https://codereview.chromium.org/2720813002/diff/1/test/inspector/debugger/was...
test/inspector/debugger/wasm-imports.js:120: evalWithUrl('new Error().stack',
'getStack')
On 2017/02/28 at 19:28:26, kozy wrote:
> On 2017/02/28 16:47:01, kozy wrote:
> > This command is asynchronous and it works currently because we run commands
in
> > FIFO order but adding any new command somewhere in between could break this
> > test.
> 
> Oops, please disregard this comment, just change last .then to:
> .then(Protocol.Debugger.resume)
> otherwise you call resume synchronously.

Good catch! Done.

[Clemens Hammacher, 2017-03-02 14:11:43]

Description was changed from

==========
[wasm] Fix importing wasm functions which are being debugged

If the imported wasm function is being debugged (i.e. redirects to the
interpreter), call it via the JS_TO_WASM stub, such that we can disable
the breakpoint later by patching the exported function.

R=titzer@chromium.org, kozyatinskiy@chromium.org
BUG=5971
==========

to

==========
[wasm] Fix importing wasm functions which are being debugged

If the imported wasm function is being debugged (i.e. redirects to the
interpreter), call it via the JS_TO_WASM stub, such that we can disable
the breakpoint later by patching the exported function.

This also contains a drive-by fix in wasm-translation.cc (for the case
that all known positions are bigger than the requested one).

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

[commit-bot: I haz the power, 2017-03-02 14:41:54]

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

[commit-bot: I haz the power, 2017-03-02 14:41:55]

Dry run: This issue passed the CQ dry run.

[kozy, 2017-03-02 21:31:10]

thanks! just FYI: since yesterday we decided that we can use async-await in
tests, I'm fine with landing this CL as is but if you'd like to play with async
await then feel free to do it :)

lgtm.

https://codereview.chromium.org/2720813002/diff/40001/test/inspector/protocol...
File test/inspector/protocol-test.js (right):

https://codereview.chromium.org/2720813002/diff/40001/test/inspector/protocol...
test/inspector/protocol-test.js:167: if (locations.length == 0) return
Promise.resolve();
We recently allow async-await syntax in tests, so:

async InspectorTest.logSourceLocations = function(locations) {
  for (var location of locations) {
    await InspectorTest.logSourceLocation(location);
  }
}

[titzer, 2017-03-03 09:30:54]

On 2017/03/02 21:31:10, kozy wrote:
> thanks! just FYI: since yesterday we decided that we can use async-await in
> tests, I'm fine with landing this CL as is but if you'd like to play with
async
> await then feel free to do it :)
> 
> lgtm.
> 
>
https://codereview.chromium.org/2720813002/diff/40001/test/inspector/protocol...
> File test/inspector/protocol-test.js (right):
> 
>
https://codereview.chromium.org/2720813002/diff/40001/test/inspector/protocol...
> test/inspector/protocol-test.js:167: if (locations.length == 0) return
> Promise.resolve();
> We recently allow async-await syntax in tests, so:
> 
> async InspectorTest.logSourceLocations = function(locations) {
>   for (var location of locations) {
>     await InspectorTest.logSourceLocation(location);
>   }
> }

LGTM

[Clemens Hammacher, 2017-03-03 12:36:28]

The CQ bit was checked by clemensh@chromium.org

[commit-bot: I haz the power, 2017-03-03 12:36:34]

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

[Clemens Hammacher, 2017-03-03 12:36:40]

The CQ bit was unchecked by clemensh@chromium.org

[Clemens Hammacher, 2017-03-03 12:36:49]

Description was changed from

==========
[wasm] Fix importing wasm functions which are being debugged

If the imported wasm function is being debugged (i.e. redirects to the
interpreter), call it via the JS_TO_WASM stub, such that we can disable
the breakpoint later by patching the exported function.

This also contains a drive-by fix in wasm-translation.cc (for the case
that all known positions are bigger than the requested one).

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

to

==========
[wasm] Fix importing wasm functions which are being debugged

If the imported wasm function is being debugged (i.e. redirects to the
interpreter), call it via the JS_TO_WASM stub, such that we can disable
the breakpoint later by patching the exported function.

This also contains a drive-by fix in wasm-translation.cc (for the case
that all known positions are bigger than the requested one).

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

[Clemens Hammacher, 2017-03-03 12:37:04]

The CQ bit was checked by clemensh@chromium.org

[commit-bot: I haz the power, 2017-03-03 12:37:14]

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-03-03 12:38:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488544624280160,
"parent_rev": "1a6487fd6599ba0e76ca59d5c0dd80a1b0532776", "commit_rev":
"eb36a7dbcfaeca660da0c5bb67d58d235e61181d"}

[commit-bot: I haz the power, 2017-03-03 12:38:47]

Description was changed from

==========
[wasm] Fix importing wasm functions which are being debugged

If the imported wasm function is being debugged (i.e. redirects to the
interpreter), call it via the JS_TO_WASM stub, such that we can disable
the breakpoint later by patching the exported function.

This also contains a drive-by fix in wasm-translation.cc (for the case
that all known positions are bigger than the requested one).

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

to

==========
[wasm] Fix importing wasm functions which are being debugged

If the imported wasm function is being debugged (i.e. redirects to the
interpreter), call it via the JS_TO_WASM stub, such that we can disable
the breakpoint later by patching the exported function.

This also contains a drive-by fix in wasm-translation.cc (for the case
that all known positions are bigger than the requested one).

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

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

[commit-bot: I haz the power, 2017-03-03 12:38:48]

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