Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2558813004:
[wasm][asm.js] Fail sooner if eval is present. (Closed)
|
Created:
4 years ago by bradnelson Modified:
4 years ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm][asm.js] Fail sooner if eval is present.
Use of eval in a function wraps it in a context.
This throws off assumptions not checked until later,
which is at odds with incremental validation and conversion.
Check that module parameters are PARAMETER location early.
BUG=672045
R=titzer@chromium.org
Committed: https://crrev.com/6deb99c6d96893471d296dbf7434066818fc1688
Cr-Commit-Position: refs/heads/master@{#41594}
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix #Patch Set 3 : fix #Patch Set 4 : fix #
Total comments: 2
Messages
Total messages: 37 (22 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...
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...) 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...)
https://codereview.chromium.org/2558813004/diff/1/src/asmjs/asm-typer.cc File src/asmjs/asm-typer.cc (right): https://codereview.chromium.org/2558813004/diff/1/src/asmjs/asm-typer.cc#newc... src/asmjs/asm-typer.cc:586: if (param->location() != VariableLocation::PARAMETER) { I don't think this is the check you want. There are some predicates on the scope that it contains an eval (or that an inner scope contains eval). I think you want to use those.
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 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...
bradnelson@google.com changed reviewers: + bradnelson@google.com
ptal https://codereview.chromium.org/2558813004/diff/1/src/asmjs/asm-typer.cc File src/asmjs/asm-typer.cc (right): https://codereview.chromium.org/2558813004/diff/1/src/asmjs/asm-typer.cc#newc... src/asmjs/asm-typer.cc:586: if (param->location() != VariableLocation::PARAMETER) { On 2016/12/08 10:39:54, titzer wrote: > I don't think this is the check you want. There are some predicates on the scope > that it contains an eval (or that an inner scope contains eval). I think you > want to use those. Done.
lgtm
The CQ bit was unchecked by bradnelson@google.com
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/17494)
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm modulo test comment https://codereview.chromium.org/2558813004/diff/60001/test/mjsunit/asm/regres... File test/mjsunit/asm/regress-672045.js (right): https://codereview.chromium.org/2558813004/diff/60001/test/mjsunit/asm/regres... test/mjsunit/asm/regress-672045.js:10: return { foo: function(y) { return eval(1); } }; Do you want to pull this function literal out to be more asmy?
https://codereview.chromium.org/2558813004/diff/60001/test/mjsunit/asm/regres... File test/mjsunit/asm/regress-672045.js (right): https://codereview.chromium.org/2558813004/diff/60001/test/mjsunit/asm/regres... test/mjsunit/asm/regress-672045.js:10: return { foo: function(y) { return eval(1); } }; On 2016/12/08 14:16:38, titzer wrote: > Do you want to pull this function literal out to be more asmy? Pulled out it doesn't tickle the issue, as then other cases catch it first.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30277)
mstarzinger@chromium.org changed reviewers: + mstarzinger@chromium.org
LGTM on "ast" changes.
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": 60001, "attempt_start_ts": 1481208073861420,
"parent_rev": "74a8a51c711fb9c8422729895e5fa657ee579900", "commit_rev":
"367a0aca65932ca1d2b86073e8b377a365989110"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [wasm][asm.js] Fail sooner if eval is present. Use of eval in a function wraps it in a context. This throws off assumptions not checked until later, which is at odds with incremental validation and conversion. Check that module parameters are PARAMETER location early. BUG=672045 R=titzer@chromium.org ========== to ========== [wasm][asm.js] Fail sooner if eval is present. Use of eval in a function wraps it in a context. This throws off assumptions not checked until later, which is at odds with incremental validation and conversion. Check that module parameters are PARAMETER location early. BUG=672045 R=titzer@chromium.org Committed: https://crrev.com/6deb99c6d96893471d296dbf7434066818fc1688 Cr-Commit-Position: refs/heads/master@{#41594} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6deb99c6d96893471d296dbf7434066818fc1688 Cr-Commit-Position: refs/heads/master@{#41594} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
