Code Coverage Support for Core Library#180
Conversation
Co-authored-by: Gnana Ganesh <gnana.ganesh@thoughtworks.com> Co-authored-by: Amit Dharmapurikar <amit.dharmapurikar@thoughtworks.com>
Co-authored-by: Amit Dharmapurikar <amit.dharmapurikar@thoughtworks.com>
Co-authored-by: Amit Dharmapurikar <amit.dharmapurikar@thoughtworks.com> Co-authored-by: Gnana Ganesh <gnana.ganesh@thoughtworks.com>
Build the core library with instrumentation as well to avoid crate duplicacy issue Co-authored-by: Amit Dharmapurikar <amit.dharmapurikar@thoughtworks.com> Co-authored-by: Gnana Ganesh <gnana.ganesh@thoughtworks.com>
Puts all the profraw files in build/coverage after each test run. This behavior can be overridden by setting a different path using the LLVM_PROFILE_FILE environment variable
emilyalbini
left a comment
There was a problem hiding this comment.
Thanks for all the work on this! Left a few small comments on how the bootstrap codebase is usually structured, plus a major comment on a removed check.
| builder, | ||
| ); | ||
|
|
||
| if collect_profraw { |
There was a problem hiding this comment.
Why do we need to copy the files? Can't we just configure LLVM_PROFILE_FILE to point to the right path?
There was a problem hiding this comment.
My rationale was that if the coverage command somehow fails, the old coverage data would be lost, that's why we write to temp and then rename.
That being said std::fs::rename still can't be used to do an atomic move replace.
There was a problem hiding this comment.
Makes sense. Maybe add an inline comment explaining this?
There was a problem hiding this comment.
Sure, I've left a comment.
In creader.rs, there is a check if the default profiler_builtins has no_core but in the custom profiler builtins there is no dependency on core. To by pass the check in creader.rs a new profiler runtime is passed to the compiler using the -Zprofiler-runtime flag. Co-authored-by: Amit Dharmapurikar <amit.dharmapurikar@thoughtworks.com> Co-authored-by: Atri Sarkar <atri.sarkar@thoughtworks.com>
9fafd02 to
1438f13
Compare
We have updated the PR as per the review comments. |
b18c702 to
0793c8b
Compare
|
🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
@gnana-ganesh-tw could you force-push the branch again / push an empty commit ( |
0793c8b to
7db0d57
Compare
|
Did a force push |
232: Split and refactor the test outcomes page r=Veykril a=pietroalbini
This PR changes how the test outcomes page's content is structured, splitting it into one page per target. This has two purposes:
* Reduce the clutter on the page, which would otherwise mix information of all qualified targets.
* Provide an accurate list of ignored tests, as each target now has the list of tests it ignored rather than just displaying a list of tests ignored by *all* targets.
To simplify the generation of the separate pages, I changed how the `ferrocene_test_outcomes` extension works. Rather than having `.. ignored-tests::` and `.. suite-summary::` directives that are rendered in Python code, the extension now gathers and organizes the data, and defers the rendering of the information to a Jinja2 rST template.
This results in the whole test results page being rendered by the template, which eases the maintenance of those pages. For example, a note that should be displayed only on cross-compiled targets can now be gated behind `{% if host != target %}`.
Finally, while refactoring I removed the whole parsing code of the debug representation of steps, replacing it with the [structured test metadata](rust-lang/rust#111936) I added a while back. This will increase the robustness of the tool.
There are still some open issues on this I'll address in a future PR:
* The list of crates for the bootstrap test suite is empty.
* The note for doc-tests not being executed is not present on aarch64.
* I'd like to add references to the test suite definitions in the evaluation plan.
* I'd like to see if I can make the information displayed in the page more concise.
238: Code Coverage Support for Core Library [DUPLICATE] r=pietroalbini a=Ax9DTW
Bors is stuck on #180, so this duplicate.
243: Automated pull from `rust-lang/libc` r=Veykril a=github-actions[bot]
This PR pulls the following changes from the [`rust-lang/libc`](https://github.com/rust-lang/libc) repository:
* `3570`: [[Backport #3548] Add ioctl FS_IOC_{G,S}{ETVERSION,ETFLAGS} for LoongArch64](https://www.github.com/rust-lang/libc/issues/3570)
* `3553`: [Add MFD_NOEXEC_SEAL and MFD_EXEC](https://www.github.com/rust-lang/libc/issues/3553)
* `3554`: [Backport of #3546 and update crate version to 0.2.153](https://www.github.com/rust-lang/libc/issues/3554)
Co-authored-by: Pietro Albini <pietro.albini@ferrous-systems.com>
Co-authored-by: Atri Sarkar <atri.sarkar@Atris-MacBook-Pro.local>
Co-authored-by: Gnana Ganesh <gnana.ganesh@thoughtworks.com>
Co-authored-by: rusty-snake <41237666+rusty-snake@users.noreply.github.com>
Co-authored-by: Guillaume Gomez <guillaume1.gomez@gmail.com>
Co-authored-by: WANG Rui <wangrui@loongson.cn>
Co-authored-by: Yuki Okushi <jtitor@2k36.org>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
231: Fix typos r=pietroalbini a=Urhengulas Various typo fixes and few actual changes. Actual changes: - Replace table with bullet points in `ferrocene/doc/safety-manual/src/customer-interactions.rst` --> before the 2nd-level bullet points were not rendered - Replace generic "his" with generic "her" in two files - Indent two tables in `ferrocene/doc/evaluation-plan/src/method.rst` --> now the table are displayed below the bullet point - `ferrocene/ferrocene` is public --> before it was written that it is private 238: Code Coverage Support for Core Library [DUPLICATE] r=pietroalbini a=Ax9DTW Bors is stuck on #180, so this duplicate. 243: Automated pull from `rust-lang/libc` r=Veykril a=github-actions[bot] This PR pulls the following changes from the [`rust-lang/libc`](https://github.com/rust-lang/libc) repository: * `3570`: [[Backport #3548] Add ioctl FS_IOC_{G,S}{ETVERSION,ETFLAGS} for LoongArch64](https://www.github.com/rust-lang/libc/issues/3570) * `3553`: [Add MFD_NOEXEC_SEAL and MFD_EXEC](https://www.github.com/rust-lang/libc/issues/3553) * `3554`: [Backport of #3546 and update crate version to 0.2.153](https://www.github.com/rust-lang/libc/issues/3554) Co-authored-by: Johann Hemmann <johann.hemmann@code.berlin> Co-authored-by: Atri Sarkar <atri.sarkar@Atris-MacBook-Pro.local> Co-authored-by: Pietro Albini <pietro.albini@ferrous-systems.com> Co-authored-by: Gnana Ganesh <gnana.ganesh@thoughtworks.com> Co-authored-by: rusty-snake <41237666+rusty-snake@users.noreply.github.com> Co-authored-by: Guillaume Gomez <guillaume1.gomez@gmail.com> Co-authored-by: WANG Rui <wangrui@loongson.cn> Co-authored-by: Yuki Okushi <jtitor@2k36.org> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
Closing as #238 has been merged |
258: Generate coverage report for core library r=tshepang a=gnana-ganesh-tw This PR adds functionality to generate code coverage reports from the coverage data. The coverage data is generated by following the implementation of [this PR](#180). ## Usage To generate the coverage report: ```bash ./x.py run tools/generate-coverage-report ``` It is generated using the `grcov` utility. So you might have to install `grcov` before running this command. ```bash cargo install grcov ``` The `grcov` utility is needed to convert the raw coverage data to a human readable report. An `html` report is created in the following path `build/coverage_report`. ## Implementation A new tool named `generate-coverage-report` is added in the `ferrocene/tools`. This tool internally uses `grcov` to generate the report. `GenerateCoverageReport` build step is added in the `src/bootstrap/ferrocene/run.rs`. This checks if the `grcov` is already installed on the system and sets the environment variables with the required values to pass it to the `generate-coverage-report` tool. `LLVM_PROFILE_FILE` environment variable can be set with the path of the `.profraw` files when running `generate-coverage-report` tool. This is useful when the coverage data is generated in a different directory. ```bash LLVM_PROFILE_FILE= path/to/profraw/files ./x.py run tools/generate-coverage-report ``` ## Quirks - The report is generated based only on the tests in the `library/core` excluding `benches` and `doc` tests for now. - In the report, certain code region are incorrectly marked as uncovered. We believe this is due to an issue with `grcov`. So we might have to use `llvm-cov` to generate the report in the future. - There are certain lines in the source code that should not be reachable and cannot be tested. Hence it cannot be covered in the report. To address this, the tool looks for "COVR_EXCL_LINE" comment in the source code and ignores these lines in the report. For example, in the given code snippet the line is never reached, In this case, by adding the comment it is ignored in the coverage report. ```rust None => unsafe { hint::unreachable_unchecked() }, // COVR_EXCL_LINE ``` Edit: 1. Added note about using `LLVM_PROFILE_FILE` `@pietroalbini` `@tw-amit` `@Ax9DTW` 439: we now maintain the repo in-tree r=Veykril a=tshepang Closes #438 Should of been part of #348 Co-authored-by: Atri Sarkar <atri.sarkar@Atris-MacBook-Pro.local> Co-authored-by: Atri Sarkar <atri.sarkar@thoughtworks.com> Co-authored-by: Gnana Ganesh <gnana.ganesh@thoughtworks.com> Co-authored-by: Tshepang Mbambo <tshepang.mbambo@ferrous-systems.com>
This PR adds code coverage support for the
corelibrary using rustc's existing support for instrumentation using the-Cinstrument-coverageflag.Usage
To generate the coverage data run the command:
./x.py test --coverage library/core --no-doc--no-docflag has to be passed..profrawfiles are created in thebuild/coveragedirectory.The Problem
When the
-Cinstrument-coverageflag is set during compilation, llvm's profiler runtime is injected into the binary. This runtime is provided by theprofiler_builtinscrate and this crate is linked by making it an implicit dependency of the current crate being compiled.This creates a problem when we want to instrument the
corelibrary, as theprofiler_builtinscrate has a dependency on core and hence can't be built it, creating a cyclic dependency.Proposed Solution
Our approach is to make the dependency on
corelibrary optional inprofiler_builtins. This allows to build and use a different version of theprofiler_builtinscrate, without thecoredependency when instrumenting thecorelibrary.Notes
Removing
coredependency inprofiler_builtinshas certain limitations as discussed in this PR.Another approach to resolve the cylic dependency is to remove the
profiler_builtinscrate entirely and build the profiler runtime as a separate build step similar to sanitizers. Though this works it has certain issues as discussed in this PR.Implementation Details
The dependency on
coreis made optional and is enabled by default by setting a feature flag calledcoreinprofiler_builtins.In bootstrap, a custom step has been added to build
profiler_builtinsseparately under a different name with--no-default-featuresand hence without a dependency oncore.When gathering code coverage we link against this custom built
profiler_builtins.@pietroalbini @tw-amit @gnana-ganesh-tw
[EDITS]
Fix PR link