[v11.x backport] deps: v8, cherry-pick 9365d09 #25728

bcoe · 2019-01-26T18:11:25Z

backport fixes for terminal return statements (https://chromium-review.googlesource.com/c/v8/v8/+/1339119).

doesn't apply cleanly, so would appreciate eyes of @hashseed, @schuay

Checklist

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

nodejs-github-bot · 2019-01-26T18:11:28Z

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

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. [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 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 <[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 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>[email protected] > > Bug: v8:8237 > Change-Id: Ibc9412b31efe900b6d8bff0d8fa8c52ddfbf460a > Reviewed-on: https://chromium-review.googlesource.com/1254127 > Reviewed-by: Georg Neis <[email protected]> > Commit-Queue: Yang Guo <[email protected]> > Cr-Commit-Position: refs/heads/master@{nodejs#56347} [email protected],[email protected],[email protected] # 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 <[email protected]> Reviewed-by: Toon Verwaest <[email protected]> Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Jakob Gruber <[email protected]> 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 <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

addaleax · 2019-01-28T17:38:45Z

Rebuild CI: https://ci.nodejs.org/job/node-test-pull-request/20379/ ✅

targos · 2019-01-28T19:16:37Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2055/ ✅

bcoe · 2019-01-28T21:23:38Z

@addaleax @targos it's looking like tests are green for both V8 and Node.js, alright to land?

hashseed · 2019-01-29T19:25:14Z

deps/v8/src/ast/ast-source-ranges.h


-  SourceRange GetRange(SourceRangeKind kind) {
+  SourceRange GetRange(SourceRangeKind kind) override {
    DCHECK_EQ(kind, SourceRangeKind::kRight);


In the original this was changed to DCHECK(HasRange(kind)) as well. Little difference. Just for the record.

hashseed · 2019-01-29T19:25:28Z

deps/v8/src/ast/ast-source-ranges.h


-  SourceRange GetRange(SourceRangeKind kind) {
+  SourceRange GetRange(SourceRangeKind kind) override {
    DCHECK_EQ(kind, SourceRangeKind::kContinuation);


hashseed · 2019-01-29T19:25:43Z

deps/v8/src/ast/ast-source-ranges.h


-  SourceRange GetRange(SourceRangeKind kind) {
+  SourceRange GetRange(SourceRangeKind kind) override {
    DCHECK_EQ(kind, SourceRangeKind::kBody);


hashseed

lgtm, but commit description is a bit misleading. I don't think aac2f8c nor 47d34a3 have been backmerged.

47d34a3 is a revert. Both the original and this revert landed in 7.1, and does not affect v11, which is based on 7.0.
aac2f8c has been initially landed in 7.1, but merged back to 7.0 and already been picked up by Node v11 as well.

bcoe · 2019-01-29T22:54:45Z

@hashseed so, in the case of this backport, we're only really cherry-picking 9365d09?

With regards to the debug checks, is it okay to leave them DCHECK_EQ(kind, SourceRangeKind::kRight);?

addaleax · 2019-01-30T21:37:57Z

@targos Do you want to land this & include it in 11.9.0?

targos · 2019-01-30T22:32:44Z

@addaleax Sorry, I'm just seeing your message now. 11.9.0 is already out.

bcoe · 2019-01-31T19:22:21Z

@targos @addaleax do we know who will be managing the release of v11.10.0; can we try to get this merged in? should make coverage feel a bit less broken.

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. [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 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 <[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 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>[email protected] > > Bug: v8:8237 > Change-Id: Ibc9412b31efe900b6d8bff0d8fa8c52ddfbf460a > Reviewed-on: https://chromium-review.googlesource.com/1254127 > Reviewed-by: Georg Neis <[email protected]> > Commit-Queue: Yang Guo <[email protected]> > Cr-Commit-Position: refs/heads/master@{#56347} [email protected],[email protected],[email protected] # 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 <[email protected]> Reviewed-by: Toon Verwaest <[email protected]> Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Jakob Gruber <[email protected]> Cr-Commit-Position: refs/heads/master@{#56513} Refs: v8/v8@47d34a3 PR-URL: #25429 Backport-PR-URL: #25728 Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

addaleax · 2019-02-01T16:01:13Z

Landed in 673e434, thanks for the reminders!

bcoe requested review from MylesBorins, addaleax and hashseed January 26, 2019 18:11

nodejs-github-bot added v11.x v8 engine Issues and PRs related to the V8 dependency. labels Jan 26, 2019

bcoe force-pushed the backport-25429-to-v11.x branch from c30e388 to c92fd82 Compare January 26, 2019 18:13

bcoe requested a review from targos January 29, 2019 00:41

addaleax approved these changes Jan 29, 2019

View reviewed changes

hashseed reviewed Jan 29, 2019

View reviewed changes

hashseed approved these changes Jan 29, 2019

View reviewed changes

bcoe changed the title ~~[v11.x backport] deps: v8, cherry-pick 9365d09, aac2f8c, 47d34a3~~ [v11.x backport] deps: v8, cherry-pick 9365d09 Jan 30, 2019

addaleax closed this Feb 1, 2019

Uh oh!

[v11.x backport] deps: v8, cherry-pick 9365d09 #25728

[v11.x backport] deps: v8, cherry-pick 9365d09 #25728

Uh oh!

Conversation

bcoe commented Jan 26, 2019

Checklist

Uh oh!

nodejs-github-bot commented Jan 26, 2019

Uh oh!

addaleax commented Jan 28, 2019 • edited by bcoe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Jan 28, 2019 • edited by bcoe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcoe commented Jan 28, 2019

Uh oh!

hashseed Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

hashseed Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

hashseed Jan 29, 2019

Choose a reason for hiding this comment

Uh oh!

hashseed left a comment

Choose a reason for hiding this comment

Uh oh!

bcoe commented Jan 29, 2019

Uh oh!

addaleax commented Jan 30, 2019

Uh oh!

targos commented Jan 30, 2019

Uh oh!

bcoe commented Jan 31, 2019

Uh oh!

addaleax commented Feb 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

addaleax commented Jan 28, 2019 •

edited by bcoe

Loading

targos commented Jan 28, 2019 •

edited by bcoe

Loading