Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2065043002:
[wasm] Check for duplicate export names (Closed)
|
Created:
4 years, 6 months ago by Clemens Hammacher Modified:
4 years, 6 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Check for duplicate export names
Without this check, instantiation of the wasm module would fail on
DefineOwnProperty on the exports object for the duplicate export.
Now we detect this as validation error.
R=rossberg@chromium.org, titzer@chromium.org, ahaas@chromium.org
Committed: https://crrev.com/6fa656fde263b77ab806c1a76dcfec49cf48357b
Cr-Commit-Position: refs/heads/master@{#37038}
Patch Set 1 #Patch Set 2 : fix signed/unsigned comparison #Patch Set 3 : fix for empty exports table #
Total comments: 7
Patch Set 4 : last change, saving one line :) #
Total comments: 4
Patch Set 5 : Address ahaas' comments #Messages
Total messages: 20 (6 generated)
https://codereview.chromium.org/2065043002/diff/40001/src/wasm/module-decoder.cc File src/wasm/module-decoder.cc (right): https://codereview.chromium.org/2065043002/diff/40001/src/wasm/module-decoder... src/wasm/module-decoder.cc:363: std::vector<WasmExport> sorted_exports(module->export_table); Can't we just do that when creating the exports object itself? After all, JS objects are just hashtables...
https://codereview.chromium.org/2065043002/diff/40001/src/wasm/module-decoder.cc File src/wasm/module-decoder.cc (right): https://codereview.chromium.org/2065043002/diff/40001/src/wasm/module-decoder... src/wasm/module-decoder.cc:363: std::vector<WasmExport> sorted_exports(module->export_table); On 2016/06/14 22:52:06, titzer wrote: > Can't we just do that when creating the exports object itself? After all, JS > objects are just hashtables... We cannot report a validation error at that stage, right? We could ignore duplicate exports though. Is this what you want? I think in the design docs it's not defined currently what should happen on multiple exports with the same name.
https://codereview.chromium.org/2065043002/diff/40001/src/wasm/module-decoder.cc File src/wasm/module-decoder.cc (right): https://codereview.chromium.org/2065043002/diff/40001/src/wasm/module-decoder... src/wasm/module-decoder.cc:363: std::vector<WasmExport> sorted_exports(module->export_table); On 2016/06/15 07:48:09, Clemens Hammacher wrote: > On 2016/06/14 22:52:06, titzer wrote: > > Can't we just do that when creating the exports object itself? After all, JS > > objects are just hashtables... > > We cannot report a validation error at that stage, right? > We could ignore duplicate exports though. Is this what you want? > I think in the design docs it's not defined currently what should happen on > multiple exports with the same name. Yes, validation is required to flag it. You cannot defer it until instantiation.
https://codereview.chromium.org/2065043002/diff/40001/src/wasm/module-decoder.cc File src/wasm/module-decoder.cc (right): https://codereview.chromium.org/2065043002/diff/40001/src/wasm/module-decoder... src/wasm/module-decoder.cc:372: std::stable_sort(sorted_exports.begin(), sorted_exports.end(), cmp); Instead of sorting the whole thing, would it be faster to just put the names in a multiset and check whether any has a count > 1? https://codereview.chromium.org/2065043002/diff/40001/src/wasm/module-decoder... src/wasm/module-decoder.cc:376: last = &*it, ++it) { Nit: superfluous newline?
https://codereview.chromium.org/2065043002/diff/40001/src/wasm/module-decoder.cc File src/wasm/module-decoder.cc (right): https://codereview.chromium.org/2065043002/diff/40001/src/wasm/module-decoder... src/wasm/module-decoder.cc:372: std::stable_sort(sorted_exports.begin(), sorted_exports.end(), cmp); On 2016/06/15 08:53:58, rossberg wrote: > Instead of sorting the whole thing, would it be faster to just put the names in > a multiset and check whether any has a count > 1? AFAIK sorting and scanning is the fastest way to check for duplicates. Any solution with set/map/multiset is also O(n*log(n)), but with a bigger constant factor. Plus we need a hash function, and it's harder to report the *first* duplicate export. So is this really worth discussing? How many exports do we expect? https://codereview.chromium.org/2065043002/diff/40001/src/wasm/module-decoder... src/wasm/module-decoder.cc:376: last = &*it, ++it) { On 2016/06/15 08:53:58, rossberg wrote: > Nit: superfluous newline? Nope, does not fit :) But I will change it to "last = &*it++" before committing, so that it fits.
lgtm
Description was changed from ========== [wasm] Check for duplicate export names Without this check, instantiation of the wasm module would fail on DefineOwnProperty on the exports object for the duplicate export. Now we detect this as validation error. R=rossberg@chromium.org, titzer@chromium.org ========== to ========== [wasm] Check for duplicate export names Without this check, instantiation of the wasm module would fail on DefineOwnProperty on the exports object for the duplicate export. Now we detect this as validation error. R=rossberg@chromium.org, titzer@chromium.org, ahaas@chromium.org ==========
clemensh@chromium.org changed reviewers: + ahaas@chromium.org
On 2016/06/15 09:24:54, rossberg wrote: > lgtm @ahaas: PTAL, still need an lgtm from a wasm owner ;)
lgtm with nits Here is a + from me :D https://codereview.chromium.org/2065043002/diff/60001/src/wasm/module-decoder.cc File src/wasm/module-decoder.cc (right): https://codereview.chromium.org/2065043002/diff/60001/src/wasm/module-decoder... src/wasm/module-decoder.cc:365: auto cmp = [base](const WasmExport& a, const WasmExport& b) { I was confused by the name "cmp" because I expected the same kind of return value as memcmp returns. Could you call it "less"? https://codereview.chromium.org/2065043002/diff/60001/test/mjsunit/wasm/expor... File test/mjsunit/wasm/export-table.js (right): https://codereview.chromium.org/2065043002/diff/60001/test/mjsunit/wasm/expor... test/mjsunit/wasm/export-table.js:82: nit: Add an additional function in between to make sure that your sorting approach works.
https://codereview.chromium.org/2065043002/diff/60001/src/wasm/module-decoder.cc File src/wasm/module-decoder.cc (right): https://codereview.chromium.org/2065043002/diff/60001/src/wasm/module-decoder... src/wasm/module-decoder.cc:365: auto cmp = [base](const WasmExport& a, const WasmExport& b) { On 2016/06/16 09:16:13, ahaas wrote: > I was confused by the name "cmp" because I expected the same kind of return > value as memcmp returns. Could you call it "less"? I renamed it to cmp_less. https://codereview.chromium.org/2065043002/diff/60001/test/mjsunit/wasm/expor... File test/mjsunit/wasm/export-table.js (right): https://codereview.chromium.org/2065043002/diff/60001/test/mjsunit/wasm/expor... test/mjsunit/wasm/export-table.js:82: On 2016/06/16 09:16:13, ahaas wrote: > nit: Add an additional function in between to make sure that your sorting > approach works. Done.
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rossberg@chromium.org, ahaas@chromium.org Link to the patchset: https://codereview.chromium.org/2065043002/#ps80001 (title: "Address ahaas' comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2065043002/80001
Message was sent while issue was closed.
Description was changed from ========== [wasm] Check for duplicate export names Without this check, instantiation of the wasm module would fail on DefineOwnProperty on the exports object for the duplicate export. Now we detect this as validation error. R=rossberg@chromium.org, titzer@chromium.org, ahaas@chromium.org ========== to ========== [wasm] Check for duplicate export names Without this check, instantiation of the wasm module would fail on DefineOwnProperty on the exports object for the duplicate export. Now we detect this as validation error. R=rossberg@chromium.org, 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.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== [wasm] Check for duplicate export names Without this check, instantiation of the wasm module would fail on DefineOwnProperty on the exports object for the duplicate export. Now we detect this as validation error. R=rossberg@chromium.org, titzer@chromium.org, ahaas@chromium.org ========== to ========== [wasm] Check for duplicate export names Without this check, instantiation of the wasm module would fail on DefineOwnProperty on the exports object for the duplicate export. Now we detect this as validation error. R=rossberg@chromium.org, titzer@chromium.org, ahaas@chromium.org Committed: https://crrev.com/6fa656fde263b77ab806c1a76dcfec49cf48357b Cr-Commit-Position: refs/heads/master@{#37038} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6fa656fde263b77ab806c1a76dcfec49cf48357b Cr-Commit-Position: refs/heads/master@{#37038} |
