Skip to content

deps: fix async await desugaring in V8#12004

Merged
targos merged 1 commit intonodejs:v7.x-stagingfrom
targos:v7.x-fix-11960-2
Mar 27, 2017
Merged

deps: fix async await desugaring in V8#12004
targos merged 1 commit intonodejs:v7.x-stagingfrom
targos:v7.x-fix-11960-2

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

bnoordhuis commented Mar 23, 2017

@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
Copy link
Member

@bnoordhuis bnoordhuis Mar 23, 2017

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

@targos targos Mar 24, 2017

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 Outdated
Copy link
Member

@bnoordhuis bnoordhuis Mar 23, 2017

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

@targos targos Mar 24, 2017

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

@targos targos Mar 24, 2017

Choose a reason for hiding this comment

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

@gsathya ^

Copy link
Member

@bnoordhuis bnoordhuis Mar 24, 2017

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

@gsathya gsathya Mar 25, 2017

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 <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targos targos force-pushed the v7.x-fix-11960-2 branch from 9ccb396 to d22346d Compare March 27, 2017 08:19
@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 <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

+1 more reviewer

@gsathya gsathya gsathya left review comments

Reviewers whose approvals may not affect merge requirements

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