Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629523007:
[wasm] JS-API: enable WebAssembly.instantiate tests; fix LinkError (Closed)
|
Created:
3 years, 11 months ago by Mircea Trofin Modified:
3 years, 11 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] JS-API: enable WebAssembly.instantiate tests; fix LinkError
We weren't throwing LinkError where appropriate
progress
BUG=v8:5835
Review-Url: https://codereview.chromium.org/2629523007
Cr-Commit-Position: refs/heads/master@{#42342}
Committed: https://chromium.googlesource.com/v8/v8/+/f8fd6ec3dd89cf1278c0ac00d5ae9e0f480e4d4a
Patch Set 1 #Patch Set 2 : fix errors.js #
Total comments: 8
Patch Set 3 : Fixes #
Total comments: 1
Patch Set 4 : Imports can be more than fcts #
Total comments: 2
Patch Set 5 : moved import validation #
Total comments: 2
Patch Set 6 : initialize module_ upfront #
Total comments: 1
Messages
Total messages: 44 (29 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...
Description was changed from ========== [wasm] JS-API: enable WebAssembly.instantiate tests; fix LinkError We weren't throwing LinkError where appropriate progress BUG= ========== to ========== [wasm] JS-API: enable WebAssembly.instantiate tests; fix LinkError We weren't throwing LinkError where appropriate progress BUG=v8:5835 ==========
mtrofin@chromium.org changed reviewers: + bradnelson@chromium.org, rossberg@chromium.org, titzer@chromium.org
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_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
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...
https://codereview.chromium.org/2629523007/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2629523007/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1490: return ReportLinkError("FFI is not an object", index, module_name); This one is not a linking error but a JS-level API misuse, so a regular type error. https://codereview.chromium.org/2629523007/diff/20001/test/mjsunit/wasm/error... File test/mjsunit/wasm/errors.js (right): https://codereview.chromium.org/2629523007/diff/20001/test/mjsunit/wasm/error... test/mjsunit/wasm/errors.js:59: // default imports to null so we get TypeError by default, thus allowing us to Shouldn't be necessary, since undefined should cause the same error already. https://codereview.chromium.org/2629523007/diff/20001/test/mjsunit/wasm/js-ap... File test/mjsunit/wasm/js-api.js (right): https://codereview.chromium.org/2629523007/diff/20001/test/mjsunit/wasm/js-ap... test/mjsunit/wasm/js-api.js:265: assertErrorMessage(() => new Instance(importingModule, undefined), LinkError, ""); This seems incorrect, undefined should cause the same error as null. https://codereview.chromium.org/2629523007/diff/20001/test/mjsunit/wasm/js-ap... test/mjsunit/wasm/js-api.js:607: assertInstantiateError([importingModuleBinary, undefined], LinkError, /TODO: error messages?/); Same here.
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 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/2629523007/diff/20001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2629523007/diff/20001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1490: return ReportLinkError("FFI is not an object", index, module_name); On 2017/01/12 23:59:53, rossberg wrote: > This one is not a linking error but a JS-level API misuse, so a regular type > error. Oh, true. I'll actually move that out and DCHECK here. https://codereview.chromium.org/2629523007/diff/20001/test/mjsunit/wasm/error... File test/mjsunit/wasm/errors.js (right): https://codereview.chromium.org/2629523007/diff/20001/test/mjsunit/wasm/error... test/mjsunit/wasm/errors.js:59: // default imports to null so we get TypeError by default, thus allowing us to On 2017/01/12 23:59:53, rossberg wrote: > Shouldn't be necessary, since undefined should cause the same error already. Done. https://codereview.chromium.org/2629523007/diff/20001/test/mjsunit/wasm/js-ap... File test/mjsunit/wasm/js-api.js (right): https://codereview.chromium.org/2629523007/diff/20001/test/mjsunit/wasm/js-ap... test/mjsunit/wasm/js-api.js:265: assertErrorMessage(() => new Instance(importingModule, undefined), LinkError, ""); On 2017/01/12 23:59:53, rossberg wrote: > This seems incorrect, undefined should cause the same error as null. Ah. Yes, in all cases when the type isn't an object. https://codereview.chromium.org/2629523007/diff/20001/test/mjsunit/wasm/js-ap... test/mjsunit/wasm/js-api.js:607: assertInstantiateError([importingModuleBinary, undefined], LinkError, /TODO: error messages?/); On 2017/01/12 23:59:53, rossberg wrote: > Same here. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2629523007/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2629523007/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1489: // a JSObject, if the module has imports. Actually, I just realised that it is incorrect that we require JSObject there and in some other places. The proper type to use is the slightly more general JSReceiver, which includes proxies (which ought to be allowed everywhere where objects are). But fixing and testing that is worth a separate CL.
On 2017/01/13 08:59:13, rossberg wrote: > lgtm > > https://codereview.chromium.org/2629523007/diff/40001/src/wasm/wasm-module.cc > File src/wasm/wasm-module.cc (right): > > https://codereview.chromium.org/2629523007/diff/40001/src/wasm/wasm-module.cc... > src/wasm/wasm-module.cc:1489: // a JSObject, if the module has imports. > Actually, I just realised that it is incorrect that we require JSObject there > and in some other places. The proper type to use is the slightly more general > JSReceiver, which includes proxies (which ought to be allowed everywhere where > objects are). > > But fixing and testing that is worth a separate CL. PTAL (imports can be other than fcts) Thanks!
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...
https://codereview.chromium.org/2629523007/diff/60001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2629523007/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:214: if (!i_module_obj->compiled_module()->module()->import_table.empty() && I think it's better if we don't reach into the implementation details from this file but instead called through and let the type error happen in wasm-module.cc
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...
moved import validation + some comments https://codereview.chromium.org/2629523007/diff/60001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2629523007/diff/60001/src/wasm/wasm-js.cc#new... src/wasm/wasm-js.cc:214: if (!i_module_obj->compiled_module()->module()->import_table.empty() && On 2017/01/13 16:47:02, titzer wrote: > I think it's better if we don't reach into the implementation details from this > file but instead called through and let the type error happen in wasm-module.cc OK, but I'd prefer doing it as early as possible - why churn resources in the error case. I'll move it early on in Build().
lgtm with comment https://codereview.chromium.org/2629523007/diff/80001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2629523007/diff/80001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1138: if (!module_object_->compiled_module()->module()->import_table.empty() && Can you do this check after we extract the module_ from the compiled_module? Otherwise it's redundant work in the normal case (success), which is better than the risk of doing extra work in the rare (error) case.
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 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/2629523007/diff/80001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2629523007/diff/80001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1138: if (!module_object_->compiled_module()->module()->import_table.empty() && On 2017/01/13 17:20:43, titzer wrote: > Can you do this check after we extract the module_ from the compiled_module? > Otherwise it's redundant work in the normal case (success), which is better than > the risk of doing extra work in the rare (error) case. we can actually initialize module_ at construction time.
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 rossberg@chromium.org, titzer@chromium.org Link to the patchset: https://codereview.chromium.org/2629523007/#ps100001 (title: "initialize module_ upfront")
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": 100001, "attempt_start_ts": 1484340332268660,
"parent_rev": "8af80a0af97b7d76a81f83b304ac276f8980e533", "commit_rev":
"f8fd6ec3dd89cf1278c0ac00d5ae9e0f480e4d4a"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] JS-API: enable WebAssembly.instantiate tests; fix LinkError We weren't throwing LinkError where appropriate progress BUG=v8:5835 ========== to ========== [wasm] JS-API: enable WebAssembly.instantiate tests; fix LinkError We weren't throwing LinkError where appropriate progress BUG=v8:5835 Review-Url: https://codereview.chromium.org/2629523007 Cr-Commit-Position: refs/heads/master@{#42342} Committed: https://chromium.googlesource.com/v8/v8/+/f8fd6ec3dd89cf1278c0ac00d5ae9e0f480... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/v8/v8/+/f8fd6ec3dd89cf1278c0ac00d5ae9e0f480...
Message was sent while issue was closed.
dschuff@chromium.org changed reviewers: + dschuff@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2629523007/diff/100001/test/mjsunit/wasm/js-a... File test/mjsunit/wasm/js-api.js (right): https://codereview.chromium.org/2629523007/diff/100001/test/mjsunit/wasm/js-a... test/mjsunit/wasm/js-api.js:628: assertEq(result instanceof Instance, true); Drive-by review! This is a change in behavior from the previous overload of WebAssembly.instantiate where the first argument is the bytes instead of a WebAssembly.Module. As tested in line 605 from before this change, it's supposed to return an object that has an instance property, not the instance itself (c.f. https://github.com/WebAssembly/design/blob/master/JS.md#webassemblyinstantiate).
Message was sent while issue was closed.
On 2017/01/20 01:46:19, Derek Schuff wrote: > https://codereview.chromium.org/2629523007/diff/100001/test/mjsunit/wasm/js-a... > File test/mjsunit/wasm/js-api.js (right): > > https://codereview.chromium.org/2629523007/diff/100001/test/mjsunit/wasm/js-a... > test/mjsunit/wasm/js-api.js:628: assertEq(result instanceof Instance, true); > Drive-by review! This is a change in behavior from the previous overload of > WebAssembly.instantiate where the first argument is the bytes instead of a > WebAssembly.Module. As tested in line 605 from before this change, it's supposed > to return an object that has an instance property, not the instance itself (c.f. > https://github.com/WebAssembly/design/blob/master/JS.md#webassemblyinstantiate). Thanks, good catch! Mind opening a crbug under v8 - and assigning it to me?
Message was sent while issue was closed.
On 2017/01/20 01:55:21, Mircea Trofin wrote: > On 2017/01/20 01:46:19, Derek Schuff wrote: > > > https://codereview.chromium.org/2629523007/diff/100001/test/mjsunit/wasm/js-a... > > File test/mjsunit/wasm/js-api.js (right): > > > > > https://codereview.chromium.org/2629523007/diff/100001/test/mjsunit/wasm/js-a... > > test/mjsunit/wasm/js-api.js:628: assertEq(result instanceof Instance, true); > > Drive-by review! This is a change in behavior from the previous overload of > > WebAssembly.instantiate where the first argument is the bytes instead of a > > WebAssembly.Module. As tested in line 605 from before this change, it's > supposed > > to return an object that has an instance property, not the instance itself > (c.f. > > > https://github.com/WebAssembly/design/blob/master/JS.md#webassemblyinstantiate). > > Thanks, good catch! > > Mind opening a crbug under v8 - and assigning it to me? Opened crbug.com/5875
Message was sent while issue was closed.
On 2017/01/20 03:57:25, Mircea Trofin wrote: > Opened crbug.com/5875 Correct link: https://bugs.chromium.org/p/v8/issues/detail?id=5875&desc=2 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
