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.

Sign up for GitHub

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

Jump to bottom

Remove support for -Zprofile (gcov-style coverage instrumentation) #131829

Merged
merged 1 commit into from
Nov 2, 2024
Merged

Remove support for -Zprofile (gcov-style coverage instrumentation) #131829

merged 1 commit into from
Nov 2, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Oct 17, 2024 •
edited
Loading

Tracking issue: #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.
  • 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.

@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2024

r? @chenyukang

rustbot has assigned @chenyukang.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 17, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@Zalathar Zalathar mentioned this pull request Oct 17, 2024
Closed
3 tasks
@Zalathar
Copy link
Contributor Author

MCP: rust-lang/compiler-team#798

@jieyouxu jieyouxu added S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 17, 2024
@Zalathar
Copy link
Contributor Author

Rebased; MCP accepted.

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 30, 2024
@chenyukang
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 1, 2024

📌 Commit ce3e14a has been approved by chenyukang

It is now in the queue for this repository.

@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 Nov 1, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Nov 1, 2024

Technically this is unstable flag, but should this get compat relnotes still?

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 1, 2024
@matthiaskrgr
Rollup merge of rust-lang#131829 - Zalathar:goodbye-zprofile, r=cheny…
11a09d4 
…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.
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 1, 2024
Closed
@Zalathar Zalathar removed the S-waiting-on-MCP Status: PR has a compiler MCP and is waiting for the compiler MCP to complete. label Nov 1, 2024
@Zalathar
Copy link
Contributor Author

Zalathar commented Nov 1, 2024

Given the flag’s age and former importance (?), a mention in relnotes is probably warranted. I’ll try to come up with a suggested wording.

@jieyouxu
Copy link
Member

jieyouxu commented Nov 1, 2024

@rustbot label +relnotes

@rustbot rustbot added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 1, 2024
@rustbot rustbot mentioned this pull request Nov 1, 2024
Open
3 tasks
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 1, 2024
@bors
Auto merge of rust-lang#132446 - matthiaskrgr:rollup-qm3yilw, r=matth…
e468296 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#131829 (Remove support for `-Zprofile` (gcov-style coverage instrumentation))
 - rust-lang#132341 (Reject raw lifetime followed by `'`, like regular lifetimes do)
 - rust-lang#132369 (style-guide: Only use the new binop heuristic for assignments)
 - rust-lang#132383 (Implement suggestion for never type fallback lints)
 - rust-lang#132437 (coverage: Regression test for inlining into an uninstrumented crate)
 - rust-lang#132438 (Remove unncessary option for default rust-analyzer setting)

r? `@ghost`
`@rustbot` modify labels: rollup
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Nov 1, 2024
@jieyouxu
Rollup merge of rust-lang#131829 - Zalathar:goodbye-zprofile, r=cheny…
968e7aa 
…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 jieyouxu mentioned this pull request Nov 1, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 1, 2024
@bors
Auto merge of rust-lang#132462 - jieyouxu:rollup-n9070v3, r=jieyouxu
8e5679f 
Rollup of 13 pull requests

Successful merges:

 - rust-lang#131829 (Remove support for `-Zprofile` (gcov-style coverage instrumentation))
 - rust-lang#132369 (style-guide: Only use the new binop heuristic for assignments)
 - rust-lang#132383 (Implement suggestion for never type fallback lints)
 - rust-lang#132437 (coverage: Regression test for inlining into an uninstrumented crate)
 - rust-lang#132438 (Remove unncessary option for default rust-analyzer setting)
 - rust-lang#132439 (Add `f16` and `f128` to `invalid_nan_comparison`)
 - rust-lang#132445 (Cleanup attributes around unchecked shifts and unchecked negation in const)
 - rust-lang#132448 (Add missing backtick)
 - rust-lang#132450 (Show actual MIR when MIR building forgot to terminate block)
 - rust-lang#132451 (remove some unnecessary rustc_allow_const_fn_unstable)
 - rust-lang#132455 (make const_alloc_layout feature gate only about functions that are already stable)
 - rust-lang#132456 (Move remaining inline assembly test files into asm directory)
 - rust-lang#132459 (feat(byte_sub_ptr): unstably add ptr::byte_sub_ptr)

r? `@ghost`
`@rustbot` modify labels: rollup
cakebaker added a commit to cakebaker/findutils that referenced this pull request Nov 11, 2024
@cakebaker
ci: use -Cinstrument-coverage instead of -Zprofile
c60baea 
Support for -Zprofile has been removed: rust-lang/rust#131829
cakebaker added a commit to cakebaker/findutils that referenced this pull request Nov 11, 2024
@cakebaker
ci: use -Cinstrument-coverage instead of -Zprofile
b6728c6 
Support for -Zprofile has been removed: rust-lang/rust#131829
@a-wing a-wing mentioned this pull request Nov 11, 2024
Merged
This was referenced Nov 12, 2024
Closed
Closed
cakebaker added a commit to cakebaker/procps that referenced this pull request Nov 12, 2024
@cakebaker
ci: use -Cinstrument-coverage instead of -Zprofile
e9a4a1d 
Support for -Zprofile has been removed from Rust nightly: rust-lang/rust#131829
cakebaker added a commit to cakebaker/findutils that referenced this pull request Nov 12, 2024
@cakebaker
ci: use -Cinstrument-coverage instead of -Zprofile
9920670 
Support for -Zprofile has been removed: rust-lang/rust#131829
cakebaker added a commit to cakebaker/findutils that referenced this pull request Nov 12, 2024
@cakebaker
ci: use -Cinstrument-coverage instead of -Zprofile
b0d7c7a 
Support for -Zprofile has been removed: rust-lang/rust#131829
cakebaker added a commit to cakebaker/coreutils that referenced this pull request Nov 12, 2024
@cakebaker
ci: use -Cprofile-generate instead of -Zprofile
e2e699e 
Support for -Zprofile has been removed in rust-lang/rust#131829
cakebaker added a commit to cakebaker/coreutils that referenced this pull request Nov 12, 2024
@cakebaker
ci: use -Cprofile-generate instead of -Zprofile
6e83cd6 
Support for -Zprofile has been removed in rust-lang/rust#131829
cakebaker added a commit to cakebaker/coreutils that referenced this pull request Nov 12, 2024
@cakebaker
ci: use -Cprofile-generate instead of -Zprofile
4529003 
Support for -Zprofile has been removed in rust-lang/rust#131829
cakebaker added a commit to cakebaker/coreutils that referenced this pull request Nov 12, 2024
@cakebaker
ci: use -Cprofile-generate instead of -Zprofile
953ab1e 
Support for -Zprofile has been removed in rust-lang/rust#131829
cakebaker added a commit to cakebaker/findutils that referenced this pull request Nov 12, 2024
@cakebaker
ci: use -Cinstrument-coverage instead of -Zprofile
28c728e 
Support for -Zprofile has been removed: rust-lang/rust#131829
cakebaker added a commit to cakebaker/findutils that referenced this pull request Nov 13, 2024
@cakebaker
ci: use -Cinstrument-coverage instead of -Zprofile
bca84ba 
and use cargo-llvm-cov instead of grcov.

Support for -Zprofile has been removed from Rust nightly: rust-lang/rust#131829
@PierreZ PierreZ mentioned this pull request Nov 13, 2024
Closed
cakebaker added a commit to cakebaker/coreutils that referenced this pull request Nov 14, 2024
@cakebaker
ci: use -Cprofile-generate instead of -Zprofile
152eccf 
Support for -Zprofile has been removed in rust-lang/rust#131829
cakebaker added a commit to cakebaker/coreutils that referenced this pull request Nov 15, 2024
@cakebaker
ci: use -Cprofile-generate instead of -Zprofile
32d8bf1 
Support for -Zprofile has been removed in rust-lang/rust#131829
@sylvestre sylvestre mentioned this pull request Nov 16, 2024
Open
@sylvestre
Copy link
Contributor

@Zalathar While not perfect, -Zprofile had advantages over the other option.
Since this change, we haven't been able to get coverage working on again on a medium size rust project:
uutils/coreutils#6857
(the size of the profraw files is way too large for Github).

Am I missing something? If not, would it be possible to reconsider this removal ? Thanks :)

@marco-c
Copy link
Contributor

marco-c commented Nov 16, 2024

Agreed with @sylvestre, there are many users, and some large projects might have difficulties switching to source-based coverage as it can be more resource intensive.
If there are no considerable benefits stemming from its removal, it would be ideal to keep it even with its limitations.

@workingjubilee
Copy link
Member

workingjubilee commented Nov 16, 2024 •
edited
Loading

My understanding is that the "limitations" are coverage info that can be incorrect by a factor greater than 10, right? It's based on debuginfo, but either not in a way that seems to be very accurate for Rust code, or we are emitting debuginfo in a way that is not useful for it, right?

Minoru added a commit to newsboat/newsboat that referenced this pull request Nov 17, 2024
@Minoru
Migrate to source-based code coverage
27e67df 
We used to rely on `-Zprofile`, but it was recently removed from Rust
Nightly: rust-lang/rust#131829 This commit
switches to a stable alternative; cf.
mozilla/grcov#1240

This change is accompanied by a change in the build environment: instead
of using Clang 18 with Rust Nightly, we now use Clang 18 with Rust 1.81.
That's the last Rust version based on LLVM 18. Previously we tried to
match LLVM versions between C++ and Rust to avoid weird coverage
results. Now, it's *required* to match them, otherwise no coverage is
gathered at all.

Fixes #2912.
@Minoru Minoru mentioned this pull request Nov 17, 2024
Merged
@Zalathar
Copy link
Contributor Author

If there are no considerable benefits stemming from its removal, it would be ideal to keep it even with its limitations.

Unfortunately, the existing code was causing real maintenance problems in other parts of the compiler, which was one of the reasons for removing it.

@jieyouxu
Copy link
Member

jieyouxu commented Nov 19, 2024 •
edited
Loading

To add, removal of -Zprofile is without prejudice against re-adding the gcov-based coverage instrumentation back under a new unstable feature, provided that:

(1) It uses a more accurate name (one that's not confusing like -Zprofile), and
(2) There are suitable maintainers who sign up to maintain it, and
(3) Passes the review bar for a new unstable feature. E.g. does not cause unwanted maintenance burden on other parts of the compiler (i.e. adding more hacks back)

i.e.

But if somebody out there wants to take the initiative and re-add the feature in a form that would pass the review bar for a new unstable feature, that should be fine.

cc https://rust-lang.zulipchat.com/#narrow/channel/233931-t-compiler.2Fmajor-changes/topic/Remove.20unstable.20.60-Zprofile.60.20.28gcov-style.20c.E2.80.A6.20compiler-team.23798/near/482809968

@workingjubilee
Copy link
Member

workingjubilee commented Nov 19, 2024 •
edited
Loading

It would also be nice if someone reported to LLVM about the storage usage being excessive. I feel it is doubtful they have done any optimizations on the profdata format's space usage.

@pulsastrix pulsastrix mentioned this pull request Nov 20, 2024
Merged
elcoosp added a commit to elcoosp/scrpr that referenced this pull request Nov 23, 2024
@elcoosp
ci: rm -Zprofile see rust-lang/rust#131829
52b6525
rwestphal added a commit to holo-routing/holo that referenced this pull request Nov 25, 2024
@rwestphal
ci: use cargo-llvm-cov to generate code coverage data
1ef362b 
The rustc's -Zprofile compiler flag was removed on nightly [1], which
means gcov-style coverage instrumentation is no longer possible.

[1] rust-lang/rust#131829

Signed-off-by: Renato Westphal <[email protected]>
rwestphal added a commit to holo-routing/holo that referenced this pull request Nov 25, 2024
@rwestphal
ci: use cargo-llvm-cov to generate code coverage data
82d63d0 
The rustc's -Zprofile compiler flag was removed on nightly [1], which
means gcov-style coverage instrumentation is no longer possible.

[1] rust-lang/rust#131829

Signed-off-by: Renato Westphal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@chenyukang chenyukang

Labels
A-run-make Area: port run-make Makefiles to rmake.rs relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.84.0
Development

Successfully merging this pull request may close these issues.
8 participants
@Zalathar @rustbot @chenyukang @bors @jieyouxu @sylvestre @marco-c @workingjubilee