Handle stack frames differently inside and on the boundary of wasm.

Handle stack frames differently inside and on the boundary of wasm.

Frames entering of inside wasm don't have a function or context argument.
Adding distinct wasm frame and function types to express this.

Fixes a GC issue on several embenchen wasm tests, reenabling them.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/wasm/embenchen
R=titzer@chromium.org,aseemgarg@chromium.org,jfb@chromium.org,yangguo@chromium.org
LOG=N

Committed: https://crrev.com/530cc16460b14544760c06484a4aec2413cbd79d
Cr-Commit-Position: refs/heads/master@{#34476}

[bradn, 2016-03-03 20:43:56]

Description was changed from

==========
Handle stack frames differently inside and on the boundary of wasm.

Frames entering of inside wasm don't have a function or context argument.
Adding distinct wasm frame and function types to express this.

Fixes a GC issue on several embenchen wasm tests, reenabling them.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/wasm/embenchen
R=titzer@chromium.org,aseemgarg@chromium.org
LOG=N
==========

to

==========
Handle stack frames differently inside and on the boundary of wasm.

Frames entering of inside wasm don't have a function or context argument.
Adding distinct wasm frame and function types to express this.

Fixes a GC issue on several embenchen wasm tests, reenabling them.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/wasm/embenchen
R=titzer@chromium.org,aseemgarg@chromium.org
LOG=N
==========

[bradn, 2016-03-03 20:43:56]

bradnelson@google.com changed reviewers:
+ jfb@chromium.org

[bradn, 2016-03-03 20:44:16]

Description was changed from

==========
Handle stack frames differently inside and on the boundary of wasm.

Frames entering of inside wasm don't have a function or context argument.
Adding distinct wasm frame and function types to express this.

Fixes a GC issue on several embenchen wasm tests, reenabling them.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/wasm/embenchen
R=titzer@chromium.org,aseemgarg@chromium.org
LOG=N
==========

to

==========
Handle stack frames differently inside and on the boundary of wasm.

Frames entering of inside wasm don't have a function or context argument.
Adding distinct wasm frame and function types to express this.

Fixes a GC issue on several embenchen wasm tests, reenabling them.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/wasm/embenchen
R=titzer@chromium.org,aseemgarg@chromium.org,jfb@chromium.org
LOG=N
==========

[bradn, 2016-03-03 20:45:27]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[JF, 2016-03-03 21:11:52]

Nice, lgtm besides C++ism.

https://codereview.chromium.org/1764603003/diff/20001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1764603003/diff/20001/src/frames.cc#newcode469
src/frames.cc:469: UNREACHABLE();
You'll already get a warning if you don't cover all enumerators, because
wasm_function_type() returns an enum. I'd drop the default: any other value is
UB anyways.

[titzer, 2016-03-03 21:36:21]

https://codereview.chromium.org/1764603003/diff/20001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1764603003/diff/20001/src/frames.cc#newcode469
src/frames.cc:469: UNREACHABLE();
On 2016/03/03 21:11:52, JF wrote:
> You'll already get a warning if you don't cover all enumerators, because
> wasm_function_type() returns an enum. I'd drop the default: any other value is
> UB anyways.

Better safe than sorry.

[JF, 2016-03-03 21:38:46]

https://codereview.chromium.org/1764603003/diff/20001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1764603003/diff/20001/src/frames.cc#newcode469
src/frames.cc:469: UNREACHABLE();
On 2016/03/03 21:36:20, titzer wrote:
> On 2016/03/03 21:11:52, JF wrote:
> > You'll already get a warning if you don't cover all enumerators, because
> > wasm_function_type() returns an enum. I'd drop the default: any other value
is
> > UB anyways.
> 
> Better safe than sorry. 

Right, what I'm saying is that having `default: UNREACHABLE();` makes it a
runtime failure, whereas not having a default makes it a compile-time failure.

[bradn, 2016-03-03 21:39:10]

https://codereview.chromium.org/1764603003/diff/20001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1764603003/diff/20001/src/frames.cc#newcode469
src/frames.cc:469: UNREACHABLE();
On 2016/03/03 21:11:52, JF wrote:
> You'll already get a warning if you don't cover all enumerators, because
> wasm_function_type() returns an enum. I'd drop the default: any other value is
> UB anyways.

I'm not sure I share your trust in the compiler :-), given there is more than
one.

[JF, 2016-03-03 21:58:33]

https://codereview.chromium.org/1764603003/diff/20001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/1764603003/diff/20001/src/frames.cc#newcode469
src/frames.cc:469: UNREACHABLE();
On 2016/03/03 21:39:10, bradn wrote:
> On 2016/03/03 21:11:52, JF wrote:
> > You'll already get a warning if you don't cover all enumerators, because
> > wasm_function_type() returns an enum. I'd drop the default: any other value
is
> > UB anyways.
> 
> I'm not sure I share your trust in the compiler :-), given there is more than
> one.

All you need is one to give you the warning. The bots will do that. But eh, if
you'd rather fail at runtime I reserve the right to point and laugh :)

[titzer, 2016-03-03 21:59:52]

On 2016/03/03 21:58:33, JF wrote:
> https://codereview.chromium.org/1764603003/diff/20001/src/frames.cc
> File src/frames.cc (right):
> 
> https://codereview.chromium.org/1764603003/diff/20001/src/frames.cc#newcode469
> src/frames.cc:469: UNREACHABLE();
> On 2016/03/03 21:39:10, bradn wrote:
> > On 2016/03/03 21:11:52, JF wrote:
> > > You'll already get a warning if you don't cover all enumerators, because
> > > wasm_function_type() returns an enum. I'd drop the default: any other
value
> is
> > > UB anyways.
> > 
> > I'm not sure I share your trust in the compiler :-), given there is more
than
> > one.
> 
> All you need is one to give you the warning. The bots will do that. But eh, if
> you'd rather fail at runtime I reserve the right to point and laugh :)

I think the issue is that some compilers won't issue the warning, *and* you will
fail at runtime; worst of both worlds.

[titzer, 2016-03-03 22:48:21]

https://codereview.chromium.org/1764603003/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1764603003/diff/40001/src/objects.h#newcode4911
src/objects.h:4911: enum WasmFunctionType {
After having a look at this, I think it's actually cleaner to have separate
Code::Kind's for each of the different cases. It probably means that the code
kind is going to occupy another bit, but that saves this extra part of the enum.
It's less impact everywhere else, too.

[bradn, 2016-03-03 23:17:29]

https://codereview.chromium.org/1764603003/diff/40001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/1764603003/diff/40001/src/objects.h#newcode4911
src/objects.h:4911: enum WasmFunctionType {
On 2016/03/03 22:48:21, titzer wrote:
> After having a look at this, I think it's actually cleaner to have separate
> Code::Kind's for each of the different cases. It probably means that the code
> kind is going to occupy another bit, but that saves this extra part of the
enum.
> It's less impact everywhere else, too.

Done.

And it has the added benefit of rendering the prior debate moot :-)

[titzer, 2016-03-03 23:25:05]

lgtm, thanks!

https://codereview.chromium.org/1764603003/diff/60001/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/1764603003/diff/60001/src/log.cc#newcode1583
src/log.cc:1583: description = "A JavaScript to Wasm thunk";
thunk sounds so violent! We generally use the term adapter for such frames.

[bradn, 2016-03-03 23:28:41]

The CQ bit was checked by bradnelson@google.com

[bradn, 2016-03-03 23:28:42]

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

[commit-bot: I haz the power, 2016-03-03 23:28:44]

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

[bradn, 2016-03-03 23:28:49]

https://codereview.chromium.org/1764603003/diff/60001/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/1764603003/diff/60001/src/log.cc#newcode1583
src/log.cc:1583: description = "A JavaScript to Wasm thunk";
On 2016/03/03 23:25:05, titzer wrote:
> thunk sounds so violent! We generally use the term adapter for such frames.

Done.

[commit-bot: I haz the power, 2016-03-03 23:32:20]

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

[commit-bot: I haz the power, 2016-03-03 23:32:21]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11843)

[bradn, 2016-03-03 23:35:04]

Description was changed from

==========
Handle stack frames differently inside and on the boundary of wasm.

Frames entering of inside wasm don't have a function or context argument.
Adding distinct wasm frame and function types to express this.

Fixes a GC issue on several embenchen wasm tests, reenabling them.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/wasm/embenchen
R=titzer@chromium.org,aseemgarg@chromium.org,jfb@chromium.org
LOG=N
==========

to

==========
Handle stack frames differently inside and on the boundary of wasm.

Frames entering of inside wasm don't have a function or context argument.
Adding distinct wasm frame and function types to express this.

Fixes a GC issue on several embenchen wasm tests, reenabling them.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/wasm/embenchen
R=titzer@chromium.org,aseemgarg@chromium.org,jfb@chromium.org,yangguo@chromiu...
LOG=N
==========

[bradn, 2016-03-03 23:35:04]

bradnelson@google.com changed reviewers:
+ yangguo@chromium.org

[bradn, 2016-03-03 23:35:20]

+yangguo for OWNERS

[Yang, 2016-03-04 04:42:29]

On 2016/03/03 23:35:20, bradn wrote:
> +yangguo for OWNERS

lgtm

[bradn, 2016-03-04 04:43:14]

The CQ bit was checked by bradnelson@google.com

[commit-bot: I haz the power, 2016-03-04 04:43:26]

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

[commit-bot: I haz the power, 2016-03-04 04:45:26]

Description was changed from

==========
Handle stack frames differently inside and on the boundary of wasm.

Frames entering of inside wasm don't have a function or context argument.
Adding distinct wasm frame and function types to express this.

Fixes a GC issue on several embenchen wasm tests, reenabling them.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/wasm/embenchen
R=titzer@chromium.org,aseemgarg@chromium.org,jfb@chromium.org,yangguo@chromiu...
LOG=N
==========

to

==========
Handle stack frames differently inside and on the boundary of wasm.

Frames entering of inside wasm don't have a function or context argument.
Adding distinct wasm frame and function types to express this.

Fixes a GC issue on several embenchen wasm tests, reenabling them.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/wasm/embenchen
R=titzer@chromium.org,aseemgarg@chromium.org,jfb@chromium.org,yangguo@chromiu...
LOG=N
==========

[commit-bot: I haz the power, 2016-03-04 04:45:27]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-04 04:45:51]

Description was changed from

==========
Handle stack frames differently inside and on the boundary of wasm.

Frames entering of inside wasm don't have a function or context argument.
Adding distinct wasm frame and function types to express this.

Fixes a GC issue on several embenchen wasm tests, reenabling them.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/wasm/embenchen
R=titzer@chromium.org,aseemgarg@chromium.org,jfb@chromium.org,yangguo@chromiu...
LOG=N
==========

to

==========
Handle stack frames differently inside and on the boundary of wasm.

Frames entering of inside wasm don't have a function or context argument.
Adding distinct wasm frame and function types to express this.

Fixes a GC issue on several embenchen wasm tests, reenabling them.

BUG= https://code.google.com/p/v8/issues/detail?id=4203
TEST=mjsunit/wasm/embenchen
R=titzer@chromium.org,aseemgarg@chromium.org,jfb@chromium.org,yangguo@chromiu...
LOG=N

Committed: https://crrev.com/530cc16460b14544760c06484a4aec2413cbd79d
Cr-Commit-Position: refs/heads/master@{#34476}
==========

[commit-bot: I haz the power, 2016-03-04 04:45:52]

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