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

Regression in code coverage (-Zprofile) at some point after nightly-2021-11-11 #100125

Closed
fadeevab opened this issue Aug 3, 2022 · 16 comments
Closed
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression.

Comments

@fadeevab
Copy link

fadeevab commented Aug 3, 2022

TL;DR:

Older nightly toolchain gives an expected code coverage: rustup default nightly-2021-11-11

UPDATE: Regression is reproduced below #100125 (comment)

Prerequisites

I have a GitHub workflow which generates a code coverage report with nightly toolchain +grcov, then sending it to https://coveralls.io.

export CARGO_INCREMENTAL=0
export RUSTFLAGS="-Zprofile -Ccodegen-units=1 -Copt-level=0 -Clink-dead-code -Coverflow-checks=off -Zpanic_abort_tests -Cpanic=abort"
export RUSTDOCFLAGS="-Zprofile -Ccodegen-units=1 -Copt-level=0 -Clink-dead-code -Coverflow-checks=off -Zpanic_abort_tests -Cpanic=abort"

rustup default nightly
cargo test --all-features
curl -L https://github.com/mozilla/grcov/releases/latest/download/grcov-x86_64-unknown-linux-gnu.tar.bz2 | tar jxf -

mkdir coverage
./grcov ./target/debug/ -s . -t lcov --llvm --branch --ignore-not-existing --ignore "/*" \
                --excl-line '#\[|=> panic!|unreachable!|Io\(std::io::Error\)' \
                --excl-br-line '#\[|=> panic!|unreachable!|assert_..!' -o ./coverage/lcov.info

I expected to see 100% code coverage on a simple file, e.g. see older coverage result: https://coveralls.io/builds/43003664/source?filename=src%2Ferror.rs - error.rs has 100% code coverage.

Instead, this happened:

  1. https://coveralls.io/builds/51398291/source?filename=src%2Ferror.rs - 9% coverage.
  2. https://coveralls.io/builds/51398291/source?filename=src%2Flib.rs - coverage goes over comment lines

Version it worked on

At least nightly-2021-11-11 toolchain works:

rustup default nightly-2021-11-11

image

However, I have not narrowed down the most recent version this worked on.

Version with regression

rustc --version --verbose:

rustc 1.64.0-nightly (0f4bcadb4 2022-07-30)
binary: rustc
commit-hash: 0f4bcadb46006bc484dad85616b484f93879ca4e
commit-date: 2022-07-30
host: x86_64-unknown-linux-gnu
release: 1.64.0-nightly
LLVM version: 14.0.6
@fadeevab fadeevab added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Aug 3, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 3, 2022
@apiraino
Copy link
Contributor

apiraino commented Aug 4, 2022

Unless I am mistaken, grcov is a third-party tool, not officially under the Rust project umbrella.

In order to find an actionable here, a bisection and ideally a reproducible are probably the way to go.

@rustbot label E-needs-bisection E-needs-mcve

@rustbot rustbot added E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example labels Aug 4, 2022
@fadeevab
Copy link
Author

fadeevab commented Aug 4, 2022

@apiraino I've spent a few hours yesterday, and that's my result:

  1. latest grcov + latest rust nightly = bad
  2. old grcov 0.8.2 (~July 2021) + latest rustc nightly = bad
  3. old grcov + old rustc nightly-2021-11-11 = good
  4. latest grcov + old nightly-2021-11-11 = good

Notes:

  • "old" is for simplicity, it means the versions of the end of the 2021 year
  • "latest" means the version at this moment, however, the it's not clear when the actual regression was introduced.
  • "bad" means a wrong coverage
  • "good" means an expected coverage

Case 1 and Case 2 above shows, that the newer nightly either has a regression, or the nightly introduces some profile format incompatibility with 3rd party tools.

@fanninpm
Copy link

fanninpm commented Aug 4, 2022

You can help narrow down the regression even more by using cargo-bisect-rustc. The cargo-bisect-rustc tool is usually used to find compilation errors, but you can use a shell script to tell cargo-bisect-rustc what your regression is.

@fadeevab
Copy link
Author

fadeevab commented Aug 9, 2022

@fanninpm @apiraino


Regression in 936eba3


==================================================================================
= Please file this regression report on the rust-lang/rust GitHub repository     =
=        New issue: https://github.com/rust-lang/rust/issues/new                 =
=     Known issues: https://github.com/rust-lang/rust/issues                     =
= Copy and paste the text below into the issue report thread.  Thanks!           =
==================================================================================

searched nightlies: from nightly-2022-05-16 to nightly-2022-05-20
regressed nightly: nightly-2022-05-19
searched commit range: https://github.com/rust-lang/rust/compare/4c5f6e6277b89e47d73a192078697f7a5f3dc0ac...cd282d7f75da9080fda0f1740a729516e7fbec68
regressed commit: https://github.com/rust-lang/rust/commit/936eba3b348e65b658b60218cc9237f02abdbeb4

<details>
<summary>bisected with <a href='https://github.com/rust-lang/cargo-bisect-rustc'>cargo-bisect-rustc</a> v0.6.4</summary>


Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc ./script.sh --start 2022-05-16 --end 2022-05-20 --preserve 

</details>

I actually bisected between 2021-11-11 and 2022-08-09.

@fanninpm
Copy link

fanninpm commented Aug 9, 2022

regressed commit: 936eba3

Thank you! That points to #96867.

@fadeevab
Copy link
Author

fadeevab commented Aug 9, 2022

Examples of regression.

It checks random lines in the documentation comment:
image


It shows 3-5% of coverage on a little error.rs file (100% without regression).
image

@fadeevab
Copy link
Author

fadeevab commented Aug 9, 2022

@fanninpm By the way, you could probably change labels on this issue :)

@fanninpm
Copy link

fanninpm commented Aug 9, 2022

By the way, you could probably change labels on this issue

Can I?

@rustbot label -E-needs-bisection

Nevertheless, it would be helpful to have a Minimal, Complete, and Verifiable Example.

@rustbot rustbot removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Aug 9, 2022
@fadeevab
Copy link
Author

fadeevab commented Aug 9, 2022

@fanninpm

Steps:

  1. Download https://github.com/fadeevab/cocoon/
  2. Put script.sh into the project directory.
    #!/bin/sh
    
    export CARGO_INCREMENTAL=0
    export RUSTDOCFLAGS="-Zprofile -Ccodegen-units=1 -Copt-level=0 -Clink-dead-code -Coverflow-checks=off -Zpanic_abort_tests -Cpanic=abort"
    export RUSTFLAGS="-Zprofile -Ccodegen-units=1 -Copt-level=0 -Clink-dead-code -Coverflow-checks=off -Zpanic_abort_tests -Cpanic=abort"
    
    cargo test --all-features
    mkdir -p ./${CARGO_TARGET_DIR}/coverage
    ./grcov ./${CARGO_TARGET_DIR}/${CARGO_BUILD_TARGET}/debug/ -s . -t html --llvm --branch --ignore-not-existing --ignore "/*"  --excl-line '#\[|=> panic!|unreachable!|Io\(std::io::Error\)'  --excl-br-line '#\[|=> panic!|unreachable!|assert_..!' -o ./${CARGO_TARGET_DIR}/coverage/lcov.html
    grep -e ">[89]" ./${CARGO_TARGET_DIR}/coverage/lcov.html/index.html
  3. Run bisection
    cargo-bisect-rustc --script ./script.sh --start 2022-05-16 --end 2022-05-20 --preserve

Explanation: if grep finds 80-90% in a coverage report (it looks for a 99% or say 84%, which is also good), so it's just looking for '>9' or '>8', then the script returns 0, else it returns error 1.
I double checked the coverage reports: it really finds a regression between the commits.

@fadeevab
Copy link
Author

@fanninpm

A Minimal, Complete, and Verifiable Example: https://github.com/fadeevab/rust-lang-rust-issues-100125

@fadeevab
Copy link
Author

@fanninpm Could "regression-untriaged" be removed?

@fanninpm
Copy link

@fanninpm Could "regression-untriaged" be removed?

Not until it has been triaged by the appropriate team. @apiraino may be more privy to the triaging processes that go on around here.

@apiraino
Copy link
Contributor

apiraino commented Aug 19, 2022

I've tried spending a bit of time on the differences between grcov reports, I'm unsure how to provide a clear actionable to act on for this behaviour. I checked the grcov issue tracker and found mozilla/grcov#725 which seems close to what is reported here. That issue lead to #91661 which has more insights and suggestions to help forming a diagnose.

For this reason I'd leave this issue marked as in need of some more accurate triaging than I can provide.

@federicomenaquintero
Copy link
Contributor

I can reproduce this with librsvg. Both with grcov 0.8.11:

  • Rust 1.62.0 - coverage seems OK, at 91.7%
  • Rust 1.63.0 - coverage goes down to 76.85%, shows some toplevel comment lines as uncovered, use somecrate::Something; lines as uncovered, etc.

RUSTFLAGS="-Zprofile -Ccodegen-units=1 -Clink-dead-code -Coverflow-checks=off"

(I'm using -Zprofile because -Cinstrument-coverage produces incorrect results when linking to C code)

@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 22, 2023
fadeevab added a commit to fadeevab/cocoon that referenced this issue May 26, 2024
Problem: `zeroize` bumped the MSRV to 1.60, it makes 2021 nightly incompatible.
However, the newest `rustc` versions have a profile coverage regression:
rust-lang/rust#100125

To keep a positive coverage >84%:
- ignore comment sections
- ignore `src/error.rs`
@Enselic Enselic added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label May 29, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 1, 2024
…ukang

Remove support for `-Zprofile` (gcov-style coverage instrumentation)

Tracking issue: rust-lang#42524

MCP: rust-lang/compiler-team#798

---

This PR removes the unstable `-Zprofile` flag, which enables ”gcov-style” coverage instrumentation, along with its associated `-Zprofile-emit` configuration flag.

(The profile flag predates and is almost entirely separate from the stable `-Cinstrument-coverage` flag.)

Notably, the `-Zprofile` flag:
- Is largely untested in-tree, having only one run-make test that does not check whether its output is correct or useful.
- Has no known maintainer.
- Has seen no push towards stabilization.
- Has at least one severe regression reported in 2022 that apparently remains unaddressed.
  - rust-lang#100125
- Is confusingly named, since it appears to be more about coverage than performance profiling, and has nothing to do with PGO.
- Is fundamentally limited by relying on counters auto-inserted by LLVM, with no knowledge of Rust beyond debuginfo.
jieyouxu added a commit to jieyouxu/rust that referenced this issue Nov 1, 2024
…ukang

Remove support for `-Zprofile` (gcov-style coverage instrumentation)

Tracking issue: rust-lang#42524

MCP: rust-lang/compiler-team#798

---

This PR removes the unstable `-Zprofile` flag, which enables ”gcov-style” coverage instrumentation, along with its associated `-Zprofile-emit` configuration flag.

(The profile flag predates and is almost entirely separate from the stable `-Cinstrument-coverage` flag.)

Notably, the `-Zprofile` flag:
- Is largely untested in-tree, having only one run-make test that does not check whether its output is correct or useful.
- Has no known maintainer.
- Has seen no push towards stabilization.
- Has at least one severe regression reported in 2022 that apparently remains unaddressed.
  - rust-lang#100125
- Is confusingly named, since it appears to be more about coverage than performance profiling, and has nothing to do with PGO.
- Is fundamentally limited by relying on counters auto-inserted by LLVM, with no knowledge of Rust beyond debuginfo.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 1, 2024
…ukang

Remove support for `-Zprofile` (gcov-style coverage instrumentation)

Tracking issue: rust-lang#42524

MCP: rust-lang/compiler-team#798

---

This PR removes the unstable `-Zprofile` flag, which enables ”gcov-style” coverage instrumentation, along with its associated `-Zprofile-emit` configuration flag.

(The profile flag predates and is almost entirely separate from the stable `-Cinstrument-coverage` flag.)

Notably, the `-Zprofile` flag:
- Is largely untested in-tree, having only one run-make test that does not check whether its output is correct or useful.
- Has no known maintainer.
- Has seen no push towards stabilization.
- Has at least one severe regression reported in 2022 that apparently remains unaddressed.
  - rust-lang#100125
- Is confusingly named, since it appears to be more about coverage than performance profiling, and has nothing to do with PGO.
- Is fundamentally limited by relying on counters auto-inserted by LLVM, with no knowledge of Rust beyond debuginfo.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 2, 2024
Rollup merge of rust-lang#131829 - Zalathar:goodbye-zprofile, r=chenyukang

Remove support for `-Zprofile` (gcov-style coverage instrumentation)

Tracking issue: rust-lang#42524

MCP: rust-lang/compiler-team#798

---

This PR removes the unstable `-Zprofile` flag, which enables ”gcov-style” coverage instrumentation, along with its associated `-Zprofile-emit` configuration flag.

(The profile flag predates and is almost entirely separate from the stable `-Cinstrument-coverage` flag.)

Notably, the `-Zprofile` flag:
- Is largely untested in-tree, having only one run-make test that does not check whether its output is correct or useful.
- Has no known maintainer.
- Has seen no push towards stabilization.
- Has at least one severe regression reported in 2022 that apparently remains unaddressed.
  - rust-lang#100125
- Is confusingly named, since it appears to be more about coverage than performance profiling, and has nothing to do with PGO.
- Is fundamentally limited by relying on counters auto-inserted by LLVM, with no knowledge of Rust beyond debuginfo.
@jieyouxu
Copy link
Member

jieyouxu commented Nov 7, 2024

Triage: closing because -Zprofile is removed following #131829.

@jieyouxu jieyouxu closed this as completed Nov 7, 2024
@jieyouxu jieyouxu removed the E-needs-mcve Call for participation: This issue has a repro, but needs a Minimal Complete and Verifiable Example label Nov 7, 2024
@fadeevab
Copy link
Author

fadeevab commented Nov 10, 2024

No body, no crime 😁

For the reference, here is what can be used instead of grcov: llvm-cov and the PR fadeevab/cocoon#31 how to make a transition from grcov to llvm-cov.

(Alternatively, see PR namib-project/dcaf-rs#27 how to continue using grcov with -Cinstrument-coverage instead of -Zprofile).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression.
Projects
None yet
Development

No branches or pull requests

7 participants