Skip to content

[POC] cmake: Introduce LLVM's Source-based Code Coverage reports #31394

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
Closed
wants to merge 4 commits into from
Closed

[POC] cmake: Introduce LLVM's Source-based Code Coverage reports #31394

wants to merge 4 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Nov 29, 2024 •
edited
Loading

The gcov-based code coverage does not work with Clang (please refer to #31047 for more details).

This PR employs the LLVM's Source-based Code Coverage.

Here are some examples of usage:

cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DBUILD_FOR_COVERAGE=ON
cmake --build build --target total_coverage
firefox build/test_bitcoin.coverage/index.html

or

cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DBUILD_FOR_COVERAGE=ON
cmake --build build --target total_coverage
firefox build/total.coverage/index.html

or

cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DBUILD_FOR_COVERAGE=ON -DBUILD_FOR_FUZZING=ON
cmake --build build --target fuzz_coverage
firefox build/fuzz.coverage/index.html

As a proof of concept, the new targets still lack the ability to customize llvm-cov options.

I haven't yet assessed the quality of the resulting coverage reports. However, messages like this:

warning: 502 functions have mismatched data

are quite concerning.

Also see a discussion in hebasto#233.

@hebasto
cmake: Introduce BUILD_FOR_COVERAGE build option
68108fa 
The new `BUILD_FOR_COVERAGE` build option enables instrumentation for
LLVM's Source-based Code Coverage reports.
@hebasto
cmake: Add test_bitcoin.coverage target
55014d1 
warning: 502 functions have mismatched data
- test_bitcoin: 2
- bitcoin-util: 329
- bitcoin-tx: 171
@hebasto
cmake: Add total_coverage target
30e25a7
@hebasto
cmake: Add fuzz_coverage target
e82cc06
@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 29, 2024 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31394.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #31884 (cmake: Make implicit libbitcoinkernel dependencies explicit by hebasto)
  • #31880 (cmake: Add optional source files to libraries directly by hebasto)
  • #31807 (kernel: Avoid duplicating symbols in the kernel and downstream users by theuni)
  • #31802 (Add bitcoin-{node,gui} to release binaries for IPC by Sjors)
  • #31741 (multiprocess: Add libmultiprocess git subtree by ryanofsky)
  • #31425 (RFC: Riscv bare metal CI job by TheCharlatan)
  • #31268 (cmake: add optional source files to bitcoin_crypto and crc32c directly by purpleKarrot)
  • #31161 (cmake: Set top-level target output locations by hebasto)
  • #30975 (ci: build multiprocess on most jobs by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot mentioned this pull request Dec 1, 2024
Merged
@dergoegge
Copy link
Member

Would we keep supporting generating coverage with gcc?

@theuni
Copy link
Member

theuni commented Dec 3, 2024

I haven't played with coverage in CMake much, but this seems like a more reasonable approach than the build type.

Would we keep supporting generating coverage with gcc?

I don't think we should keep both approaches. If we do want to continue to support gcc, IMO it should be migrated to this setting rather than keeping it as a build type.

But.. there's a lot of duplicated/hard-coded logic here. Can we not deduce how to filter/what to run based on target properties?

@DrahtBot DrahtBot mentioned this pull request Dec 5, 2024
Open
@roconnor-blockstream roconnor-blockstream mentioned this pull request Dec 6, 2024
Merged
@maflcko
Copy link
Member

maflcko commented Dec 13, 2024

Would we keep supporting generating coverage with gcc?

I don't think we should keep both approaches. If we do want to continue to support gcc, IMO it should be migrated to this setting rather than keeping it as a build type.

If gcc support is removed, it would be good to make sure https://github.com/corecheck/corecheck/blob/d15e92618a519e125758a6294e85ee9d6ce89265/workers/coverage-worker/entrypoint.sh#L50 and other places that currently rely on it are taken care of.

@DrahtBot DrahtBot mentioned this pull request Jan 10, 2025
Merged
@DrahtBot DrahtBot mentioned this pull request Feb 5, 2025
Draft
@maflcko maflcko mentioned this pull request Feb 10, 2025
Closed
@theuni theuni added this to the 29.0 milestone Feb 11, 2025
@sipa
Copy link
Member

sipa commented Feb 13, 2025

I suggest treating the broken GCC-based coverage as a 29.0 blocker, as it seems it's been effectively unusable anyway.

Adding an LLVM-based coverage mechanism then seems like an independent improvement.

@maflcko maflcko mentioned this pull request Feb 14, 2025
Open
@maflcko
Copy link
Member

maflcko commented Feb 14, 2025

I suggest treating the broken GCC-based coverage as a 29.0 blocker, as it seems it's been effectively unusable anyway.

I don't see why this should be a 29.0 blocker at all, or why GCC is unusable, given that it is used, for example, see the previous comment above: #31394 (comment)

Let's continue discussion in the thread #31047 (comment)?

@vasild
Copy link
Contributor

vasild commented Feb 14, 2025

Would we keep supporting generating coverage with gcc?

I don't think we should keep both approaches.

Why not? GCC is the native compiler on Linux, a widely used OS for developing Bitcoin Core.

This was referenced Feb 14, 2025
Merged
Merged
Closed
Merged
Merged
@maflcko maflcko removed this from the 29.0 milestone Feb 17, 2025
@maflcko
Copy link
Member

maflcko commented Feb 17, 2025

I've removed the milestone here for now, because this looks like it is adding a new feature that never existed before, so shouldn't be a blocker.

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

@vasild vasild mentioned this pull request Mar 4, 2025
Merged
@maflcko
Copy link
Member

maflcko commented Apr 2, 2025

What is the status here? This still needs rebase, if relevant, due to the conflict from February. Also, #31933 was merged.

@vasild
Copy link
Contributor

vasild commented Apr 16, 2025

My 2 sats:

  • It would be good to have this in cmake for convenience. Without it everybody will have to hack their own shell scripts to do the coverage.
  • As a build option (like in this PR), and not as a built type.
  • It should use gcc workflow if it detects that the compiler is gcc and clang workflow if the compiler is clang, but I am not willing to go through it again.

@maflcko
Copy link
Member

maflcko commented Apr 16, 2025

It would be good to have this in cmake for convenience. Without it everybody will have to hack their own shell scripts to do the coverage.

A good chunk of the time people will want to create coverage only for a subset of tests (or pass test options) (see #28772 (comment) or https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#deterministic-fuzz-coverage). Also, they may want to modify the coverage report generation.

Not sure how easy it is to support all that in cmake. It may be easier to just use a justfile with snippets for each part. However, a self-brewed script or bash alias seems most flexible.

(The same is conceptually true for running the functional tests, see #18816 (comment))

@hebasto hebasto closed this May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Build system Needs rebase Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hebasto @DrahtBot @dergoegge @theuni @maflcko @sipa @vasild