Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 10 months ago by Mircea Trofin Modified:
3 years, 10 months ago Reviewers:
titzer, bradnelson CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Embedder can control what buffers wasm compilation works on.
Two controls, one for instantiation and one for compilation. They allow
the embedder (e.g. Chrome) check properties of the parameters of those
two operations, and decide if they are allowed to continue.
For example, Chrome may now decline compilation of certain size buffers,
in synchronous cases; same for instantiation (where the buffer size
refers to the size of the buffer containing wasm wire bytes)
BUG=v8:5981
Review-Url: https://codereview.chromium.org/2699843003
Cr-Original-Commit-Position: refs/heads/master@{#43295}
Committed: https://chromium.googlesource.com/v8/v8/+/d9bc0ffb16e633d52d7bcfd547a6125f0e4dfb87
Review-Url: https://codereview.chromium.org/2699843003
Cr-Commit-Position: refs/heads/master@{#43336}
Committed: https://chromium.googlesource.com/v8/v8/+/b40d44ecc1ea662ea6d30d703cf1820cea9ee190
Patch Set 1 #Patch Set 2 : RangeError #
Total comments: 10
Patch Set 3 : Feedback #Patch Set 4 : Post refactoring; passing isolate to fix flakes #Patch Set 5 : Post refactoring; passing isolate to fix flakes #Patch Set 6 : static initializers #
Created: 3 years, 10 months ago
Messages
Total messages: 37 (28 generated)
The CQ bit was checked by mtrofin@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] Embedder can control what buffers wasm compilation works on BUG= ========== to ========== [wasm] Embedder can control what buffers wasm compilation works on Two controls, one for instantiation and one for compilation. They allow the embedder (e.g. Chrome) check properties of the parameters of those two operations, and decide if they are allowed to continue. For example, Chrome may now decline compilation of certain size buffers, in synchronous cases; same for instantiation (where the buffer size refers to the size of the buffer containing wasm wire bytes) BUG= ==========
mtrofin@chromium.org changed reviewers: + bradnelson@chromium.org, titzer@chromium.org
The CQ bit was checked by mtrofin@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...
Patchset #2 (id:20001) has been deleted
Updated to throw RangeError ...and some rebasing. Sorry for the confusion!
Description was changed from ========== [wasm] Embedder can control what buffers wasm compilation works on Two controls, one for instantiation and one for compilation. They allow the embedder (e.g. Chrome) check properties of the parameters of those two operations, and decide if they are allowed to continue. For example, Chrome may now decline compilation of certain size buffers, in synchronous cases; same for instantiation (where the buffer size refers to the size of the buffer containing wasm wire bytes) BUG= ========== to ========== [wasm] Embedder can control what buffers wasm compilation works on. Two controls, one for instantiation and one for compilation. They allow the embedder (e.g. Chrome) check properties of the parameters of those two operations, and decide if they are allowed to continue. For example, Chrome may now decline compilation of certain size buffers, in synchronous cases; same for instantiation (where the buffer size refers to the size of the buffer containing wasm wire bytes) BUG= ==========
Description was changed from ========== [wasm] Embedder can control what buffers wasm compilation works on. Two controls, one for instantiation and one for compilation. They allow the embedder (e.g. Chrome) check properties of the parameters of those two operations, and decide if they are allowed to continue. For example, Chrome may now decline compilation of certain size buffers, in synchronous cases; same for instantiation (where the buffer size refers to the size of the buffer containing wasm wire bytes) BUG= ========== to ========== [wasm] Embedder can control what buffers wasm compilation works on. Two controls, one for instantiation and one for compilation. They allow the embedder (e.g. Chrome) check properties of the parameters of those two operations, and decide if they are allowed to continue. For example, Chrome may now decline compilation of certain size buffers, in synchronous cases; same for instantiation (where the buffer size refers to the size of the buffer containing wasm wire bytes) BUG=v8:5981 ==========
lgtm https://codereview.chromium.org/2699843003/diff/40001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2699843003/diff/40001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:90: "Wasm compilation disallowed in this context for provided argument"); How about: Wasm compilation exceeds internal limits in this context for provided arguments https://codereview.chromium.org/2699843003/diff/40001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:229: "WebAssembly Instantiation is not permitted in this context"); How about: Wasm instantiation exceeds internal limits in this context for provided arguments https://codereview.chromium.org/2699843003/diff/40001/test/mjsunit/wasm/test-... File test/mjsunit/wasm/test-wasm-compilation-control.js (right): https://codereview.chromium.org/2699843003/diff/40001/test/mjsunit/wasm/test-... test/mjsunit/wasm/test-wasm-compilation-control.js:11: var builder = new WasmModuleBuilder(); Some reason this ended up indented by 4 (should be 2 by style guide)? https://codereview.chromium.org/2699843003/diff/40001/test/mjsunit/wasm/test-... test/mjsunit/wasm/test-wasm-compilation-control.js:25: // the buffer can't compile synchrnously, but we allow async compile typo https://codereview.chromium.org/2699843003/diff/40001/test/mjsunit/wasm/test-... test/mjsunit/wasm/test-wasm-compilation-control.js:92: function TestReturnInBlock() { Leftover?
https://codereview.chromium.org/2699843003/diff/40001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2699843003/diff/40001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:90: "Wasm compilation disallowed in this context for provided argument"); On 2017/02/18 00:28:42, bradnelson wrote: > How about: > Wasm compilation exceeds internal limits in this context for provided arguments Done. https://codereview.chromium.org/2699843003/diff/40001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:229: "WebAssembly Instantiation is not permitted in this context"); On 2017/02/18 00:28:42, bradnelson wrote: > How about: > Wasm instantiation exceeds internal limits in this context for provided > arguments Done. https://codereview.chromium.org/2699843003/diff/40001/test/mjsunit/wasm/test-... File test/mjsunit/wasm/test-wasm-compilation-control.js (right): https://codereview.chromium.org/2699843003/diff/40001/test/mjsunit/wasm/test-... test/mjsunit/wasm/test-wasm-compilation-control.js:11: var builder = new WasmModuleBuilder(); On 2017/02/18 00:28:43, bradnelson wrote: > Some reason this ended up indented by 4 (should be 2 by style guide)? huh... no idea. Emacs? https://codereview.chromium.org/2699843003/diff/40001/test/mjsunit/wasm/test-... test/mjsunit/wasm/test-wasm-compilation-control.js:25: // the buffer can't compile synchrnously, but we allow async compile On 2017/02/18 00:28:42, bradnelson wrote: > typo Acknowledged. https://codereview.chromium.org/2699843003/diff/40001/test/mjsunit/wasm/test-... test/mjsunit/wasm/test-wasm-compilation-control.js:92: function TestReturnInBlock() { On 2017/02/18 00:28:42, bradnelson wrote: > Leftover? No! Intended. See comment above: we're making sure asm-wasm works, despite these controls. Or you mean the name "TestReturnInBlock"? ya... can be renamed.
The CQ bit was checked by mtrofin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bradnelson@chromium.org Link to the patchset: https://codereview.chromium.org/2699843003/#ps60001 (title: "Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487378532530690,
"parent_rev": "18ad0f13afeaabff4e035fddd9edc3d319152160", "commit_rev":
"d9bc0ffb16e633d52d7bcfd547a6125f0e4dfb87"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Embedder can control what buffers wasm compilation works on. Two controls, one for instantiation and one for compilation. They allow the embedder (e.g. Chrome) check properties of the parameters of those two operations, and decide if they are allowed to continue. For example, Chrome may now decline compilation of certain size buffers, in synchronous cases; same for instantiation (where the buffer size refers to the size of the buffer containing wasm wire bytes) BUG=v8:5981 ========== to ========== [wasm] Embedder can control what buffers wasm compilation works on. Two controls, one for instantiation and one for compilation. They allow the embedder (e.g. Chrome) check properties of the parameters of those two operations, and decide if they are allowed to continue. For example, Chrome may now decline compilation of certain size buffers, in synchronous cases; same for instantiation (where the buffer size refers to the size of the buffer containing wasm wire bytes) BUG=v8:5981 Review-Url: https://codereview.chromium.org/2699843003 Cr-Commit-Position: refs/heads/master@{#43295} Committed: https://chromium.googlesource.com/v8/v8/+/d9bc0ffb16e633d52d7bcfd547a6125f0e4... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/v8/v8/+/d9bc0ffb16e633d52d7bcfd547a6125f0e4...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:60001) has been created in https://codereview.chromium.org/2701413002/ by hablich@chromium.org. The reason for reverting is: Introduces a new test failure/flake: https://build.chromium.org/p/client.v8/builders/V8%20Linux/builds/16427.
Message was sent while issue was closed.
Description was changed from ========== [wasm] Embedder can control what buffers wasm compilation works on. Two controls, one for instantiation and one for compilation. They allow the embedder (e.g. Chrome) check properties of the parameters of those two operations, and decide if they are allowed to continue. For example, Chrome may now decline compilation of certain size buffers, in synchronous cases; same for instantiation (where the buffer size refers to the size of the buffer containing wasm wire bytes) BUG=v8:5981 Review-Url: https://codereview.chromium.org/2699843003 Cr-Commit-Position: refs/heads/master@{#43295} Committed: https://chromium.googlesource.com/v8/v8/+/d9bc0ffb16e633d52d7bcfd547a6125f0e4... ========== to ========== [wasm] Embedder can control what buffers wasm compilation works on. Two controls, one for instantiation and one for compilation. They allow the embedder (e.g. Chrome) check properties of the parameters of those two operations, and decide if they are allowed to continue. For example, Chrome may now decline compilation of certain size buffers, in synchronous cases; same for instantiation (where the buffer size refers to the size of the buffer containing wasm wire bytes) BUG=v8:5981 Review-Url: https://codereview.chromium.org/2699843003 Cr-Commit-Position: refs/heads/master@{#43295} Committed: https://chromium.googlesource.com/v8/v8/+/d9bc0ffb16e633d52d7bcfd547a6125f0e4... ==========
The CQ bit was checked by mtrofin@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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/21362) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
The CQ bit was checked by mtrofin@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 mtrofin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bradnelson@chromium.org Link to the patchset: https://codereview.chromium.org/2699843003/#ps120001 (title: "static initializers")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487648447868020,
"parent_rev": "2b9840d86f3b51f42918b6bf1a06ff8b62b2464d", "commit_rev":
"b40d44ecc1ea662ea6d30d703cf1820cea9ee190"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Embedder can control what buffers wasm compilation works on. Two controls, one for instantiation and one for compilation. They allow the embedder (e.g. Chrome) check properties of the parameters of those two operations, and decide if they are allowed to continue. For example, Chrome may now decline compilation of certain size buffers, in synchronous cases; same for instantiation (where the buffer size refers to the size of the buffer containing wasm wire bytes) BUG=v8:5981 Review-Url: https://codereview.chromium.org/2699843003 Cr-Commit-Position: refs/heads/master@{#43295} Committed: https://chromium.googlesource.com/v8/v8/+/d9bc0ffb16e633d52d7bcfd547a6125f0e4... ========== to ========== [wasm] Embedder can control what buffers wasm compilation works on. Two controls, one for instantiation and one for compilation. They allow the embedder (e.g. Chrome) check properties of the parameters of those two operations, and decide if they are allowed to continue. For example, Chrome may now decline compilation of certain size buffers, in synchronous cases; same for instantiation (where the buffer size refers to the size of the buffer containing wasm wire bytes) BUG=v8:5981 Review-Url: https://codereview.chromium.org/2699843003 Cr-Original-Commit-Position: refs/heads/master@{#43295} Committed: https://chromium.googlesource.com/v8/v8/+/d9bc0ffb16e633d52d7bcfd547a6125f0e4... Review-Url: https://codereview.chromium.org/2699843003 Cr-Commit-Position: refs/heads/master@{#43336} Committed: https://chromium.googlesource.com/v8/v8/+/b40d44ecc1ea662ea6d30d703cf1820cea9... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/v8/v8/+/b40d44ecc1ea662ea6d30d703cf1820cea9... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
