[wasm] Patch trapping position into stack trace

[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=titzer@chromium.org, yangguo@chromium.org

Committed: https://crrev.com/bafa239da034d2d914609eb614994efd9381e53e
Cr-Commit-Position: refs/heads/master@{#36202}

[titzer, 2016-04-28 12:07:59]

https://codereview.chromium.org/1924253002/diff/1/src/runtime/runtime-interna...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/1924253002/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:107: // Now patch the given byte offset into the
top-most frame (which must be a
Can you adjust the comments a bit here to explain why we do this?

I.e. it's because the above stack trace is inaccurate in the top frame due to
the shared trap code, so we adjust it here.

https://codereview.chromium.org/1924253002/diff/1/test/cctest/wasm/test-wasm-...
File test/cctest/wasm/test-wasm-trap-position.cc (right):

https://codereview.chromium.org/1924253002/diff/1/test/cctest/wasm/test-wasm-...
test/cctest/wasm/test-wasm-trap-position.cc:96: {"exec_unreachable", true, 1}, 
// Prevent clang-format changes.
We usually just do // --
and then // --
for the next line,
leaving the last curly on the last line
}

But NBD in the grand scheme of things.

[Clemens Hammacher, 2016-04-28 12:25:35]

Description was changed from

==========
[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=mstarzinger@chromium.org, titzer@chromium.org
==========

to

==========
[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=titzer@chromium.org, adamk@chromium.org
==========

[Clemens Hammacher, 2016-04-28 12:25:36]

clemensh@chromium.org changed reviewers:
+ adamk@chromium.org
- mstarzinger@chromium.org

[Clemens Hammacher, 2016-04-28 12:32:12]

https://codereview.chromium.org/1924253002/diff/1/src/runtime/runtime-interna...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/1924253002/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:107: // Now patch the given byte offset into the
top-most frame (which must be a
On 2016/04/28 at 12:07:59, titzer wrote:
> Can you adjust the comments a bit here to explain why we do this?
> 
> I.e. it's because the above stack trace is inaccurate in the top frame due to
the shared trap code, so we adjust it here.

Done.

https://codereview.chromium.org/1924253002/diff/1/test/cctest/wasm/test-wasm-...
File test/cctest/wasm/test-wasm-trap-position.cc (right):

https://codereview.chromium.org/1924253002/diff/1/test/cctest/wasm/test-wasm-...
test/cctest/wasm/test-wasm-trap-position.cc:96: {"exec_unreachable", true, 1}, 
// Prevent clang-format changes.
On 2016/04/28 at 12:07:59, titzer wrote:
> We usually just do // --
> and then // --
> for the next line,
> leaving the last curly on the last line
> }
> 
> But NBD in the grand scheme of things.

OK, changed it anyway.

[titzer, 2016-05-03 10:56:29]

On 2016/04/28 12:32:12, Clemens Hammacher wrote:
>
https://codereview.chromium.org/1924253002/diff/1/src/runtime/runtime-interna...
> File src/runtime/runtime-internal.cc (right):
> 
>
https://codereview.chromium.org/1924253002/diff/1/src/runtime/runtime-interna...
> src/runtime/runtime-internal.cc:107: // Now patch the given byte offset into
the
> top-most frame (which must be a
> On 2016/04/28 at 12:07:59, titzer wrote:
> > Can you adjust the comments a bit here to explain why we do this?
> > 
> > I.e. it's because the above stack trace is inaccurate in the top frame due
to
> the shared trap code, so we adjust it here.
> 
> Done.
> 
>
https://codereview.chromium.org/1924253002/diff/1/test/cctest/wasm/test-wasm-...
> File test/cctest/wasm/test-wasm-trap-position.cc (right):
> 
>
https://codereview.chromium.org/1924253002/diff/1/test/cctest/wasm/test-wasm-...
> test/cctest/wasm/test-wasm-trap-position.cc:96: {"exec_unreachable", true, 1},

> // Prevent clang-format changes.
> On 2016/04/28 at 12:07:59, titzer wrote:
> > We usually just do // --
> > and then // --
> > for the next line,
> > leaving the last curly on the last line
> > }
> > 
> > But NBD in the grand scheme of things.
> 
> OK, changed it anyway.

lgtm

[Clemens Hammacher, 2016-05-03 11:20:26]

Description was changed from

==========
[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=titzer@chromium.org, adamk@chromium.org
==========

to

==========
[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=titzer@chromium.org, adamk@chromium.org, yangguo@chromium.org
==========

[Clemens Hammacher, 2016-05-03 11:20:26]

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

[Yang, 2016-05-11 08:22:18]

LGTM if comments are addressed.

https://codereview.chromium.org/1924253002/diff/140001/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1924253002/diff/140001/src/js/messages.js#new...
src/js/messages.js:831: var pos = IS_NUMBER(fun) && pc < 0 ? (-1 - pc) :
fun can only be a number for wasm, right? Why do we still need to encode the
offset as a negative number?

https://codereview.chromium.org/1924253002/diff/140001/src/runtime/runtime-in...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/1924253002/diff/140001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:111: // frame (a wasm frame) on the collected
stack trace.
I'm very hesitant to accept this hack. But since we have a plan to solve this
cleanly eventually, can we at least file a bug to track progress and add a TODO
here?

[Clemens Hammacher, 2016-05-12 07:49:02]

Description was changed from

==========
[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=titzer@chromium.org, adamk@chromium.org, yangguo@chromium.org
==========

to

==========
[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=titzer@chromium.org, yangguo@chromium.org
==========

[Clemens Hammacher, 2016-05-12 07:49:02]

clemensh@chromium.org changed reviewers:
- adamk@chromium.org

[Clemens Hammacher, 2016-05-12 07:49:02]

Description was changed from

==========
[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=titzer@chromium.org, adamk@chromium.org, yangguo@chromium.org
==========

to

==========
[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=titzer@chromium.org, yangguo@chromium.org
==========

[Clemens Hammacher, 2016-05-12 07:49:03]

clemensh@chromium.org changed reviewers:
- adamk@chromium.org

[Clemens Hammacher, 2016-05-12 07:49:39]

https://codereview.chromium.org/1924253002/diff/140001/src/js/messages.js
File src/js/messages.js (right):

https://codereview.chromium.org/1924253002/diff/140001/src/js/messages.js#new...
src/js/messages.js:831: var pos = IS_NUMBER(fun) && pc < 0 ? (-1 - pc) :
On 2016/05/11 08:22:18, Yang wrote:
> fun can only be a number for wasm, right? Why do we still need to encode the
> offset as a negative number?

Because there are still two cases for wasm:
1) For "inner" stack frames, the position is determined by the
StackFrameIterator; it a positive number describing the program code offset.
That number needs to be translated via FunctionGetPositionForOffset to a byte
offset.
2) For top frames, the byte offset is patched in directly, there is no
translation needed.

I know that this is not nice; it will be cleaned up together with the whole
stack-frame-patching hack.

https://codereview.chromium.org/1924253002/diff/140001/src/runtime/runtime-in...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/1924253002/diff/140001/src/runtime/runtime-in...
src/runtime/runtime-internal.cc:111: // frame (a wasm frame) on the collected
stack trace.
On 2016/05/11 08:22:18, Yang wrote:
> I'm very hesitant to accept this hack. But since we have a plan to solve this
> cleanly eventually, can we at least file a bug to track progress and add a
TODO
> here?

Tracking bug #5007.
https://bugs.chromium.org/p/v8/issues/detail?id=5007

I also added a TODO with a reference to this bug.

[Yang, 2016-05-12 08:48:03]

On 2016/05/12 07:49:39, Clemens Hammacher wrote:
> https://codereview.chromium.org/1924253002/diff/140001/src/js/messages.js
> File src/js/messages.js (right):
> 
>
https://codereview.chromium.org/1924253002/diff/140001/src/js/messages.js#new...
> src/js/messages.js:831: var pos = IS_NUMBER(fun) && pc < 0 ? (-1 - pc) :
> On 2016/05/11 08:22:18, Yang wrote:
> > fun can only be a number for wasm, right? Why do we still need to encode the
> > offset as a negative number?
> 
> Because there are still two cases for wasm:
> 1) For "inner" stack frames, the position is determined by the
> StackFrameIterator; it a positive number describing the program code offset.
> That number needs to be translated via FunctionGetPositionForOffset to a byte
> offset.
> 2) For top frames, the byte offset is patched in directly, there is no
> translation needed.
> 
> I know that this is not nice; it will be cleaned up together with the whole
> stack-frame-patching hack.
> 
>
https://codereview.chromium.org/1924253002/diff/140001/src/runtime/runtime-in...
> File src/runtime/runtime-internal.cc (right):
> 
>
https://codereview.chromium.org/1924253002/diff/140001/src/runtime/runtime-in...
> src/runtime/runtime-internal.cc:111: // frame (a wasm frame) on the collected
> stack trace.
> On 2016/05/11 08:22:18, Yang wrote:
> > I'm very hesitant to accept this hack. But since we have a plan to solve
this
> > cleanly eventually, can we at least file a bug to track progress and add a
> TODO
> > here?
> 
> Tracking bug #5007.
> https://bugs.chromium.org/p/v8/issues/detail?id=5007
> 
> I also added a TODO with a reference to this bug.

LGTM.

[Clemens Hammacher, 2016-05-12 08:57:46]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-05-12 08:57:47]

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/1924253002/#ps160001
(title: "add TODO with tracking bug")

[commit-bot: I haz the power, 2016-05-12 08:57:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1924253002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1924253002/160001

[commit-bot: I haz the power, 2016-05-12 09:06:55]

Description was changed from

==========
[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=titzer@chromium.org, yangguo@chromium.org
==========

to

==========
[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=titzer@chromium.org, yangguo@chromium.org
==========

[commit-bot: I haz the power, 2016-05-12 09:06:56]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-05-12 09:08:46]

Description was changed from

==========
[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=titzer@chromium.org, yangguo@chromium.org
==========

to

==========
[wasm] Patch trapping position into stack trace

And add more tests for traps at different locations.

R=titzer@chromium.org, yangguo@chromium.org

Committed: https://crrev.com/bafa239da034d2d914609eb614994efd9381e53e
Cr-Commit-Position: refs/heads/master@{#36202}
==========

[commit-bot: I haz the power, 2016-05-12 09:08:47]

Patchset 9 (id:??) landed as
https://crrev.com/bafa239da034d2d914609eb614994efd9381e53e
Cr-Commit-Position: refs/heads/master@{#36202}