[WASM] Implements catch for the wasm low level exception mechanism.

[WASM] Implements catch for the wasm low level exception mechanism.

BUG=

Committed: https://crrev.com/93e5425c46453764779bd557628d61fae670027a
Cr-Commit-Position: refs/heads/master@{#39881}

[John, 2016-09-21 14:07:10]

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

[commit-bot: I haz the power, 2016-09-21 14:07: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-09-21 14:35:40]

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

[commit-bot: I haz the power, 2016-09-21 14:35:41]

Dry run: This issue passed the CQ dry run.

[John, 2016-09-21 19:55:01]

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

[commit-bot: I haz the power, 2016-09-21 19:55:08]

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-09-21 20:11:05]

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

[commit-bot: I haz the power, 2016-09-21 20:11:06]

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...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[John, 2016-09-21 21:03:11]

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

[commit-bot: I haz the power, 2016-09-21 21:03:15]

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-09-21 21:38:03]

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

[commit-bot: I haz the power, 2016-09-21 21:38:04]

Dry run: This issue passed the CQ dry run.

[John, 2016-09-26 17:14:34]

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

[commit-bot: I haz the power, 2016-09-26 17:14:41]

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

[John, 2016-09-26 17:37:34]

jpp@chromium.org changed reviewers:
+ bradnelson@chromium.org, titzer@chromium.org

[commit-bot: I haz the power, 2016-09-26 17:48:04]

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

[commit-bot: I haz the power, 2016-09-26 17:48:05]

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

[bradnelson, 2016-09-27 04:25:56]

Mystified by failures.
Will look more tomorrow.

https://codereview.chromium.org/2275293002/diff/220001/src/compiler/instructi...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/2275293002/diff/220001/src/compiler/instructi...
src/compiler/instruction-selector.cc:1700: LinkageLocation ExceptionLocation() {
Could land this as a separate fix if you link (rolled in is fine too)?

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

https://codereview.chromium.org/2275293002/diff/220001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1737: Node* value =
BuildCallToRuntime(Runtime::kWasmCatch, jsgraph(),
I think this might be a problem (defer to Ben / if he can grab hpayer).
If this runtime function can return a heap value, I think that makes it
problematic if a GC collect comes by,
as it will be invisible to the collector (in a wasm frame. This may be ok, as
during the throw there are probably always other roots, but wanted to double
check?

https://codereview.chromium.org/2275293002/diff/220001/test/mjsunit/wasm/exce...
File test/mjsunit/wasm/exceptions.js (right):

https://codereview.chromium.org/2275293002/diff/220001/test/mjsunit/wasm/exce...
test/mjsunit/wasm/exceptions.js:221: kExprTry,
Shame how verbose these are.
We've been chatting about adding a more ergonomic / clever assembler for wasm in
C++.
Something in JS might be nice too.
Sigh (nothing for you to do here).

[John, 2016-09-27 14:23:43]

Description was changed from

==========
[WASM] Implements catch for the wasm low level exception mechanism.

BUG=
==========

to

==========
[WASM] Implements catch for the wasm low level exception mechanism.

BUG=
==========

[titzer, 2016-09-28 12:53:31]

Looks mostly good from my side.

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

https://codereview.chromium.org/2275293002/diff/220001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1737: Node* value =
BuildCallToRuntime(Runtime::kWasmCatch, jsgraph(),
On 2016/09/27 04:25:56, bradnelson wrote:
> I think this might be a problem (defer to Ben / if he can grab hpayer).
> If this runtime function can return a heap value, I think that makes it
> problematic if a GC collect comes by,
> as it will be invisible to the collector (in a wasm frame. This may be ok, as
> during the throw there are probably always other roots, but wanted to double
> check?

The collector does walk WASM frames and process reference maps in them. The only
problem is that it doesn't find references in outgoing stack parameter slots. So
as long as you don't push a reference onto the stack (e.g. to call the runtime
directly from WASM functions), this is OK. Even passing references in registers
is OK.

https://codereview.chromium.org/2275293002/diff/220001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2275293002/diff/220001/src/isolate.cc#newcode...
src/isolate.cc:1172: if (frame->is_wasm() && catchable_by_wasm) {
Maybe inline this condition? Since frame->is_wasm() is probably very often
false, that avoids evaluating the other conditions in the common case.

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

https://codereview.chromium.org/2275293002/diff/220001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:1454: SsaEnv* success_env = Steal(ssa_env_);
Is it possible to move the building of the projections into the graph builder,
having it return the if_success and if_exception projections, a bit like how ifs
are handled?

https://codereview.chromium.org/2275293002/diff/220001/test/mjsunit/wasm/exce...
File test/mjsunit/wasm/exceptions.js (right):

https://codereview.chromium.org/2275293002/diff/220001/test/mjsunit/wasm/exce...
test/mjsunit/wasm/exceptions.js:327:
kExprI32Const].concat(varint32(-1)).concat([
The concats here are a little messy. Maybe just always output the 5-byte version
of a constant?

[John, 2016-09-28 13:11:26]

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

https://codereview.chromium.org/2275293002/diff/220001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:1454: SsaEnv* success_env = Steal(ssa_env_);
On 2016/09/28 12:53:31, titzer wrote:
> Is it possible to move the building of the projections into the graph builder,
> having it return the if_success and if_exception projections, a bit like how
ifs
> are handled?

I don't understand... are you suggesting I should have a method for creating
IfException/IfSuccess instead of one for each?

[John, 2016-09-28 13:37:18]

https://codereview.chromium.org/2275293002/diff/220001/src/compiler/instructi...
File src/compiler/instruction-selector.cc (right):

https://codereview.chromium.org/2275293002/diff/220001/src/compiler/instructi...
src/compiler/instruction-selector.cc:1700: LinkageLocation ExceptionLocation() {
On 2016/09/27 04:25:56, bradnelson wrote:
> Could land this as a separate fix if you link (rolled in is fine too)?

Small enough, and mildly related to this CL... I'll land here.

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

https://codereview.chromium.org/2275293002/diff/220001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1737: Node* value =
BuildCallToRuntime(Runtime::kWasmCatch, jsgraph(),
On 2016/09/28 12:53:31, titzer wrote:
> On 2016/09/27 04:25:56, bradnelson wrote:
> > I think this might be a problem (defer to Ben / if he can grab hpayer).
> > If this runtime function can return a heap value, I think that makes it
> > problematic if a GC collect comes by,
> > as it will be invisible to the collector (in a wasm frame. This may be ok,
as
> > during the throw there are probably always other roots, but wanted to double
> > check?
> 
> The collector does walk WASM frames and process reference maps in them. The
only
> problem is that it doesn't find references in outgoing stack parameter slots.
So
> as long as you don't push a reference onto the stack (e.g. to call the runtime
> directly from WASM functions), this is OK. Even passing references in
registers
> is OK.

Acknowledged.

https://codereview.chromium.org/2275293002/diff/220001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2275293002/diff/220001/src/isolate.cc#newcode...
src/isolate.cc:1172: if (frame->is_wasm() && catchable_by_wasm) {
On 2016/09/28 12:53:31, titzer wrote:
> Maybe inline this condition? Since frame->is_wasm() is probably very often
> false, that avoids evaluating the other conditions in the common case.

I am cargo-culting on the catchable_by_js. I also find it more clear to read

catchable_by_wasm

than reading an expression in an if condition. I'll leave as is, but I will
change if you feel strongly about it.

https://codereview.chromium.org/2275293002/diff/220001/test/mjsunit/wasm/exce...
File test/mjsunit/wasm/exceptions.js (right):

https://codereview.chromium.org/2275293002/diff/220001/test/mjsunit/wasm/exce...
test/mjsunit/wasm/exceptions.js:221: kExprTry,
On 2016/09/27 04:25:56, bradnelson wrote:
> Shame how verbose these are.
> We've been chatting about adding a more ergonomic / clever assembler for wasm
in
> C++.
> Something in JS might be nice too.
> Sigh (nothing for you to do here).

Acknowledged.

https://codereview.chromium.org/2275293002/diff/220001/test/mjsunit/wasm/exce...
test/mjsunit/wasm/exceptions.js:327:
kExprI32Const].concat(varint32(-1)).concat([
On 2016/09/28 12:53:31, titzer wrote:
> The concats here are a little messy. Maybe just always output the 5-byte
version
> of a constant?

Done.

[titzer, 2016-09-28 14:08:20]

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

https://codereview.chromium.org/2275293002/diff/220001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:1454: SsaEnv* success_env = Steal(ssa_env_);
On 2016/09/28 13:11:26, John wrote:
> On 2016/09/28 12:53:31, titzer wrote:
> > Is it possible to move the building of the projections into the graph
builder,
> > having it return the if_success and if_exception projections, a bit like how
> ifs
> > are handled?
> 
> I don't understand... are you suggesting I should have a method for creating
> IfException/IfSuccess instead of one for each?

Yes.

[John, 2016-09-28 16:45:04]

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

[commit-bot: I haz the power, 2016-09-28 16:45:13]

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

[John, 2016-09-28 16:45:17]

ptal

[titzer, 2016-09-28 17:09:14]

lgtm with comments

https://codereview.chromium.org/2275293002/diff/260001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2275293002/diff/260001/src/isolate.cc#newcode...
src/isolate.cc:1173: if (frame->is_wasm() && catchable_by_wasm) {
What about nesting this whole thing inside a if(FLAG_eh_wasm_prototype) ?

That way, it is essentially no overhead without that flag on.

https://codereview.chromium.org/2275293002/diff/260001/src/runtime/runtime-wa...
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2275293002/diff/260001/src/runtime/runtime-wa...
src/runtime/runtime-wasm.cc:132: RUNTIME_FUNCTION(Runtime_WasmCatch) {
Can we name this Runtime_WasmGetCaughtExceptionValue (or something similar)?

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

https://codereview.chromium.org/2275293002/diff/260001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:104: TryInfo* try_info;  // Information used for
compiling try statements.
This will of course, require rebase onto 0xC.

[commit-bot: I haz the power, 2016-09-28 17:12:44]

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

[commit-bot: I haz the power, 2016-09-28 17:12:45]

Dry run: This issue passed the CQ dry run.

[John, 2016-09-29 13:28:26]

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

[John, 2016-09-29 13:28:31]

https://codereview.chromium.org/2275293002/diff/260001/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2275293002/diff/260001/src/isolate.cc#newcode...
src/isolate.cc:1173: if (frame->is_wasm() && catchable_by_wasm) {
On 2016/09/28 17:09:14, titzer wrote:
> What about nesting this whole thing inside a if(FLAG_eh_wasm_prototype) ?
> 
> That way, it is essentially no overhead without that flag on.

Done.

https://codereview.chromium.org/2275293002/diff/260001/src/runtime/runtime-wa...
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2275293002/diff/260001/src/runtime/runtime-wa...
src/runtime/runtime-wasm.cc:132: RUNTIME_FUNCTION(Runtime_WasmCatch) {
On 2016/09/28 17:09:14, titzer wrote:
> Can we name this Runtime_WasmGetCaughtExceptionValue (or something similar)?

Done.

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

https://codereview.chromium.org/2275293002/diff/260001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:104: TryInfo* try_info;  // Information used for
compiling try statements.
On 2016/09/28 17:09:14, titzer wrote:
> This will of course, require rebase onto 0xC.

Oh, this was painful... <rant />

[commit-bot: I haz the power, 2016-09-29 13:28:32]

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

[titzer, 2016-09-29 13:33:59]

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

https://codereview.chromium.org/2275293002/diff/280001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2045: Node*
WasmGraphBuilder::BuildWasmCall(wasm::FunctionSig* sig, Node** args,
I think this would be cleaner if it returned the return value array as before
and took a double pointer to the call node.

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

https://codereview.chromium.org/2275293002/diff/280001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:106: TFNode* node;            // result node for the
construct.
You can drop the node and type now, since this is encapsulated in MergeValues.

[John, 2016-09-29 13:40:12]

The CQ bit was unchecked by jpp@chromium.org

[John, 2016-09-29 13:52:05]

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

https://codereview.chromium.org/2275293002/diff/280001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:2045: Node*
WasmGraphBuilder::BuildWasmCall(wasm::FunctionSig* sig, Node** args,
On 2016/09/29 13:33:59, titzer wrote:
> I think this would be cleaner if it returned the return value array as before
> and took a double pointer to the call node.

I made this change because of the way this is used in ast-builder.

Essentially, BUILD() dispatches the returned value to CheckForException, which
in turn performs the exception handling wiring. I can change this, but then the
ast-builder's code for creating calls will not look like the rest. It will
become something like:

if (build()) {
  Node** call = nullptr;
  Node** rets = BUILD(CallDirect, ..., &call, ...);
  CheckForException(call);
}

Leaving as is, per offline discussion.

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

https://codereview.chromium.org/2275293002/diff/280001/src/wasm/ast-decoder.c...
src/wasm/ast-decoder.cc:106: TFNode* node;            // result node for the
construct.
On 2016/09/29 13:33:59, titzer wrote:
> You can drop the node and type now, since this is encapsulated in MergeValues.

Done.

[John, 2016-09-29 13:52:12]

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

[commit-bot: I haz the power, 2016-09-29 13:52:21]

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

[titzer, 2016-09-29 13:55:35]

lgtm

[commit-bot: I haz the power, 2016-09-29 14:25:42]

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

[commit-bot: I haz the power, 2016-09-29 14:25:43]

Dry run: This issue passed the CQ dry run.

[John, 2016-09-29 15:56:37]

The CQ bit was checked by jpp@chromium.org

[commit-bot: I haz the power, 2016-09-29 15:56:48]

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-09-29 15:59:19]

Description was changed from

==========
[WASM] Implements catch for the wasm low level exception mechanism.

BUG=
==========

to

==========
[WASM] Implements catch for the wasm low level exception mechanism.

BUG=
==========

[commit-bot: I haz the power, 2016-09-29 15:59:20]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-09-29 15:59:43]

Description was changed from

==========
[WASM] Implements catch for the wasm low level exception mechanism.

BUG=
==========

to

==========
[WASM] Implements catch for the wasm low level exception mechanism.

BUG=

Committed: https://crrev.com/93e5425c46453764779bd557628d61fae670027a
Cr-Commit-Position: refs/heads/master@{#39881}
==========

[commit-bot: I haz the power, 2016-09-29 15:59:44]

Patchset 16 (id:??) landed as
https://crrev.com/93e5425c46453764779bd557628d61fae670027a
Cr-Commit-Position: refs/heads/master@{#39881}

[Michael Achenbach, 2016-09-29 18:00:15]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/2383613002/ by machenbach@chromium.org.

The reason for reverting is: nosse4 errors:
https://build.chromium.org/p/client.v8/builders/V8%20Linux/builds/13524
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20debug/builds....