Tracking Issue for LLVM InstrProf code coverage (current flag: `-Zinstrument-coverage`)

richkadel commented 3 years ago

This is a tracking issue for the MCP "Implement LLVM-compatible source-based code coverage" (rust-lang/compiler-team#278).

Original feature request: Issue #34701.

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

[x] Implement the MCP
[x] Adjust documentation (see instructions on rustc-dev-guide)
- The Rustc Dev Guide includes detailed documentation on the implementation in a new section, LLVM Source-Based Code Coverage
- Usage is documented in the Rust Unstable Book at https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/source-based-code-coverage.html
- Also see the Inside Rust Blog article at https://blog.rust-lang.org/inside-rust/2020/11/12/source-based-code-coverage.html
[x] Stabilization PR (see instructions on rustc-dev-guide)

Unresolved Questions

There are no unresolved questions from the MCP.

Some implementation options were debated and, if deferred, were logged as FIXME comments in the compiler source code or tests. FIXME comments related to stabilization have been added as rust-lang/rust issues.

If/when stabilized, will the compiler flag be updated to -C instrument-coverage? (If so, the -Z variant could also be supported for some time, to ease migrations for existing users and scripts.)
The Rust coverage implementation depends on (and automatically turns on) -Z symbol-mangling-version=v0. Will stabilizing this feature depend on stabilizing v0 symbol-mangling first? If so, what is the current status and timeline?
The Rust coverage implementation implements the latest version of LLVM's Coverage Mapping Format (version 4), which forces a dependency on LLVM 11 or later. A compiler error is generated if attempting to compile with coverage, and using an older version of LLVM.
What other questions need to be answered before the feature can be stabilized?

Open Issues related to this Tracking Issue

Somewhat ordered by priority:

"Rustdoc doctests should have their file/lines remapped when using -Zinstrument-coverage" (#79417)
- Fixed to correctly offset the lines, but still need a fix to offset the column (character) number
"MIR InstrumentCoverage: Improve coverage of #[should_panic] tests and catch_unwind() handlers" (#78544)
"Improve coverage results on certain derived traits that show two different counters" (#79626)
- This may have been resolved by another coincidental improvement, and just needs verification
"Add a cargo option to generate coverage reports using -Z instrument-coverage" (#85902)
"MIR InstrumentCoverage - Can the source_info.span for FakeRead statements be more consistent?" (#78546)
"std::process::exit() on MSVC bypasses writing LLVM InstrProf counters to profraw file" (#77553)

Also, note the issue tracking label: A-code-coverage

Implementation history

This feature was functionally complete as of PR #78267 and available via the nightly channel as of November 8, 2020.
There have been many subsequent PRs to expand and improve coverage, including support for coverage of and within macros, coverage of dead code within a function (based on const eval), coverage of unused functions, and coverage of unused templates: Summary of merged PRs contributing to the implementation

richkadel commented 3 years ago

@tmandry @wesleywiser

richkadel commented 3 years ago

Just a quick note to the compiler team and interested contributors:

As of today, I'm going to be much less active on Rust Coverage, due to other work priorities.

I believe the implementation is essentially "complete" (including detailed documentation in the Rust Dev Guide that should help future contributors).

I plan to support any serious bugs in the implementation, as they arise, but otherwise, I encourage any contributors interested in taking on any of the "Open Issues" listed above. I should be able to advise/answer questions as needed.

Thank you so much to @tmandry and @wesleywiser for partnering with me on this feature, exceptionally detailed code reviews, and incredibly helpful guidance along the way!

Thanks also to the many other members of the Rust community who contributed requirements, testing, bug reports, and helpful tips along the way!

wesleywiser commented 3 years ago

Thanks for your work on this @richkadel!

clarfonthey commented 3 years ago

Is there an issue somewhere tracking potentially adding a cargo coverage command, or some way of making this interface nicely with cargo?

Right now all of the guides I see still rely on manually merging/searching for different files, so I imagine this would be nice to have, even if it didn't do anything other than just put the coverage data in an easy-to-access place.

chadbrewbaker commented 3 years ago

I ran into an issue for coreutils, https://github.com/uutils/coreutils/issues/2324. The "ls" tests are angry at the default.profraw file. How do you set an environment variable so profiles write to a fixed directory like /tmp/coreutils-coverage?

richkadel commented 3 years ago

Is there an issue somewhere tracking potentially adding a cargo coverage command, or some way of making this interface nicely with cargo?

Thanks for asking. This has been discussed, and I think it's a good idea. I don't have a timeline for working on this but I think there should at least be an issue to track it. I'll add one.

richkadel commented 3 years ago

I ran into an issue for coreutils, uutils/coreutils#2324. The "ls" tests are angry at the default.profraw file. How do you set an environment variable so profiles write to a fixed directory like /tmp/coreutils-coverage?

@chadbrewbaker - Thanks for the comment. Can you create a separate issue on this, preferably with MCVE? Thank you!

chadbrewbaker commented 3 years ago

@richkadel According to https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#id4 there is a LLVM_PROFILE_FILE environment variable. I can't seem to figure out how to get it to write to a fixed directory.

richkadel commented 3 years ago

@chadbrewbaker - This Tracking Issue isn't the right forum for discussing issues like that.

chadbrewbaker commented 3 years ago

Sorry, didn't mean to spam the wrong forum. What is "MCVE", and where would you suggest I move discussion of fixing the profile writing location within the current nightly toolchain?

richkadel commented 3 years ago

If you believe there is a bug, please file a new issue.

The template includes a section "I tried this code". MCVE is cryptic (but google-able) for "a small example of code and steps so someone else can quickly and easily reproduce the issue" (my words). I use the term because it's used by Rust (label E-needs-mcve).

jhpratt commented 3 years ago

Haven't seen this mentioned yet, but @taiki-e has created cargo llvm-cov. I just tested it out and it works well.

cc @clarfonthey

joshtriplett commented 3 years ago

From the unresolved questions:

The Rust coverage implementation implements the latest version of LLVM's Coverage Mapping Format (version 4), which forces a dependency on LLVM 11 or later. A compiler error is generated if attempting to compile with coverage, and using an older version of LLVM.

Given that LLVM 13 has now been released, requiring LLVM 11 for coverage support seems like a reasonable requirement. If people don't have at least LLVM 11, nothing else breaks; they just can't use coverage support. Given that coverage support currently requires a nightly compiler and LLVM 11 or newer, allowing it on a stable compiler built with LLVM 11 or newer seems like an improvement.

joshtriplett commented 3 years ago

The Rust coverage implementation depends on (and automatically turns on) -Z symbol-mangling-version=v0. Will stabilizing this feature depend on stabilizing v0 symbol-mangling first? If so, what is the current status and timeline?

I've submitted https://github.com/rust-lang/rust/pull/90128 to propose stabilizing -C symbol-mangling-version=v0 (without changing the default).

richkadel commented 3 years ago

Given that LLVM 13 has now been released, requiring LLVM 11 for coverage support seems like a reasonable requirement.

@Swatinem - FYI, another data point on maintaining LLVM 11 support as long as it's feasible.

joshtriplett commented 3 years ago

I submitted https://github.com/rust-lang/rust/pull/90132 to propose stabilizing -C instrument-coverage.

sourcefrog commented 2 years ago

Thanks for working on this.

I just wanted to note that the blog post https://blog.rust-lang.org/inside-rust/2020/11/12/source-based-code-coverage.html now has a broken link to https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/source-based-code-coverage.html under "how to get started" - maybe because it's stablizing? (Which is very exciting!)

pierwill commented 2 years ago

@sourcefrog Feel free to submit a PR at https://github.com/rust-lang/blog.rust-lang.org

clarfonthey commented 2 years ago

So, I commented on #79556 which seems to be the primary issue tracking this, but since this issue hasn't been closed yet, I figured I'd state this here for visibility.

A large number of targets seem to not support -Cinstrument-coverage out of the box due to the lack of a profiler_builtins crate, failing with a message along the lines of:

error[E0463]: can't find crate for `profiler_builtins`
  |
  = note: the compiler may have been built without the profiler runtime

For more information about this error, try `rustc --explain E0463`.

After trialing multiple targets on cross, it appears that this applies for all targets besides the tier 1 targets.

I'm not sure the exact reasoning for holding off on closing this issue considering how this has already been released on stable rust and a blog post has been written about it, but I do think that the lack of support for tier 2 targets is a big deal that should probably be looked into.

tmandry commented 2 years ago

@clarfonthey Thanks for bringing this up, I responded on that issue.

I'm also going to close this issue, since the feature has now been stabilized. Issues with the feature can be tracked using the label, A-code-coverage.

rust-lang / rust