Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix -Z instrument-coverage on MSVC #76002

Merged
merged 1 commit into from
Sep 1, 2020

Conversation

richkadel
Copy link
Contributor

@richkadel richkadel commented Aug 27, 2020

Found that -C link-dead-code (which was enabled automatically
under -Z instrument-coverage) was causing the linking error that
resulted in segmentation faults in coverage instrumented binaries. Link
dead code is now disabled under MSVC, allowing -Z instrument-coverage
to be enabled under MSVC for the first time.

More details are included in Issue #76038 .

Note this PR makes it possible to support Z instrument-coverage but
does not enable instrument coverage for MSVC in existing tests. It will be
enabled in another PR to follow this one (both PRs coming from original
PR #75828).

r? @tmandry
FYI: @wesleywiser

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 27, 2020
src/librustc_session/options.rs Outdated Show resolved Hide resolved
src/librustc_session/session.rs Outdated Show resolved Hide resolved
src/librustc_session/session.rs Outdated Show resolved Hide resolved
@richkadel
Copy link
Contributor Author

@tmandry @wesleywiser - I've addressed all of Tyler's review comments here, and (as part of that) I created a new, more targeted issue #76038

Thanks!

@tmandry
Copy link
Member

tmandry commented Aug 28, 2020

@richkadel and I discussed enabling the tests for MSVC. It requires some test refactoring that's already done in a follow-up change, so we'll do it in that change.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 28, 2020

📌 Commit b1c31c353c361290cb99a8c848d3956ca8417424 has been approved by tmandry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2020
@bors
Copy link
Contributor

bors commented Aug 30, 2020

☔ The latest upstream changes (presumably #74862) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 30, 2020
@tmandry
Copy link
Member

tmandry commented Aug 31, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 31, 2020

📌 Commit ec9744d7be3c3f025686253b3757781be557f603 has been approved by tmandry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 31, 2020
Found that -C link-dead-code (which was enabled automatically
under -Z instrument-coverage) was causing the linking error that
resulted in segmentation faults in coverage instrumented binaries. Link
dead code is now disabled under MSVC, allowing `-Z instrument-coverage`
to be enabled under MSVC for the first time.

More details are included in Issue rust-lang#76038.

(This PR was broken out from PR rust-lang#75828)
@tmandry
Copy link
Member

tmandry commented Sep 1, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 1, 2020

📌 Commit ddb054a has been approved by tmandry

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 1, 2020
Rollup of 12 pull requests

Successful merges:

 - rust-lang#75945 (Use `env::func()`, not 'the function env::func' in docs for std::env)
 - rust-lang#76002 (Fix `-Z instrument-coverage` on MSVC)
 - rust-lang#76003 (Adds two source span utility functions used in source-based coverage)
 - rust-lang#76059 (Clean up E0764)
 - rust-lang#76103 (Clean up E0769)
 - rust-lang#76139 (Make `cow_is_borrowed` methods const)
 - rust-lang#76154 (Fix rustdoc strings indentation)
 - rust-lang#76161 (Remove notrust in rustc_middle)
 - rust-lang#76163 (README: Adjust Linux and macOS support platform and architecture)
 - rust-lang#76166 (Make `StringReader` private)
 - rust-lang#76172 (Revert rust-lang#75463)
 - rust-lang#76178 (Update expect-test to 1.0)

Failed merges:

r? @ghost
@bors bors merged commit 6d834a4 into rust-lang:master Sep 1, 2020
richkadel added a commit to richkadel/rust that referenced this pull request Sep 3, 2020
Adds a new mir_dump output file in HTML/CSS to visualize code regions
and the MIR features that they came from (including overlapping spans).
See example below:

Includes a basic, MIR-block-based implementation of coverage injection,
available via `-Zexperimental-coverage`. This implementation has known
flaws and omissions, but is simple enough to validate the new tools and
tests.

The existing `-Zinstrument-coverage` option currently enables
function-level coverage only, which at least appears to generate
accurate coverage reports at that level.

Experimental coverage is not accurate at this time. When branch coverage
works as intended, the `-Zexperimental-coverage` option should be
removed.

This PR replaces the bulk of PR rust-lang#75828, with the remaining parts of
that PR distributed among other separate and indentpent PRs.

This PR depends on three of those other PRs: rust-lang#76000, rust-lang#76002, and

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage
instrumentation

![Screen-Recording-2020-08-21-at-2](https://user-images.githubusercontent.com/3827298/90972923-ff417880-e4d1-11ea-92bb-8713c6198f6d.gif)
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2020
… r=tmandry

Tools, tests, and experimenting with MIR-derived coverage counters

Leverages the new mir_dump output file in HTML+CSS (from rust-lang#76074) to visualize coverage code regions
and the MIR features that they came from (including overlapping spans).

See example below.

The `run-make-fulldeps/instrument-coverage` test has been refactored to maximize test coverage and reduce code duplication. The new tests support testing with and without `-Clink-dead-code`, so Rust coverage can be tested on MSVC (which, currently, only works with `link-dead-code` _disabled_).

New tests validate coverage region generation and coverage reports with multiple counters per function. Starting with a simple `if-else` branch tests, coverage tests for each additional syntax type can be added by simply dropping in a new Rust sample program.

Includes a basic, MIR-block-based implementation of coverage injection,
available via `-Zexperimental-coverage`. This implementation has known
flaws and omissions, but is simple enough to validate the new tools and
tests.

The existing `-Zinstrument-coverage` option currently enables
function-level coverage only, which at least appears to generate
accurate coverage reports at that level.

Experimental coverage is not accurate at this time. When branch coverage
works as intended, the `-Zexperimental-coverage` option should be
removed.

This PR replaces the bulk of PR rust-lang#75828, with the remaining parts of
that PR distributed among other separate and indentpent PRs.

This PR depends on two of those other PRs: rust-lang#76002, rust-lang#76003 and rust-lang#76074

Rust compiler MCP rust-lang/compiler-team#278

Relevant issue: rust-lang#34701 - Implement support for LLVMs code coverage
instrumentation

![Screen-Recording-2020-08-21-at-2](https://user-images.githubusercontent.com/3827298/90972923-ff417880-e4d1-11ea-92bb-8713c6198f6d.gif)

r? @tmandry
FYI: @wesleywiser
@cuviper cuviper added this to the 1.48.0 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants