Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2981883002:
[wasm] More extensive indirect dispatch signature mismatch tests. (Closed)
|
Created:
3 years, 5 months ago by titzer Modified:
3 years, 4 months ago Reviewers:
Clemens Hammacher CC:
v8-reviews_googlegroups.com, wasm-v8_google.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] More extensive indirect dispatch signature mismatch tests.
BUG=chromium:747995
Review-Url: https://codereview.chromium.org/2981883002
Cr-Commit-Position: refs/heads/master@{#46848}
Committed: https://chromium.googlesource.com/v8/v8/+/1bbbc8cc61d8243dacba0061432d73db9e24050e
Patch Set 1 #Patch Set 2 : [wasm] More extensive indirect dispatch signature mismatch tests. #
Total comments: 7
Patch Set 3 : Fix copyright year. #Patch Set 4 : Add a mutex to SignatureMap #Patch Set 5 : Allocate a mutex on the heap. #
Total comments: 1
Patch Set 6 : Shorter constructor #
Messages
Total messages: 31 (20 generated)
The CQ bit was checked by titzer@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] WIP more extensive indirect dispatch signature mismatch tests. BUG= ========== to ========== [wasm] WIP more extensive indirect dispatch signature mismatch tests. BUG= ==========
titzer@chromium.org changed reviewers: + clemensh@chromium.org
Description was changed from ========== [wasm] WIP more extensive indirect dispatch signature mismatch tests. BUG= ========== to ========== [wasm] More extensive indirect dispatch signature mismatch tests. BUG= ==========
On 2017/07/24 11:32:45, titzer wrote: > Description was changed from > > ========== > [wasm] WIP more extensive indirect dispatch signature mismatch tests. > > BUG= > ========== > > to > > ========== > [wasm] More extensive indirect dispatch signature mismatch tests. > > BUG= > ========== PTAL, this is the more expensive test for signature mismatches via indirect calls.
https://codereview.chromium.org/2981883002/diff/20001/test/mjsunit/wasm/indir... File test/mjsunit/wasm/indirect-sig-mismatch.js (right): https://codereview.chromium.org/2981883002/diff/20001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-sig-mismatch.js:1: // Copyright 2016 the V8 project authors. All rights reserved. 2017 https://codereview.chromium.org/2981883002/diff/20001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-sig-mismatch.js:52: m_i_ii.sig = kSig_i_ii; I don't get what this test is supposed to test. https://codereview.chromium.org/2981883002/diff/20001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-sig-mismatch.js:111: print(" exp=" + exp); Do you want to print the variable {exported}? {exp} is always null.
https://codereview.chromium.org/2981883002/diff/20001/test/mjsunit/wasm/indir... File test/mjsunit/wasm/indirect-sig-mismatch.js (right): https://codereview.chromium.org/2981883002/diff/20001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-sig-mismatch.js:1: // Copyright 2016 the V8 project authors. All rights reserved. On 2017/07/24 11:50:23, Clemens Hammacher wrote: > 2017 Done. https://codereview.chromium.org/2981883002/diff/20001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-sig-mismatch.js:52: m_i_ii.sig = kSig_i_ii; On 2017/07/24 11:50:23, Clemens Hammacher wrote: > I don't get what this test is supposed to test. What this does is record the signature of the global exported functions m_* (from the first module) as a property on those functions. That way the test loop below can check this signature against the signature attached below on the functions exported from the second module. https://codereview.chromium.org/2981883002/diff/20001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-sig-mismatch.js:111: print(" exp=" + exp); On 2017/07/24 11:50:23, Clemens Hammacher wrote: > Do you want to print the variable {exported}? {exp} is always null. Good catch. One does not simply rename variables in JavaScript ;-)
lgtm https://codereview.chromium.org/2981883002/diff/20001/test/mjsunit/wasm/indir... File test/mjsunit/wasm/indirect-sig-mismatch.js (right): https://codereview.chromium.org/2981883002/diff/20001/test/mjsunit/wasm/indir... test/mjsunit/wasm/indirect-sig-mismatch.js:52: m_i_ii.sig = kSig_i_ii; On 2017/07/24 at 11:57:39, titzer wrote: > On 2017/07/24 11:50:23, Clemens Hammacher wrote: > > I don't get what this test is supposed to test. > > What this does is record the signature of the global exported functions m_* (from the first module) as a property on those functions. That way the test loop below can check this signature against the signature attached below on the functions exported from the second module. Ah, I see. Thanks for the explanation.
The CQ bit was checked by titzer@chromium.org
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
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...)
The CQ bit was checked by titzer@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...
On 2017/07/24 12:44:05, commit-bot: I haz the power wrote: > 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...) PTAL. Added a mutex to protect against the race condition that this test exposed.
lgtm with nit. https://codereview.chromium.org/2981883002/diff/80001/src/wasm/signature-map.cc File src/wasm/signature-map.cc (right): https://codereview.chromium.org/2981883002/diff/80001/src/wasm/signature-map.... src/wasm/signature-map.cc:11: SignatureMap::SignatureMap() { you can just use ": mutex_(new base::Mutex())" (as initializer list).
The CQ bit was checked by titzer@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] More extensive indirect dispatch signature mismatch tests. BUG= ========== to ========== [wasm] More extensive indirect dispatch signature mismatch tests. BUG=chromium:747995 ==========
On 2017/07/24 15:39:49, Clemens Hammacher wrote: > lgtm with nit. > > https://codereview.chromium.org/2981883002/diff/80001/src/wasm/signature-map.cc > File src/wasm/signature-map.cc (right): > > https://codereview.chromium.org/2981883002/diff/80001/src/wasm/signature-map.... > src/wasm/signature-map.cc:11: SignatureMap::SignatureMap() { > you can just use ": mutex_(new base::Mutex())" (as initializer list). Ah yes, thanks.
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 titzer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clemensh@chromium.org Link to the patchset: https://codereview.chromium.org/2981883002/#ps100001 (title: "Shorter constructor")
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": 1500923911361020,
"parent_rev": "ec1c5009feaacad398dc2cf1ba9975390cbd9a66", "commit_rev":
"1bbbc8cc61d8243dacba0061432d73db9e24050e"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] More extensive indirect dispatch signature mismatch tests. BUG=chromium:747995 ========== to ========== [wasm] More extensive indirect dispatch signature mismatch tests. BUG=chromium:747995 Review-Url: https://codereview.chromium.org/2981883002 Cr-Commit-Position: refs/heads/master@{#46848} Committed: https://chromium.googlesource.com/v8/v8/+/1bbbc8cc61d8243dacba0061432d73db9e2... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/v8/v8/+/1bbbc8cc61d8243dacba0061432d73db9e2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
