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.

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

[v10.x backport] deps: v8, backport coverage fixes #26579

Closed
wants to merge 4 commits into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Mar 11, 2019

Pulls in patches from V8 (v8/v8@52e2b5, v8/v8@512175, v8/v8@aac2f8, v8/v8@9365d09, v8/v8@2d08967)
which address a variety of coverage bugs

This brings coverage in Node v10 to parity with coverage in v11, making coverage more universally consistent for users.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v10.x v8 engine Issues and PRs related to the V8 dependency. labels Mar 11, 2019
@bcoe
Copy link
Contributor Author

bcoe commented Mar 11, 2019

Pulls in patches from V8 (52e2b5, 512175, aac2f8, 9365d09, 2d08967)
which address a variety of coverage bugs, bringing Node v10 coverage
to parity with Node v11.

  Original commit message:

    [coverage] Extend SourceRangeAstVisitor for throw statements

    The SourceRangeAstVisitor has custom logic for blocks ending with a
    statement that has a continuation range. In these cases, the trailing
    continuation is removed which makes the reported coverage ranges a bit
    nicer.

    throw Error('foo') consists of an ExpressionStatement, with a
    Throw expression stored within the statement. The source range itself
    is stored with the Throw, not the statement.

    We now properly extract the correct AST node for trailing throw
    statements.

  [email protected], [email protected], [email protected]

  Bug: v8:8691
  Change-Id: Ibcbab79fbe54719a8993045040349c863b139011
  Reviewed-on: https://chromium-review.googlesource.com/c/1480632
  Commit-Queue: Georg Neis <[email protected]>
  Reviewed-by: Georg Neis <[email protected]>
  Reviewed-by: Jakob Gruber <[email protected]>
  Cr-Commit-Position: refs/heads/master@{#59936}

  Refs: v8/v8@2d08967

  Original commit message:

    [coverage] Rework continuation counter handling

    This changes a few bits about how continuation counters are handled.

    It introduces a new mechanism that allows removal of a continuation
    range after it has been created. If coverage is enabled, we run a first
    post-processing pass on the AST immediately after parsing, which
    removes problematic continuation ranges in two situations:

    1. nested continuation counters - only the outermost stays alive.
    2. trailing continuation counters within a block-like structure are
        removed if the containing structure itself has a continuation.

  [email protected], [email protected], [email protected]

  Bug: v8:8381, v8:8539
  Change-Id: I6bcaea5060d8c481d7bae099f6db9f993cc30ee3
  Reviewed-on: https://chromium-review.googlesource.com/c/1339119
  Reviewed-by: Yang Guo <[email protected]>
  Reviewed-by: Leszek Swirski <[email protected]>
  Reviewed-by: Georg Neis <[email protected]>
  Commit-Queue: Jakob Gruber <[email protected]>
  Cr-Commit-Position: refs/heads/master@{#58443}

  Refs: v8/v8@9365d09

  Original commit message:

    [coverage] Filter out singleton ranges that alias full ranges

    Block coverage is based on a system of ranges that can either have
    both a start and end position, or only a start position (so-called
    singleton ranges). When formatting coverage information, singletons
    are expanded until the end of the immediate full parent range. E.g.
    in:

    {0, 10}  // Full range.
    {5, -1}  // Singleton range.

    the singleton range is expanded to {5, 10}.

    Singletons are produced mostly for continuation counters that track
    whether we execute past a specific language construct.

    Unfortunately, continuation counters can turn up in spots that confuse
    our post-processing. For example:

    if (true) { ... block1 ... } else { ... block2 ... }

    If block1 produces a continuation counter, it could end up with the
    same start position as the else-branch counter. Since we merge
    identical blocks, the else-branch could incorrectly end up with an
    execution count of one.

    We need to avoid merging such cases. A full range should always take
    precedence over a singleton range; a singleton range should never
    expand to completely fill a full range. An additional post-processing
    pass ensures this.

    Bug: v8:8237
    Change-Id: Idb3ec7b2feddc0585313810b9c8be1e9f4ec64bf
    Reviewed-on: https://chromium-review.googlesource.com/c/1273095
    Reviewed-by: Georg Neis <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Jakob Gruber <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#56531}

  Refs: v8/v8@aac2f8c

  Original commit message:

    [ast] Introduce ZonePtrList<T> typedef for ZoneList<T*>.

    This is a preliminary step before changing the way we store zone pointers
    in the zones.

    Bug: v8:7903, v8:7754
    Change-Id: I1e1af1823766c888ee0f8fe190f205f5b7e21973
    Reviewed-on: https://chromium-review.googlesource.com/1118887
    Reviewed-by: Ross McIlroy <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Commit-Queue: Igor Sheludko <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#54193}

  Refs: v8/v8@512175a

  Original commit message:

    [explicit isolates] Replace every Handle(T*) in parsing/

    Replace all but one Handle<T*>(T*) calls with ones that explicitly pass
    in an Isolate.

    Requires plumbing Isolate* through several Parser functions which
    previously avoided it because of worries about accessing the heap off
    the main thread. In all off-main-thread cases, isolate will be nullptr
    and every such function asserts with:
    DCHECK_EQ(parsing_on_main_thread_, isolate != nullptr);

    Also deletes unused function ParseInfo::ReopenHandlesInNewHandleScope.

    Bug: v8:7786
    Change-Id: I3dd9c49dcde49fdbcb684ba73f47a30d00fc495e
    Reviewed-on: https://chromium-review.googlesource.com/1087272
    Commit-Queue: Dan Elphick <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#53820}

  Refs: v8/v8@52e2b5a
@bcoe
Copy link
Contributor Author

bcoe commented Mar 12, 2019

@nodejs/v8 @nodejs/testing 👋 would love some eyes on this, it potentially gets test coverage working in Node 10.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM. Up to @nodejs/releasers if this ends up in a v10.x release or not but hopefully not controversial....

@bcoe bcoe changed the title [v10.x backport] deps: v8, cherry-pick coverage fixes [v10.x backport] deps: v8, backport coverage fixes Mar 12, 2019
@bcoe bcoe added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 14, 2019
@bcoe
Copy link
Contributor Author

bcoe commented Mar 14, 2019

@BridgeAR can you take a look at this (are you part of the release team?)

@BridgeAR
Copy link
Member

@bcoe I am (but not LTS). Backports are normally merged right before a release. We might want to change that for patch commits though. I'll bring it up in the release group's meeting.

@richardlau
Copy link
Member

Should each backported commit be in its own commit?

@bcoe
Copy link
Contributor Author

bcoe commented Mar 14, 2019

@richardlau it takes some manual work to get the patches to land cleanly, and all of the commits step on each other's toes. When landing in master we brought the patches in as a set, and I think it was the right approach.

@nodejs-github-bot
Copy link
Collaborator

@BethGriggs
Copy link
Member

Landed on v10.x-staging

BethGriggs pushed a commit that referenced this pull request Apr 8, 2019
Pulls in patches from V8 (52e2b5, 512175, aac2f8, 9365d09, 2d08967)
which address a variety of coverage bugs, bringing Node v10 coverage
to parity with Node v11.

  Original commit message:

    [coverage] Extend SourceRangeAstVisitor for throw statements

    The SourceRangeAstVisitor has custom logic for blocks ending with a
    statement that has a continuation range. In these cases, the trailing
    continuation is removed which makes the reported coverage ranges a bit
    nicer.

    throw Error('foo') consists of an ExpressionStatement, with a
    Throw expression stored within the statement. The source range itself
    is stored with the Throw, not the statement.

    We now properly extract the correct AST node for trailing throw
    statements.

  [email protected], [email protected], [email protected]

  Bug: v8:8691
  Change-Id: Ibcbab79fbe54719a8993045040349c863b139011
  Reviewed-on: https://chromium-review.googlesource.com/c/1480632
  Commit-Queue: Georg Neis <[email protected]>
  Reviewed-by: Georg Neis <[email protected]>
  Reviewed-by: Jakob Gruber <[email protected]>
  Cr-Commit-Position: refs/heads/master@{#59936}

  Refs: v8/v8@2d08967

  Original commit message:

    [coverage] Rework continuation counter handling

    This changes a few bits about how continuation counters are handled.

    It introduces a new mechanism that allows removal of a continuation
    range after it has been created. If coverage is enabled, we run a first
    post-processing pass on the AST immediately after parsing, which
    removes problematic continuation ranges in two situations:

    1. nested continuation counters - only the outermost stays alive.
    2. trailing continuation counters within a block-like structure are
        removed if the containing structure itself has a continuation.

  [email protected], [email protected], [email protected]

  Bug: v8:8381, v8:8539
  Change-Id: I6bcaea5060d8c481d7bae099f6db9f993cc30ee3
  Reviewed-on: https://chromium-review.googlesource.com/c/1339119
  Reviewed-by: Yang Guo <[email protected]>
  Reviewed-by: Leszek Swirski <[email protected]>
  Reviewed-by: Georg Neis <[email protected]>
  Commit-Queue: Jakob Gruber <[email protected]>
  Cr-Commit-Position: refs/heads/master@{#58443}

  Refs: v8/v8@9365d09

  Original commit message:

    [coverage] Filter out singleton ranges that alias full ranges

    Block coverage is based on a system of ranges that can either have
    both a start and end position, or only a start position (so-called
    singleton ranges). When formatting coverage information, singletons
    are expanded until the end of the immediate full parent range. E.g.
    in:

    {0, 10}  // Full range.
    {5, -1}  // Singleton range.

    the singleton range is expanded to {5, 10}.

    Singletons are produced mostly for continuation counters that track
    whether we execute past a specific language construct.

    Unfortunately, continuation counters can turn up in spots that confuse
    our post-processing. For example:

    if (true) { ... block1 ... } else { ... block2 ... }

    If block1 produces a continuation counter, it could end up with the
    same start position as the else-branch counter. Since we merge
    identical blocks, the else-branch could incorrectly end up with an
    execution count of one.

    We need to avoid merging such cases. A full range should always take
    precedence over a singleton range; a singleton range should never
    expand to completely fill a full range. An additional post-processing
    pass ensures this.

    Bug: v8:8237
    Change-Id: Idb3ec7b2feddc0585313810b9c8be1e9f4ec64bf
    Reviewed-on: https://chromium-review.googlesource.com/c/1273095
    Reviewed-by: Georg Neis <[email protected]>
    Reviewed-by: Yang Guo <[email protected]>
    Commit-Queue: Jakob Gruber <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#56531}

  Refs: v8/v8@aac2f8c

  Original commit message:

    [ast] Introduce ZonePtrList<T> typedef for ZoneList<T*>.

    This is a preliminary step before changing the way we store zone pointers
    in the zones.

    Bug: v8:7903, v8:7754
    Change-Id: I1e1af1823766c888ee0f8fe190f205f5b7e21973
    Reviewed-on: https://chromium-review.googlesource.com/1118887
    Reviewed-by: Ross McIlroy <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Reviewed-by: Camillo Bruni <[email protected]>
    Commit-Queue: Igor Sheludko <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#54193}

  Refs: v8/v8@512175a

  Original commit message:

    [explicit isolates] Replace every Handle(T*) in parsing/

    Replace all but one Handle<T*>(T*) calls with ones that explicitly pass
    in an Isolate.

    Requires plumbing Isolate* through several Parser functions which
    previously avoided it because of worries about accessing the heap off
    the main thread. In all off-main-thread cases, isolate will be nullptr
    and every such function asserts with:
    DCHECK_EQ(parsing_on_main_thread_, isolate != nullptr);

    Also deletes unused function ParseInfo::ReopenHandlesInNewHandleScope.

    Bug: v8:7786
    Change-Id: I3dd9c49dcde49fdbcb684ba73f47a30d00fc495e
    Reviewed-on: https://chromium-review.googlesource.com/1087272
    Commit-Queue: Dan Elphick <[email protected]>
    Reviewed-by: Toon Verwaest <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#53820}

  Refs: v8/v8@52e2b5a

PR-URL: #26579
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
@BethGriggs BethGriggs closed this Apr 8, 2019
@bcoe
Copy link
Contributor Author

bcoe commented Apr 8, 2019

@BethGriggs 🎉 thank you for lading this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants