[wasm] Throw real Error on wasm trap

[wasm] Throw real Error on wasm trap

Before, just a string was thrown, so no stack trace was attached there.
Generated code from wasm does not grow by this change, we just pass a
message id to the respective (new) runtime function.

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

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

[Michael Starzinger, 2016-04-11 18:13:27]

LGTM with nits.

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

https://codereview.chromium.org/1874383002/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:101: DCHECK(args.length() == 1);
nit: I know we are inconsistent, but lets use "DCHECK_EQ(1, args.length())" for
new code.

https://codereview.chromium.org/1874383002/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:107: return isolate->Throw(*exception);
nit: This can also be written as ...

THROW_NEW_ERROR_RETURN_FAILURE(isolate,
NewError(static_cast<MessageTemplate::Template>(message_id)));

[Clemens Hammacher, 2016-04-12 09:33:44]

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

https://codereview.chromium.org/1874383002/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:101: DCHECK(args.length() == 1);
On 2016/04/11 at 18:13:26, Michael Starzinger wrote:
> nit: I know we are inconsistent, but lets use "DCHECK_EQ(1, args.length())"
for new code.

Sure, didn't see that.

https://codereview.chromium.org/1874383002/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:107: return isolate->Throw(*exception);
On 2016/04/11 at 18:13:26, Michael Starzinger wrote:
> nit: This can also be written as ...
> 
> THROW_NEW_ERROR_RETURN_FAILURE(isolate,
NewError(static_cast<MessageTemplate::Template>(message_id)));

and that's considered nicer? ;)
but OK, I changed it :)

[Michael Starzinger, 2016-04-12 10:47:56]

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

https://codereview.chromium.org/1874383002/diff/1/src/runtime/runtime-interna...
src/runtime/runtime-internal.cc:107: return isolate->Throw(*exception);
On 2016/04/12 09:33:44, Clemens Hammacher wrote:
> On 2016/04/11 at 18:13:26, Michael Starzinger wrote:
> > nit: This can also be written as ...
> > 
> > THROW_NEW_ERROR_RETURN_FAILURE(isolate,
> NewError(static_cast<MessageTemplate::Template>(message_id)));
> 
> and that's considered nicer? ;)
> but OK, I changed it :)

We can discuss which version is nicer and change the existing code to use
Isolate::Throw directly instead of the macro. But having part of the code use
the macro and other parts of the code use Isolate::Throw is not a "nice"
situation. IMHO keeping this call-site in sync with existing code trumps having
this one call-site be "nice".

Also I realize that I am contradicting myself here with the comment above where
I say DCHECK_EQ should be used "at this one call-site". But that DCHECK_EQ above
is one incremental cleanup that some of is collectively agreed to, to eventually
get into a state where the "readability/check" linter check passes. We did not
(yet) agree to cleanup Isolate::Throw in favor of the
THROW_NEW_ERROR_RETURN_FAILURE macro.

Does that make sense or do you think I am completely off here?

[Clemens Hammacher, 2016-04-12 11:02:35]

On 2016/04/12 at 10:47:56, mstarzinger wrote:
>
https://codereview.chromium.org/1874383002/diff/1/src/runtime/runtime-interna...
> File src/runtime/runtime-internal.cc (right):
> 
>
https://codereview.chromium.org/1874383002/diff/1/src/runtime/runtime-interna...
> src/runtime/runtime-internal.cc:107: return isolate->Throw(*exception);
> On 2016/04/12 09:33:44, Clemens Hammacher wrote:
> > On 2016/04/11 at 18:13:26, Michael Starzinger wrote:
> > > nit: This can also be written as ...
> > > 
> > > THROW_NEW_ERROR_RETURN_FAILURE(isolate,
> > NewError(static_cast<MessageTemplate::Template>(message_id)));
> > 
> > and that's considered nicer? ;)
> > but OK, I changed it :)
> 
> We can discuss which version is nicer and change the existing code to use
Isolate::Throw directly instead of the macro. But having part of the code use
the macro and other parts of the code use Isolate::Throw is not a "nice"
situation. IMHO keeping this call-site in sync with existing code trumps having
this one call-site be "nice".
> 
> Also I realize that I am contradicting myself here with the comment above
where I say DCHECK_EQ should be used "at this one call-site". But that DCHECK_EQ
above is one incremental cleanup that some of is collectively agreed to, to
eventually get into a state where the "readability/check" linter check passes.
We did not (yet) agree to cleanup Isolate::Throw in favor of the
THROW_NEW_ERROR_RETURN_FAILURE macro.
> 
> Does that make sense or do you think I am completely off here?

No, you are right. It's always a trade-off. But since the macro is there, and
the surrounding code uses it, it totally makes sense to also use it in the new
code.
IMHO this macro does not save any typing effort, and does not make the code more
readable, but the decision to introduce it has been made by "the team" or at
least parts of it, so of course I am fine with that.

Thanks for the detailed answer!

[titzer, 2016-04-18 16:22:13]

lgtm

https://codereview.chromium.org/1874383002/diff/40001/src/runtime/runtime-int...
File src/runtime/runtime-internal.cc (right):

https://codereview.chromium.org/1874383002/diff/40001/src/runtime/runtime-int...
src/runtime/runtime-internal.cc:103: // TODO(clemensh): discuss with titzer
whether a WasmError class would make
When in doubt, leave it out :-)

[Clemens Hammacher, 2016-04-20 14:26:45]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2016-04-20 14:26:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/1874383002/#ps60001
(title: "remove a TODO")

[commit-bot: I haz the power, 2016-04-20 14:26:49]

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

[commit-bot: I haz the power, 2016-04-20 14:51:42]

Committed patchset #4 (id:60001)

[nodir1, 2016-04-22 18:44:22]

Description was changed from

==========
[wasm] Throw real Error on wasm trap

Before, just a string was thrown, so no stack trace was attached there.
Generated code from wasm does not grow by this change, we just pass a
message id to the respective (new) runtime function.

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

to

==========
[wasm] Throw real Error on wasm trap

Before, just a string was thrown, so no stack trace was attached there.
Generated code from wasm does not grow by this change, we just pass a
message id to the respective (new) runtime function.

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

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

[nodir1, 2016-04-22 18:44:23]

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