Skip to content

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

Closed
bcoe wants to merge 1 commit into
nodejs:v11.x-stagingfrom
bcoe:backport-25429-to-v11.x
Closed

[v11.x backport] deps: v8, cherry-pick 9365d09#25728
bcoe wants to merge 1 commit into
nodejs:v11.x-stagingfrom
bcoe:backport-25429-to-v11.x

Conversation

@bcoe

@bcoebcoe commented Jan 26, 2019

Copy link
Copy Markdown
Contributor

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

see: #25429

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

@nodejs-github-bot

nodejs-github-bot commented Jan 26, 2019

Copy link
Copy Markdown
Collaborator

@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

@nodejs-github-botnodejs-github-bot added v11.x v8 engine Issues and PRs related to the V8 dependency. labels 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>
@bcoe
bcoeforce-pushed the backport-25429-to-v11.x branch from c30e388 to c92fd82CompareJanuary 26, 2019 18:13
@addaleax

addaleax commented Jan 28, 2019•
edited by bcoe
Loading

Copy link
Copy Markdown
Member

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

@targos

targos commented Jan 28, 2019•
edited by bcoe
Loading

Copy link
Copy Markdown
Member

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

@bcoe

bcoe commented Jan 28, 2019

Copy link
Copy Markdown
ContributorAuthor

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

@bcoe
bcoe requested a review from targosJanuary 29, 2019 00:41
hashseed
hashseed reviewed Jan 29, 2019
View reviewed changes
Comment threaddeps/v8/src/ast/ast-source-ranges.h

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

@hashseedhashseedJan 29, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

hashseed
hashseed reviewed Jan 29, 2019
View reviewed changes
Comment threaddeps/v8/src/ast/ast-source-ranges.h

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

@hashseedhashseedJan 29, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto.

hashseed
hashseed reviewed Jan 29, 2019
View reviewed changes
Comment threaddeps/v8/src/ast/ast-source-ranges.h

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

@hashseedhashseedJan 29, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto.

hashseed
hashseed approved these changes Jan 29, 2019
View reviewed changes

@hashseedhashseed left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

bcoe commented Jan 29, 2019

Copy link
Copy Markdown
ContributorAuthor

@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);?

@bcoebcoe changed the title [v11.x backport] deps: v8, cherry-pick 9365d09, aac2f8c, 47d34a3[v11.x backport] deps: v8, cherry-pick 9365d09Jan 30, 2019
@addaleax

addaleax commented Jan 30, 2019

Copy link
Copy Markdown
Member

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

@targos

targos commented Jan 30, 2019

Copy link
Copy Markdown
Member

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

@bcoe

bcoe commented Jan 31, 2019

Copy link
Copy Markdown
ContributorAuthor

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

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>
@addaleax

addaleax commented Feb 1, 2019

Copy link
Copy Markdown
Member

Landed in 673e434, thanks for the reminders!

@addaleaxaddaleax closed this Feb 1, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax approved these changes
@MylesBorinsMylesBorinsAwaiting requested review from MylesBorins
@targostargosAwaiting requested review from targos
+1 more reviewer
@hashseedhashseedhashseed approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

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