Skip to content

Code Coverage Support for Core Library#180

Closed
Ax9DTW wants to merge 13 commits intoferrocene:mainfrom
ThoughtWorksInc:ferrocene_libcore_coverage_pr
Closed

Code Coverage Support for Core Library#180
Ax9DTW wants to merge 13 commits intoferrocene:mainfrom
ThoughtWorksInc:ferrocene_libcore_coverage_pr

Conversation

@Ax9DTW
Copy link
Contributor

@Ax9DTW Ax9DTW commented Jan 2, 2024

This PR adds code coverage support for the core library using rustc's existing support for instrumentation using the -Cinstrument-coverage flag.

Usage

To generate the coverage data run the command:
./x.py test --coverage library/core --no-doc

  1. Currently coverage doesn't work with doc tests, hence the --no-doc flag has to be passed.
  2. The .profraw files are created in the build/coverage directory.

The Problem

When the -Cinstrument-coverage flag is set during compilation, llvm's profiler runtime is injected into the binary. This runtime is provided by the profiler_builtins crate 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 core library, as the profiler_builtins crate has a dependency on core and hence can't be built it, creating a cyclic dependency.

Note: profiler_builtins doesn't need to be dependent on core, the dependency is there to define a specific build order.

Proposed Solution

Our approach is to make the dependency on core library optional in profiler_builtins. This allows to build and use a different version of the profiler_builtins crate, without the core dependency when instrumenting the core library.

Notes

  1. Removing core dependency in profiler_builtins has certain limitations as discussed in this PR.

  2. Another approach to resolve the cylic dependency is to remove the profiler_builtins crate 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

  1. The dependency on core is made optional and is enabled by default by setting a feature flag called core in profiler_builtins.

  2. In bootstrap, a custom step has been added to build profiler_builtins separately under a different name with --no-default-features and hence without a dependency on core.

  3. When gathering code coverage we link against this custom built profiler_builtins.

@pietroalbini @tw-amit @gnana-ganesh-tw

[EDITS]
Fix PR link

Atri Sarkar and others added 8 commits December 22, 2023 11:05
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
Copy link
Contributor

@emilyalbini emilyalbini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to copy the files? Can't we just configure LLVM_PROFILE_FILE to point to the right path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Maybe add an inline comment explaining this?

Copy link
Contributor Author

@Ax9DTW Ax9DTW Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I've left a comment.

Atri Sarkar and others added 3 commits January 9, 2024 14:25
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>
@gnana-ganesh-tw gnana-ganesh-tw force-pushed the ferrocene_libcore_coverage_pr branch from 9fafd02 to 1438f13 Compare January 10, 2024 07:38
@gnana-ganesh-tw
Copy link
Contributor

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.

We have updated the PR as per the review comments.

@Ax9DTW Ax9DTW marked this pull request as ready for review January 19, 2024 09:32
@Ax9DTW Ax9DTW force-pushed the ferrocene_libcore_coverage_pr branch from b18c702 to 0793c8b Compare January 19, 2024 09:46
Copy link
Contributor

@emilyalbini emilyalbini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

bors merge

@bors-ferrocene
Copy link
Contributor

🕐 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.

@emilyalbini
Copy link
Contributor

@gnana-ganesh-tw could you force-push the branch again / push an empty commit (git commit --allow-empty -m "empty commit")? CI was not executed on the PR as it was a draft, and without the PR CI passing bors will not enqueue the PR for merging.

@Ax9DTW Ax9DTW force-pushed the ferrocene_libcore_coverage_pr branch from 0793c8b to 7db0d57 Compare January 25, 2024 13:55
@Ax9DTW
Copy link
Contributor Author

Ax9DTW commented Jan 25, 2024

Did a force push

bors-ferrocene bot added a commit that referenced this pull request Feb 1, 2024
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>
bors-ferrocene bot added a commit that referenced this pull request Feb 1, 2024
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>
@Ax9DTW
Copy link
Contributor Author

Ax9DTW commented Feb 1, 2024

Closing as #238 has been merged

@Ax9DTW Ax9DTW closed this Feb 1, 2024
bors-ferrocene bot added a commit that referenced this pull request Mar 22, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants