Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2672313003:
[async await] fix async await desugaring
Description[async await] Fix async function desugaring
Previously we rewrote the return statement in an async function from
`return expr;` to `return %ResolvePromise(.promise, expr),
.promise`. This can lead to incorrect behavior in the presence of try-finally.
This patch stores the `expr` of the return statement in a temporary
variable, resolves and returns the promise at the end of the finally
block.
BUG=v8:5896
Patch Set 1 #Patch Set 2 : rename var #Patch Set 3 : fixes #Patch Set 4 : refactor #
Total comments: 33
Patch Set 5 : fixes #Patch Set 6 : increase capacity #Patch Set 7 : separate the functions #Patch Set 8 : fmt #
Messages
Total messages: 31 (24 generated)
Description was changed from ========== [wip] fixes to async await desugaring BUG= ========== to ========== fix async await desugaring BUG= ==========
The CQ bit was checked by gsathya@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...
Description was changed from ========== fix async await desugaring BUG= ========== to ========== [async await] Fix async function desugaring Previously we rewrote the return statement in an async function from `return expr;` to `return %ResolvePromise(.promise, expr), .promise`. This breaks when there is a return statement in a finally block because we don't return the correct value. This patch stores the `expr` of the return statement in a temporary variable, resolves and returns the promise at the end of the finally block. BUG=v8:5896 ==========
Description was changed from ========== [async await] Fix async function desugaring Previously we rewrote the return statement in an async function from `return expr;` to `return %ResolvePromise(.promise, expr), .promise`. This breaks when there is a return statement in a finally block because we don't return the correct value. This patch stores the `expr` of the return statement in a temporary variable, resolves and returns the promise at the end of the finally block. BUG=v8:5896 ========== to ========== [async await] Fix async function desugaring Previously we rewrote the return statement in an async function from `return expr;` to `return %ResolvePromise(.promise, expr), .promise`. This breaks when there is a return statement in a finally block because we don't return the correct value. This patch stores the `expr` of the return statement in a temporary variable, resolves and returns the promise at the end of the finally block. BUG=v8:5896 ==========
gsathya@chromium.org changed reviewers: + adamk@chromium.org, caitp@igalia.com, littledan@chromium.org, neis@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This approach looks to me like it should work; would like both caitp and neis to have a look. A few nits below. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3157: int pos = kNoSourcePosition; No need for this variable, just pass kNoSourcePosition to NewCallRuntime https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3170: int pos = kNoSourcePosition; Same here as above.
LGTM. I think i can live with this approach, if it's the way we want to go for now. Optional nit which could help reduce the size of the bytecode inline https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3010: // : %ResolvePromise(.promise, .async_return_value); Given that the parameter-init-block doesn't need this, do you think it's worth adding the try-finally outside of this function, and parameterizing whether the catch block needs to write the field or not? This would avoid an unneeded try/finally which emits a fair bit of byte code. Maybe we don't care about that, but i believe we're assuming to minimize the size of interpreted code blocks.
https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser-base... src/parsing/parser-base.h:450: DCHECK(variable != NULL); DCHECK_NOT_NULL https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:1622: // return .async_return_value = expr, undefined; Why bother returning undefined? You can just do return .async_return_value = expr or am I missing something? https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:1625: return_value, kNoSourcePosition); This must be ASSIGN, not INIT. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3054: kNoSourcePosition); This must be ASSIGN, not INIT. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3059: This must be ASSIGN, not INIT. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3080: // ? %RejectPromise(.promise, .catch) .catch should be .async_return_value https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3119: // } Please also mention this in the main comment at the top of the function. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3124: kNoSourcePosition); No need for a comparison here. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3194: // paths which may be the first to access the Promise temporary. Whichever s/Promise/something else/ https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3891: return_value, kNoSourcePosition); This must be ASSIGN, not INIT. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.h#ne... src/parsing/parser.h:545: Block* BuildRejectPromiseOnException(Block* block); I find it a bit confusing to have both foo(T) and foo(T, bool). Maybe only keep BuildRejectPromiseOnException(Block* block, bool do_return_promise) and remove the other two. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.h#ne... src/parsing/parser.h:547: Block* BuildRejectPromiseOnException(Block* block, bool do_return_promise); Please keep the argument name in sync with the one in the function definition (is_parameter_block). https://codereview.chromium.org/2672313003/diff/60001/test/mjsunit/regress/re... File test/mjsunit/regress/regress-5896.js (right): https://codereview.chromium.org/2672313003/diff/60001/test/mjsunit/regress/re... test/mjsunit/regress/regress-5896.js:17: .catch((e) => { %AbortJS(''+e); }); How about also adding Caitlin's tests from https://codereview.chromium.org/2667983004/
From the description: "This breaks when there is a return statement in a finally block because we don't return the correct value." This isn't quite correct, please clarify. If you don't want to go into details, maybe just say "This can lead to incorrect behavior in the presence of try-finally."
Description was changed from ========== [async await] Fix async function desugaring Previously we rewrote the return statement in an async function from `return expr;` to `return %ResolvePromise(.promise, expr), .promise`. This breaks when there is a return statement in a finally block because we don't return the correct value. This patch stores the `expr` of the return statement in a temporary variable, resolves and returns the promise at the end of the finally block. BUG=v8:5896 ========== to ========== [async await] Fix async function desugaring Previously we rewrote the return statement in an async function from `return expr;` to `return %ResolvePromise(.promise, expr), .promise`. This can lead to incorrect behavior in the presence of try-finally. This patch stores the `expr` of the return statement in a temporary variable, resolves and returns the promise at the end of the finally block. BUG=v8:5896 ==========
The CQ bit was checked by gsathya@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 checked by gsathya@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 checked by gsathya@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 gsathya@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 checked by gsathya@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...
https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser-base... src/parsing/parser-base.h:450: DCHECK(variable != NULL); On 2017/02/08 10:28:41, neis wrote: > DCHECK_NOT_NULL Done. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:1622: // return .async_return_value = expr, undefined; On 2017/02/08 10:28:41, neis wrote: > Why bother returning undefined? You can just do > return .async_return_value = expr > or am I missing something? Yeah it doesn't matter; the returned value is thrown away anyway. I thought returning undefined made it clear that the return value is unused. Changed. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:1625: return_value, kNoSourcePosition); On 2017/02/08 10:28:41, neis wrote: > This must be ASSIGN, not INIT. Done. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3010: // : %ResolvePromise(.promise, .async_return_value); On 2017/02/07 23:47:17, caitp wrote: > Given that the parameter-init-block doesn't need this, do you think it's worth > adding the try-finally outside of this function, and parameterizing whether the > catch block needs to write the field or not? This would avoid an unneeded > try/finally which emits a fair bit of byte code. Maybe we don't care about that, > but i believe we're assuming to minimize the size of interpreted code blocks. I agree, it is wasteful. I've split this into two separate function instead of parameterizing since there really isn't any code sharing and it's simpler now. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3054: kNoSourcePosition); On 2017/02/08 10:28:41, neis wrote: > This must be ASSIGN, not INIT. Done. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3059: On 2017/02/08 10:28:41, neis wrote: > This must be ASSIGN, not INIT. Done. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3080: // ? %RejectPromise(.promise, .catch) On 2017/02/08 10:28:41, neis wrote: > .catch should be .async_return_value Done. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3119: // } On 2017/02/08 10:28:41, neis wrote: > Please also mention this in the main comment at the top of the function. Done. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3124: kNoSourcePosition); On 2017/02/08 10:28:41, neis wrote: > No need for a comparison here. Why not? We need to return only in the case of rejection, is there a better way to do this? Note -- this isn't required any more with the change caitp@ mentioned. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3157: int pos = kNoSourcePosition; On 2017/02/07 21:42:01, adamk wrote: > No need for this variable, just pass kNoSourcePosition to NewCallRuntime Done. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3170: int pos = kNoSourcePosition; On 2017/02/07 21:42:01, adamk wrote: > Same here as above. Done. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3194: // paths which may be the first to access the Promise temporary. Whichever On 2017/02/08 10:28:41, neis wrote: > s/Promise/something else/ Done. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3891: return_value, kNoSourcePosition); On 2017/02/08 10:28:41, neis wrote: > This must be ASSIGN, not INIT. Done. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.h#ne... src/parsing/parser.h:545: Block* BuildRejectPromiseOnException(Block* block); On 2017/02/08 10:28:41, neis wrote: > I find it a bit confusing to have both foo(T) and foo(T, bool). > > Maybe only keep BuildRejectPromiseOnException(Block* block, bool > do_return_promise) and remove the other two. There are no longer two functions with the same name. https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.h#ne... src/parsing/parser.h:547: Block* BuildRejectPromiseOnException(Block* block, bool do_return_promise); On 2017/02/08 10:28:41, neis wrote: > Please keep the argument name in sync with the one in the function definition > (is_parameter_block). Done. https://codereview.chromium.org/2672313003/diff/60001/test/mjsunit/regress/re... File test/mjsunit/regress/regress-5896.js (right): https://codereview.chromium.org/2672313003/diff/60001/test/mjsunit/regress/re... test/mjsunit/regress/regress-5896.js:17: .catch((e) => { %AbortJS(''+e); }); On 2017/02/08 10:28:41, neis wrote: > How about also adding Caitlin's tests from > https://codereview.chromium.org/2667983004/ It looks like caitlin is working on upstreaming those to test262: https://github.com/tc39/test262/pull/846
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2672313003/diff/60001/src/parsing/parser.cc#n... src/parsing/parser.cc:3124: kNoSourcePosition); On 2017/02/09 00:55:44, gsathya wrote: > On 2017/02/08 10:28:41, neis wrote: > > No need for a comparison here. > > Why not? We need to return only in the case of rejection, is there a better way > to do this? > > Note -- this isn't required any more with the change caitp@ mentioned. I meant you can just do "if (is_rejection_var) ..." instead of "if (is_rejection_var == true) ..."
gsathya@chromium.org changed reviewers: - adamk@chromium.org, caitp@igalia.com, littledan@chromium.org, neis@chromium.org |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
