[wasm] Add stack checks at the beginning of each function.

[wasm] Add stack checks at the beginning of each function.

TEST=mjsunit/wasm/stack.js:testStackOverflow

R=titzer@chromium.org

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

[ahaas, 2016-08-17 07:55:21]

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

[commit-bot: I haz the power, 2016-08-17 07:55:27]

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-08-17 08:11:44]

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

[commit-bot: I haz the power, 2016-08-17 08:11:45]

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

[titzer, 2016-08-17 08:23:28]

https://codereview.chromium.org/2256603002/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2256603002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:387:
trap_->AddTrapIfFalse(wasm::kTrapStackOverflow, check, position);
We'll need to use the regular JS machinery here and not a WASM trap, since
V8/Blink may "request an interrupt" by changing the stack limit, which is not a
stack overflow.

[ahaas, 2016-08-17 09:18:05]

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

[commit-bot: I haz the power, 2016-08-17 09:18:13]

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

[ahaas, 2016-08-17 09:18:43]

https://codereview.chromium.org/2256603002/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2256603002/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:387:
trap_->AddTrapIfFalse(wasm::kTrapStackOverflow, check, position);
On 2016/08/17 at 08:23:28, titzer wrote:
> We'll need to use the regular JS machinery here and not a WASM trap, since
V8/Blink may "request an interrupt" by changing the stack limit, which is not a
stack overflow.

Done.

[commit-bot: I haz the power, 2016-08-17 09:39:03]

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

[commit-bot: I haz the power, 2016-08-17 09:39:04]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-08-17 11:17:23]

https://codereview.chromium.org/2256603002/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2256603002/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:390: Node* branch = graph()->NewNode(
Can use a Diamond here?

https://codereview.chromium.org/2256603002/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/2256603002/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.h:140: void InitStackCheck(wasm::WasmCodePosition
position);
Why not simply StackCheck?

https://codereview.chromium.org/2256603002/diff/20001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2256603002/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:562: if (builder_) {
Will the BUILD() macro work here?

[ahaas, 2016-08-17 16:12:36]

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

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

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

[ahaas, 2016-08-17 16:12:57]

https://codereview.chromium.org/2256603002/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2256603002/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:390: Node* branch = graph()->NewNode(
On 2016/08/17 at 11:17:23, titzer wrote:
> Can use a Diamond here?

Done.

https://codereview.chromium.org/2256603002/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/2256603002/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.h:140: void InitStackCheck(wasm::WasmCodePosition
position);
On 2016/08/17 at 11:17:23, titzer wrote:
> Why not simply StackCheck?

Done.

https://codereview.chromium.org/2256603002/diff/20001/src/wasm/ast-decoder.cc
File src/wasm/ast-decoder.cc (right):

https://codereview.chromium.org/2256603002/diff/20001/src/wasm/ast-decoder.cc...
src/wasm/ast-decoder.cc:562: if (builder_) {
On 2016/08/17 at 11:17:23, titzer wrote:
> Will the BUILD() macro work here?

Actually not, because InitStackCheck does not return a Node*.

[commit-bot: I haz the power, 2016-08-17 16:46:24]

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

[commit-bot: I haz the power, 2016-08-17 16:46:25]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-08-18 05:52:47]

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

[commit-bot: I haz the power, 2016-08-18 05:52:55]

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-08-18 06:21:42]

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

[commit-bot: I haz the power, 2016-08-18 06:21:44]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-08-18 10:55:09]

lgtm with nit

https://codereview.chromium.org/2256603002/diff/10008/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2256603002/diff/10008/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:410: // Generate a call to the runtime if there is
a stack overflow.
s/stack overflow/stack check failure/

Since, as mentioned in previous comment, the runtime uses stack checks to
interrupt running programs.

[ahaas, 2016-08-18 10:58:55]

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

[commit-bot: I haz the power, 2016-08-18 10:59:07]

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

[ahaas, 2016-08-18 11:04:07]

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

[commit-bot: I haz the power, 2016-08-18 11:04:09]

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-08-18 11:26:59]

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

[commit-bot: I haz the power, 2016-08-18 11:27:00]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-08-18 17:09:28]

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

[commit-bot: I haz the power, 2016-08-18 17:09:30]

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

[ahaas, 2016-08-18 17:10:49]

PTAL, I changed the {StackCheck} function to use the {BuildCallToRuntime}
function instead of duplicating that code.

https://codereview.chromium.org/2256603002/diff/10008/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2256603002/diff/10008/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:410: // Generate a call to the runtime if there is
a stack overflow.
On 2016/08/18 at 10:55:09, titzer wrote:
> s/stack overflow/stack check failure/
> 
> Since, as mentioned in previous comment, the runtime uses stack checks to
interrupt running programs.

Done.

[Michael Starzinger, 2016-08-18 17:22:48]

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

[Michael Starzinger, 2016-08-18 17:22:49]

https://codereview.chromium.org/2256603002/diff/110001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2256603002/diff/110001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:413: effect_, control_);
This should be s/control_/diamond.if_false/ here, otherwise it is pure luck that
the runtime call is scheduled inside the else branch. Unconditionally calling
the runtime would be a valid schedule too.

[Michael Starzinger, 2016-08-18 17:23:42]

Description was changed from

==========
[wasm] Add stack checks at the beginning of each function.

TEST=mjsunit/wasm/stack.js:testStackOverflow

R=titzer@chromium.org
==========

to

==========
[wasm] Add stack checks at the beginning of each function.

TEST=mjsunit/wasm/stack.js:testStackOverflow

R=titzer@chromium.org
==========

[Michael Starzinger, 2016-08-18 17:23:42]

mstarzinger@chromium.org changed reviewers:
- mstarzinger@chromium.org

[commit-bot: I haz the power, 2016-08-18 17:30:40]

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

[commit-bot: I haz the power, 2016-08-18 17:30:41]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-08-19 07:13:27]

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

[commit-bot: I haz the power, 2016-08-19 07:13:35]

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

[ahaas, 2016-08-19 07:13:42]

https://codereview.chromium.org/2256603002/diff/110001/src/compiler/wasm-comp...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2256603002/diff/110001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:413: effect_, control_);
On 2016/08/18 at 17:22:48, Michael Starzinger wrote:
> This should be s/control_/diamond.if_false/ here, otherwise it is pure luck
that the runtime call is scheduled inside the else branch. Unconditionally
calling the runtime would be a valid schedule too.

Thanks for noticing this. I fixed it now.

[commit-bot: I haz the power, 2016-08-19 08:11:18]

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

[commit-bot: I haz the power, 2016-08-19 08:11:19]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-08-19 08:49:26]

On 2016/08/19 07:13:42, ahaas wrote:
>
https://codereview.chromium.org/2256603002/diff/110001/src/compiler/wasm-comp...
> File src/compiler/wasm-compiler.cc (right):
> 
>
https://codereview.chromium.org/2256603002/diff/110001/src/compiler/wasm-comp...
> src/compiler/wasm-compiler.cc:413: effect_, control_);
> On 2016/08/18 at 17:22:48, Michael Starzinger wrote:
> > This should be s/control_/diamond.if_false/ here, otherwise it is pure luck
> that the runtime call is scheduled inside the else branch. Unconditionally
> calling the runtime would be a valid schedule too.
> 
> Thanks for noticing this. I fixed it now.

lgtm

[ahaas, 2016-08-19 08:52:39]

The CQ bit was checked by ahaas@chromium.org

[commit-bot: I haz the power, 2016-08-19 08:52:50]

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-08-19 08:54:27]

Description was changed from

==========
[wasm] Add stack checks at the beginning of each function.

TEST=mjsunit/wasm/stack.js:testStackOverflow

R=titzer@chromium.org
==========

to

==========
[wasm] Add stack checks at the beginning of each function.

TEST=mjsunit/wasm/stack.js:testStackOverflow

R=titzer@chromium.org
==========

[commit-bot: I haz the power, 2016-08-19 08:54:28]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2016-08-19 08:54:49]

Description was changed from

==========
[wasm] Add stack checks at the beginning of each function.

TEST=mjsunit/wasm/stack.js:testStackOverflow

R=titzer@chromium.org
==========

to

==========
[wasm] Add stack checks at the beginning of each function.

TEST=mjsunit/wasm/stack.js:testStackOverflow

R=titzer@chromium.org

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

[commit-bot: I haz the power, 2016-08-19 08:54:51]

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