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

bcoe · 2019-03-11T02:43:00Z

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 · 2019-03-11T02:43:02Z

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

bcoe · 2019-03-11T03:14:04Z

build: https://ci.nodejs.org/job/node-test-commit/26592/ ✔️
v8 build: https://ci.nodejs.org/job/node-test-commit-v8-linux/2158/ ✔️

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@{nodejs#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@{nodejs#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@{nodejs#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 · 2019-03-12T15:37:32Z

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

Trott

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

bcoe · 2019-03-14T00:58:52Z

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

BridgeAR · 2019-03-14T01:33:04Z

@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 · 2019-03-14T01:38:49Z

Should each backported commit be in its own commit?

bcoe · 2019-03-14T16:26:42Z

@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 · 2019-04-08T10:03:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/22272/

BethGriggs · 2019-04-08T14:06:47Z

Landed on v10.x-staging

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]>

bcoe · 2019-04-08T15:54:00Z

@BethGriggs 🎉 thank you for lading this.

bcoe requested review from Trott, hashseed, mhdawson and targos March 11, 2019 02:43

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 force-pushed the backport-coverage-fixes branch from 0e946b5 to 4768a6f Compare March 11, 2019 02:49

bcoe added 2 commits March 10, 2019 22:01

chore: attempted fix for compiler error

de7ffec

bcoe force-pushed the backport-coverage-fixes branch from 4768a6f to de7ffec Compare March 12, 2019 15:43

Trott approved these changes Mar 12, 2019

View reviewed changes

bcoe added 2 commits March 12, 2019 10:28

chore: address slight differences in test structure

40b8f96

chore: fix typo in tests

50893ee

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 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 14, 2019

ryzokuken approved these changes Mar 14, 2019

View reviewed changes

codebytere approved these changes Mar 14, 2019

View reviewed changes

guybedford mentioned this pull request Apr 7, 2019

[v10.x-backport] module: use compileFunction over Module.wrap #27124

Closed

4 tasks

guybedford approved these changes Apr 7, 2019

View reviewed changes

BethGriggs approved these changes Apr 8, 2019

View reviewed changes

BethGriggs closed this Apr 8, 2019

BethGriggs mentioned this pull request May 1, 2019

v10.16.0 proposal #27514

Merged

oliversalzburg mentioned this pull request Jun 5, 2019

LCOV report misaligned after upgrade to Node 10.16 bcoe/c8#111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

bcoe commented Mar 11, 2019 •

edited

Loading

Uh oh!

nodejs-github-bot commented Mar 11, 2019

Uh oh!

bcoe commented Mar 11, 2019 •

edited

Loading

Uh oh!

bcoe commented Mar 12, 2019

Uh oh!

Trott left a comment

Uh oh!

bcoe commented Mar 14, 2019

Uh oh!

BridgeAR commented Mar 14, 2019

Uh oh!

richardlau commented Mar 14, 2019

Uh oh!

bcoe commented Mar 14, 2019

Uh oh!

nodejs-github-bot commented Apr 8, 2019

Uh oh!

BethGriggs commented Apr 8, 2019

Uh oh!

bcoe commented Apr 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

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

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

Uh oh!

Conversation

bcoe commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Mar 11, 2019

Uh oh!

bcoe commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcoe commented Mar 12, 2019

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

bcoe commented Mar 14, 2019

Uh oh!

BridgeAR commented Mar 14, 2019

Uh oh!

richardlau commented Mar 14, 2019

Uh oh!

bcoe commented Mar 14, 2019

Uh oh!

nodejs-github-bot commented Apr 8, 2019

Uh oh!

BethGriggs commented Apr 8, 2019

Uh oh!

bcoe commented Apr 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

bcoe commented Mar 11, 2019 •

edited

Loading

bcoe commented Mar 11, 2019 •

edited

Loading