Skip to content

deps: V8: cherry-pick 0dfd9ea51241 - #30713

Closed
bcoe wants to merge 1 commit into
nodejs:masterfrom
bcoe:fix-coverage-bug
Closed

deps: V8: cherry-pick 0dfd9ea51241#30713
bcoe wants to merge 1 commit into
nodejs:masterfrom
bcoe:fix-coverage-bug

Conversation

@bcoe

@bcoebcoe commented Nov 29, 2019

Copy link
Copy Markdown
Contributor

Original commit message:

[coverage] Fix coverage with default arguments
In the presence of default arguments, the body of the function gets
wrapped into another block. This caused our trailing-range-after-return
optimization to not apply, because the wrapper block had no source
range assigned. This CL correctly assignes a source range to that block,
which allows already present code to handle it correctly.
Note that this is not a real coverage bug; we've just been reporting
whitespace as uncovered. We're fixing it for consistency.
Originally reported on github.com/bcoe/c8/issues/66
Bug: v8:9952
Change-Id: Iab3905f558eb99126e0dad8072d03d0a312fdcd3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1903430
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64836}

Refs: v8/v8@0dfd9ea

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

This addresses: bcoe/c8#66, bcoe/c8#151

@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Nov 29, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejsnodejs deleted a comment from nodejs-github-botNov 30, 2019
Original commit message:
[coverage] Fix coverage with default arguments
In the presence of default arguments, the body of the function gets
wrapped into another block. This caused our trailing-range-after-return
optimization to not apply, because the wrapper block had no source
range assigned. This CL correctly assignes a source range to that block,
which allows already present code to handle it correctly.
Note that this is not a real coverage bug; we've just been reporting
whitespace as uncovered. We're fixing it for consistency.
Originally reported on github.com/bcoe/c8/issues/66
Bug: v8:9952
Change-Id: Iab3905f558eb99126e0dad8072d03d0a312fdcd3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1903430
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{nodejs#64836}
Refs: v8/v8@0dfd9ea
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 30, 2019
@addaleax

Copy link
Copy Markdown
Member

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@bcoebcoe added the fast-track PRs that do not need to wait for 48 hours to land. label Nov 30, 2019
@bcoe

bcoe commented Nov 30, 2019

Copy link
Copy Markdown
ContributorAuthor

@addaleax it looks like the pertinent V8 tests (coverage-block-async, and coverage-block) have passed 👍 (it looks like unrelated V8 failures have been failing for all builds recently).

I would like to fast-track landing this patch, while I have some cycles today.

👍 or 👎

bcoe pushed a commit that referenced this pull request Nov 30, 2019
Original commit message:
[coverage] Fix coverage with default arguments
In the presence of default arguments, the body of the function gets
wrapped into another block. This caused our trailing-range-after-return
optimization to not apply, because the wrapper block had no source
range assigned. This CL correctly assignes a source range to that block,
which allows already present code to handle it correctly.
Note that this is not a real coverage bug; we've just been reporting
whitespace as uncovered. We're fixing it for consistency.
Originally reported on github.com/bcoe/c8/issues/66
Bug: v8:9952
Change-Id: Iab3905f558eb99126e0dad8072d03d0a312fdcd3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1903430
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64836}
Refs: v8/v8@0dfd9ea
PR-URL: #30713
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@bcoe

bcoe commented Nov 30, 2019

Copy link
Copy Markdown
ContributorAuthor

Landed in 5961161

@bcoebcoe closed this Nov 30, 2019
@bcoe
bcoe deleted the fix-coverage-bug branch November 30, 2019 20:15
targos pushed a commit that referenced this pull request Dec 1, 2019
Original commit message:
[coverage] Fix coverage with default arguments
In the presence of default arguments, the body of the function gets
wrapped into another block. This caused our trailing-range-after-return
optimization to not apply, because the wrapper block had no source
range assigned. This CL correctly assignes a source range to that block,
which allows already present code to handle it correctly.
Note that this is not a real coverage bug; we've just been reporting
whitespace as uncovered. We're fixing it for consistency.
Originally reported on github.com/bcoe/c8/issues/66
Bug: v8:9952
Change-Id: Iab3905f558eb99126e0dad8072d03d0a312fdcd3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1903430
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64836}
Refs: v8/v8@0dfd9ea
PR-URL: #30713
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeARBridgeAR mentioned this pull request Dec 3, 2019
@targos

Copy link
Copy Markdown
Member

Should this be backported to v12.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

bcoe commented Jan 14, 2020

Copy link
Copy Markdown
ContributorAuthor

@targos ideally we'd backport this, sounds like I should go through that guide?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.buildIssues and PRs related to build files or the CI.fast-trackPRs that do not need to wait for 48 hours to land.v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@bcoe@nodejs-github-bot@addaleax@targos@Trott@cjihrig@gengjiawen@MylesBorins