[wasm] Fix location for error in asm.js ToNumber conversion

[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- the StackFrame::State struct now also holds the callee_pc_address,
  which is set in ComputeCallerState. The WASM frame uses this
  information to determine whether the callee frame is WASM_TO_JS, and
  whether that frame is at the ToNumber conversion call.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

R=titzer@chromium.org, bradnelson@chromium.org
CC=jgruber@chromium.org
BUG=v8:4203, v8:5724
Committed: https://crrev.com/94cd46b55e24fa2bb7b06b3da4d5ba7f029bc262
Committed: https://crrev.com/890d28f3615769c3aa82b1bdf7df12c55774f909
Cr-Original-Commit-Position: refs/heads/master@{#41599}
Cr-Commit-Position: refs/heads/master@{#41613}

[Clemens Hammacher, 2016-12-07 18:34:48]

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

[commit-bot: I haz the power, 2016-12-07 18:34: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-12-07 18:39:40]

Description was changed from

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, where as
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- during stack trace generation (in the StackTraceIterator), when we
  move from the WASM_TO_JS frame to the WASM frame, we remember at which
  call inside the WASM_TO_JS wrapper we are, and encode this information
  in the generated caller state, used for the WASM frame.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

to

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- during stack trace generation (in the StackTraceIterator), when we
  move from the WASM_TO_JS frame to the WASM frame, we remember at which
  call inside the WASM_TO_JS wrapper we are, and encode this information
  in the generated caller state, used for the WASM frame.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

[Clemens Hammacher, 2016-12-07 18:40:09]

Description was changed from

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- during stack trace generation (in the StackTraceIterator), when we
  move from the WASM_TO_JS frame to the WASM frame, we remember at which
  call inside the WASM_TO_JS wrapper we are, and encode this information
  in the generated caller state, used for the WASM frame.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

to

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- during stack trace generation (in the StackTraceIterator), when we
  move from the WASM_TO_JS frame to the WASM frame, we remember at which
  call inside the WASM_TO_JS wrapper we are, and encode this information
  in the generated caller state, used for the WASM frame.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

[commit-bot: I haz the power, 2016-12-07 18:52:24]

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

[commit-bot: I haz the power, 2016-12-07 18:52:24]

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

[Clemens Hammacher, 2016-12-07 18:57:30]

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

[commit-bot: I haz the power, 2016-12-07 18:57:33]

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-12-07 19:00:07]

This is a rather big change for just fixing some slightly wrong locations on the
stack trace.
PTAL, WDYT?

Note that this is still blocked by a bug in Ignition triggered by the new test
case.

[commit-bot: I haz the power, 2016-12-07 19:11:41]

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

[commit-bot: I haz the power, 2016-12-07 19:11:42]

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

[bradnelson, 2016-12-07 19:19:06]

lgtm

https://codereview.chromium.org/2555243002/diff/20001/src/asmjs/asm-wasm-buil...
File src/asmjs/asm-wasm-builder.cc (right):

https://codereview.chromium.org/2555243002/diff/20001/src/asmjs/asm-wasm-buil...
src/asmjs/asm-wasm-builder.cc:40: bool IsChild(Expression* parent, Expression*
child) {
IsBinOpChild ?

https://codereview.chromium.org/2555243002/diff/20001/src/wasm/wasm-objects.cc
File src/wasm/wasm-objects.cc (right):

https://codereview.chromium.org/2555243002/diff/20001/src/wasm/wasm-objects.c...
src/wasm/wasm-objects.cc:540: int right = offset_table->length() / kIntSize / 3;
 // exclusive
At the point you've got 3 fields, maybe have an enum and given them a name
(including size as the last one)?

https://codereview.chromium.org/2555243002/diff/20001/test/mjsunit/wasm/asm-w...
File test/mjsunit/wasm/asm-wasm-exception-in-tonumber.js (right):

https://codereview.chromium.org/2555243002/diff/20001/test/mjsunit/wasm/asm-w...
test/mjsunit/wasm/asm-wasm-exception-in-tonumber.js:87: '^ *at sym \\(' +
filename + ':41:9\\)$',
Might be worth being less picky about the row to be less brittle?

[Clemens Hammacher, 2016-12-08 10:49:23]

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

[commit-bot: I haz the power, 2016-12-08 10:49:31]

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-12-08 10:50:23]

https://codereview.chromium.org/2555243002/diff/20001/src/asmjs/asm-wasm-buil...
File src/asmjs/asm-wasm-builder.cc (right):

https://codereview.chromium.org/2555243002/diff/20001/src/asmjs/asm-wasm-buil...
src/asmjs/asm-wasm-builder.cc:40: bool IsChild(Expression* parent, Expression*
child) {
On 2016/12/07 at 19:19:06, bradnelson wrote:
> IsBinOpChild ?

I just removed the function completely :)
By storing a BinaryOperation* as parent, the condition is simple enough to be
inlined.

https://codereview.chromium.org/2555243002/diff/20001/src/wasm/wasm-objects.cc
File src/wasm/wasm-objects.cc (right):

https://codereview.chromium.org/2555243002/diff/20001/src/wasm/wasm-objects.c...
src/wasm/wasm-objects.cc:540: int right = offset_table->length() / kIntSize / 3;
 // exclusive
On 2016/12/07 at 19:19:06, bradnelson wrote:
> At the point you've got 3 fields, maybe have an enum and given them a name
(including size as the last one)?

Cool idea, done!

https://codereview.chromium.org/2555243002/diff/20001/test/mjsunit/wasm/asm-w...
File test/mjsunit/wasm/asm-wasm-exception-in-tonumber.js (right):

https://codereview.chromium.org/2555243002/diff/20001/test/mjsunit/wasm/asm-w...
test/mjsunit/wasm/asm-wasm-exception-in-tonumber.js:87: '^ *at sym \\(' +
filename + ':41:9\\)$',
On 2016/12/07 at 19:19:06, bradnelson wrote:
> Might be worth being less picky about the row to be less brittle?

Yep, removed all line number information.

[titzer, 2016-12-08 10:50:42]

Description was changed from

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- during stack trace generation (in the StackTraceIterator), when we
  move from the WASM_TO_JS frame to the WASM frame, we remember at which
  call inside the WASM_TO_JS wrapper we are, and encode this information
  in the generated caller state, used for the WASM frame.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

to

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- during stack trace generation (in the StackTraceIterator), when we
  move from the WASM_TO_JS frame to the WASM frame, we remember at which
  call inside the WASM_TO_JS wrapper we are, and encode this information
  in the generated caller state, used for the WASM frame.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

[titzer, 2016-12-08 10:50:42]

titzer@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[titzer, 2016-12-08 10:50:42]

titzer@chromium.org changed required reviewers:
+ mstarzinger@chromium.org

[titzer, 2016-12-08 10:51:07]

On 2016/12/08 10:50:23, Clemens Hammacher wrote:
>
https://codereview.chromium.org/2555243002/diff/20001/src/asmjs/asm-wasm-buil...
> File src/asmjs/asm-wasm-builder.cc (right):
> 
>
https://codereview.chromium.org/2555243002/diff/20001/src/asmjs/asm-wasm-buil...
> src/asmjs/asm-wasm-builder.cc:40: bool IsChild(Expression* parent, Expression*
> child) {
> On 2016/12/07 at 19:19:06, bradnelson wrote:
> > IsBinOpChild ?
> 
> I just removed the function completely :)
> By storing a BinaryOperation* as parent, the condition is simple enough to be
> inlined.
> 
> https://codereview.chromium.org/2555243002/diff/20001/src/wasm/wasm-objects.cc
> File src/wasm/wasm-objects.cc (right):
> 
>
https://codereview.chromium.org/2555243002/diff/20001/src/wasm/wasm-objects.c...
> src/wasm/wasm-objects.cc:540: int right = offset_table->length() / kIntSize /
3;
>  // exclusive
> On 2016/12/07 at 19:19:06, bradnelson wrote:
> > At the point you've got 3 fields, maybe have an enum and given them a name
> (including size as the last one)?
> 
> Cool idea, done!
> 
>
https://codereview.chromium.org/2555243002/diff/20001/test/mjsunit/wasm/asm-w...
> File test/mjsunit/wasm/asm-wasm-exception-in-tonumber.js (right):
> 
>
https://codereview.chromium.org/2555243002/diff/20001/test/mjsunit/wasm/asm-w...
> test/mjsunit/wasm/asm-wasm-exception-in-tonumber.js:87: '^ *at sym \\(' +
> filename + ':41:9\\)$',
> On 2016/12/07 at 19:19:06, bradnelson wrote:
> > Might be worth being less picky about the row to be less brittle?
> 
> Yep, removed all line number information.

+ mstarzinger so he can take a look as well

[commit-bot: I haz the power, 2016-12-08 11:28:21]

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

[commit-bot: I haz the power, 2016-12-08 11:28:23]

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/19228)
  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...)

[Michael Starzinger, 2016-12-08 11:38:05]

LGTM with comments to address. Mostly looked at the gluing code, not so much at
the WASM internals.

https://codereview.chromium.org/2555243002/diff/40001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/2555243002/diff/40001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:147: source_positions_(position_table ? position_table
nit: Can we move this piece that maybe allocates a source position table into
the caller? The {Pipeline::GenerateCodeForCodeStub} caller always
unconditionally allocates a source position table. The
{Pipeline::GenerateCodeForTesting} caller would use the conditional allocation.

https://codereview.chromium.org/2555243002/diff/40001/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/2555243002/diff/40001/src/frames.h#newcode565
src/frames.h:565: const StackFrameIteratorBase* iterator_;
nit: AFAICT this is only made protected due to a DCHECK. Can the DCHECK be
written differently (or dropped) so that we don't have to expose this field?

[Clemens Hammacher, 2016-12-08 12:14:48]

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

[commit-bot: I haz the power, 2016-12-08 12:14:54]

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-12-08 12:15:37]

BTW, the Ignition fix landed, so we are also green now :)

https://codereview.chromium.org/2555243002/diff/40001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/2555243002/diff/40001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:147: source_positions_(position_table ? position_table
On 2016/12/08 at 11:38:05, Michael Starzinger wrote:
> nit: Can we move this piece that maybe allocates a source position table into
the caller? The {Pipeline::GenerateCodeForCodeStub} caller always
unconditionally allocates a source position table. The
{Pipeline::GenerateCodeForTesting} caller would use the conditional allocation.

Done. I am wondering though if it would not be better to store a nullptr in the
{PipelineData}, and handle this case whenever source position is looked up.
Allocating the empty source position table and doing a number of lookups on it
might produce some overhead. And GenerateCodeForTesting is not only used for
testing unfortunately.
Maybe we can try to fix this later.

https://codereview.chromium.org/2555243002/diff/40001/src/frames.h
File src/frames.h (right):

https://codereview.chromium.org/2555243002/diff/40001/src/frames.h#newcode565
src/frames.h:565: const StackFrameIteratorBase* iterator_;
On 2016/12/08 at 11:38:05, Michael Starzinger wrote:
> nit: AFAICT this is only made protected due to a DCHECK. Can the DCHECK be
written differently (or dropped) so that we don't have to expose this field?

Sure, I just dropped the DCHECK.

[commit-bot: I haz the power, 2016-12-08 12:43:24]

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

[commit-bot: I haz the power, 2016-12-08 12:43:25]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-12-08 14:07:44]

On 2016/12/08 12:15:37, Clemens Hammacher wrote:
> BTW, the Ignition fix landed, so we are also green now :)
> 
> https://codereview.chromium.org/2555243002/diff/40001/src/compiler/pipeline.cc
> File src/compiler/pipeline.cc (right):
> 
>
https://codereview.chromium.org/2555243002/diff/40001/src/compiler/pipeline.c...
> src/compiler/pipeline.cc:147: source_positions_(position_table ?
position_table
> On 2016/12/08 at 11:38:05, Michael Starzinger wrote:
> > nit: Can we move this piece that maybe allocates a source position table
into
> the caller? The {Pipeline::GenerateCodeForCodeStub} caller always
> unconditionally allocates a source position table. The
> {Pipeline::GenerateCodeForTesting} caller would use the conditional
allocation.
> 
> Done. I am wondering though if it would not be better to store a nullptr in
the
> {PipelineData}, and handle this case whenever source position is looked up.
> Allocating the empty source position table and doing a number of lookups on it
> might produce some overhead. And GenerateCodeForTesting is not only used for
> testing unfortunately.
> Maybe we can try to fix this later.

Can you please add a TODO here?

> 
> https://codereview.chromium.org/2555243002/diff/40001/src/frames.h
> File src/frames.h (right):
> 
> https://codereview.chromium.org/2555243002/diff/40001/src/frames.h#newcode565
> src/frames.h:565: const StackFrameIteratorBase* iterator_;
> On 2016/12/08 at 11:38:05, Michael Starzinger wrote:
> > nit: AFAICT this is only made protected due to a DCHECK. Can the DCHECK be
> written differently (or dropped) so that we don't have to expose this field?
> 
> Sure, I just dropped the DCHECK.

[titzer, 2016-12-08 14:08:51]

lgtm

This needs to have some comments sprinkled around so we don't lose the context
of why this exists.

https://codereview.chromium.org/2555243002/diff/60001/src/asmjs/asm-wasm-buil...
File src/asmjs/asm-wasm-builder.cc (right):

https://codereview.chromium.org/2555243002/diff/60001/src/asmjs/asm-wasm-buil...
src/asmjs/asm-wasm-builder.cc:1404: BinaryOperation* this_parent =
bin_op_parent_;
name this parent_binop?

https://codereview.chromium.org/2555243002/diff/60001/src/asmjs/asm-wasm-buil...
src/asmjs/asm-wasm-builder.cc:1441: int parent_pos = returns_value ?
this_parent->position() : pos;
Why not just

int parent_pos = this_parent ? this_parent->position() : pos;

https://codereview.chromium.org/2555243002/diff/60001/src/asmjs/asm-wasm-buil...
src/asmjs/asm-wasm-builder.cc:1648: bin_op_parent_ = expr;
parent_binop_?

https://codereview.chromium.org/2555243002/diff/60001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/2555243002/diff/60001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:1699: source_positions = new (info->zone())
SourcePositionTable(graph);
I don't think you need this in the CompilationInfo zone...could we add another
temp zone here?

https://codereview.chromium.org/2555243002/diff/60001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2555243002/diff/60001/src/isolate.cc#newcode1566
src/isolate.cc:1566: bool at_tonumber_conversion =
at_to_number_conversion to be consistent with the name in frames.h

[Clemens Hammacher, 2016-12-08 15:25:11]

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

[commit-bot: I haz the power, 2016-12-08 15:25: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-12-08 15:26:21]

https://codereview.chromium.org/2555243002/diff/60001/src/asmjs/asm-wasm-buil...
File src/asmjs/asm-wasm-builder.cc (right):

https://codereview.chromium.org/2555243002/diff/60001/src/asmjs/asm-wasm-buil...
src/asmjs/asm-wasm-builder.cc:1404: BinaryOperation* this_parent =
bin_op_parent_;
On 2016/12/08 at 14:08:50, titzer wrote:
> name this parent_binop?

Done.

https://codereview.chromium.org/2555243002/diff/60001/src/asmjs/asm-wasm-buil...
src/asmjs/asm-wasm-builder.cc:1441: int parent_pos = returns_value ?
this_parent->position() : pos;
On 2016/12/08 at 14:08:50, titzer wrote:
> Why not just
> 
> int parent_pos = this_parent ? this_parent->position() : pos;

The AsmWasmOffsetTable is delta-encoded, so it's actually better to emit the
position of the call expression twice instead of emitting an arbitrary position
(of some previously visited binary operation).
Note that this_parent might also be set for void functions, but then it's not
guaranteed that it's the immediate parent.

https://codereview.chromium.org/2555243002/diff/60001/src/asmjs/asm-wasm-buil...
src/asmjs/asm-wasm-builder.cc:1648: bin_op_parent_ = expr;
On 2016/12/08 at 14:08:50, titzer wrote:
> parent_binop_?

Done.

https://codereview.chromium.org/2555243002/diff/60001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/2555243002/diff/60001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:1699: source_positions = new (info->zone())
SourcePositionTable(graph);
On 2016/12/08 at 14:08:50, titzer wrote:
> I don't think you need this in the CompilationInfo zone...could we add another
temp zone here?

The table will actually never be filled, so there is not much waste here. Just
sizeof(SourcePositionTable). We could also stack-allocate it, and make the
source_positions pointer point to it if it is null.
I dropped a TODO here that we want to remove this anyway, since there is no
point in allocating and passing an empty source position table.

https://codereview.chromium.org/2555243002/diff/60001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2555243002/diff/60001/src/isolate.cc#newcode1566
src/isolate.cc:1566: bool at_tonumber_conversion =
On 2016/12/08 at 14:08:50, titzer wrote:
> at_to_number_conversion to be consistent with the name in frames.h

Done.

[commit-bot: I haz the power, 2016-12-08 15:54:25]

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

[commit-bot: I haz the power, 2016-12-08 15:54:26]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2016-12-08 16:45:55]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-12-08 16:45:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org, mstarzinger@chromium.org, titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2555243002/#ps80001
(title: "Address comments")

[commit-bot: I haz the power, 2016-12-08 16:45: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-12-08 16:47:35]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481215555100550,
"parent_rev": "f6687346c82fe66276e00f64da2a346d6852f260", "commit_rev":
"2d57dedacf6eb063287b93d364938cc8cb16c0fc"}

[commit-bot: I haz the power, 2016-12-08 16:47:41]

Description was changed from

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- during stack trace generation (in the StackTraceIterator), when we
  move from the WASM_TO_JS frame to the WASM frame, we remember at which
  call inside the WASM_TO_JS wrapper we are, and encode this information
  in the generated caller state, used for the WASM frame.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

to

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- during stack trace generation (in the StackTraceIterator), when we
  move from the WASM_TO_JS frame to the WASM frame, we remember at which
  call inside the WASM_TO_JS wrapper we are, and encode this information
  in the generated caller state, used for the WASM frame.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

[commit-bot: I haz the power, 2016-12-08 16:47:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-08 16:48:16]

Description was changed from

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- during stack trace generation (in the StackTraceIterator), when we
  move from the WASM_TO_JS frame to the WASM frame, we remember at which
  call inside the WASM_TO_JS wrapper we are, and encode this information
  in the generated caller state, used for the WASM frame.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

to

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- during stack trace generation (in the StackTraceIterator), when we
  move from the WASM_TO_JS frame to the WASM frame, we remember at which
  call inside the WASM_TO_JS wrapper we are, and encode this information
  in the generated caller state, used for the WASM frame.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

Committed: https://crrev.com/94cd46b55e24fa2bb7b06b3da4d5ba7f029bc262
Cr-Commit-Position: refs/heads/master@{#41599}
==========

[commit-bot: I haz the power, 2016-12-08 16:48:17]

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

[Clemens Hammacher, 2016-12-08 17:33:59]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2563613003/ by clemensh@chromium.org.

The reason for reverting is: gc-stress failures.

[Clemens Hammacher, 2016-12-09 09:45:16]

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

[commit-bot: I haz the power, 2016-12-09 09:45: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-12-09 09:46:10]

Description was changed from

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- during stack trace generation (in the StackTraceIterator), when we
  move from the WASM_TO_JS frame to the WASM frame, we remember at which
  call inside the WASM_TO_JS wrapper we are, and encode this information
  in the generated caller state, used for the WASM frame.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

Committed: https://crrev.com/94cd46b55e24fa2bb7b06b3da4d5ba7f029bc262
Cr-Commit-Position: refs/heads/master@{#41599}
==========

to

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- the StackFrame::State struct now also holds the callee_pc_address,
  which is set in ComputeCallerState. The WASM frame uses this
  information to determine whether the callee frame is WASM_TO_JS, and
  whether that frame is at the ToNumber conversion call.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

Committed: https://crrev.com/94cd46b55e24fa2bb7b06b3da4d5ba7f029bc262
Cr-Commit-Position: refs/heads/master@{#41599}
==========

[Clemens Hammacher, 2016-12-09 10:15:30]

Fixed GC failures by not touching the source position table during stack walk.
Instead, FrameArray::State now stores the callee_pc_address, as discussed with
mstarzinger.

@mstarzinger PTAL, thanks!

[commit-bot: I haz the power, 2016-12-09 10:15:55]

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

[commit-bot: I haz the power, 2016-12-09 10:15:56]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-12-09 10:27:01]

LGTM on patch set 6.

[Clemens Hammacher, 2016-12-09 10:27:51]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-12-09 10:27:51]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org,
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2555243002/#ps90001
(title: "Fix gc error by storing callee_pc_address")

[commit-bot: I haz the power, 2016-12-09 10:27:56]

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-12-09 10:29:53]

CQ is committing da patch.

Bot data: {"patchset_id": 90001, "attempt_start_ts": 1481279271531760,
"parent_rev": "c39644dd0853950cf0a1a0ad316e658be5f0b681", "commit_rev":
"d1b2ef79813d37a2514d0daedc4b9487c6d8706a"}

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

Description was changed from

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- the StackFrame::State struct now also holds the callee_pc_address,
  which is set in ComputeCallerState. The WASM frame uses this
  information to determine whether the callee frame is WASM_TO_JS, and
  whether that frame is at the ToNumber conversion call.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

Committed: https://crrev.com/94cd46b55e24fa2bb7b06b3da4d5ba7f029bc262
Cr-Commit-Position: refs/heads/master@{#41599}
==========

to

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- the StackFrame::State struct now also holds the callee_pc_address,
  which is set in ComputeCallerState. The WASM frame uses this
  information to determine whether the callee frame is WASM_TO_JS, and
  whether that frame is at the ToNumber conversion call.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

Committed: https://crrev.com/94cd46b55e24fa2bb7b06b3da4d5ba7f029bc262
Cr-Commit-Position: refs/heads/master@{#41599}
Review-Url: https://codereview.chromium.org/2555243002
==========

[commit-bot: I haz the power, 2016-12-09 10:29:58]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-12-09 10:30:23]

Description was changed from

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- the StackFrame::State struct now also holds the callee_pc_address,
  which is set in ComputeCallerState. The WASM frame uses this
  information to determine whether the callee frame is WASM_TO_JS, and
  whether that frame is at the ToNumber conversion call.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

Committed: https://crrev.com/94cd46b55e24fa2bb7b06b3da4d5ba7f029bc262
Cr-Commit-Position: refs/heads/master@{#41599}
Review-Url: https://codereview.chromium.org/2555243002
==========

to

==========
[wasm] Fix location for error in asm.js ToNumber conversion

In the asm.js code translated to wasm, we call imported functions via a
WASM_TO_JS stub, which first calls the function and then calls ToNumber
on the return value. Exceptions can happen in both calls.
We were only ever reporting the location of the function call, whereas
asm.js code executed via turbofan reported the location of the type
coercion operator ("+" on "+foo()" or "|" on "foo()|0").

This CL implements the same behaviour for asm.js code translated to
wasm. The following is changed:
- the AsmWasmBuilder records the parent node when descending on a binary
  operator (also "+foo()" is represented by a binary operation).
- it stores not one location per call in the source position side
  table, but two (one for the call, one for the parent which does the
  type coercion).
- the wasm compiler annotates the source positions "0" and "1" to the
  two calls in the WASM_TO_JS wrapper (only if the module origin is
  asm.js).
- the StackFrame::State struct now also holds the callee_pc_address,
  which is set in ComputeCallerState. The WASM frame uses this
  information to determine whether the callee frame is WASM_TO_JS, and
  whether that frame is at the ToNumber conversion call.
- the same information is also stored in the FrameArray which is used
  to reconstruct the stack trace later.

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

Committed: https://crrev.com/94cd46b55e24fa2bb7b06b3da4d5ba7f029bc262
Committed: https://crrev.com/890d28f3615769c3aa82b1bdf7df12c55774f909
Cr-Original-Commit-Position: refs/heads/master@{#41599}
Cr-Commit-Position: refs/heads/master@{#41613}
==========

[commit-bot: I haz the power, 2016-12-09 10:30:24]

Patchset 6 (id:??) landed as
https://crrev.com/890d28f3615769c3aa82b1bdf7df12c55774f909
Cr-Commit-Position: refs/heads/master@{#41613}