[wasm] Move all heap-allocated WASM structures into wasm-objects.h.

[wasm] Move all heap-allocated WASM structures into wasm-objects.h.

This CL moves all heap-allocated WASM data structures, both ones
that are bonafide JSObjects and ones that are FixedArrays only, into a
consistent place with consistent layout. Note that not all accessors are complete, and I haven't fully spread the new static typing goodness
to all places in the code.

R=ahaas@chromium.org,rossberg@chromium.org
CC=gdeepti@chromium.org,mtrofin@chromium.org,clemensh@chromium.org
BUG=

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

[ahaas, 2016-11-10 09:51:14]

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

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1512: auto memory =
Handle<WasmMemoryObject>::cast(object);
Can you not just use the type here instead of auto?

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1689: (module_->max_mem_pages != 0) ?
module_->max_mem_pages : -1);
Could you do this check also in {WasmMemoryObject::New}?

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

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:76: if (origin == ModuleOrigin::kWasmOrigin) {
I think the code would be more readable if this {if (origin ==
ModuleOrigin::kWasmOrigin)} block would be merged into the {if (origin ==
ModuleOrigin::kWasmOrigin)} above. Duplicating one line does not seem too bad to
me.

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:103:
table_obj->SetInternalField(WasmTableObject::kArray, *(*js_functions));
Is there a reason why the field is called {WasmTableObject::kArray}? It seems to
me that the implementation as an array is just an implementation detail. I think
{WasmTableObject::kFunctions} would be a better name.

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:141: WasmTableObject* WasmTableObject::cast(Object*
object) {
a newline is missing here.

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:142: DCHECK(object->IsJSObject());
I think the DCHECK will crash if object is nullptr.

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:178: DCHECK(object->IsJSObject());
same here

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:184: DCHECK(IsWasmInstance(object));
same here

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:189: DCHECK(object->IsJSFunction());
same here

[titzer, 2016-11-10 10:32:46]

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

[commit-bot: I haz the power, 2016-11-10 10:32:55]

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

[titzer, 2016-11-10 10:33:15]

Description was changed from

==========
[wasm] Move all heap-allocated WASM structures into wasm-objects.h.

This is a WIP to move all heap-allocated WASM data structures, both ones
that are bonafide JSObjects and ones that are FixedArrays only, into a
consistent place with consistent layout. Note that not all accessors are
complete, and I haven't fully spread the new static typing goodness
to all places in the code.

R=ahaas@chromium.org,rossberg@chromium.org
CC=gdeepti@chromium.org,mtrofin@chromium.org,clemensh@chromium.org
BUG=
==========

to

==========
[wasm] Move all heap-allocated WASM structures into wasm-objects.h.

This CL moves all heap-allocated WASM data structures, both ones
that are bonafide JSObjects and ones that are FixedArrays only, into a
consistent place with consistent layout. Note that not all accessors are
complete, and I haven't fully spread the new static typing goodness
to all places in the code.

R=ahaas@chromium.org,rossberg@chromium.org
CC=gdeepti@chromium.org,mtrofin@chromium.org,clemensh@chromium.org
BUG=
==========

[Yang, 2016-11-10 10:33:25]

On 2016/11/10 09:51:14, ahaas wrote:
> https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1512: auto memory =
> Handle<WasmMemoryObject>::cast(object);
> Can you not just use the type here instead of auto?
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1689: (module_->max_mem_pages != 0) ?
> module_->max_mem_pages : -1);
> Could you do this check also in {WasmMemoryObject::New}?
> 
> https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc
> File src/wasm/wasm-objects.cc (right):
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:76: if (origin == ModuleOrigin::kWasmOrigin) {
> I think the code would be more readable if this {if (origin ==
> ModuleOrigin::kWasmOrigin)} block would be merged into the {if (origin ==
> ModuleOrigin::kWasmOrigin)} above. Duplicating one line does not seem too bad
to
> me.
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:103:
> table_obj->SetInternalField(WasmTableObject::kArray, *(*js_functions));
> Is there a reason why the field is called {WasmTableObject::kArray}? It seems
to
> me that the implementation as an array is just an implementation detail. I
think
> {WasmTableObject::kFunctions} would be a better name.
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:141: WasmTableObject* WasmTableObject::cast(Object*
> object) {
> a newline is missing here.
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:142: DCHECK(object->IsJSObject());
> I think the DCHECK will crash if object is nullptr.
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:178: DCHECK(object->IsJSObject());
> same here
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:184: DCHECK(IsWasmInstance(object));
> same here
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:189: DCHECK(object->IsJSFunction());
> same here

src/snapshot and src/debug lgtm.

[titzer, 2016-11-10 10:33:25]

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

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1512: auto memory =
Handle<WasmMemoryObject>::cast(object);
On 2016/11/10 09:51:13, ahaas wrote:
> Can you not just use the type here instead of auto?

I think that's redundant, since a Handle<X>::cast() always returns a Handle<X>.

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1689: (module_->max_mem_pages != 0) ?
module_->max_mem_pages : -1);
On 2016/11/10 09:51:13, ahaas wrote:
> Could you do this check also in {WasmMemoryObject::New}?

WasmMemoryObject::New() is just a constructor, so it shouldn't do any magic.

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

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:76: if (origin == ModuleOrigin::kWasmOrigin) {
On 2016/11/10 09:51:14, ahaas wrote:
> I think the code would be more readable if this {if (origin ==
> ModuleOrigin::kWasmOrigin)} block would be merged into the {if (origin ==
> ModuleOrigin::kWasmOrigin)} above. Duplicating one line does not seem too bad
to
> me.

Done.

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:103:
table_obj->SetInternalField(WasmTableObject::kArray, *(*js_functions));
On 2016/11/10 09:51:14, ahaas wrote:
> Is there a reason why the field is called {WasmTableObject::kArray}? It seems
to
> me that the implementation as an array is just an implementation detail. I
think
> {WasmTableObject::kFunctions} would be a better name. 

Done.

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:141: WasmTableObject* WasmTableObject::cast(Object*
object) {
On 2016/11/10 09:51:14, ahaas wrote:
> a newline is missing here.

Done.

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:142: DCHECK(object->IsJSObject());
On 2016/11/10 09:51:13, ahaas wrote:
> I think the DCHECK will crash if object is nullptr.

Done.

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:178: DCHECK(object->IsJSObject());
On 2016/11/10 09:51:14, ahaas wrote:
> same here

Done.

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:184: DCHECK(IsWasmInstance(object));
On 2016/11/10 09:51:13, ahaas wrote:
> same here

Done.

https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
src/wasm/wasm-objects.cc:189: DCHECK(object->IsJSFunction());
On 2016/11/10 09:51:14, ahaas wrote:
> same here

Done.

[titzer, 2016-11-10 10:35:49]

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

[titzer, 2016-11-10 10:36:06]

On 2016/11/10 10:33:25, titzer wrote:
> https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1512: auto memory =
> Handle<WasmMemoryObject>::cast(object);
> On 2016/11/10 09:51:13, ahaas wrote:
> > Can you not just use the type here instead of auto?
> 
> I think that's redundant, since a Handle<X>::cast() always returns a
Handle<X>.
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1689: (module_->max_mem_pages != 0) ?
> module_->max_mem_pages : -1);
> On 2016/11/10 09:51:13, ahaas wrote:
> > Could you do this check also in {WasmMemoryObject::New}?
> 
> WasmMemoryObject::New() is just a constructor, so it shouldn't do any magic.
> 
> https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc
> File src/wasm/wasm-objects.cc (right):
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:76: if (origin == ModuleOrigin::kWasmOrigin) {
> On 2016/11/10 09:51:14, ahaas wrote:
> > I think the code would be more readable if this {if (origin ==
> > ModuleOrigin::kWasmOrigin)} block would be merged into the {if (origin ==
> > ModuleOrigin::kWasmOrigin)} above. Duplicating one line does not seem too
bad
> to
> > me.
> 
> Done.
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:103:
> table_obj->SetInternalField(WasmTableObject::kArray, *(*js_functions));
> On 2016/11/10 09:51:14, ahaas wrote:
> > Is there a reason why the field is called {WasmTableObject::kArray}? It
seems
> to
> > me that the implementation as an array is just an implementation detail. I
> think
> > {WasmTableObject::kFunctions} would be a better name. 
> 
> Done.
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:141: WasmTableObject* WasmTableObject::cast(Object*
> object) {
> On 2016/11/10 09:51:14, ahaas wrote:
> > a newline is missing here.
> 
> Done.
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:142: DCHECK(object->IsJSObject());
> On 2016/11/10 09:51:13, ahaas wrote:
> > I think the DCHECK will crash if object is nullptr.
> 
> Done.
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:178: DCHECK(object->IsJSObject());
> On 2016/11/10 09:51:14, ahaas wrote:
> > same here
> 
> Done.
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:184: DCHECK(IsWasmInstance(object));
> On 2016/11/10 09:51:13, ahaas wrote:
> > same here
> 
> Done.
> 
>
https://codereview.chromium.org/2490663002/diff/1/src/wasm/wasm-objects.cc#ne...
> src/wasm/wasm-objects.cc:189: DCHECK(object->IsJSFunction());
> On 2016/11/10 09:51:14, ahaas wrote:
> > same here
> 
> Done.

+verwaest for src/runtime

[commit-bot: I haz the power, 2016-11-10 10:59:49]

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

[commit-bot: I haz the power, 2016-11-10 10:59:50]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-11-10 12:36:34]

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

[commit-bot: I haz the power, 2016-11-10 12:36:44]

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

[Toon Verwaest, 2016-11-10 12:52:14]

lgtm

[commit-bot: I haz the power, 2016-11-10 13:03:22]

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

[commit-bot: I haz the power, 2016-11-10 13:03:23]

Dry run: This issue passed the CQ dry run.

[rossberg, 2016-11-10 15:12:16]

So much better! LGTM

https://codereview.chromium.org/2490663002/diff/60001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2490663002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:1585: static_cast<int>(function.sig->parameter_count()),
Having to perform this cast seems stupid.

[titzer, 2016-11-10 17:18:00]

On 2016/11/10 15:12:16, rossberg wrote:
> So much better! LGTM
> 
> https://codereview.chromium.org/2490663002/diff/60001/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2490663002/diff/60001/src/wasm/wasm-module.cc...
> src/wasm/wasm-module.cc:1585:
static_cast<int>(function.sig->parameter_count()),
> Having to perform this cast seems stupid.

Agree. Signature::parameter_count() returns a size_t, and then it's off to the
C++ signed/unsigned races...

[titzer, 2016-11-11 10:48:02]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-11-11 10:48:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, rossberg@chromium.org, verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2490663002/#ps80001
(title: "[wasm] Move all heap-allocated WASM structures into wasm-objects.h.")

[commit-bot: I haz the power, 2016-11-11 10:48:12]

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-11-11 11:12:36]

Description was changed from

==========
[wasm] Move all heap-allocated WASM structures into wasm-objects.h.

This CL moves all heap-allocated WASM data structures, both ones
that are bonafide JSObjects and ones that are FixedArrays only, into a
consistent place with consistent layout. Note that not all accessors are
complete, and I haven't fully spread the new static typing goodness
to all places in the code.

R=ahaas@chromium.org,rossberg@chromium.org
CC=gdeepti@chromium.org,mtrofin@chromium.org,clemensh@chromium.org
BUG=
==========

to

==========
[wasm] Move all heap-allocated WASM structures into wasm-objects.h.

This CL moves all heap-allocated WASM data structures, both ones
that are bonafide JSObjects and ones that are FixedArrays only, into a
consistent place with consistent layout. Note that not all accessors are
complete, and I haven't fully spread the new static typing goodness
to all places in the code.

R=ahaas@chromium.org,rossberg@chromium.org
CC=gdeepti@chromium.org,mtrofin@chromium.org,clemensh@chromium.org
BUG=
==========

[commit-bot: I haz the power, 2016-11-11 11:12:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-17 22:30:10]

Description was changed from

==========
[wasm] Move all heap-allocated WASM structures into wasm-objects.h.

This CL moves all heap-allocated WASM data structures, both ones
that are bonafide JSObjects and ones that are FixedArrays only, into a
consistent place with consistent layout. Note that not all accessors are
complete, and I haven't fully spread the new static typing goodness
to all places in the code.

R=ahaas@chromium.org,rossberg@chromium.org
CC=gdeepti@chromium.org,mtrofin@chromium.org,clemensh@chromium.org
BUG=
==========

to

==========
[wasm] Move all heap-allocated WASM structures into wasm-objects.h.

This CL moves all heap-allocated WASM data structures, both ones
that are bonafide JSObjects and ones that are FixedArrays only, into a
consistent place with consistent layout. Note that not all accessors are
complete, and I haven't fully spread the new static typing goodness
to all places in the code.

R=ahaas@chromium.org,rossberg@chromium.org
CC=gdeepti@chromium.org,mtrofin@chromium.org,clemensh@chromium.org
BUG=

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

[commit-bot: I haz the power, 2016-11-17 22:30:12]

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