Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2416543002:
[wasm] Fix bounds check for zero initial memory. (Closed)
Description[wasm] Fix bounds check for zero initial memory.
Currently when memory size references are updated with zero initial memory during GrowMemory/Relocation of Instance objects, the bounds check does not take into account the size of memtype.
R=titzer@chromium.org, bradnelson@chromium.org
Committed: https://crrev.com/70416a2b360c0d993cffb48284b143d484d1e290
Committed: https://crrev.com/3d6f7743528ba5fd5524376a23711fb87df3d5fc
Cr-Original-Commit-Position: refs/heads/master@{#40326}
Cr-Commit-Position: refs/heads/master@{#40329}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Remove additional reloc modes, refactor #Patch Set 3 : Cleanup #Patch Set 4 : Fix formatting #Patch Set 5 : Fix test #Patch Set 6 : Add aTODO #
Total comments: 4
Patch Set 7 : Brad's review #Patch Set 8 : Rebase #
Total comments: 4
Patch Set 9 : Review comments #
Total comments: 4
Patch Set 10 : Review #Patch Set 11 : Cleanup #Patch Set 12 : Fix bot failure #
Messages
Total messages: 68 (48 generated)
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.
bradnelson@google.com changed reviewers: + bradnelson@google.com
lgtm w/ suggestions https://codereview.chromium.org/2416543002/diff/1/src/assembler.cc File src/assembler.cc (right): https://codereview.chromium.org/2416543002/diff/1/src/assembler.cc#newcode371 src/assembler.cc:371: DCHECK(rmode_ == RelocInfo::WASM_MEMORY_BYTE_SIZE_REFERENCE); Handle explicit and make default unreachable? https://codereview.chromium.org/2416543002/diff/1/test/cctest/compiler/test-r... File test/cctest/compiler/test-run-wasm-machops.cc (right): https://codereview.chromium.org/2416543002/diff/1/test/cctest/compiler/test-r... test/cctest/compiler/test-run-wasm-machops.cc:27: RelocInfo::ModeMask(RelocInfo::WASM_MEMORY_DWORD_SIZE_REFERENCE); Test all the sizes. https://codereview.chromium.org/2416543002/diff/1/test/cctest/wasm/test-run-w... File test/cctest/wasm/test-run-wasm-asmjs.cc (right): https://codereview.chromium.org/2416543002/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm-asmjs.cc:250: RelocInfo::WASM_MEMORY_DWORD_SIZE_REFERENCE)); \ Parameterize this on the size. https://codereview.chromium.org/2416543002/diff/1/test/mjsunit/wasm/grow-memo... File test/mjsunit/wasm/grow-memory.js (right): https://codereview.chromium.org/2416543002/diff/1/test/mjsunit/wasm/grow-memo... test/mjsunit/wasm/grow-memory.js:207: function testGrowMemoryZeroInitialSize16() { Might be worth programatically generating all the sizes from like 0-128 by using toString() You'll have to do something special for int64 (maybe return xor or high and low).
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.
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 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...
PTAL. Changed implementation to use one additional RelocInfo mode instead of one for each memtype. https://codereview.chromium.org/2416543002/diff/1/src/assembler.cc File src/assembler.cc (right): https://codereview.chromium.org/2416543002/diff/1/src/assembler.cc#newcode371 src/assembler.cc:371: DCHECK(rmode_ == RelocInfo::WASM_MEMORY_BYTE_SIZE_REFERENCE); On 2016/10/12 18:50:45, bradn wrote: > Handle explicit and make default unreachable? Got rid of additional RelocInfo modes, code removed in latest patch. https://codereview.chromium.org/2416543002/diff/1/test/cctest/compiler/test-r... File test/cctest/compiler/test-run-wasm-machops.cc (right): https://codereview.chromium.org/2416543002/diff/1/test/cctest/compiler/test-r... test/cctest/compiler/test-run-wasm-machops.cc:27: RelocInfo::ModeMask(RelocInfo::WASM_MEMORY_DWORD_SIZE_REFERENCE); On 2016/10/12 18:50:45, bradn wrote: > Test all the sizes. Got rid of additional RelocInfo modes, code removed in latest patch. https://codereview.chromium.org/2416543002/diff/1/test/cctest/wasm/test-run-w... File test/cctest/wasm/test-run-wasm-asmjs.cc (right): https://codereview.chromium.org/2416543002/diff/1/test/cctest/wasm/test-run-w... test/cctest/wasm/test-run-wasm-asmjs.cc:250: RelocInfo::WASM_MEMORY_DWORD_SIZE_REFERENCE)); \ On 2016/10/12 18:50:45, bradn wrote: > Parameterize this on the size. Got rid of additional RelocInfo modes, code removed in latest patch. https://codereview.chromium.org/2416543002/diff/1/test/mjsunit/wasm/grow-memo... File test/mjsunit/wasm/grow-memory.js (right): https://codereview.chromium.org/2416543002/diff/1/test/mjsunit/wasm/grow-memo... test/mjsunit/wasm/grow-memory.js:207: function testGrowMemoryZeroInitialSize16() { On 2016/10/12 18:50:45, bradn wrote: > Might be worth programatically generating all the sizes from like 0-128 by using > toString() > You'll have to do something special for int64 (maybe return xor or high and > low). Added a TODO for now as I have to look into what needs to be done for sizes larger than 32bit that throw type errors when exported to JS.
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/2416543002/diff/100001/src/compiler/wasm-comp... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2416543002/diff/100001/src/compiler/wasm-comp... src/compiler/wasm-compiler.cc:2852: uint32_t size = module_->instance->mem_size; Shouldn't this be from min_mem_pages in the module, not the instance (since set of instructions has to be fixed from compile time on). It probably works now because there's a dummy instance created, but it's better to base this on the values in the module structure. https://codereview.chromium.org/2416543002/diff/100001/src/compiler/wasm-comp... src/compiler/wasm-compiler.cc:2859: // memtype for when code is relocated. Maybe a single reloc type will work after all? If we make the first check always: offset + memtype - 1 < size And the second: index < size - offset - memtype + 1 We can then drop each check based on if the minimum allowed size means the condition is always true?
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 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...)
lgtm https://codereview.chromium.org/2416543002/diff/140001/src/compiler/wasm-comp... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2416543002/diff/140001/src/compiler/wasm-comp... src/compiler/wasm-compiler.cc:2864: if (size == 0 || offset >= size || Drop size==0, always true if the next clause is. Maybe put the size on the right in both cases? https://codereview.chromium.org/2416543002/diff/140001/src/compiler/wasm-comp... src/compiler/wasm-compiler.cc:2878: effective_size = size - offset - memsize + 1; Maybe comment that this relies on the above check when it wraps around.
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/2416543002/diff/100001/src/compiler/wasm-comp... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2416543002/diff/100001/src/compiler/wasm-comp... src/compiler/wasm-compiler.cc:2852: uint32_t size = module_->instance->mem_size; On 2016/10/13 22:43:41, bradn wrote: > Shouldn't this be from min_mem_pages in the module, not the instance (since set > of instructions has to be fixed from compile time on). > It probably works now because there's a dummy instance created, but it's better > to base this on the values in the module structure. Tried this, but it looks like the test harness still relies on the older structure. Deferring this till I can look into this more. https://codereview.chromium.org/2416543002/diff/100001/src/compiler/wasm-comp... src/compiler/wasm-compiler.cc:2859: // memtype for when code is relocated. On 2016/10/13 22:43:41, bradn wrote: > Maybe a single reloc type will work after all? > > If we make the first check always: > offset + memtype - 1 < size > > And the second: > index < size - offset - memtype + 1 > > We can then drop each check based on if the minimum allowed size means the > condition is always true? Done. https://codereview.chromium.org/2416543002/diff/140001/src/compiler/wasm-comp... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2416543002/diff/140001/src/compiler/wasm-comp... src/compiler/wasm-compiler.cc:2864: if (size == 0 || offset >= size || On 2016/10/14 01:17:07, bradn wrote: > Drop size==0, always true if the next clause is. > Maybe put the size on the right in both cases? Done. https://codereview.chromium.org/2416543002/diff/140001/src/compiler/wasm-comp... src/compiler/wasm-compiler.cc:2878: effective_size = size - offset - memsize + 1; On 2016/10/14 01:17:07, bradn wrote: > Maybe comment that this relies on the above check when it wraps around. Done.
https://codereview.chromium.org/2416543002/diff/160001/src/compiler/wasm-comp... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2416543002/diff/160001/src/compiler/wasm-comp... src/compiler/wasm-compiler.cc:2863: size_t effective_size; Move down to the initialization. https://codereview.chromium.org/2416543002/diff/160001/src/compiler/wasm-comp... src/compiler/wasm-compiler.cc:2880: CHECK(effective_size <= kMaxUInt32); This check won't work now (it could be negative). You need to wrap this and the below matcher check in an: if (size > offset + memsize -1) Maybe also clarify that effective_size can be negative in the comment?
https://codereview.chromium.org/2416543002/diff/160001/src/compiler/wasm-comp... File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2416543002/diff/160001/src/compiler/wasm-comp... src/compiler/wasm-compiler.cc:2863: size_t effective_size; On 2016/10/14 01:42:38, bradn wrote: > Move down to the initialization. As the check below is gated by if (size > offset + memsize -1), the declaration here now makes sense. https://codereview.chromium.org/2416543002/diff/160001/src/compiler/wasm-comp... src/compiler/wasm-compiler.cc:2880: CHECK(effective_size <= kMaxUInt32); On 2016/10/14 01:42:38, bradn wrote: > This check won't work now (it could be negative). > You need to wrap this and the below matcher check in an: > if (size > offset + memsize -1) > Maybe also clarify that effective_size can be negative in the comment? Done.
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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/14566) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
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.
The CQ bit was checked by gdeepti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bradnelson@google.com Link to the patchset: https://codereview.chromium.org/2416543002/#ps200001 (title: "Cleanup")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
gdeepti@chromium.org changed reviewers: + mtrofin@chromium.org
lgtm
The CQ bit was checked by gdeepti@chromium.org
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.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Fix bounds check for zero initial memory. Currently when memory size references are updated with zero initial memory during GrowMemory/Relocation of Instance objects, the bounds check does not take into account the size of memtype. R=titzer@chromium.org, bradnelson@chromium.org ========== to ========== [wasm] Fix bounds check for zero initial memory. Currently when memory size references are updated with zero initial memory during GrowMemory/Relocation of Instance objects, the bounds check does not take into account the size of memtype. R=titzer@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/70416a2b360c0d993cffb48284b143d484d1e290 Cr-Commit-Position: refs/heads/master@{#40326} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/70416a2b360c0d993cffb48284b143d484d1e290 Cr-Commit-Position: refs/heads/master@{#40326}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2416393002/ by gdeepti@chromium.org. The reason for reverting is: Reverting because of failure on V8 Linux64 GC Stress http://build.chromium.org/p/client.v8/builders/V8%20Linux64%20GC%20Stress%20-....
Message was sent while issue was closed.
Description was changed from ========== [wasm] Fix bounds check for zero initial memory. Currently when memory size references are updated with zero initial memory during GrowMemory/Relocation of Instance objects, the bounds check does not take into account the size of memtype. R=titzer@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/70416a2b360c0d993cffb48284b143d484d1e290 Cr-Commit-Position: refs/heads/master@{#40326} ========== to ========== [wasm] Fix bounds check for zero initial memory. Currently when memory size references are updated with zero initial memory during GrowMemory/Relocation of Instance objects, the bounds check does not take into account the size of memtype. R=titzer@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/70416a2b360c0d993cffb48284b143d484d1e290 Cr-Commit-Position: refs/heads/master@{#40326} ==========
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.
The CQ bit was checked by gdeepti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org, bradnelson@google.com Link to the patchset: https://codereview.chromium.org/2416543002/#ps220001 (title: "Fix bot failure")
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] Fix bounds check for zero initial memory. Currently when memory size references are updated with zero initial memory during GrowMemory/Relocation of Instance objects, the bounds check does not take into account the size of memtype. R=titzer@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/70416a2b360c0d993cffb48284b143d484d1e290 Cr-Commit-Position: refs/heads/master@{#40326} ========== to ========== [wasm] Fix bounds check for zero initial memory. Currently when memory size references are updated with zero initial memory during GrowMemory/Relocation of Instance objects, the bounds check does not take into account the size of memtype. R=titzer@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/70416a2b360c0d993cffb48284b143d484d1e290 Cr-Commit-Position: refs/heads/master@{#40326} ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Fix bounds check for zero initial memory. Currently when memory size references are updated with zero initial memory during GrowMemory/Relocation of Instance objects, the bounds check does not take into account the size of memtype. R=titzer@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/70416a2b360c0d993cffb48284b143d484d1e290 Cr-Commit-Position: refs/heads/master@{#40326} ========== to ========== [wasm] Fix bounds check for zero initial memory. Currently when memory size references are updated with zero initial memory during GrowMemory/Relocation of Instance objects, the bounds check does not take into account the size of memtype. R=titzer@chromium.org, bradnelson@chromium.org Committed: https://crrev.com/70416a2b360c0d993cffb48284b143d484d1e290 Committed: https://crrev.com/3d6f7743528ba5fd5524376a23711fb87df3d5fc Cr-Original-Commit-Position: refs/heads/master@{#40326} Cr-Commit-Position: refs/heads/master@{#40329} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/3d6f7743528ba5fd5524376a23711fb87df3d5fc Cr-Commit-Position: refs/heads/master@{#40329} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
