Skip to content

deps: v8, cherry-pick 9365d09, aac2f8c, 47d34a3#25429

Closed
bcoe wants to merge 1 commit intonodejs:masterfrom
bcoe:float-coverage-patch
Closed

deps: v8, cherry-pick 9365d09, aac2f8c, 47d34a3#25429
bcoe wants to merge 1 commit intonodejs:masterfrom
bcoe:float-coverage-patch

Conversation

@bcoe
Copy link
Contributor

@bcoe bcoe commented Jan 10, 2019 •
edited
Loading

merge patches related to test coverage:

v8/v8@aac2f8c
v8/v8@9365d09
v8/v8@47d34a3

this should correct some of the oddities seen by @mhdawson in #25157

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

@bcoe bcoe added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 10, 2019
@bcoe bcoe requested review from addaleax and mhdawson January 10, 2019 08:34
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 10, 2019

@bcoe build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2218/pipeline

@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jan 10, 2019
@bcoe bcoe removed the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 10, 2019
@bcoe
Copy link
Contributor Author

bcoe commented Jan 10, 2019

CC: @schuay

@bcoe bcoe requested a review from hashseed January 10, 2019 08:36
@mhdawson
Copy link
Member

mhdawson commented Jan 10, 2019

You also need to update 'v8_embedder_string': '-node.10', in common.gypi. Which we bump with each commit backported. I think but I'm not sure that it should actually be bumped twice in the PR once for each of the commits backported but @hashseed can confirm as he does this regularly.

@bcoe bcoe changed the title deps: V8: backport 9365d09, aac2f8c [WIP] deps: V8: backport 9365d09, aac2f8c Jan 10, 2019
@bcoe bcoe force-pushed the float-coverage-patch branch from 62941f8 to 56aaecc Compare January 12, 2019 01:30
@bcoe bcoe changed the title [WIP] deps: V8: backport 9365d09, aac2f8c deps: V8: backport 9365d09, aac2f8c Jan 12, 2019
@bcoe bcoe force-pushed the float-coverage-patch branch from 56aaecc to d4577e4 Compare January 12, 2019 22:37
@bcoe bcoe requested review from BridgeAR and TimothyGu January 12, 2019 22:41
@bcoe
Copy link
Contributor Author

bcoe commented Jan 12, 2019

👋 flagged a couple folks who I saw reviewing v8 backports recently (CC: @BridgeAR, @TimothyGu).

This is an attempt to back port some of the work that @hashseed, @schuay, and myself have done in v8 related to test coverage -- the patch didn't apply perfectly, so would love an extra set of eyes to make sure I made reasonable decisions when back porting.

bcoe
bcoe commented Jan 12, 2019
View reviewed changes
deps/v8/src/ast/source-range-ast-visitor.cc Outdated
Copy link
Contributor Author

@bcoe bcoe Jan 12, 2019

Choose a reason for hiding this comment

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

statements is sometimes a nullptr, which was leading to a segfault. This does not seem to be possible in the mainline v8; my guess is that a patch that has not been back-ported eliminated the possibility of expr->body() returning a nullptr. It felt like a reasonable stop gap (until we land a fully updated v8) to add the check statements == nullptr.

Copy link
Member

@hashseed hashseed Jan 15, 2019

Choose a reason for hiding this comment

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

I think this is the relevant change.

@bcoe bcoe force-pushed the float-coverage-patch branch from d4577e4 to 28ccf19 Compare January 13, 2019 02:37
@bcoe bcoe changed the title deps: V8: backport 9365d09, aac2f8c deps: cherry-pick 9365d09, aac2f8c from upstream V8 Jan 13, 2019
@bcoe bcoe changed the title deps: cherry-pick 9365d09, aac2f8c from upstream V8 deps: cherry-pick 9365d09, aac2f8c from V8 Jan 13, 2019
@bcoe bcoe changed the title deps: cherry-pick 9365d09, aac2f8c from V8 deps: cherry-pick 9365d09, aac2f8c from v8 Jan 13, 2019
@bcoe bcoe changed the title deps: cherry-pick 9365d09, aac2f8c from v8 deps: cherry-pick 9365d09, aac2f8c from V8 Jan 13, 2019
@bcoe bcoe added the test Issues and PRs related to the tests. label Jan 13, 2019
@bcoe bcoe force-pushed the float-coverage-patch branch 2 times, most recently from a6d6939 to 8b58ffe Compare January 14, 2019 21:48
@mhdawson
Copy link
Member

mhdawson commented Jan 14, 2019

@hashseed would be great to get your review on this since you made some of the original changes.

@mhdawson
Copy link
Member

mhdawson commented Jan 14, 2019

New coverage build with this PR integrated: https://ci.nodejs.org/view/All/job/node-test-commit-linux-coverage-new/3/

hashseed
hashseed approved these changes Jan 15, 2019
View reviewed changes
Copy link
Member

@hashseed hashseed left a comment

Choose a reason for hiding this comment

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

lgtm!

@BridgeAR
Copy link
Member

BridgeAR commented Jan 15, 2019 •
edited
Loading

@mhdawson seems like your test build did not start properly. Could you please have another look at it?

@bcoe bcoe force-pushed the float-coverage-patch branch from 8b58ffe to 7657903 Compare January 15, 2019 20:00
@bcoe bcoe closed this Jan 17, 2019
@bcoe bcoe deleted the float-coverage-patch branch January 17, 2019 02:04
@sam-github sam-github mentioned this pull request Jan 18, 2019
Closed
@addaleax
Copy link
Member

addaleax commented Jan 23, 2019

Should this be backported to v11.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

@bcoe
Copy link
Contributor Author

bcoe commented Jan 23, 2019

@addaleax I think this would be worth back-porting; I honestly wonder if we could get away with landing it on v10.x, which would improve the coverage story for it.

@addaleax
Copy link
Member

addaleax commented Jan 23, 2019

@bcoe In that case you or somebody from the V8 team would need to do the work to backport these patches manually (they have merge conflicts on v11.x).

@bcoe
Copy link
Contributor Author

bcoe commented Jan 23, 2019

@addaleax okay, I'll make an effort to do so after my block of meetings frees up later this afternoon; will update this conversation here if it looks like the back-port is possible.

@bcoe bcoe mentioned this pull request Jan 26, 2019
Closed
2 tasks
bcoe added a commit to bcoe/node-1 that referenced this pull request Jan 26, 2019
@bcoe
deps: v8, cherry-pick 9365d09, aac2f8c, 47d34a3
c92fd82 
  Original commit message 9365d09:

      [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.

      R=bmeurer@chromium.org, jgruber@chromium.org, yangguo@chromium.org

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

  Refs: v8/v8@9365d09

  Original commit message aac2f8c:

      [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 <neis@chromium.org>
      Reviewed-by: Yang Guo <yangguo@chromium.org>
      Commit-Queue: Jakob Gruber <jgruber@chromium.org>
      Cr-Commit-Position: refs/heads/master@{nodejs#56531}

  Refs: v8/v8@aac2f8c

  deps: V8: backport 47d34a3

  Original commit message:

      Revert "[coverage] change block range to avoid ambiguity."

      This reverts commit 471fef0469d04d7c487f3a08e81f3d77566a2f50.

      Reason for revert: A more general fix incoming at https://crrev.com/c/1273095.

      Original change's description:
      > [coverage] change block range to avoid ambiguity.
      >
      > By moving the block range end to left of closing bracket,
      > we can avoid ambiguity where an open-ended singleton range
      > could be both interpreted as inside the parent range, or
      > next to it.
      >
      > R=<U+200B>verwaest@chromium.org
      >
      > Bug: v8:8237
      > Change-Id: Ibc9412b31efe900b6d8bff0d8fa8c52ddfbf460a
      > Reviewed-on: https://chromium-review.googlesource.com/1254127
      > Reviewed-by: Georg Neis <neis@chromium.org>
      > Commit-Queue: Yang Guo <yangguo@chromium.org>
      > Cr-Commit-Position: refs/heads/master@{nodejs#56347}

      TBR=yangguo@chromium.org,neis@chromium.org,verwaest@chromium.org

      # Not skipping CQ checks because original CL landed > 1 day ago.

      Bug: v8:8237
      Change-Id: I39310cf3c2f06a0d98ff314740aaeefbfffc0834
      Reviewed-on: https://chromium-review.googlesource.com/c/1273096
      Reviewed-by: Jakob Gruber <jgruber@chromium.org>
      Reviewed-by: Toon Verwaest <verwaest@chromium.org>
      Reviewed-by: Yang Guo <yangguo@chromium.org>
      Commit-Queue: Jakob Gruber <jgruber@chromium.org>
      Cr-Commit-Position: refs/heads/master@{nodejs#56513}

  Refs: v8/v8@47d34a3

PR-URL: nodejs#25429
Backport-PR-URL: nodejs#25728
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
addaleax pushed a commit that referenced this pull request Feb 1, 2019
@bcoe @addaleax
deps: v8, cherry-pick 9365d09, aac2f8c, 47d34a3
673e434 
  Original commit message 9365d09:

      [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.

      R=bmeurer@chromium.org, jgruber@chromium.org, yangguo@chromium.org

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

  Refs: v8/v8@9365d09

  Original commit message aac2f8c:

      [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 <neis@chromium.org>
      Reviewed-by: Yang Guo <yangguo@chromium.org>
      Commit-Queue: Jakob Gruber <jgruber@chromium.org>
      Cr-Commit-Position: refs/heads/master@{#56531}

  Refs: v8/v8@aac2f8c

  deps: V8: backport 47d34a3

  Original commit message:

      Revert "[coverage] change block range to avoid ambiguity."

      This reverts commit 471fef0469d04d7c487f3a08e81f3d77566a2f50.

      Reason for revert: A more general fix incoming at https://crrev.com/c/1273095.

      Original change's description:
      > [coverage] change block range to avoid ambiguity.
      >
      > By moving the block range end to left of closing bracket,
      > we can avoid ambiguity where an open-ended singleton range
      > could be both interpreted as inside the parent range, or
      > next to it.
      >
      > R=<U+200B>verwaest@chromium.org
      >
      > Bug: v8:8237
      > Change-Id: Ibc9412b31efe900b6d8bff0d8fa8c52ddfbf460a
      > Reviewed-on: https://chromium-review.googlesource.com/1254127
      > Reviewed-by: Georg Neis <neis@chromium.org>
      > Commit-Queue: Yang Guo <yangguo@chromium.org>
      > Cr-Commit-Position: refs/heads/master@{#56347}

      TBR=yangguo@chromium.org,neis@chromium.org,verwaest@chromium.org

      # Not skipping CQ checks because original CL landed > 1 day ago.

      Bug: v8:8237
      Change-Id: I39310cf3c2f06a0d98ff314740aaeefbfffc0834
      Reviewed-on: https://chromium-review.googlesource.com/c/1273096
      Reviewed-by: Jakob Gruber <jgruber@chromium.org>
      Reviewed-by: Toon Verwaest <verwaest@chromium.org>
      Reviewed-by: Yang Guo <yangguo@chromium.org>
      Commit-Queue: Jakob Gruber <jgruber@chromium.org>
      Cr-Commit-Position: refs/heads/master@{#56513}

  Refs: v8/v8@47d34a3

PR-URL: #25429
Backport-PR-URL: #25728
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
Closed
Closed
Closed
@bcoe bcoe mentioned this pull request Mar 14, 2019
Closed
2 tasks
This was referenced Aug 9, 2020
Open
Open
Open
Open
Open
Open
Merged
@snyk-bot snyk-bot mentioned this pull request Oct 29, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhdawson mhdawson mhdawson approved these changes

@addaleax addaleax Awaiting requested review from addaleax

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

@TimothyGu TimothyGu Awaiting requested review from TimothyGu

+1 more reviewer

@hashseed hashseed hashseed approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test Issues and PRs related to the tests. 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.

7 participants

@bcoe @nodejs-github-bot @mhdawson @BridgeAR @addaleax @hashseed @targos