Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1768123002:
[Interpreter] Fixes a bug when popping context to correct level on break/continue. (Closed)
Description[Interpreter] Fixes a bug when popping context to correct level on break/continue.
The current implementation does not consider the case when the context of
the control scope and the current context differ. It is possible that they are
different in some cases for example: with statements. This cl fixes this.
BUG=v8:4280, v8:4680
LOG=N
Committed: https://crrev.com/e260bd5356f19d8538dcadfe6d619922106719a3
Cr-Commit-Position: refs/heads/master@{#34609}
Patch Set 1 #
Total comments: 2
Patch Set 2 : "removes should_pop_context. Now context is popped always on return." #Patch Set 3 : Restores should_pop_context_ and fixes it to pop contexts only on break/continue #Patch Set 4 : fixed test-bytecode-generator tests #
Total comments: 8
Patch Set 5 : Fixes comments #
Messages
Total messages: 37 (17 generated)
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/1768123002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768123002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
mythria@chromium.org changed reviewers: + rmcilroy@chromium.org
PTAL. Thanks, Mythri
https://codereview.chromium.org/1768123002/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1768123002/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:26: bool should_pop_context = true) Now that we are popping more explicitly on return, and we drop dead code after a return, the should_pop_context argument seems less useful - could you remove this while you are here please?
And lgtm, thanks!
Description was changed from ========== [Interpreter] Fixes a bug when popping context to correct level on break/continue. The current implementation does not consider the case when the context of the control scope and the current context differ. It is possible that they are different in some cases for example: with statements. This cl fixes this. BUG=v8:4280,v8:4680 LOG=N ========== to ========== [Interpreter] Fixes a bug when popping context to correct level on break/continue. The current implementation does not consider the case when the context of the control scope and the current context differ. It is possible that they are different in some cases for example: with statements. This cl fixes this. Also removes should_pop_context in the context scope. BUG=v8:4280,v8:4680 LOG=N ==========
Thanks, I removed it. https://codereview.chromium.org/1768123002/diff/1/src/interpreter/bytecode-ge... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1768123002/diff/1/src/interpreter/bytecode-ge... src/interpreter/bytecode-generator.cc:26: bool should_pop_context = true) On 2016/03/08 07:34:37, rmcilroy wrote: > Now that we are popping more explicitly on return, and we drop dead code after a > return, the should_pop_context argument seems less useful - could you remove > this while you are here please? Done.
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/1768123002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768123002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/2526) v8_linux64_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...) v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)
Description was changed from ========== [Interpreter] Fixes a bug when popping context to correct level on break/continue. The current implementation does not consider the case when the context of the control scope and the current context differ. It is possible that they are different in some cases for example: with statements. This cl fixes this. Also removes should_pop_context in the context scope. BUG=v8:4280,v8:4680 LOG=N ========== to ========== [Interpreter] Fixes a bug when popping context to correct level on break/continue. The current implementation does not consider the case when the context of the control scope and the current context differ. It is possible that they are different in some cases for example: with statements. This cl fixes this. BUG=v8:4280,v8:4680 LOG=N ==========
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/1768123002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768123002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
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/1768123002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768123002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. I pop context only on break or continue now.
LGTM once comments addressed. https://codereview.chromium.org/1768123002/diff/60001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1768123002/diff/60001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:369: // TODO(rmcilroy): Only emit a single context pop. nit - move TODO down below comment. https://codereview.chromium.org/1768123002/diff/60001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:370: // pop contexts only for break and continue. For Return and throw it is nit /s/pop/Pop https://codereview.chromium.org/1768123002/diff/60001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:370: // pop contexts only for break and continue. For Return and throw it is /s/Return/return https://codereview.chromium.org/1768123002/diff/60001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:371: // not required to pop. Debuuger expects that the context is not popped on /s/debuuger/debugger
Thanks, done. Mythri https://codereview.chromium.org/1768123002/diff/60001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1768123002/diff/60001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:369: // TODO(rmcilroy): Only emit a single context pop. On 2016/03/08 17:33:40, rmcilroy wrote: > nit - move TODO down below comment. Done. https://codereview.chromium.org/1768123002/diff/60001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:370: // pop contexts only for break and continue. For Return and throw it is On 2016/03/08 17:33:40, rmcilroy wrote: > nit /s/pop/Pop Done. https://codereview.chromium.org/1768123002/diff/60001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:370: // pop contexts only for break and continue. For Return and throw it is On 2016/03/08 17:33:40, rmcilroy wrote: > /s/Return/return Done. https://codereview.chromium.org/1768123002/diff/60001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:371: // not required to pop. Debuuger expects that the context is not popped on On 2016/03/08 17:33:40, rmcilroy wrote: > /s/debuuger/debugger Done.
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/1768123002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768123002/80001
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 mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1768123002/#ps80001 (title: "Fixes comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1768123002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1768123002/80001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Fixes a bug when popping context to correct level on break/continue. The current implementation does not consider the case when the context of the control scope and the current context differ. It is possible that they are different in some cases for example: with statements. This cl fixes this. BUG=v8:4280,v8:4680 LOG=N ========== to ========== [Interpreter] Fixes a bug when popping context to correct level on break/continue. The current implementation does not consider the case when the context of the control scope and the current context differ. It is possible that they are different in some cases for example: with statements. This cl fixes this. BUG=v8:4280,v8:4680 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Fixes a bug when popping context to correct level on break/continue. The current implementation does not consider the case when the context of the control scope and the current context differ. It is possible that they are different in some cases for example: with statements. This cl fixes this. BUG=v8:4280,v8:4680 LOG=N ========== to ========== [Interpreter] Fixes a bug when popping context to correct level on break/continue. The current implementation does not consider the case when the context of the control scope and the current context differ. It is possible that they are different in some cases for example: with statements. This cl fixes this. BUG=v8:4280,v8:4680 LOG=N Committed: https://crrev.com/e260bd5356f19d8538dcadfe6d619922106719a3 Cr-Commit-Position: refs/heads/master@{#34609} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e260bd5356f19d8538dcadfe6d619922106719a3 Cr-Commit-Position: refs/heads/master@{#34609} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
