Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1523423003:
[Interpreter] Marks that 'throw' has returned a value. (Closed)
Description[Interpreter] Marks that 'throw' has returned a value.
This is to fix some of the failing test262 tests with ignition flag.
In few test262 tests, there is a throw from the script scope. Rewriter::Rewrite
pass converts expression statements into assignment statements in script scope.
This causes interpreter to fail because assignment expression expects a result
in accumulator but throw statement does not return a value. To fix this, we
now mark that accumulator contains a value when visiting throw statement.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/232e28d65e95ec928cdc339ab51bfb93654bc1e2
Cr-Commit-Position: refs/heads/master@{#33408}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebased the patch. #Patch Set 3 : Throw always marks that a result is returned. #
Total comments: 5
Patch Set 4 : Fixes nits and updates mjsunit.status and test262.status #
Messages
Total messages: 25 (11 generated)
mythria@chromium.org changed reviewers: + bmeurer@chromium.org, oth@chromium.org, rmcilroy@chromium.org, rossberg@chromium.org
Could you please take a look. Andreas, I never looked at parsing code, so I just wanted to check if my understanding regarding Rewrite pass is correct. In my understanding expression statements are converted to assignment statements in rewrite pass. Thanks, Mythri
mythria@chromium.org changed reviewers: + mstarzinger@chromium.org - bmeurer@chromium.org, oth@chromium.org
Michi, This is the cl I was explaining you today afternoon. Could you please take a look. Thanks, Mythri
Looking good. https://codereview.chromium.org/1523423003/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1523423003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:1546: if (info()->literal()->scope()->is_script_scope() || I wouldn't even narrow this to script or eval scopes, but in general just mark all throw expressions as delivering the completion value in the accumulator unconditionally. As discussed offline today: This is a short-coming of our AST, where "throw"is modeled as an expression instead of a statement (as the spec defines it), exactly because of the internal replacement we perform. With proper do-expressions such a rewriting could be done without this hack AFAICT. But that is cleanup for another day.
Description was changed from ========== [Interpreter] Marks that 'throw' has returned a value in script and eval scopes. This is to fix some of the failing test262 tests with ignition flag. In few test262 tests, there is a throw from the script scope. Rewriter::Rewrite pass converts expression statements into assignment statements in script scope. This causes interpreter to fail because assignment expression expects a result in accumulator but throw statement does not return a value. To fix this, we now mark that accumulator contains a value when visiting throw statement in script and eval scopes. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Marks that 'throw' has returned a value. This is to fix some of the failing test262 tests with ignition flag. In few test262 tests, there is a throw from the script scope. Rewriter::Rewrite pass converts expression statements into assignment statements in script scope. This causes interpreter to fail because assignment expression expects a result in accumulator but throw statement does not return a value. To fix this, we now mark that accumulator contains a value when visiting throw statement. BUG=v8:4280 LOG=N ==========
Thanks, I fixed it. PTAL. Thanks, Mythri https://codereview.chromium.org/1523423003/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1523423003/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:1546: if (info()->literal()->scope()->is_script_scope() || On 2016/01/18 21:23:03, Michael Starzinger wrote: > I wouldn't even narrow this to script or eval scopes, but in general just mark > all throw expressions as delivering the completion value in the accumulator > unconditionally. > > As discussed offline today: This is a short-coming of our AST, where "throw"is > modeled as an expression instead of a statement (as the spec defines it), > exactly because of the internal replacement we perform. With proper > do-expressions such a rewriting could be done without this hack AFAICT. But that > is cleanup for another day. Done. https://codereview.chromium.org/1523423003/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1523423003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:412: VisitForEffect(scope()->GetIllegalRedeclaration()); GetIllegalRedeclaration is an expression, so I added a visitForEffect instead of visit. Otherwise, we will not have an ExpressionResultScope when we visit throw from here.
The CQ bit was checked by mythria@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/1523423003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1523423003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
LGTM thanks! Before landing, could you check the tests I disabled in https://codereview.chromium.org/1595223004/ and see if this fixes any of those before landing please. https://codereview.chromium.org/1523423003/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1523423003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:412: VisitForEffect(scope()->GetIllegalRedeclaration()); On 2016/01/19 10:48:04, mythria wrote: > GetIllegalRedeclaration is an expression, so I added a visitForEffect instead of > visit. Otherwise, we will not have an ExpressionResultScope when we visit throw > from here. SGTM
LGTM. https://codereview.chromium.org/1523423003/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1523423003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:412: VisitForEffect(scope()->GetIllegalRedeclaration()); On 2016/01/19 10:48:04, mythria wrote: > GetIllegalRedeclaration is an expression, so I added a visitForEffect instead of > visit. Otherwise, we will not have an ExpressionResultScope when we visit throw > from here. Acknowledged. https://codereview.chromium.org/1523423003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1484: // converted to assignment statements in Rewriter::ReWrite pass. An nit: s/to/from/
The CQ bit was checked by mythria@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/1523423003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1523423003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I updated test262.status and mjsunit.status. It fixes 2 out of 3 from test262 and very few in mjsunit that were failing in this cl: https://codereview.chromium.org/1595223004/ Thanks, Mythri https://codereview.chromium.org/1523423003/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1523423003/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1484: // converted to assignment statements in Rewriter::ReWrite pass. An On 2016/01/19 11:45:59, Michael Starzinger wrote: > nit: s/to/from/ Done.
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/1523423003/#ps60001 (title: "Fixes nits and updates mjsunit.status and test262.status")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1523423003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1523423003/60001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Marks that 'throw' has returned a value. This is to fix some of the failing test262 tests with ignition flag. In few test262 tests, there is a throw from the script scope. Rewriter::Rewrite pass converts expression statements into assignment statements in script scope. This causes interpreter to fail because assignment expression expects a result in accumulator but throw statement does not return a value. To fix this, we now mark that accumulator contains a value when visiting throw statement. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Marks that 'throw' has returned a value. This is to fix some of the failing test262 tests with ignition flag. In few test262 tests, there is a throw from the script scope. Rewriter::Rewrite pass converts expression statements into assignment statements in script scope. This causes interpreter to fail because assignment expression expects a result in accumulator but throw statement does not return a value. To fix this, we now mark that accumulator contains a value when visiting throw statement. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Marks that 'throw' has returned a value. This is to fix some of the failing test262 tests with ignition flag. In few test262 tests, there is a throw from the script scope. Rewriter::Rewrite pass converts expression statements into assignment statements in script scope. This causes interpreter to fail because assignment expression expects a result in accumulator but throw statement does not return a value. To fix this, we now mark that accumulator contains a value when visiting throw statement. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Marks that 'throw' has returned a value. This is to fix some of the failing test262 tests with ignition flag. In few test262 tests, there is a throw from the script scope. Rewriter::Rewrite pass converts expression statements into assignment statements in script scope. This causes interpreter to fail because assignment expression expects a result in accumulator but throw statement does not return a value. To fix this, we now mark that accumulator contains a value when visiting throw statement. BUG=v8:4280 LOG=N Committed: https://crrev.com/232e28d65e95ec928cdc339ab51bfb93654bc1e2 Cr-Commit-Position: refs/heads/master@{#33408} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/232e28d65e95ec928cdc339ab51bfb93654bc1e2 Cr-Commit-Position: refs/heads/master@{#33408} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
