Implement Wasm GrowMemory opcode as a wasm runtime call

Implement Wasm GrowMemory opcode as a wasm runtime call
  - GrowMemory runtime function, tests added to checks if memory can be grown
  and relocation information is updated correctly

R=titzer@chromium.org, bradnelson@chromium.org

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

[titzer, 2016-06-10 09:38:13]

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

https://codereview.chromium.org/2051043002/diff/60001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:1622: input = BuildChangeInt32ToTagged(input);
I think this is OK for now, but this does mean that there is an allocation in
WASM code. We might want to factor this out into a little stub so that WASM
functions themselves don't do allocations.

https://codereview.chromium.org/2051043002/diff/60001/src/runtime/runtime-was...
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2051043002/diff/60001/src/runtime/runtime-was...
src/runtime/runtime-wasm.cc:73: // Zero initializinf uninitialized memory from
realloc
s/initializinf/initializing

https://codereview.chromium.org/2051043002/diff/60001/src/runtime/runtime-was...
src/runtime/runtime-wasm.cc:84: module_object->SetInternalField(2, *buffer);
Can we introduce some constants for the module fields to avoid a magic 2 here?

https://codereview.chromium.org/2051043002/diff/60001/src/runtime/runtime-was...
src/runtime/runtime-wasm.cc:100: for (RelocIterator it(*code, mode_mask);
!it.done(); it.next()) {
Can you factor this routine out into a unittestable function, probably living in
the wasm/ directory?

[gdeepti, 2016-06-10 22:39:58]

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

https://codereview.chromium.org/2051043002/diff/60001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:1622: input = BuildChangeInt32ToTagged(input);
On 2016/06/10 09:38:13, titzer wrote:
> I think this is OK for now, but this does mean that there is an allocation in
> WASM code. We might want to factor this out into a little stub so that WASM
> functions themselves don't do allocations.

I'm assuming by allocation you mean heap number allocation. Not sure I
understand how to factor out the allocation, I was hoping you could elaborate?
It looks like that's what is causing the tryjobs to fail as well, still
debugging.

https://codereview.chromium.org/2051043002/diff/60001/src/runtime/runtime-was...
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2051043002/diff/60001/src/runtime/runtime-was...
src/runtime/runtime-wasm.cc:73: // Zero initializinf uninitialized memory from
realloc
On 2016/06/10 09:38:13, titzer wrote:
> s/initializinf/initializing

Done.

https://codereview.chromium.org/2051043002/diff/60001/src/runtime/runtime-was...
src/runtime/runtime-wasm.cc:84: module_object->SetInternalField(2, *buffer);
On 2016/06/10 09:38:13, titzer wrote:
> Can we introduce some constants for the module fields to avoid a magic 2 here?

Done.

https://codereview.chromium.org/2051043002/diff/60001/src/runtime/runtime-was...
src/runtime/runtime-wasm.cc:100: for (RelocIterator it(*code, mode_mask);
!it.done(); it.next()) {
On 2016/06/10 09:38:13, titzer wrote:
> Can you factor this routine out into a unittestable function, probably living
in
> the wasm/ directory?

Moved to wasm-module.cc

[ahaas, 2016-06-14 13:03:12]

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

[ahaas, 2016-06-14 13:03:14]

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

https://codereview.chromium.org/2051043002/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:1624: input,
jsgraph()->Constant(module->instance->js_object),
Use HeapConstant(module->instance->js_object) instead of
jsgraph()->Constant(module->instance->js_object) to avoid problems with parallel
compilation here.

https://codereview.chromium.org/2051043002/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:1628:
jsgraph()->Constant(module->instance->context),   // context
Use HeapConstant(module->instance->context) instead of
jsgraph()->Constant(module->instance->context) to avoid problems with parallel
compilation here.

[gdeepti, 2016-06-22 23:46:38]

Patchset #19 (id:360001) has been deleted

[gdeepti, 2016-06-22 23:46:45]

Patchset #18 (id:340001) has been deleted

[gdeepti, 2016-06-23 15:40:29]

Patchset #19 (id:390001) has been deleted

[gdeepti, 2016-06-23 16:57:02]

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

https://codereview.chromium.org/2051043002/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:1624: input,
jsgraph()->Constant(module->instance->js_object),
On 2016/06/14 13:03:13, ahaas wrote:
> Use HeapConstant(module->instance->js_object) instead of
> jsgraph()->Constant(module->instance->js_object) to avoid problems with
parallel
> compilation here.

Done.

https://codereview.chromium.org/2051043002/diff/80001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:1628:
jsgraph()->Constant(module->instance->context),   // context
On 2016/06/14 13:03:13, ahaas wrote:
> Use HeapConstant(module->instance->context) instead of
> jsgraph()->Constant(module->instance->context) to avoid problems with parallel
> compilation here.

Done.

[gdeepti, 2016-06-23 16:57:58]

PTAL. 

Quick note, windows ia32 grow_memory tests disabled for now because of some
flaky crashes - doesn't seem to be consistently reproducible, added a TODO for
now.

[ahaas, 2016-06-24 11:10:15]

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

https://codereview.chromium.org/2051043002/diff/410001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1577: Runtime::FunctionId f =
Runtime::kWasmGrowMemory;
personal preference: would it be possible to use better variable names than f
and fun? At least function_id instead of f would be nice.

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

https://codereview.chromium.org/2051043002/diff/410001/src/runtime/runtime-wa...
src/runtime/runtime-wasm.cc:36: byte* old_mem_start;
I think you could use {Address} as the type for old_mem_start, then you would
not have to reinterpret_cast everywhere.

https://codereview.chromium.org/2051043002/diff/410001/src/runtime/runtime-wa...
src/runtime/runtime-wasm.cc:43: Handle<JSArrayBuffer> old_buffer =
Handle<JSArrayBuffer>::null();
Could you not just write?
Handle<JSArrayBuffer> old_buffer =
Handle<JSArrayBuffer>(JSArrayBuffer::cast(obj));

https://codereview.chromium.org/2051043002/diff/410001/src/runtime/runtime-wa...
src/runtime/runtime-wasm.cc:87: for (size_t i = old_size; i < new_size; i++) {
use memset to zero initialize memory?

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

https://codereview.chromium.org/2051043002/diff/410001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1045: code = Handle<Code>(Code::cast(obj));
Why don't you just say Handle<Code> code(Code::cast(obj));?

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
File test/mjsunit/wasm/grow-memory.js (right):

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:29: function peek() { return
module.exports.load(offset); }
why don't you pass in offset as a parameter?

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:33: for(offset = 0; offset < 65533; offset++) {
could you use a named constant here instead of 65533?

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:37: for (offset = 65534; offset < 66538;
offset++) {
same here

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:50: for (offset = 65534; offset < 262141;
offset++) {
same here

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:54: for (offset = 262142; offset < 262145;
offset++) {
same here

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:71: for (offset = 1245184; offset < 1245188;
offset++) {
could you add a test for the upper bound?, e.g. by setting offset to 1245180 and
poke and peek?

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:121: if (e.message === "failed to allocate
memory" ||
How can both of these messages be possible?

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:168: assertEquals(0, read());
I don't understand this test. It seems to be either trivial if memory is zero
initialized, or I have no idea why it works.

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/wasm...
File test/mjsunit/wasm/wasm-constants.js (right):

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/wasm...
test/mjsunit/wasm/wasm-constants.js:315: var kTrapMemAllocationFail    = 8;
where do you use this trap?

[gdeepti, 2016-06-25 00:28:42]

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

https://codereview.chromium.org/2051043002/diff/410001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1577: Runtime::FunctionId f =
Runtime::kWasmGrowMemory;
On 2016/06/24 11:10:15, ahaas wrote:
> personal preference: would it be possible to use better variable names than f
> and fun? At least function_id instead of f would be nice.

Done.

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

https://codereview.chromium.org/2051043002/diff/410001/src/runtime/runtime-wa...
src/runtime/runtime-wasm.cc:36: byte* old_mem_start;
On 2016/06/24 11:10:15, ahaas wrote:
> I think you could use {Address} as the type for old_mem_start, then you would
> not have to reinterpret_cast everywhere.

Done.

https://codereview.chromium.org/2051043002/diff/410001/src/runtime/runtime-wa...
src/runtime/runtime-wasm.cc:43: Handle<JSArrayBuffer> old_buffer =
Handle<JSArrayBuffer>::null();
On 2016/06/24 11:10:15, ahaas wrote:
> Could you not just write?
> Handle<JSArrayBuffer> old_buffer =
> Handle<JSArrayBuffer>(JSArrayBuffer::cast(obj));

Done.

https://codereview.chromium.org/2051043002/diff/410001/src/runtime/runtime-wa...
src/runtime/runtime-wasm.cc:87: for (size_t i = old_size; i < new_size; i++) {
On 2016/06/24 11:10:15, ahaas wrote:
> use memset to zero initialize memory?

Done.

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

https://codereview.chromium.org/2051043002/diff/410001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1045: code = Handle<Code>(Code::cast(obj));
On 2016/06/24 11:10:15, ahaas wrote:
> Why don't you just say Handle<Code> code(Code::cast(obj));?

Done.

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
File test/mjsunit/wasm/grow-memory.js (right):

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:29: function peek() { return
module.exports.load(offset); }
On 2016/06/24 11:10:15, ahaas wrote:
> why don't you pass in offset as a parameter?

Because the assertTraps function in wasm-constants.js does not take function
arguments, I figured this might be cleaner than taking in arguments in
assertTraps, would you prefer it the other way around?

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:33: for(offset = 0; offset < 65533; offset++) {
On 2016/06/24 11:10:15, ahaas wrote:
> could you use a named constant here instead of 65533?

Done.

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:37: for (offset = 65534; offset < 66538;
offset++) {
On 2016/06/24 11:10:15, ahaas wrote:
> same here

Done.

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:50: for (offset = 65534; offset < 262141;
offset++) {
On 2016/06/24 11:10:15, ahaas wrote:
> same here

Done.

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:54: for (offset = 262142; offset < 262145;
offset++) {
On 2016/06/24 11:10:15, ahaas wrote:
> same here

Done.

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:71: for (offset = 1245184; offset < 1245188;
offset++) {
On 2016/06/24 11:10:15, ahaas wrote:
> could you add a test for the upper bound?, e.g. by setting offset to 1245180
and
> poke and peek?

Done.

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:121: if (e.message === "failed to allocate
memory" ||
On 2016/06/24 11:10:15, ahaas wrote:
> How can both of these messages be possible?

Left over from when I was trying to catch windows failures, thanks for catching
this. Fixed here and in the case below.

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/grow...
test/mjsunit/wasm/grow-memory.js:168: assertEquals(0, read());
On 2016/06/24 11:10:15, ahaas wrote:
> I don't understand this test. It seems to be either trivial if memory is zero
> initialized, or I have no idea why it works.

I added this test as a sanity check for oob with growMemory from
module-memory.js before zero initializing memory, seems redundant now -
removing.

[ahaas, 2016-06-27 15:08:25]

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

https://codereview.chromium.org/2051043002/diff/430001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1585: *control_ptr =
graph()->NewNode(jsgraph()->common()->Merge(1), *control_ptr);
why do you create a Merge and an EffectPhi node here? Can you add a test which
fails when these nodes are missing?

[gdeepti, 2016-06-27 18:26:45]

Patchset #21 (id:450001) has been deleted

[gdeepti, 2016-06-27 19:18:54]

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

https://codereview.chromium.org/2051043002/diff/430001/src/compiler/wasm-comp...
src/compiler/wasm-compiler.cc:1585: *control_ptr =
graph()->NewNode(jsgraph()->common()->Merge(1), *control_ptr);
On 2016/06/27 15:08:25, ahaas wrote:
> why do you create a Merge and an EffectPhi node here? Can you add a test which
> fails when these nodes are missing?

I misunderstood how control/effect pointers should be updated for runtime
functions. Merge and an EffectPhi nodes not required here, removed now.

[gdeepti, 2016-06-27 19:55:12]

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/wasm...
File test/mjsunit/wasm/wasm-constants.js (right):

https://codereview.chromium.org/2051043002/diff/410001/test/mjsunit/wasm/wasm...
test/mjsunit/wasm/wasm-constants.js:315: var kTrapMemAllocationFail    = 8;
On 2016/06/24 11:10:15, ahaas wrote:
> where do you use this trap?

Added here to keep wasm errors in sync with messages.h and used in
https://codereview.chromium.org/2051043002/diff/490001/test/mjsunit/wasm/grow...
to check for allocation fails.

[ahaas, 2016-06-28 14:51:50]

lgtm

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

https://codereview.chromium.org/2051043002/diff/490001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1038: Handle<FixedArray> code_table;
Use Handle<FixedArray> code_table(FixedArray::cast(obj)); here.

https://codereview.chromium.org/2051043002/diff/490001/test/mjsunit/mjsunit.s...
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/2051043002/diff/490001/test/mjsunit/mjsunit.s...
test/mjsunit/mjsunit.status:883: # TODO(gdeepti): Flaky on multiple runs,
windows nosnap crashes
Did you find out why this test is crashing on Windows?

[gdeepti, 2016-06-28 16:10:47]

Description was changed from

==========
Implement Wasm GrowMemory opcode as a wasm runtime call
  - GrowMemory runtime function, tests added to checks if memory can be grown
  and relocation information is updated correctly
  - Assembler/Compiler fixes for Zero initial memory
  - x64 assembler fix to allocate 32 bits for cmp instructions so that
GrowMemory
  does not corrupt generated code

R=titzer@chromium.org, bradnelson@chromium.org
==========

to

==========
Implement Wasm GrowMemory opcode as a wasm runtime call
  - GrowMemory runtime function, tests added to checks if memory can be grown
  and relocation information is updated correctly

R=titzer@chromium.org, bradnelson@chromium.org
==========

[gdeepti, 2016-06-28 16:19:10]

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

https://codereview.chromium.org/2051043002/diff/490001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1038: Handle<FixedArray> code_table;
On 2016/06/28 14:51:50, ahaas wrote:
> Use Handle<FixedArray> code_table(FixedArray::cast(obj)); here.

Done.

https://codereview.chromium.org/2051043002/diff/490001/test/mjsunit/mjsunit.s...
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/2051043002/diff/490001/test/mjsunit/mjsunit.s...
test/mjsunit/mjsunit.status:883: # TODO(gdeepti): Flaky on multiple runs,
windows nosnap crashes
On 2016/06/28 14:51:50, ahaas wrote:
> Did you find out why this test is crashing on Windows?

Not yet, it does not reproduce on my windows machine consistently and different
tests fail on the bots. My guess is that I'm tickling some subtle
allocation/free mechanism in the array buffer tracking because the crash happens
after the test passes. Still investigating.

[gdeepti, 2016-06-28 16:19:56]

The CQ bit was checked by gdeepti@chromium.org

[gdeepti, 2016-06-28 16:19:57]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2051043002/#ps510001
(title: "Review changes")

[commit-bot: I haz the power, 2016-06-28 16:20:02]

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-06-28 16:24:07]

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

[commit-bot: I haz the power, 2016-06-28 16:24:08]

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

[titzer, 2016-06-28 16:40:06]

On 2016/06/28 16:24:08, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   v8_presubmit on master.tryserver.v8 (JOB_FAILED,
> http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/18246)

lgtm

[gdeepti, 2016-06-28 16:43:52]

The CQ bit was checked by gdeepti@chromium.org

[commit-bot: I haz the power, 2016-06-28 16:43:56]

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-06-28 16:46:46]

Description was changed from

==========
Implement Wasm GrowMemory opcode as a wasm runtime call
  - GrowMemory runtime function, tests added to checks if memory can be grown
  and relocation information is updated correctly

R=titzer@chromium.org, bradnelson@chromium.org
==========

to

==========
Implement Wasm GrowMemory opcode as a wasm runtime call
  - GrowMemory runtime function, tests added to checks if memory can be grown
  and relocation information is updated correctly

R=titzer@chromium.org, bradnelson@chromium.org
==========

[commit-bot: I haz the power, 2016-06-28 16:46:47]

Committed patchset #23 (id:510001)

[commit-bot: I haz the power, 2016-06-28 16:49:24]

Description was changed from

==========
Implement Wasm GrowMemory opcode as a wasm runtime call
  - GrowMemory runtime function, tests added to checks if memory can be grown
  and relocation information is updated correctly

R=titzer@chromium.org, bradnelson@chromium.org
==========

to

==========
Implement Wasm GrowMemory opcode as a wasm runtime call
  - GrowMemory runtime function, tests added to checks if memory can be grown
  and relocation information is updated correctly

R=titzer@chromium.org, bradnelson@chromium.org

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

[commit-bot: I haz the power, 2016-06-28 16:49:26]

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