Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

deps: fix async await desugaring in V8 #12004

Merged
merged 1 commit into from
Mar 27, 2017
Merged

deps: fix async await desugaring in V8 #12004

merged 1 commit into from
Mar 27, 2017

Conversation

targos
Copy link
Member

@targos targos commented Mar 23, 2017

This is a backport of https://codereview.chromium.org/2672313003/. The
patch did not land in V8 because it was superseded by another one but it
is much easier to backport to V8 5.5, was reviewed and passed tests.

Original commit message:

[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

Fixes: #11960

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows [commit guidelines][]
Affected core subsystem(s)

V8

@nodejs-github-bot nodejs-github-bot added v7.x v8 engine Issues and PRs related to the V8 dependency. labels Mar 23, 2017
@targos targos mentioned this pull request Mar 23, 2017
Closed
@targos
Copy link
Member Author

targos commented Mar 23, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/6997/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/620/

/cc @nodejs/v8

@targos
Copy link
Member Author

targos commented Mar 23, 2017

There seems to be a problem with Clang: https://ci.nodejs.org/job/node-test-commit-linux/8639/nodes=ubuntu1204-clang341-64/console

@bnoordhuis
Copy link
Member

@targos It's not just you, #11652 (comment).

@jasnell
Copy link
Member

jasnell commented Mar 23, 2017

@nodejs/build ^^^

bnoordhuis
bnoordhuis approved these changes Mar 23, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit and a question.

deps/v8/src/parsing/parser.cc Outdated
Scope* catch_scope = NewScope(CATCH_SCOPE);
catch_scope->set_is_hidden();
Variable* catch_variable =
catch_scope->DeclareLocal(ast_value_factory()->dot_catch_string(), VAR, kCreatedInitialized, NORMAL_VARIABLE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

deps/v8/src/parsing/parser.cc
// the FunctionState.
Variable* async_return = function_state_->async_return_variable();
if (async_return == nullptr) {
async_return = scope()->NewTemporary(ast_value_factory()->empty_string());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize the original CL does that too but do you happen to know why it uses empty_string() instead of undefined_value() or the_hole_value()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, there doesn't seem to be a good reason for that. Do you want me to change it for undefined_value()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsathya ^

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logic is that the initial value won't matter because it's overwritten in all code paths? I'd have used the_hole_value() because its presence traps in many cases and it's something you can unambiguously debug-check since it's never produced by JS code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty_string isn't the value, it's the name of the variable. we never actually lookup the variable by name, so it doesn't need a name.

@targos
deps: fix async await desugaring in V8
d22346d 
This is a backport of https://codereview.chromium.org/2672313003/. The
patch did not land in V8 because it was superseded by another one but it
is much easier to backport to V8 5.5, was reviewed and passed tests.

Original commit message:

    [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

PR-URL: nodejs#12004
Fixes: nodejs#11960
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@targos targos merged commit d22346d into nodejs:v7.x-staging Mar 27, 2017
@targos targos deleted the v7.x-fix-11960-2 branch March 27, 2017 08:20
@MylesBorins MylesBorins mentioned this pull request Mar 28, 2017
Merged
MylesBorins added a commit that referenced this pull request Mar 28, 2017
@MylesBorins
2017-03-28, Version 7.8.0 (Current)
f04524e 
Notable changes:

* buffer:
  - do not segfault on out-of-range index (Timothy Gu)
    #11927
* crypto:
  - Fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  * upgrade npm to 4.2.0 (Kat Marchán)
    #11389
  * fix async await desugaring in V8 (Michaël Zasso)
    #12004
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    #2982
* src:
  - add native URL class (James M Snell)
    #11801

PR-URL: #12104
MylesBorins added a commit that referenced this pull request Mar 29, 2017
@MylesBorins
2017-03-28, Version 7.8.0 (Current)
9b2dd47 
Notable changes:

* buffer:
  - do not segfault on out-of-range index (Timothy Gu)
    #11927
* crypto:
  - Fix memory leak if certificate is revoked (Tom Atkinson)
    #12089
* deps:
  * upgrade npm to 4.2.0 (Kat Marchán)
    #11389
  * fix async await desugaring in V8 (Michaël Zasso)
    #12004
* readline:
  - add option to stop duplicates in history (Danny Nemer)
    #2982
* src:
  - add native URL class (James M Snell)
    #11801

PR-URL: #12104
@italoacasas italoacasas mentioned this pull request Apr 10, 2017
Merged
2 tasks
imyller added a commit to imyller/meta-nodejs that referenced this pull request Apr 20, 2017
@imyller
2017-03-28, Version 7.8.0 (Current)
e0661dd 
    Notable changes:

    * buffer:
      - do not segfault on out-of-range index (Timothy Gu)
        nodejs/node#11927
    * crypto:
      - Fix memory leak if certificate is revoked (Tom Atkinson)
        nodejs/node#12089
    * deps:
      * upgrade npm to 4.2.0 (Kat Marchán)
        nodejs/node#11389
      * fix async await desugaring in V8 (Michaël Zasso)
        nodejs/node#12004
    * readline:
      - add option to stop duplicates in history (Danny Nemer)
        nodejs/node#2982
    * src:
      - add native URL class (James M Snell)
        nodejs/node#11801

    PR-URL: nodejs/node#12104

Signed-off-by: Ilkka Myller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsathya gsathya gsathya left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@targos @bnoordhuis @jasnell @gsathya @MylesBorins @nodejs-github-bot