[wasm] Provide better stack traces for asm.js code

[wasm] Provide better stack traces for asm.js code

For the asm.js to WASM pipeline, the current stack traces only show
low-level WASM information.
This CL maps this back to asm.js source positions.
It does so by attaching the asm.js source Script to the compiled WASM
module, and emitting a delta-encoded table which maps from WASM byte
offsets to positions within that Script. As asm.js code does not throw
exceptions, we only store a mapping for call instructions.

The new AsmJsWasmStackFrame implementation inherits from
WasmStackFrame, but contains the logic to provide the source script and
the position inside of it.
What is still missing is the JSFunction object returned by
CallSite.getFunction(). We currently return null.

R=jgruber@chromium.org, titzer@chromium.org
BUG=v8:4203
Committed: https://crrev.com/5d9fa102a792a9882be0ff463eaee7d89d259c4e
Cr-Commit-Position: refs/heads/master@{#40205}

[Clemens Hammacher, 2016-10-11 11:10:39]

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

[commit-bot: I haz the power, 2016-10-11 11:10:48]

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, 2016-10-11 11:15:58]

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

[commit-bot: I haz the power, 2016-10-11 11:15:59]

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

[Clemens Hammacher, 2016-10-11 11:30:24]

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

[commit-bot: I haz the power, 2016-10-11 11:30: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, 2016-10-11 11:36:06]

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

[commit-bot: I haz the power, 2016-10-11 11:36:07]

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

[Clemens Hammacher, 2016-10-11 11:40:12]

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

[commit-bot: I haz the power, 2016-10-11 11:40:13]

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, 2016-10-11 11:47:43]

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

[commit-bot: I haz the power, 2016-10-11 11:47:44]

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

[Clemens Hammacher, 2016-10-11 12:00:49]

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

[commit-bot: I haz the power, 2016-10-11 12:00:57]

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

[Clemens Hammacher, 2016-10-11 12:14:58]

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

[commit-bot: I haz the power, 2016-10-11 12:15:02]

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, 2016-10-11 12:47:44]

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

[commit-bot: I haz the power, 2016-10-11 12:47:46]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/15916)
  v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[Clemens Hammacher, 2016-10-11 12:55:35]

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

[commit-bot: I haz the power, 2016-10-11 12:55:44]

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, 2016-10-11 13:22:11]

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

[commit-bot: I haz the power, 2016-10-11 13:22:12]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-10-11 14:51:26]

https://codereview.chromium.org/2404253002/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2404253002/diff/100001/src/isolate.cc#newcode524
src/isolate.cc:524: if (wasm::WasmIsAsm(*wasm_object, this)) {
WasmIsAsmJs?

https://codereview.chromium.org/2404253002/diff/100001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2404253002/diff/100001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:536: ModuleOrigin origin, Handle<Script> asm_js_script);
What if, instead of using a special section, we just passed the encoded
Handle<ByteArray> directly here and have the asm-wasm builder build that byte
array? That skips an extra encoding/decoding step and all the associated logic.

[Clemens Hammacher, 2016-10-11 17:04:10]

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

[commit-bot: I haz the power, 2016-10-11 17:04: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, 2016-10-11 17:04:22]

https://codereview.chromium.org/2404253002/diff/100001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2404253002/diff/100001/src/isolate.cc#newcode524
src/isolate.cc:524: if (wasm::WasmIsAsm(*wasm_object, this)) {
On 2016/10/11 14:51:25, titzer wrote:
> WasmIsAsmJs?

Done.

https://codereview.chromium.org/2404253002/diff/100001/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/2404253002/diff/100001/src/wasm/wasm-module.h...
src/wasm/wasm-module.h:536: ModuleOrigin origin, Handle<Script> asm_js_script);
On 2016/10/11 14:51:25, titzer wrote:
> What if, instead of using a special section, we just passed the encoded
> Handle<ByteArray> directly here and have the asm-wasm builder build that byte
> array? That skips an extra encoding/decoding step and all the associated
logic.

That's a very nice idea. Done. It makes things a lot easier and improves
performance.

[commit-bot: I haz the power, 2016-10-11 17:33:16]

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

[commit-bot: I haz the power, 2016-10-11 17:33:17]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2016-10-11 18:36:52]

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

[commit-bot: I haz the power, 2016-10-11 18:37:01]

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

[jgruber, 2016-10-11 19:07:41]

Looking good. Some comments:

https://codereview.chromium.org/2404253002/diff/120001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2404253002/diff/120001/src/isolate.cc#newcode525
src/isolate.cc:525: flags |= FrameArray::kIsAsmWasmFrame;
Would it make sense to mark a frame as either kIsWasmFrame or kIsAsmWasmFrame?
If possible, I think I'd prefer going down that route.

https://codereview.chromium.org/2404253002/diff/120001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/2404253002/diff/120001/src/messages.cc#newcod...
src/messages.cc:674: return isolate_->global_object();
As far I understand, the global object should never be exposed to the user, and
instead the global proxy should be passed. Maybe it'd be a good idea to ask e.g.
Toon about this.

https://codereview.chromium.org/2404253002/diff/120001/src/messages.cc#newcod...
src/messages.cc:757: const bool is_asm_wasm_frame = (flags &
FrameArray::kIsAsmWasmFrame) != 0;
Can we make this a switch on flags & (kIsWasmFrame | kIsAsmWasmFrame), and
ensure that we fall into exactly one of the three possible cases?

https://codereview.chromium.org/2404253002/diff/120001/test/mjsunit/wasm/asm-...
File test/mjsunit/wasm/asm-wasm-stack.js (right):

https://codereview.chromium.org/2404253002/diff/120001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-stack.js:59: //assertWasm(fun);
Is the commented code here still needed?

https://codereview.chromium.org/2404253002/diff/120001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-stack.js:63: assertUnreachable('exception should have
been thrown');
This pattern may not work as intended - when assertUnreachable throws an
exception, execution continues in the try block. test/mjsunit/stack-trace.js
unfortunately does the same in a couple of cases, and should also be fixed at
some point.

https://codereview.chromium.org/2404253002/diff/120001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-stack.js:83: function generateWasmFromAsmJs(stdlib,
foreign, heap) {
Could you reuse the function from above?

https://codereview.chromium.org/2404253002/diff/120001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-stack.js:102: ['throwException', 42, 9],         //
--
What are the '// --' comments?

[commit-bot: I haz the power, 2016-10-11 19:15:41]

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

[commit-bot: I haz the power, 2016-10-11 19:15:42]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2016-10-12 07:32:48]

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

[commit-bot: I haz the power, 2016-10-12 07:32:53]

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

[Clemens Hammacher, 2016-10-12 07:37:19]

https://codereview.chromium.org/2404253002/diff/120001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2404253002/diff/120001/src/isolate.cc#newcode525
src/isolate.cc:525: flags |= FrameArray::kIsAsmWasmFrame;
On 2016/10/11 19:07:40, jgruber wrote:
> Would it make sense to mark a frame as either kIsWasmFrame or kIsAsmWasmFrame?
> If possible, I think I'd prefer going down that route.

Done.

https://codereview.chromium.org/2404253002/diff/120001/src/messages.cc
File src/messages.cc (right):

https://codereview.chromium.org/2404253002/diff/120001/src/messages.cc#newcod...
src/messages.cc:674: return isolate_->global_object();
On 2016/10/11 19:07:40, jgruber wrote:
> As far I understand, the global object should never be exposed to the user,
and
> instead the global proxy should be passed. Maybe it'd be a good idea to ask
e.g.
> Toon about this.

You are right. Fixed it.

https://codereview.chromium.org/2404253002/diff/120001/src/messages.cc#newcod...
src/messages.cc:757: const bool is_asm_wasm_frame = (flags &
FrameArray::kIsAsmWasmFrame) != 0;
On 2016/10/11 19:07:40, jgruber wrote:
> Can we make this a switch on flags & (kIsWasmFrame | kIsAsmWasmFrame), and
> ensure that we fall into exactly one of the three possible cases?

Done.

https://codereview.chromium.org/2404253002/diff/120001/test/mjsunit/wasm/asm-...
File test/mjsunit/wasm/asm-wasm-stack.js (right):

https://codereview.chromium.org/2404253002/diff/120001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-stack.js:59: //assertWasm(fun);
On 2016/10/11 19:07:40, jgruber wrote:
> Is the commented code here still needed?

Fixed already.

https://codereview.chromium.org/2404253002/diff/120001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-stack.js:63: assertUnreachable('exception should have
been thrown');
On 2016/10/11 19:07:40, jgruber wrote:
> This pattern may not work as intended - when assertUnreachable throws an
> exception, execution continues in the try block. test/mjsunit/stack-trace.js
> unfortunately does the same in a couple of cases, and should also be fixed at
> some point.

Good catch! Fixed it for this test case.

https://codereview.chromium.org/2404253002/diff/120001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-stack.js:83: function generateWasmFromAsmJs(stdlib,
foreign, heap) {
On 2016/10/11 19:07:41, jgruber wrote:
> Could you reuse the function from above?

Fixed already.

https://codereview.chromium.org/2404253002/diff/120001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-stack.js:102: ['throwException', 42, 9],         //
--
On 2016/10/11 19:07:41, jgruber wrote:
> What are the '// --' comments?

They prevent clang format from joining the lines.

[commit-bot: I haz the power, 2016-10-12 07:38:42]

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

[commit-bot: I haz the power, 2016-10-12 07:38:44]

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

[Clemens Hammacher, 2016-10-12 07:41:40]

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

[commit-bot: I haz the power, 2016-10-12 07:41:42]

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

[titzer, 2016-10-12 08:03:06]

lgtm modulo JS test

https://codereview.chromium.org/2404253002/diff/180001/src/wasm/module-decode...
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/2404253002/diff/180001/src/wasm/module-decode...
src/wasm/module-decoder.cc:131: // Check for the known "name" or "asm_offsets"
section.
Comment is out of date.

https://codereview.chromium.org/2404253002/diff/180001/src/wasm/wasm-debug.cc
File src/wasm/wasm-debug.cc (right):

https://codereview.chromium.org/2404253002/diff/180001/src/wasm/wasm-debug.cc...
src/wasm/wasm-debug.cc:245: if (offset_tables->IsUndefined(isolate)) {
Can you extract a helper method to materialize the offset tables?

https://codereview.chromium.org/2404253002/diff/180001/test/mjsunit/wasm/asm-...
File test/mjsunit/wasm/asm-wasm-stack.js (right):

https://codereview.chromium.org/2404253002/diff/180001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-stack.js:22: for (var i = 0; i < stack.length; ++i) {
I think we can improve the checking here by making it a bit more manual. E.g. I
have had to fix tests like this a couple of times, and it was pretty tricky to
find out which frame was mismatched and what was expected. If we rewrite this to
compare entry by entry and generate a special error message on failure
(including the index), this is way easier to fix in the future.

[Clemens Hammacher, 2016-10-12 08:17:24]

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

[commit-bot: I haz the power, 2016-10-12 08:17:26]

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

[Clemens Hammacher, 2016-10-12 08:17:46]

https://codereview.chromium.org/2404253002/diff/180001/src/wasm/module-decode...
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/2404253002/diff/180001/src/wasm/module-decode...
src/wasm/module-decoder.cc:131: // Check for the known "name" or "asm_offsets"
section.
On 2016/10/12 08:03:05, titzer wrote:
> Comment is out of date.

Done.

https://codereview.chromium.org/2404253002/diff/180001/src/wasm/wasm-debug.cc
File src/wasm/wasm-debug.cc (right):

https://codereview.chromium.org/2404253002/diff/180001/src/wasm/wasm-debug.cc...
src/wasm/wasm-debug.cc:245: if (offset_tables->IsUndefined(isolate)) {
On 2016/10/12 08:03:05, titzer wrote:
> Can you extract a helper method to materialize the offset tables?

Done.

https://codereview.chromium.org/2404253002/diff/180001/test/mjsunit/wasm/asm-...
File test/mjsunit/wasm/asm-wasm-stack.js (right):

https://codereview.chromium.org/2404253002/diff/180001/test/mjsunit/wasm/asm-...
test/mjsunit/wasm/asm-wasm-stack.js:22: for (var i = 0; i < stack.length; ++i) {
On 2016/10/12 08:03:05, titzer wrote:
> I think we can improve the checking here by making it a bit more manual. E.g.
I
> have had to fix tests like this a couple of times, and it was pretty tricky to
> find out which frame was mismatched and what was expected. If we rewrite this
to
> compare entry by entry and generate a special error message on failure
> (including the index), this is way easier to fix in the future.

This is exactly what happens here. I first print the complete stack trace, then
compare entry by entry, and output the index (i) on error.
What was missing though was the check that the length of the stack matches. I
added this.
Am I missing something else?

[titzer, 2016-10-12 08:19:31]

On 2016/10/12 08:17:46, Clemens Hammacher wrote:
>
https://codereview.chromium.org/2404253002/diff/180001/src/wasm/module-decode...
> File src/wasm/module-decoder.cc (right):
> 
>
https://codereview.chromium.org/2404253002/diff/180001/src/wasm/module-decode...
> src/wasm/module-decoder.cc:131: // Check for the known "name" or "asm_offsets"
> section.
> On 2016/10/12 08:03:05, titzer wrote:
> > Comment is out of date.
> 
> Done.
> 
> https://codereview.chromium.org/2404253002/diff/180001/src/wasm/wasm-debug.cc
> File src/wasm/wasm-debug.cc (right):
> 
>
https://codereview.chromium.org/2404253002/diff/180001/src/wasm/wasm-debug.cc...
> src/wasm/wasm-debug.cc:245: if (offset_tables->IsUndefined(isolate)) {
> On 2016/10/12 08:03:05, titzer wrote:
> > Can you extract a helper method to materialize the offset tables?
> 
> Done.
> 
>
https://codereview.chromium.org/2404253002/diff/180001/test/mjsunit/wasm/asm-...
> File test/mjsunit/wasm/asm-wasm-stack.js (right):
> 
>
https://codereview.chromium.org/2404253002/diff/180001/test/mjsunit/wasm/asm-...
> test/mjsunit/wasm/asm-wasm-stack.js:22: for (var i = 0; i < stack.length; ++i)
{
> On 2016/10/12 08:03:05, titzer wrote:
> > I think we can improve the checking here by making it a bit more manual.
E.g.
> I
> > have had to fix tests like this a couple of times, and it was pretty tricky
to
> > find out which frame was mismatched and what was expected. If we rewrite
this
> to
> > compare entry by entry and generate a special error message on failure
> > (including the index), this is way easier to fix in the future.
> 
> This is exactly what happens here. I first print the complete stack trace,
then
> compare entry by entry, and output the index (i) on error.
> What was missing though was the check that the length of the stack matches. I
> added this.
> Am I missing something else?

Didn't notice that you were including the index into the error message for those
asserts. That helps a lot!

[commit-bot: I haz the power, 2016-10-12 09:04:13]

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

[commit-bot: I haz the power, 2016-10-12 09:04:14]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2016-10-12 09:04:28]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-10-12 09:04:28]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2404253002/#ps200001
(title: "Address titzer's comments")

[commit-bot: I haz the power, 2016-10-12 09:04:34]

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

[commit-bot: I haz the power, 2016-10-12 09:04:36]

This CL has an open dependency (Issue 2410913002 Patch 1). Please resolve the
dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Clemens Hammacher, 2016-10-12 09:14:48]

The CQ bit was checked by clemensh@chromium.org

[commit-bot: I haz the power, 2016-10-12 09:14:57]

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, 2016-10-12 09:17:19]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-10-12 09:17:31]

Description was changed from

==========
[wasm] Provide better stack traces for asm.js code

For the asm.js to WASM pipeline, the current stack traces only show
low-level WASM information.
This CL maps this back to asm.js source positions.
It does so by attaching the asm.js source Script to the compiled WASM
module, and emitting a delta-encoded table which maps from WASM byte
offsets to positions within that Script. As asm.js code does not throw
exceptions, we only store a mapping for call instructions.

The new AsmJsWasmStackFrame implementation inherits from
WasmStackFrame, but contains the logic to provide the source script and
the position inside of it.
What is still missing is the JSFunction object returned by
CallSite.getFunction(). We currently return null.

R=jgruber@chromium.org, titzer@chromium.org
BUG=v8:4203
==========

to

==========
[wasm] Provide better stack traces for asm.js code

For the asm.js to WASM pipeline, the current stack traces only show
low-level WASM information.
This CL maps this back to asm.js source positions.
It does so by attaching the asm.js source Script to the compiled WASM
module, and emitting a delta-encoded table which maps from WASM byte
offsets to positions within that Script. As asm.js code does not throw
exceptions, we only store a mapping for call instructions.

The new AsmJsWasmStackFrame implementation inherits from
WasmStackFrame, but contains the logic to provide the source script and
the position inside of it.
What is still missing is the JSFunction object returned by
CallSite.getFunction(). We currently return null.

R=jgruber@chromium.org, titzer@chromium.org
BUG=v8:4203

Committed: https://crrev.com/5d9fa102a792a9882be0ff463eaee7d89d259c4e
Cr-Commit-Position: refs/heads/master@{#40205}
==========

[commit-bot: I haz the power, 2016-10-12 09:17:32]

Patchset 11 (id:??) landed as
https://crrev.com/5d9fa102a792a9882be0ff463eaee7d89d259c4e
Cr-Commit-Position: refs/heads/master@{#40205}