Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1481763002:
[Interpreter] Adds support for throw to bytecode graph builder. (Closed)
Description[Interpreter] Adds support for throw to bytecode graph builder.
Adds support and tests for throw to bytecode graph builder.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/c11e7bc2195e51613efbbb6de6ca56947192c0b9
Cr-Commit-Position: refs/heads/master@{#32399}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed review comments. Added TryCatch Scope instead of try-catch wrapper to test Throw. #
Total comments: 4
Patch Set 3 : Fixed nits. #
Total comments: 2
Patch Set 4 : Fixed nits #
Messages
Total messages: 17 (6 generated)
mythria@chromium.org changed reviewers: + mstarzinger@chromium.org, oth@chromium.org
Could you please review my changes. Thanks, Mythri
Description was changed from ========== [Interpreter] Add support for throw to bytecode graph builder. Adds support and tests for throw to bytecode graph builder. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Adds support for throw to bytecode graph builder. Adds support and tests for throw to bytecode graph builder. BUG=v8:4280 LOG=N ==========
https://codereview.chromium.org/1481763002/diff/1/src/compiler/bytecode-graph... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1481763002/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:820: NewNode(javascript()->CallRuntime(Runtime::kReThrow, 1), value); This should be Runtime::kThrow, not Runtime::kReThrow, otherwise the exception message will not be created and the throw location won't be setup. https://codereview.chromium.org/1481763002/diff/1/test/cctest/compiler/test-r... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1481763002/diff/1/test/cctest/compiler/test-r... test/cctest/compiler/test-run-bytecode-graph-builder.cc:985: Handle<Object> thrown_obj = v8::Utils::OpenHandle(*CompileRun(try_wrapper)); suggestion: We could also use a v8::TryCatch scope around CompileRun, that way we get by without having a JavaScript try-wrapper. Also the exception message can be checked that way. The test-run-jsexceptions.cc file has some examples of that. It's up to you, I could be convinced with landing tests as they are.
https://codereview.chromium.org/1481763002/diff/1/src/compiler/bytecode-graph... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1481763002/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:820: NewNode(javascript()->CallRuntime(Runtime::kReThrow, 1), value); On 2015/11/26 17:27:45, Michael Starzinger wrote: > This should be Runtime::kThrow, not Runtime::kReThrow, otherwise the exception > message will not be created and the throw location won't be setup. I realized later that creating the exception message will require deoptimization information, which we don't yet have for these graphs. So I would be fine with keeping kReThrow here for now (until we have deopt info) but at least leave a big TODO here.
Thanks for your comments. I addressed the review comments. Can you please review them. Thanks, Mythri https://codereview.chromium.org/1481763002/diff/1/src/compiler/bytecode-graph... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1481763002/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:820: NewNode(javascript()->CallRuntime(Runtime::kReThrow, 1), value); On 2015/11/27 09:28:46, Michael Starzinger wrote: > On 2015/11/26 17:27:45, Michael Starzinger wrote: > > This should be Runtime::kThrow, not Runtime::kReThrow, otherwise the exception > > message will not be created and the throw location won't be setup. > > I realized later that creating the exception message will require deoptimization > information, which we don't yet have for these graphs. So I would be fine with > keeping kReThrow here for now (until we have deopt info) but at least leave a > big TODO here. Thanks for pointing this. We should use Runtime::kReThrow because interpreter does not support deopt yet right? I just wanted to be explicit in the TODO comment. https://codereview.chromium.org/1481763002/diff/1/test/cctest/compiler/test-r... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1481763002/diff/1/test/cctest/compiler/test-r... test/cctest/compiler/test-run-bytecode-graph-builder.cc:985: Handle<Object> thrown_obj = v8::Utils::OpenHandle(*CompileRun(try_wrapper)); On 2015/11/26 17:27:45, Michael Starzinger wrote: > suggestion: We could also use a v8::TryCatch scope around CompileRun, that way > we get by without having a JavaScript try-wrapper. Also the exception message > can be checked that way. The test-run-jsexceptions.cc file has some examples of > that. It's up to you, I could be convinced with landing tests as they are. Thanks, I changed it to TryCatch scope. Done. https://codereview.chromium.org/1481763002/diff/20001/test/cctest/compiler/te... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1481763002/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:973: {"throw undefined;", {"undefined"}}, I tried to add tests that throw an Error object. i.e. throw new Error('Error'); Theses tests crash in OptimizeFrame::Summarize. I thought it is related to the unavailability of deopt information. Please correct me if I am wrong. I will try to investigate why they failed.
LGTM. https://codereview.chromium.org/1481763002/diff/1/src/compiler/bytecode-graph... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1481763002/diff/1/src/compiler/bytecode-graph... src/compiler/bytecode-graph-builder.cc:820: NewNode(javascript()->CallRuntime(Runtime::kReThrow, 1), value); On 2015/11/27 14:54:47, mythria wrote: > On 2015/11/27 09:28:46, Michael Starzinger wrote: > > On 2015/11/26 17:27:45, Michael Starzinger wrote: > > > This should be Runtime::kThrow, not Runtime::kReThrow, otherwise the > exception > > > message will not be created and the throw location won't be setup. > > > > I realized later that creating the exception message will require > deoptimization > > information, which we don't yet have for these graphs. So I would be fine with > > keeping kReThrow here for now (until we have deopt info) but at least leave a > > big TODO here. > > Thanks for pointing this. We should use Runtime::kReThrow because interpreter > does not support deopt yet right? I just wanted to be explicit in the TODO > comment. Acknowledged. Yep, correct, TODO sounds good to me. https://codereview.chromium.org/1481763002/diff/20001/test/cctest/compiler/te... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1481763002/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:973: {"throw undefined;", {"undefined"}}, On 2015/11/27 14:54:47, mythria wrote: > I tried to add tests that throw an Error object. i.e. throw new Error('Error'); > Theses tests crash in OptimizeFrame::Summarize. I thought it is related to the > unavailability of deopt information. Please correct me if I am wrong. I will try > to investigate why they failed. Acknowledged. Yes, sounds about right. The constructor of an "Error" object will try to collect a stack trace (i.e. see "function DefineError" in messages.js file) and eventually lands in Runtime_CollectStackTrace and OptimizeFrame::Summarize, causing the crash. Throwing any other object (e.g. "throw new Object()") should work though, because only the "Error" constructors capture stack traces. With the TODO you added above, the tests look good to me. https://codereview.chromium.org/1481763002/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:988: std::string str("Uncaught "); nit: I would just make the "Uncaught " prefix part of the ExpectedSnippet return value above, makes it easier to read and I think the duplication is OK.
Thanks for your review and comments. Fixed the nits. Thanks, Mythri. https://codereview.chromium.org/1481763002/diff/20001/test/cctest/compiler/te... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1481763002/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:988: std::string str("Uncaught "); On 2015/11/27 15:09:19, Michael Starzinger wrote: > nit: I would just make the "Uncaught " prefix part of the ExpectedSnippet return > value above, makes it easier to read and I think the duplication is OK. Done.
lgtm https://codereview.chromium.org/1481763002/diff/40001/src/compiler/bytecode-g... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1481763002/diff/40001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.cc:820: // TODO(mythria): Change it Runtime::kThrow when we have deoptimization nit s/it/to/
Thanks for the review. I fixed it. https://codereview.chromium.org/1481763002/diff/40001/src/compiler/bytecode-g... File src/compiler/bytecode-graph-builder.cc (right): https://codereview.chromium.org/1481763002/diff/40001/src/compiler/bytecode-g... src/compiler/bytecode-graph-builder.cc:820: // TODO(mythria): Change it Runtime::kThrow when we have deoptimization On 2015/11/30 10:01:11, oth wrote: > nit s/it/to/ Done.
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org, oth@chromium.org Link to the patchset: https://codereview.chromium.org/1481763002/#ps60001 (title: "Fixed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1481763002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1481763002/60001
Message was sent while issue was closed.
Description was changed from ========== [Interpreter] Adds support for throw to bytecode graph builder. Adds support and tests for throw to bytecode graph builder. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Adds support for throw to bytecode graph builder. Adds support and tests for throw to bytecode graph builder. 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] Adds support for throw to bytecode graph builder. Adds support and tests for throw to bytecode graph builder. BUG=v8:4280 LOG=N ========== to ========== [Interpreter] Adds support for throw to bytecode graph builder. Adds support and tests for throw to bytecode graph builder. BUG=v8:4280 LOG=N Committed: https://crrev.com/c11e7bc2195e51613efbbb6de6ca56947192c0b9 Cr-Commit-Position: refs/heads/master@{#32399} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c11e7bc2195e51613efbbb6de6ca56947192c0b9 Cr-Commit-Position: refs/heads/master@{#32399} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
