Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1508293003:
[Interpreter] Add support for calling eval. (Closed)
Description[Interpreter] Add support for calling eval.
Adds support for calling eval to the interpreter.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/32879ae0fa524f036f6855fd15c7de82d96e1395
Cr-Commit-Position: refs/heads/master@{#33184}
Patch Set 1 : Use CallRuntimePair support #Patch Set 2 : Add flag fix #Patch Set 3 : Rebased #
Total comments: 4
Patch Set 4 : Rebase #Patch Set 5 : Fix comments #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 32 (15 generated)
Patchset #1 (id:1) has been deleted
rmcilroy@chromium.org changed reviewers: + bmeurer@chromium.org, oth@chromium.org
Orion PTAL at the interpreter changes. Benedikt PTAL at the runtime changes. Mythri FYI.
Ping?
Thanks, lgtm.
Please be aware that this implementation is not entirely correct. Doing the
lookup twice is observable by JavaScript. In the following repro the "MARK" is
printed twice.
// Flags: --ignition --ignition-filter=f
var world = "WORLD";
function g() {
print("HELLO", this.world);
}
function getter() {
print("MARK");
return g;
}
Object.defineProperty(this, "eval", { get: getter });
function f() {
eval("123")
}
f();
rmcilroy@chromium.org changed reviewers: + mstarzinger@chromium.org
On 2015/12/11 15:17:31, Michael Starzinger wrote:
> Please be aware that this implementation is not entirely correct. Doing the
> lookup twice is observable by JavaScript. In the following repro the "MARK" is
> printed twice.
>
> // Flags: --ignition --ignition-filter=f
>
> var world = "WORLD";
>
> function g() {
> print("HELLO", this.world);
> }
>
> function getter() {
> print("MARK");
> return g;
> }
>
> Object.defineProperty(this, "eval", { get: getter });
>
> function f() {
> eval("123")
> }
>
> f();
Good point, thanks for the warning. I've re-implemented this to only do a single
lookup using the CallRuntimePair support in
https://codereview.chromium.org/1568493002/. PTAL, thanks.
Mythri: I've added the tests which you had in your original lookup CL to this
change (some modified) - could you please take a look and check that this is all
the tests you wanted to do?
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508293003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508293003/20001
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508293003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508293003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
The CQ bit was checked by rmcilroy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508293003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508293003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/1275) v8_win64_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
mythria@chromium.org changed reviewers: + mythria@chromium.org
Thanks Ross, yes those are the tests I planned to add.
LGTM with nits. https://codereview.chromium.org/1508293003/diff/80001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1508293003/diff/80001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1653: // Call LoadLookupSlotCallee/Receiver to get the Callee and Receiver nit: Comment is outdated, also s/Callee/callee/ and s/Receiver/receiver/ here. https://codereview.chromium.org/1508293003/diff/80001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1684: // will mutate the callee and receiver values. nit: This comment is outdated (also in the AstGraphBuilder), the below block no longer mutates the receiver, it only mutates the callee value. Could you update here (and if you feel like it also in the AstGraphBuilder)?
Also, the CL description is slightly outdated.
Description was changed from ========== [Interpreter] Add support for calling eval. Adds support for calling eval. Currently there is no support for lookup variables, so the eval call can't yet access / modify variables in the function's scope. Temporarily adds Runtime::kInterpreterLoadLookupSlotCallee and Runtime::kInterpreterLoadLookupSlotCallee to enable the interpreter to get the results of LoadLookupSlot until it has support for calling runtime functions which return pairs. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Add support for calling eval. Adds support for calling eval to the interpreter. BUG=v8:4280 LOG=N ==========
https://codereview.chromium.org/1508293003/diff/80001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1508293003/diff/80001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1653: // Call LoadLookupSlotCallee/Receiver to get the Callee and Receiver On 2016/01/08 13:40:47, Michael Starzinger wrote: > nit: Comment is outdated, also s/Callee/callee/ and s/Receiver/receiver/ here. Done. https://codereview.chromium.org/1508293003/diff/80001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1684: // will mutate the callee and receiver values. On 2016/01/08 13:40:47, Michael Starzinger wrote: > nit: This comment is outdated (also in the AstGraphBuilder), the below block no > longer mutates the receiver, it only mutates the callee value. Could you update > here (and if you feel like it also in the AstGraphBuilder)? Done (also in AstGraphBuilder).
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org, mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/1508293003/#ps120001 (title: "Fix comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508293003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508293003/120001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Add support for calling eval. Adds support for calling eval to the interpreter. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Add support for calling eval. Adds support for calling eval to the interpreter. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Add support for calling eval. Adds support for calling eval to the interpreter. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Add support for calling eval. Adds support for calling eval to the interpreter. BUG=v8:4280 LOG=N Committed: https://crrev.com/32879ae0fa524f036f6855fd15c7de82d96e1395 Cr-Commit-Position: refs/heads/master@{#33184} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/32879ae0fa524f036f6855fd15c7de82d96e1395 Cr-Commit-Position: refs/heads/master@{#33184} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
