Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1721983005:
[Interpreter] Handles stack overflow in interpreter. (Closed)
Description[Interpreter] Handles stack overflow in interpreter.
Handles stack overflow in interpreter.
1. When visiting function literal, if the shared function
info cannot be found we should return a stack overflow.
2. When visiting the ast graph, if stack overflow happens
then all the ast nodes are not visited, so we need to have
appropriate handling in the AccumulatorResultScope and
RegisterResultScope.
3. MakeBytecode should not return a suceess unconditionally.
If there is a stack overflow, it should return false, so
RangeError can be thrown.
BUG=v8:4280, v8:4680
LOG=N
Committed: https://crrev.com/4d325854f29c8ed498d29a5f2f343233646d2815
Cr-Commit-Position: refs/heads/master@{#34282}
Patch Set 1 #Patch Set 2 : disabled the check to see if result is available on stack overflow. #
Total comments: 3
Patch Set 3 : Fixed comments #
Total comments: 1
Patch Set 4 : Fixes RegisterResultScope to handle stack overflow. #Patch Set 5 : Rebased, and removes regress/regress-crbug-589472.js from skip list. #Patch Set 6 : Fixes a bug. #
Total comments: 2
Patch Set 7 : Fixed comments. #
Messages
Total messages: 46 (22 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/1721983005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721983005/1
mythria@chromium.org changed reviewers: + oth@chromium.org
PTAL. Thanks, Mythri
lgtm, but this'll need an additional reviewer for the test/ update.
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/1775) 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_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/1773) v8_linux_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)
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/1721983005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721983005/20001
mythria@chromium.org changed reviewers: + rmcilroy@chromium.org
PTAL. Thanks, Mythri https://codereview.chromium.org/1721983005/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1721983005/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:458: if (!generator_->HasStackOverflow()) DCHECK(result_identified()); when there is a stack overflow, we don't visit any new nodes. we just unwind the stack and return. So this check will fail. I am not sure if we have to handle stack overflows at other places. I think ContextScope, RegisterAllocationScope, ControlScope does not need any special handling.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yep, looks good here. Minor comment, but still lgtm. https://codereview.chromium.org/1721983005/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1721983005/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:458: if (!generator_->HasStackOverflow()) DCHECK(result_identified()); On 2016/02/24 11:49:39, mythria wrote: > when there is a stack overflow, we don't visit any new nodes. we just unwind the > stack and return. So this check will fail. I am not sure if we have to handle > stack overflows at other places. I think ContextScope, RegisterAllocationScope, > ControlScope does not need any special handling. All the logic could go in the DCHECK(result_identified || ...HasStackOverflow).
Thanks, fixed it. https://codereview.chromium.org/1721983005/diff/20001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1721983005/diff/20001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:458: if (!generator_->HasStackOverflow()) DCHECK(result_identified()); On 2016/02/24 13:19:47, oth wrote: > On 2016/02/24 11:49:39, mythria wrote: > > when there is a stack overflow, we don't visit any new nodes. we just unwind > the > > stack and return. So this check will fail. I am not sure if we have to handle > > stack overflows at other places. I think ContextScope, > RegisterAllocationScope, > > ControlScope does not need any special handling. > > All the logic could go in the DCHECK(result_identified || ...HasStackOverflow). Done.
LGTM
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org Link to the patchset: https://codereview.chromium.org/1721983005/#ps40001 (title: "Fixed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721983005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721983005/40001
https://codereview.chromium.org/1721983005/diff/40001/src/interpreter/bytecod... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1721983005/diff/40001/src/interpreter/bytecod... src/interpreter/bytecode-generator.cc:1286: execution_result()->SetResultInAccumulator(); Do you need the SetResultInAccumulator now? I don't think you do?
The CQ bit was unchecked by mythria@chromium.org
Description was changed from ========== [Interpreter] Handles the case when shared info is null in VisitFunctionLiteral. When visiting function literals a stack overflow error is returned if the SharedFunctionInfo is not available for the function literal. BUG=v8:4280,v8:4680 LOG=N ========== to ========== [Interpreter] Handles stack overflow in interpreter. Handles stack overflow in interpreter. 1. When visiting function literal, if the shared function info cannot be found we should return a stack overflow. 2. When visiting the ast graph, if stack overflow happens then all the ast nodes are not visited, so we need to have appropriate handling in the AccumulatorResultScope and RegisterResultScope. 3. MakeBytecode should not return a suceess unconditionally. If there is a stack overflow, it shoudl return false, so RangeError can be thrown. BUG=v8:4280,v8:4680 LOG=N ==========
Description was changed from ========== [Interpreter] Handles stack overflow in interpreter. Handles stack overflow in interpreter. 1. When visiting function literal, if the shared function info cannot be found we should return a stack overflow. 2. When visiting the ast graph, if stack overflow happens then all the ast nodes are not visited, so we need to have appropriate handling in the AccumulatorResultScope and RegisterResultScope. 3. MakeBytecode should not return a suceess unconditionally. If there is a stack overflow, it shoudl return false, so RangeError can be thrown. BUG=v8:4280,v8:4680 LOG=N ========== to ========== [Interpreter] Handles stack overflow in interpreter. Handles stack overflow in interpreter. 1. When visiting function literal, if the shared function info cannot be found we should return a stack overflow. 2. When visiting the ast graph, if stack overflow happens then all the ast nodes are not visited, so we need to have appropriate handling in the AccumulatorResultScope and RegisterResultScope. 3. MakeBytecode should not return a suceess unconditionally. If there is a stack overflow, it shoudl return false, so RangeError can be thrown. BUG=v8:4280,v8:4680 LOG=N ==========
Description was changed from ========== [Interpreter] Handles stack overflow in interpreter. Handles stack overflow in interpreter. 1. When visiting function literal, if the shared function info cannot be found we should return a stack overflow. 2. When visiting the ast graph, if stack overflow happens then all the ast nodes are not visited, so we need to have appropriate handling in the AccumulatorResultScope and RegisterResultScope. 3. MakeBytecode should not return a suceess unconditionally. If there is a stack overflow, it shoudl return false, so RangeError can be thrown. BUG=v8:4280,v8:4680 LOG=N ========== to ========== [Interpreter] Handles stack overflow in interpreter. Handles stack overflow in interpreter. 1. When visiting function literal, if the shared function info cannot be found we should return a stack overflow. 2. When visiting the ast graph, if stack overflow happens then all the ast nodes are not visited, so we need to have appropriate handling in the AccumulatorResultScope and RegisterResultScope. 3. MakeBytecode should not return a suceess unconditionally. If there is a stack overflow, it should return false, so RangeError can be thrown. 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/1721983005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721983005/80001
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...)
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/1721983005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721983005/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The test added by Michi works as well. I updated the mjsunit.status PTAL. Thanks, Mythri.
Lgtm, thanks! https://codereview.chromium.org/1721983005/diff/100001/src/interpreter/byteco... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1721983005/diff/100001/src/interpreter/byteco... src/interpreter/bytecode-generator.cc:458: bool HasStackOverflow() const { return generator_->HasStackOverflow(); } Could you just expose generator() as a protected method and use that below instead please.
Thanks done. https://codereview.chromium.org/1721983005/diff/100001/src/interpreter/byteco... File src/interpreter/bytecode-generator.cc (right): https://codereview.chromium.org/1721983005/diff/100001/src/interpreter/byteco... src/interpreter/bytecode-generator.cc:458: bool HasStackOverflow() const { return generator_->HasStackOverflow(); } On 2016/02/25 10:35:13, rmcilroy wrote: > Could you just expose generator() as a protected method and use that below > instead 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/1721983005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721983005/120001
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 oth@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1721983005/#ps120001 (title: "Fixed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721983005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721983005/120001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Handles stack overflow in interpreter. Handles stack overflow in interpreter. 1. When visiting function literal, if the shared function info cannot be found we should return a stack overflow. 2. When visiting the ast graph, if stack overflow happens then all the ast nodes are not visited, so we need to have appropriate handling in the AccumulatorResultScope and RegisterResultScope. 3. MakeBytecode should not return a suceess unconditionally. If there is a stack overflow, it should return false, so RangeError can be thrown. BUG=v8:4280,v8:4680 LOG=N ========== to ========== [Interpreter] Handles stack overflow in interpreter. Handles stack overflow in interpreter. 1. When visiting function literal, if the shared function info cannot be found we should return a stack overflow. 2. When visiting the ast graph, if stack overflow happens then all the ast nodes are not visited, so we need to have appropriate handling in the AccumulatorResultScope and RegisterResultScope. 3. MakeBytecode should not return a suceess unconditionally. If there is a stack overflow, it should return false, so RangeError can be thrown. BUG=v8:4280,v8:4680 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Handles stack overflow in interpreter. Handles stack overflow in interpreter. 1. When visiting function literal, if the shared function info cannot be found we should return a stack overflow. 2. When visiting the ast graph, if stack overflow happens then all the ast nodes are not visited, so we need to have appropriate handling in the AccumulatorResultScope and RegisterResultScope. 3. MakeBytecode should not return a suceess unconditionally. If there is a stack overflow, it should return false, so RangeError can be thrown. BUG=v8:4280,v8:4680 LOG=N ========== to ========== [Interpreter] Handles stack overflow in interpreter. Handles stack overflow in interpreter. 1. When visiting function literal, if the shared function info cannot be found we should return a stack overflow. 2. When visiting the ast graph, if stack overflow happens then all the ast nodes are not visited, so we need to have appropriate handling in the AccumulatorResultScope and RegisterResultScope. 3. MakeBytecode should not return a suceess unconditionally. If there is a stack overflow, it should return false, so RangeError can be thrown. BUG=v8:4280,v8:4680 LOG=N Committed: https://crrev.com/4d325854f29c8ed498d29a5f2f343233646d2815 Cr-Commit-Position: refs/heads/master@{#34282} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4d325854f29c8ed498d29a5f2f343233646d2815 Cr-Commit-Position: refs/heads/master@{#34282} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
