Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 10 months ago by bradnelson Modified:
3 years, 10 months ago Reviewers:
titzer, Mircea Trofin, aseemgarg, bradn CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm][asm.js] Make asm.js->wasm return a regular object.
Return a regular JSObject in the asm.js -> wasm case.
BUG=v8:5877
R=mtrofin@chromium.org,aseemgarg@chromium.org,titzer@chromium.org
Review-Url: https://codereview.chromium.org/2664493002
Cr-Commit-Position: refs/heads/master@{#42757}
Committed: https://chromium.googlesource.com/v8/v8/+/437735ed342faae8ec6c7b4291930e0416a5346b
Patch Set 1 #Patch Set 2 : fix #
Total comments: 2
Patch Set 3 : fix #
Total comments: 1
Created: 3 years, 10 months ago
Messages
Total messages: 25 (18 generated)
The CQ bit was checked by bradnelson@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][asm.js] Make asm.js->wasm return a regular object. Return a regular JSObject in the asm.js -> wasm case. BUG=v8:5877 ========== to ========== [wasm][asm.js] Make asm.js->wasm return a regular object. Return a regular JSObject in the asm.js -> wasm case. BUG=v8:5877 R=mtrofin@chromium.org,aseemgarg@chromium.org,titzer@chromium.org ==========
bradnelson@google.com changed reviewers: + aseemgarg@chromium.org, mtrofin@chromium.org, titzer@chromium.org
bradnelson@google.com changed reviewers: + bradnelson@google.com
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...)
The CQ bit was checked by bradnelson@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/2664493002/diff/20001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2664493002/diff/20001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:224: Handle<JSObject> export_to = Handle<JSObject>::null()); It's not really clear to me why the object to which to export needs to be passed in from the outside. We already do a origin check inside, so can't we just create the right kind of exports object in wasm-module.cc and fish out the "exports" property from the returned module in asm-js.cc?
The CQ bit was checked by bradnelson@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 https://codereview.chromium.org/2664493002/diff/20001/src/wasm/wasm-module.h File src/wasm/wasm-module.h (right): https://codereview.chromium.org/2664493002/diff/20001/src/wasm/wasm-module.h#... src/wasm/wasm-module.h:224: Handle<JSObject> export_to = Handle<JSObject>::null()); On 2017/01/27 09:11:47, titzer wrote: > It's not really clear to me why the object to which to export needs to be passed > in from the outside. We already do a origin check inside, so can't we just > create the right kind of exports object in wasm-module.cc and fish out the > "exports" property from the returned module in asm-js.cc? Thought about doing it that way, but figured this was a smaller diff. But probably a better structure. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by bradnelson@google.com
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": 40001, "attempt_start_ts": 1485645255370960,
"parent_rev": "6cd2d4ba41d7adc83f993620fe2e43ab8277e476", "commit_rev":
"437735ed342faae8ec6c7b4291930e0416a5346b"}
Message was sent while issue was closed.
Description was changed from ========== [wasm][asm.js] Make asm.js->wasm return a regular object. Return a regular JSObject in the asm.js -> wasm case. BUG=v8:5877 R=mtrofin@chromium.org,aseemgarg@chromium.org,titzer@chromium.org ========== to ========== [wasm][asm.js] Make asm.js->wasm return a regular object. Return a regular JSObject in the asm.js -> wasm case. BUG=v8:5877 R=mtrofin@chromium.org,aseemgarg@chromium.org,titzer@chromium.org Review-Url: https://codereview.chromium.org/2664493002 Cr-Commit-Position: refs/heads/master@{#42757} Committed: https://chromium.googlesource.com/v8/v8/+/437735ed342faae8ec6c7b4291930e0416a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/437735ed342faae8ec6c7b4291930e0416a...
Message was sent while issue was closed.
LGTM with comment https://codereview.chromium.org/2664493002/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2664493002/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:1975: if (module_->origin == kAsmJsOrigin && exp.kind == kExternalFunction && Can you leave a TODO here? We're adding to string comparisons here, but maybe we'll want to just remember the foreign init and single function indices (e.g. either by numbering convention or by another asm.js-specific section) so we don't have to compare each time. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
