Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2240743003:
[v8][wasm] Handles finally in try/finally blocks. (Closed)
Description[v8][wasm] Handles finally in try/finally blocks.
Introduces support for using
try {
} finally {
}
in wasm.
BUG=
Committed: https://crrev.com/cf5180c3ef1805e29dd29c02f8604212d6de911b
Cr-Commit-Position: refs/heads/master@{#38845}
Patch Set 1 #Patch Set 2 : First working version #Patch Set 3 : pre-review pass #Patch Set 4 : Disables WASM TryFinally tests in interpreted execution. #Patch Set 5 : git pull #Patch Set 6 : Fixes broken tests; git pull #Patch Set 7 : Fixes broken windows build #
Total comments: 5
Patch Set 8 : Addresses comments #Patch Set 9 : Fixes broken build #Patch Set 10 : Handles breaks within finallies #
Total comments: 28
Patch Set 11 : addresses comments #Patch Set 12 : Addresses comments. #
Messages
Total messages: 60 (49 generated)
The CQ bit was checked by jpp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jpp@chromium.org changed reviewers: + bradnelson@chromium.org, titzer@chromium.org
This seems to work (with a 9-input phi for the TryFinally_multiple test. :)), but please spend some time with this. I am not particularly sure if the try blocks are yielding the "right" value (by the way, should try blocks yield values?), so please pay special attention to that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by jpp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/12661) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
The CQ bit was checked by jpp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #7 (id:160001) has been deleted
Patchset #7 (id:180001) has been deleted
Patchset #7 (id:200001) has been deleted
Looks like it's on the right track so far. https://codereview.chromium.org/2240743003/diff/220001/src/wasm/ast-decoder.cc File src/wasm/ast-decoder.cc (right): https://codereview.chromium.org/2240743003/diff/220001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:82: struct EH { TryScope? TryInfo? IIUC this can only occur for Control blocks that are some form of try. https://codereview.chromium.org/2240743003/diff/220001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:84: SsaEnv* finish_try_env; // the environment where a try with finally lives. just finally_env? https://codereview.chromium.org/2240743003/diff/220001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:1500: Control* StichFinallyChain(const BreakDepthOperand& operand, const Value& val, AddToFinallyChain? https://codereview.chromium.org/2240743003/diff/220001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:1867: const int32_t WasmFullDecoder::kFirstFinallyToken = 1000; Can you just make these file-local statics? https://codereview.chromium.org/2240743003/diff/220001/test/cctest/wasm/test-... File test/cctest/wasm/test-run-wasm.cc (right): https://codereview.chromium.org/2240743003/diff/220001/test/cctest/wasm/test-... test/cctest/wasm/test-run-wasm.cc:2780: // TODO(jpp): WASM_EXEC_TEST(TryCatch) Can you start a new file test-run-wasm-try-catch.cc? https://codereview.chromium.org/2240743003/diff/220001/test/cctest/wasm/test-... test/cctest/wasm/test-run-wasm.cc:2943: BUILD( Yikes, that's a big test. Generally big tests like this are a pain to maintain, so we try to be more systematic with smaller tests. I didn't dig into the details of this one, but there might be a way to split it up into smaller tests.
Patchset #7 (id:220001) has been deleted
Patchset #7 (id:240001) has been deleted
Patchset #7 (id:260001) has been deleted
Patchset #7 (id:280001) has been deleted
Patchset #7 (id:300001) has been deleted
The CQ bit was checked by jpp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 jpp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I somehow managed to delete the patch with ben's comments... :-/ https://codereview.chromium.org/2240743003/diff/330003/src/wasm/ast-decoder.cc File src/wasm/ast-decoder.cc (right): https://codereview.chromium.org/2240743003/diff/330003/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:81: // we group everything into a separate struct. s/EH/TryInfo/g https://codereview.chromium.org/2240743003/diff/330003/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:84: SsaEnv* finish_try_env; // the environment where a try with finally lives. >> https://codereview.chromium.org/2240743003/diff/220001/src/wasm/ast-decoder.c... >> src/wasm/ast-decoder.cc:84: SsaEnv* finish_try_env; // the environment where a >> try with finally lives. > just finally_env? Well, this is not the environment for the finally, it is the environment just out of the finally: if (something) { try { maybeThrow(); } finally { cleanup(); } // <-- This is where finish_try_env lives. } https://codereview.chromium.org/2240743003/diff/330003/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:1496: Control* StichFinallyChain(const BreakDepthOperand& operand, const Value& val, >> https://codereview.chromium.org/2240743003/diff/220001/src/wasm/ast-decoder.c... >> src/wasm/ast-decoder.cc:1500: Control* StichFinallyChain(const >> BreakDepthOperand& operand, const Value& val, > AddToFinallyChain? This method builds the finally chain. s/StichFinallyChain/BuildFinallyChain/g https://codereview.chromium.org/2240743003/diff/330003/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:1863: const int32_t WasmFullDecoder::kFirstFinallyToken = 1000; >> https://codereview.chromium.org/2240743003/diff/220001/src/wasm/ast-decoder.c... >> src/wasm/ast-decoder.cc:1867: const int32_t >> WasmFullDecoder::kFirstFinallyToken >> = 1000; > Can you just make these file-local statics? I can make them local, but having them "inside" the WasmFullDecoder (where they are private) helps limit the scope where these are visible. https://codereview.chromium.org/2240743003/diff/330003/test/cctest/wasm/test-... File test/cctest/wasm/test-run-wasm.cc (right): https://codereview.chromium.org/2240743003/diff/330003/test/cctest/wasm/test-... test/cctest/wasm/test-run-wasm.cc:2874: if (execution_mode == kExecuteInterpreted) { Yeah, this test is complicated, but I don't see any other way of exercising multiple predecessors, multiple successors for finally if not for a bunch of nested tries, and a bunch of breaks out of those tries.
Patchset #7 (id:320001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/11336) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by jpp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 jpp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Think I have a better picture of this in my head now between your description on the whiteboard and staring at the code. lgtm, but probably should have titzer sign-off too https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.cc File src/wasm/ast-decoder.cc (right): https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:73: // PathToken is used by exception handling code for managing finally's. Don't really care for the PathToken name. This is really a list of incoming branches to the finally, how about BranchesToThisFinally .. bah don't like that either... https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:104: int32_t prev_finally; // previous control (on stack) that has a finally. The bookkeeping on the linked list of previous controls with a finally is rather tricky. Any reason this can't be a pointer to the control block, instead of index? https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:461: // The full WASM decoder for bytecode. Both verifies bytecode and generates This comment got separated from the class. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:464: static const int32_t kFallthroughToken = 100; These choices of values are a little confusing, but not sure what to suggest. I assume you picked a big gap just to make things obvious when debugging? https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:953: // finally -> dispatch on the path token Make clear your counting inbound paths other than the current block? https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:964: TFNode* sw = BUILD(Switch, static_cast<uint32_t>(targets + 1), Worth pulling the resolve of the finally into a separate method? https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:988: MergeInto(t->end_env, &t->node, &t->type, val); Just use c instead of copy to t ? https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:1501: static_cast<uint32_t>(control_.size() - operand.depth - 1); DCHECK the operand.depth makes sense? https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:1512: if (first_control == nullptr) { It seems like this would read better if the empty case got handled above the for loop? Then you could move the first_control == nullptr stuff up, and unconditionally allocate the new token up front? https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:1618: if (builder_ == nullptr) { Why?
https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.cc File src/wasm/ast-decoder.cc (right): https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:73: // PathToken is used by exception handling code for managing finally's. On 2016/08/23 05:49:42, bradnelson wrote: > Don't really care for the PathToken name. > This is really a list of incoming branches to the finally, how about > BranchesToThisFinally .. bah don't like that either... The three hardest problems in CS: Cache Coherency, Multi-thread safety, Naming, and off-by-one errors. <badjoke /> I dislike PathToken, too, but I dislike BranchToThisFinally a bit more. Leaving as is, but glad to change. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:104: int32_t prev_finally; // previous control (on stack) that has a finally. On 2016/08/23 05:49:42, bradnelson wrote: > The bookkeeping on the linked list of previous controls with a finally is rather > tricky. > Any reason this can't be a pointer to the control block, instead of index? I was initially thinking about that. Then the problem became (in BuildFinallyChain) figuring out whether a branch target crossed a finally or not. With the pointers, I figured, I'd have to search the entire stack to find that out. Then I thought about keeping these indexes. IMO, comparing pointers for anything other than (in)equality is a strong indication of code smell (of course, there are legitimate uses of < for pointers) Now, after your comment, I gave this a second thought, and realized "hey, these pointers wouldn't be *random* pointers: they are array elements! so comparing them with < should be fine!" But then it hit me: "shoot! these are array elements! more importantly, these are elements in an std::vector!" The problem here would be keeping a pointer to an std::vector element. If the vector is ever resized (and it may), we're in for a really bad time. I'd argue that keeping the indices is more correct than keeping the pointers. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:461: // The full WASM decoder for bytecode. Both verifies bytecode and generates On 2016/08/23 05:49:43, bradnelson wrote: > This comment got separated from the class. Done. It looks weird, but this is kinda by design: the constants are really only used in that class. I thought that maybe keeping them between the comment and the class would help convey that message, but I don't really love the solution. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:464: static const int32_t kFallthroughToken = 100; On 2016/08/23 05:49:42, bradnelson wrote: > These choices of values are a little confusing, but not sure what to suggest. > I assume you picked a big gap just to make things obvious when debugging? Exactly. I was going to pick 1024 (a nice round number) and 121 (a nice palindrome, and also the 121 == 11**2) because their values don't really matter. I can change them if you want, but I am leaving them as is for now. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:953: // finally -> dispatch on the path token On 2016/08/23 05:49:43, bradnelson wrote: > Make clear your counting inbound paths other than the current block? Done. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:964: TFNode* sw = BUILD(Switch, static_cast<uint32_t>(targets + 1), On 2016/08/23 05:49:42, bradnelson wrote: > Worth pulling the resolve of the finally into a separate method? Done. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:988: MergeInto(t->end_env, &t->node, &t->type, val); On 2016/08/23 05:49:42, bradnelson wrote: > Just use c instead of copy to t ? Done. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:1501: static_cast<uint32_t>(control_.size() - operand.depth - 1); On 2016/08/23 05:49:43, bradnelson wrote: > DCHECK the operand.depth makes sense? Well, the code already handles that, but I am a firm believer in asserts (plus, you should never blindly trust your inputs anyway.) Done. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:1512: if (first_control == nullptr) { On 2016/08/23 05:49:42, bradnelson wrote: > It seems like this would read better if the empty case got handled above the for > loop? > Then you could move the first_control == nullptr stuff up, and unconditionally > allocate the new token up front? Nice! done. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:1618: if (builder_ == nullptr) { On 2016/08/23 05:49:42, bradnelson wrote: > Why? I honestly do not know why the builder would be nullptr, but the code seems to handle this, so I assume it is possible. So, assuming builder_ may be nullptr, then the switch statement below would need extra logic in the kMerged case (I would no longer be able to DCHECK_NOT_NULL(to->eh->token) because the token would never have been created in the kReached case.)
The CQ bit was checked by jpp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.cc File src/wasm/ast-decoder.cc (right): https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:73: // PathToken is used by exception handling code for managing finally's. On 2016/08/23 11:38:20, John wrote: > On 2016/08/23 05:49:42, bradnelson wrote: > > Don't really care for the PathToken name. > > This is really a list of incoming branches to the finally, how about > > BranchesToThisFinally .. bah don't like that either... > > The three hardest problems in CS: Cache Coherency, Multi-thread safety, Naming, > and off-by-one errors. <badjoke /> > > I dislike PathToken, too, but I dislike BranchToThisFinally a bit more. Leaving > as is, but glad to change. IncomingBranches? And would that mean that it is Control* from? https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:82: struct TryInfo { If this were a ZoneObject, you could simply new(zone) it. It's not great that we override new, but it's better to be consistent if we ever get a mind to fix all of them later. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:104: int32_t prev_finally; // previous control (on stack) that has a finally. On 2016/08/23 11:38:20, John wrote: > On 2016/08/23 05:49:42, bradnelson wrote: > > The bookkeeping on the linked list of previous controls with a finally is > rather > > tricky. > > Any reason this can't be a pointer to the control block, instead of index? > > I was initially thinking about that. Then the problem became (in > BuildFinallyChain) figuring out whether a branch target crossed a finally or > not. With the pointers, I figured, I'd have to search the entire stack to find > that out. Then I thought about keeping these indexes. IMO, comparing pointers > for anything other than (in)equality is a strong indication of code smell (of > course, there are legitimate uses of < for pointers) > > Now, after your comment, I gave this a second thought, and realized "hey, these > pointers wouldn't be *random* pointers: they are array elements! so comparing > them with < should be fine!" But then it hit me: "shoot! these are array > elements! more importantly, these are elements in an std::vector!" > > The problem here would be keeping a pointer to an std::vector element. If the > vector is ever resized (and it may), we're in for a really bad time. > > I'd argue that keeping the indices is more correct than keeping the pointers. I agree on the index. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:464: static const int32_t kFallthroughToken = 100; On 2016/08/23 11:38:20, John wrote: > On 2016/08/23 05:49:42, bradnelson wrote: > > These choices of values are a little confusing, but not sure what to suggest. > > I assume you picked a big gap just to make things obvious when debugging? > > Exactly. I was going to pick 1024 (a nice round number) and 121 (a nice > palindrome, and also the 121 == 11**2) because their values don't really matter. > I can change them if you want, but I am leaving them as is for now. Are there any problems with collisions with these values, e.g. if we have more than 1024 nested finally's? IIUC then we could start fallthrough with 0 and first with 1; small numbers are good since they will likely end up being immediates in moves in the code. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:1618: if (builder_ == nullptr) { On 2016/08/23 11:38:20, John wrote: > On 2016/08/23 05:49:42, bradnelson wrote: > > Why? > > I honestly do not know why the builder would be nullptr, but the code seems to > handle this, so I assume it is possible. > > So, assuming builder_ may be nullptr, then the switch statement below would need > extra logic in the kMerged case (I would no longer be able to > DCHECK_NOT_NULL(to->eh->token) because the token would never have been created > in the kReached case.) It can become nullptr if you are in unreachable code or if there is an error.
https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.cc File src/wasm/ast-decoder.cc (right): https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:73: // PathToken is used by exception handling code for managing finally's. On 2016/08/23 13:56:16, titzer wrote: > On 2016/08/23 11:38:20, John wrote: > > On 2016/08/23 05:49:42, bradnelson wrote: > > > Don't really care for the PathToken name. > > > This is really a list of incoming branches to the finally, how about > > > BranchesToThisFinally .. bah don't like that either... > > > > The three hardest problems in CS: Cache Coherency, Multi-thread safety, > Naming, > > and off-by-one errors. <badjoke /> > > > > I dislike PathToken, too, but I dislike BranchToThisFinally a bit more. > Leaving > > as is, but glad to change. > > IncomingBranches? And would that mean that it is Control* from? IncomingBranch is great! Thanks. However, it is still Control* target: each of these structs "saves" an incoming branch's information, including the target that should be branched to once the finally's run. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:82: struct TryInfo { On 2016/08/23 13:56:16, titzer wrote: > If this were a ZoneObject, you could simply new(zone) it. > > It's not great that we override new, but it's better to be consistent if we ever > get a mind to fix all of them later. Well, it is not great, but consistency trumps great... and overriding new for arena allocation ain't **that** bad. :) Done. https://codereview.chromium.org/2240743003/diff/380001/src/wasm/ast-decoder.c... src/wasm/ast-decoder.cc:464: static const int32_t kFallthroughToken = 100; On 2016/08/23 13:56:16, titzer wrote: > On 2016/08/23 11:38:20, John wrote: > > On 2016/08/23 05:49:42, bradnelson wrote: > > > These choices of values are a little confusing, but not sure what to > suggest. > > > I assume you picked a big gap just to make things obvious when debugging? > > > > Exactly. I was going to pick 1024 (a nice round number) and 121 (a nice > > palindrome, and also the 121 == 11**2) because their values don't really > matter. > > I can change them if you want, but I am leaving them as is for now. > > Are there any problems with collisions with these values, e.g. if we have more > than 1024 nested finally's? > > IIUC then we could start fallthrough with 0 and first with 1; small numbers are > good since they will likely end up being immediates in moves in the code. Done.
The CQ bit was checked by jpp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by jpp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bradnelson@chromium.org Link to the patchset: https://codereview.chromium.org/2240743003/#ps420001 (title: "Addresses comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #12 (id:420001)
Message was sent while issue was closed.
Description was changed from
==========
[v8][wasm] Handles finally in try/finally blocks.
Introduces support for using
try {
} finally {
}
in wasm.
BUG=
==========
to
==========
[v8][wasm] Handles finally in try/finally blocks.
Introduces support for using
try {
} finally {
}
in wasm.
BUG=
Committed: https://crrev.com/cf5180c3ef1805e29dd29c02f8604212d6de911b
Cr-Commit-Position: refs/heads/master@{#38845}
==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/cf5180c3ef1805e29dd29c02f8604212d6de911b Cr-Commit-Position: refs/heads/master@{#38845} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
