Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2216443002:
[wasm] Grow memory should return -1 on failure. (Closed)
Description[wasm] Grow memory should return -1 on failure.
Return -1 instead of out of throwing errors, update tests.
R=titzer@chromium.org, ahaas@chromium.org
Committed: https://crrev.com/1269306a3b78b8f4bb75b4db9c78feb2771900bf
Cr-Commit-Position: refs/heads/master@{#38350}
Patch Set 1 #Patch Set 2 : fix build #Patch Set 3 #
Total comments: 6
Patch Set 4 : Review comments #
Total comments: 6
Patch Set 5 : Review comments #
Messages
Total messages: 36 (27 generated)
Description was changed from ========== [wasm] Grow memory should return -1 as per spec BUG= ========== to ========== [wasm] Grow memory should return -1 as per spec Return -1 instead of out of throwing errors, update tests. R=titzer@chromium.org, ahaas@chromium.org ==========
gdeepti@chromium.org changed reviewers: + ahaas@chromium.org, titzer@chromium.org
The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) 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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10302) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [wasm] Grow memory should return -1 as per spec Return -1 instead of out of throwing errors, update tests. R=titzer@chromium.org, ahaas@chromium.org ========== to ========== [wasm] Grow memory should return -1 on failure. Return -1 instead of out of throwing errors, update tests. R=titzer@chromium.org, ahaas@chromium.org ==========
lgtm with nits https://codereview.chromium.org/2216443002/diff/40001/src/runtime/runtime-was... File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2216443002/diff/40001/src/runtime/runtime-was... src/runtime/runtime-wasm.cc:109: return *isolate->factory()->NewNumberFromUint(old_size / This should be NewNumberFromInt now, should it not? I do not think that it is a good idea to have different return types for this function. https://codereview.chromium.org/2216443002/diff/40001/test/mjsunit/wasm/grow-... File test/mjsunit/wasm/grow-memory.js (right): https://codereview.chromium.org/2216443002/diff/40001/test/mjsunit/wasm/grow-... test/mjsunit/wasm/grow-memory.js:45: assertTrue(result === 1 || result === -1); What do you actually assert here? Should growMem fail and return -1, or should it succeed and return 1? I think it would be better to check just for one of these outcomes. https://codereview.chromium.org/2216443002/diff/40001/test/mjsunit/wasm/grow-... test/mjsunit/wasm/grow-memory.js:57: assertTrue(result === 4 || result === -1); Same here. https://codereview.chromium.org/2216443002/diff/40001/test/mjsunit/wasm/grow-... test/mjsunit/wasm/grow-memory.js:87: assertTrue(result === 0 || result === -1); Same here.
The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2216443002/diff/40001/src/runtime/runtime-was... File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2216443002/diff/40001/src/runtime/runtime-was... src/runtime/runtime-wasm.cc:109: return *isolate->factory()->NewNumberFromUint(old_size / On 2016/08/04 07:17:34, ahaas wrote: > This should be NewNumberFromInt now, should it not? I do not think that it is a > good idea to have different return types for this function. Done. https://codereview.chromium.org/2216443002/diff/40001/test/mjsunit/wasm/grow-... File test/mjsunit/wasm/grow-memory.js (right): https://codereview.chromium.org/2216443002/diff/40001/test/mjsunit/wasm/grow-... test/mjsunit/wasm/grow-memory.js:45: assertTrue(result === 1 || result === -1); On 2016/08/04 07:17:34, ahaas wrote: > What do you actually assert here? Should growMem fail and return -1, or should > it succeed and return 1? I think it would be better to check just for one of > these outcomes. The intent here was to reduce flakiness due to allocation failures but I see the inconsistency here. This happens on windows 32 bit when memory is grown by a larger number of pages repeatedly during GC stress tests. Removed for now I'll add a return on failure instead of the check above if this test actually does turn out to be flaky.
lgtm with more nits https://codereview.chromium.org/2216443002/diff/60001/src/runtime/runtime-was... File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2216443002/diff/60001/src/runtime/runtime-was... src/runtime/runtime-wasm.cc:61: return *isolate->factory()->NewNumberFromInt(-1); You can use a Smi here directly, if it ends up being shorter. https://codereview.chromium.org/2216443002/diff/60001/test/mjsunit/wasm/grow-... File test/mjsunit/wasm/grow-memory.js (right): https://codereview.chromium.org/2216443002/diff/60001/test/mjsunit/wasm/grow-... test/mjsunit/wasm/grow-memory.js:44: assertTrue(growMem(3) === 1); Use assertEquals(1, growMem(3)) https://codereview.chromium.org/2216443002/diff/60001/test/mjsunit/wasm/grow-... test/mjsunit/wasm/grow-memory.js:105: assertEquals(growMem(maxPages), -1); -1, the expected value, should come first here
https://codereview.chromium.org/2216443002/diff/60001/src/runtime/runtime-was... File src/runtime/runtime-wasm.cc (right): https://codereview.chromium.org/2216443002/diff/60001/src/runtime/runtime-was... src/runtime/runtime-wasm.cc:61: return *isolate->factory()->NewNumberFromInt(-1); On 2016/08/04 at 18:20:19, titzer wrote: > You can use a Smi here directly, if it ends up being shorter. factory()->NewNumberFromInt returns a Smi anyways and does not allocate a heap number if the value is a Smi.
The CQ bit was checked by gdeepti@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2216443002/diff/60001/test/mjsunit/wasm/grow-... File test/mjsunit/wasm/grow-memory.js (right): https://codereview.chromium.org/2216443002/diff/60001/test/mjsunit/wasm/grow-... test/mjsunit/wasm/grow-memory.js:44: assertTrue(growMem(3) === 1); On 2016/08/04 18:20:19, titzer wrote: > Use assertEquals(1, growMem(3)) Fixed here and the ones below. https://codereview.chromium.org/2216443002/diff/60001/test/mjsunit/wasm/grow-... test/mjsunit/wasm/grow-memory.js:105: assertEquals(growMem(maxPages), -1); On 2016/08/04 18:20:19, titzer wrote: > -1, the expected value, should come first here Fixed here, and in the rest of the file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gdeepti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org, titzer@chromium.org Link to the patchset: https://codereview.chromium.org/2216443002/#ps80001 (title: "Review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [wasm] Grow memory should return -1 on failure. Return -1 instead of out of throwing errors, update tests. R=titzer@chromium.org, ahaas@chromium.org ========== to ========== [wasm] Grow memory should return -1 on failure. Return -1 instead of out of throwing errors, update tests. R=titzer@chromium.org, ahaas@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Grow memory should return -1 on failure. Return -1 instead of out of throwing errors, update tests. R=titzer@chromium.org, ahaas@chromium.org ========== to ========== [wasm] Grow memory should return -1 on failure. Return -1 instead of out of throwing errors, update tests. R=titzer@chromium.org, ahaas@chromium.org Committed: https://crrev.com/1269306a3b78b8f4bb75b4db9c78feb2771900bf Cr-Commit-Position: refs/heads/master@{#38350} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1269306a3b78b8f4bb75b4db9c78feb2771900bf Cr-Commit-Position: refs/heads/master@{#38350} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
