gcov
in MacOs and NixOs. This PR adds the steps to generate a coverage report based on the default llvm/clang tooling.
doc: Add Clang/LLVM based coverage report generation #31933
pull Prabhat1308 wants to merge 1 commits into bitcoin:master from Prabhat1308:pv/add_llvm_based_coverage changing 1 files +59 −0-
Prabhat1308 commented at 2:20 am on February 22, 2025: none
-
DrahtBot commented at 2:20 am on February 22, 2025: contributor
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/31933.
Reviews
See the guideline for information on the review process.
Type Reviewers ACK hodlinator, janb84 Concept ACK brunoerg, BrandonOdiwuor Stale ACK pablomartin4btc, vasild, jonatack If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
-
DrahtBot renamed this:
doc: Update documentation to include Clang/llvm based coverage report generation
doc: Update documentation to include Clang/llvm based coverage report generation
on Feb 22, 2025 -
DrahtBot added the label Docs on Feb 22, 2025
-
Prabhat1308 force-pushed on Feb 22, 2025
-
DrahtBot added the label CI failed on Feb 22, 2025
-
DrahtBot commented at 2:34 am on February 22, 2025: contributor
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/37639080376
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
-
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
-
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
-
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
-
-
DrahtBot removed the label CI failed on Feb 22, 2025
-
in doc/developer-notes.md:520 in b004a84e75 outdated
514@@ -513,6 +515,46 @@ To enable test parallelism: 515 cmake -DJOBS=$(nproc) -P build/Coverage.cmake 516 ``` 517 518+#### Using LLVM/Clang tooling 519+ 520+Default LLVM/Clang tooling can be used to generate coverage report for unit and functional tests.
maflcko commented at 10:28 am on February 22, 2025:ctest (below) only checks unit tests
Prabhat1308 commented at 1:31 pm on February 22, 2025:updated.in doc/developer-notes.md:575 in b004a84e75 outdated
553+ --output-dir=coverage_report \ 554+ --project-title="Bitcoin Core Coverage Report" 555+``` 556+ 557+The coverage report will be generated in the `coverage_report` directory inside the `build` directory. 558 ### Performance profiling with perf
maflcko commented at 10:28 am on February 22, 2025:missing newline?
Prabhat1308 commented at 1:31 pm on February 22, 2025:added the newlinemaflcko approvedmaflcko commented at 10:29 am on February 22, 2025: memberlgtmPrabhat1308 force-pushed on Feb 22, 2025in doc/developer-notes.md:567 in 6436bdb301 outdated
549+ --format=html \ 550+ --show-instantiation-summary \ 551+ --show-line-counts-or-regions \ 552+ --show-expansions \ 553+ --output-dir=coverage_report \ 554+ --project-title="Bitcoin Core Coverage Report"
jonatack commented at 6:59 pm on February 22, 2025:Following these instructions (macos 15.3.1 m1 max), this command failed for me with
error: coverage.profdata: could not read profile data!No such file or directory
.I then ran
rm -rf ./build/
and re-ran the instructions. Now seeing:warning: 2 functions have mismatched data
.Haven’t looked further yet.
I usually build with both
rm -rf ./build/
and-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++"
, among other flags, so I may need to try that.
Prabhat1308 commented at 11:28 pm on February 22, 2025:The first error never occurs if I always remove my
build
folder and then proceeding with the commands specified.For MacOs (15.3.1 M4 pro) , I have llvm 19 which is also a known issue so I generally have to specify my commands with this to use llvm 18.
0cmake -B build \ 1 -DCMAKE_C_COMPILER="$(brew --prefix llvm@18)/bin/clang" \ 2 -DCMAKE_CXX_COMPILER="$(brew --prefix llvm@18)/bin/clang++" \ 3 -DCMAKE_C_FLAGS="-fprofile-instr-generate -fcoverage-mapping" \ 4 -DCMAKE_CXX_FLAGS="-fprofile-instr-generate -fcoverage-mapping" \ 5 -DAPPEND_LDFLAGS="-Wl,-no_warn_duplicate_libraries" \
and also replace
llvm-profdata
with/opt/homebrew/opt/llvm@18/bin/llvm-profdata
andllvm-cov
with/opt/homebrew/opt/llvm@18/bin/llvm-cov
I do get the warning . The warning can be because of many reasons like Link-Time Optimisations , multiple defined compilations units etc. which results in different profile data. Its harmless and can be ignored.
maflcko commented at 8:50 am on February 24, 2025:For MacOs (15.3.1 M4 pro) , I have llvm 19 which is also a known issue so I generally have to specify my commands with this to use llvm 18.
Is this issue specific to building the fuzz tests, or just a general issue? From your command, it seems like it is a general issue. In this case, I wonder if it makes sense to mention here that all binaries (
clang
,clang++
,llvm-cov
,llvm-profdata
) will have to be called via$(brew --prefix llvm@18)/bin/__binary_name__
?
pablomartin4btc commented at 3:16 pm on February 27, 2025:Its harmless and can be ignored.
I got it in both Ubuntu and macOS. Perhaps we can document it as well to avoid confusion?
0$ find build -name "default_*.profraw" | xargs llvm-profdata merge -sparse -o build/coverage.profdata 1build/src/univalue/default_44199.profraw: main: function basic block count change detected (counter mismatch) 2Make sure that all profile data to be merged is generated from the same binary. 3$ ll build/coverage.profdata 4... 15926688 feb 27 09:44 build/coverage.profdata 5$ llvm-cov show build/src/test/test_bitcoin \ 6 --instr-profile=build/coverage.profdata \ 7 --format=html \ 8 --show-instantiation-summary \ 9 --show-line-counts-or-regions \ 10 --show-expansions \ 11 --output-dir=build/coverage_report \ 12 --project-title="Bitcoin Core Coverage Report" 13warning: 2 functions have mismatched data
Prabhat1308 commented at 4:09 pm on February 27, 2025:I got it in both Ubuntu and macOS. Perhaps we can document it as well to avoid confusion?
0$ find build -name "default_*.profraw" | xargs llvm-profdata merge -sparse -o build/coverage.profdata 1build/src/univalue/default_44199.profraw: main: function basic block count change detected (counter mismatch) 2Make sure that all profile data to be merged is generated from the same binary.
Have you tried building with clang 19 ? I dont get this counter mismatch on MacOs.
0╰─ find build -name "default_*.profraw" | xargs llvm-profdata merge -sparse -o build/coverage.profdata 1 2 3╭────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────── ✔ 21:32:24 4 llvm-cov show build/src/test/test_bitcoin \ 5 --instr-profile=build/coverage.profdata \ 6 --format=html \ 7 --show-instantiation-summary \ 8 --show-line-counts-or-regions \ 9 --show-expansions \ 10 --output-dir=build/coverage_report \ 11 --project-title="Bitcoin Core Coverage Report" 12warning: 2 functions have mismatched data
pablomartin4btc commented at 6:06 pm on February 27, 2025:Ok, I had 18.1.8, updated it to 19, rerun the merge and I can confirm that the “counter mismatch” has gone. nit: Please consider adding a note there regarding the message and the warning with the 2 functions mismatch, any of those didn’t affect the generation of the report nor its visualisation. Thanks!
Prabhat1308 commented at 9:56 pm on February 27, 2025:Added the notes under the commands.jonatack commented at 6:59 pm on February 22, 2025: memberConcept ACKbrunoerg commented at 2:33 pm on February 23, 2025: contributorConcept ACKin doc/developer-notes.md:538 in 6436bdb301 outdated
533+Generating the raw profile data based on `ctest` execution: 534+ 535+```shell 536+cd build 537+export LLVM_PROFILE_FILE="default_%p.profraw" 538+ctest # Use "-j N" here for N parallel jobs.
hodlinator commented at 9:33 am on February 24, 2025:I prefer:
- Keeping my shell in the root of the repo
- Not accumulating lingering environment variables
How about:
0LLVM_PROFILE_FILE="default_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs.
(
ctest --test-dir build
is also how we recommend running tests in a bunch of places).It would also require this below:
find build -name "default_*.profraw"
llvm-cov show build/src/test/test_bitcoin
Might also want to change
--output-dir
forllvm-cov show
to point into the build/ directory.in doc/developer-notes.md:520 in 6436bdb301 outdated
514@@ -513,6 +515,47 @@ To enable test parallelism: 515 cmake -DJOBS=$(nproc) -P build/Coverage.cmake 516 ``` 517 518+#### Using LLVM/Clang tooling 519+ 520+Default LLVM/Clang tooling can be used to generate coverage report for unit tests.
hodlinator commented at 9:58 am on February 24, 2025:What do you mean by “Default” here? Are your distinguishing from “XCode-special-sauce-LLVM/Clang”? I would have referred to it as “mainline” or “vanilla” in that case.
Prabhat1308 commented at 4:52 pm on February 25, 2025:What I meant here was thatclang/llvm
comes installed by default in the machines (although the entire llvm suite might not be there).lcov
generally has to be downloaded before using in almost every machine.
hodlinator commented at 10:27 am on February 26, 2025:Replying to #31933 (review): I see. According to doc/dependencies.md, one only needs to have one of the compilers installed. I think it’s clearer to remove the “Default” qualifier and needless to repeat the name of the toolchain.
0The following generates a coverage report for unit tests.
hodlinator commented at 10:08 am on February 24, 2025: contributorConcept ACK 6436bdb301278abb3526eab2138b2f23f8905b5ePrabhat1308 force-pushed on Feb 25, 2025Prabhat1308 force-pushed on Feb 25, 2025Prabhat1308 commented at 4:49 pm on February 25, 2025: noneAddressed the comments
- Change the commands so we dont have to change shell from the root of the repo.
Added a comment for MacOs users to specify the command changes needed to use different llvm version if faced with issues.Just noticed that I had the fuzz flags in my.zshrc
which were interfering with llvm version creating the profile data .llvm
version 19 works fine now . I have removed the earlier MacOs comment and replaced it with the flags specified by @jonatack above.
Prabhat1308 force-pushed on Feb 25, 2025Prabhat1308 force-pushed on Feb 25, 2025Prabhat1308 requested review from hodlinator on Feb 25, 2025in doc/developer-notes.md:518 in 5f7b42642a outdated
514@@ -513,6 +515,45 @@ To enable test parallelism: 515 cmake -DJOBS=$(nproc) -P build/Coverage.cmake 516 ``` 517 518+#### Using LLVM/Clang tooling
hodlinator commented at 10:27 am on February 26, 2025:nit: Could use more frequently used term:
0#### Using LLVM/Clang toolchain
in doc/developer-notes.md:534 in 5f7b42642a outdated
526+ -DCMAKE_CXX_COMPILER="clang++" \ 527+ -DCMAKE_C_FLAGS="-fprofile-instr-generate -fcoverage-mapping" \ 528+ -DCMAKE_CXX_FLAGS="-fprofile-instr-generate -fcoverage-mapping" 529+ # for MacOS users , replace -DCMAKE_CXX_COMPILER="clang++" with -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" and -DCMAKE_C_COMPILER="clang" with -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" 530+cmake --build build # Use "-j N" here for N parallel jobs. 531+```
hodlinator commented at 10:39 am on February 26, 2025:No sure about the random 2-space indentation, also prefer comments preceding the command they’re about.
0```shell 1# MacOS may require -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" and -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" 2cmake -B build -DCMAKE_C_COMPILER="clang" \ 3 -DCMAKE_CXX_COMPILER="clang++" \ 4 -DCMAKE_C_FLAGS="-fprofile-instr-generate -fcoverage-mapping" \ 5 -DCMAKE_CXX_FLAGS="-fprofile-instr-generate -fcoverage-mapping" 6cmake --build build # Use "-j N" here for N parallel jobs. 7```
in doc/developer-notes.md:555 in 5f7b42642a outdated
550+ --show-expansions \ 551+ --output-dir=build/coverage_report \ 552+ --project-title="Bitcoin Core Coverage Report" 553+``` 554+ 555+The coverage report will be generated in the `coverage_report` directory inside the `build` directory.
hodlinator commented at 10:49 am on February 26, 2025:nit: A bit verbose to repeat “directory”, more helpful to include index.html?
0The generated coverage report can be accessed at `build/coverage_report/index.html`.
hodlinator commented at 10:59 am on February 26, 2025: contributorReviewed 5f7b42642ad83faf23c8e75511f2379f9eae25ad
- Re-testing the commands.
- Managed to generate coverage for a functional tests, not sure if we should include it, includes copy-pasting temporary paths for the root test directory.
janb84 commented at 11:17 am on February 27, 2025: contributorConcept ACK 5f7b426
Using the new instructions of the document on MacOS using Nix-shell (so using the non mac instructions) I do endup gettting the warning that was mentioned before: “warning: 2 functions have mismatched data”
The rapport is still generated.
Tested both under clang 19.1.7 / 18.1.8 and LLVM 19.1.7 / 18.1.8
This is an improvement because I cannot get the “old way” of coverage generation to work!
Prabhat1308 force-pushed on Feb 27, 2025Prabhat1308 commented at 2:22 pm on February 27, 2025: none- Managed to generate coverage for a functional tests, not sure if we should include it, includes copy-pasting temporary paths for the root test directory.
I think adding the coverage generation for functional tests and fuzz tests (which I believe everybody does using llvm toolchain only ?) can be added in another PR .
DrahtBot requested review from hodlinator on Feb 27, 2025DrahtBot requested review from jonatack on Feb 27, 2025DrahtBot requested review from brunoerg on Feb 27, 2025pablomartin4btc approvedpablomartin4btc commented at 3:45 pm on February 27, 2025: membertACK 05605dae2983c11f0ba662dbdc495f84de12724b
Maybe we could advice the user about the warnings, left a comment there.
BrandonOdiwuor commented at 8:24 pm on February 27, 2025: contributorConcept ACKin doc/developer-notes.md:484 in 05605dae29 outdated
483@@ -484,6 +484,8 @@ $ ./build/test/functional/test_runner.py --valgrind 484
hodlinator commented at 8:38 pm on February 27, 2025:I just realized the PR description shouldn’t contain the @-symbol. See:
Prabhat1308 commented at 9:04 pm on February 27, 2025:Removed the @ symbolDrahtBot requested review from hodlinator on Feb 27, 2025Prabhat1308 force-pushed on Feb 27, 2025hodlinator approvedhodlinator commented at 8:00 pm on March 1, 2025: contributorACK 1245e05325b41101eddc76ba9214d910489e35b6
- Good to include some comments on expected warnings (I was getting some too).
- Thanks for incorporating my latest feedback!
- Agree that functional/fuzz test instructions could either be a separate PR or left as an exercise to the reader.
DrahtBot requested review from janb84 on Mar 1, 2025DrahtBot requested review from pablomartin4btc on Mar 1, 2025DrahtBot requested review from BrandonOdiwuor on Mar 1, 2025in doc/developer-notes.md:517 in 1245e05325 outdated
514@@ -513,6 +515,49 @@ To enable test parallelism: 515 cmake -DJOBS=$(nproc) -P build/Coverage.cmake 516 ``` 517
hodlinator commented at 8:06 pm on March 1, 2025:nit: Could shorten PR title. (Doesn’t invalidate ACKs).
0-doc: Update documentation to include Clang/llvm based coverage report generation 1+doc: Add Clang/LLVM based coverage report generation
hodlinator commented at 7:52 am on March 26, 2025:Thread https://github.com/bitcoin/bitcoin/pull/31933/files#r1976483663: @Prabhat1308 what do you think about shrinking the title?in doc/developer-notes.md:549 in 1245e05325 outdated
538+# merge all the raw profile data into a single file 539+find build -name "default_*.profraw" | xargs llvm-profdata merge -sparse -o build/coverage.profdata 540+# Note: The "counter mismatch" warning can be safely ignored, though it can be resolved by updating to Clang 19. 541+# The warning occurs due to version mismatches but doesn't affect the coverage report generation. 542+``` 543+
janb84 commented at 8:20 pm on March 1, 2025:Small nit, creating a blockquote note remark section is maybe more fitting:
Note: The “counter mismatch” warning can be safely ignored, though it can be resolved by updating to Clang 19.
The warning occurs due to version mismatches but doesn’t affect the coverage report generation.in doc/developer-notes.md:529 in 1245e05325 outdated
524+```shell 525+# MacOS may require -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" and -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" 526+cmake -B build -DCMAKE_C_COMPILER="clang" \ 527+ -DCMAKE_CXX_COMPILER="clang++" \ 528+ -DCMAKE_C_FLAGS="-fprofile-instr-generate -fcoverage-mapping" \ 529+ -DCMAKE_CXX_FLAGS="-fprofile-instr-generate -fcoverage-mapping"
vasild commented at 10:06 am on March 4, 2025:This defaults to
CMAKE_BUILD_TYPE=RelWithDebInfo
which uses-O2
. Using a debugger on an optimized binary usually results in bogus line numbers due to optimizations that can’t be mapped back to source code lines. I do not know if it is the same with coverage.To be on the safe side I have been using
-DCMAKE_BUILD_TYPE=Debug
(which uses-O0
). Also, I don’t see a reason to use-O2
, other than to have the tests finish faster.The example in https://clang.llvm.org/docs/UsersManual.html#profiling-with-instrumentation uses
-O2
:0clang++ -O2 -fprofile-instr-generate code.cc -o code
vasild commented at 10:36 am on March 4, 2025:This works, but maybe it is better to use the
APPEND_*
alternatives?i.e.
0 -DAPPEND_CFLAGS="-fprofile-instr-generate -fcoverage-mapping" \ 1 -DAPPEND_CXXFLAGS="-fprofile-instr-generate -fcoverage-mapping"
If you decide to do that, then you will also need
-DAPPEND_LDFLAGS="-fprofile-instr-generate -fcoverage-mapping"
becauseCMAKE_CXX_FLAGS
causes the flags to be used during linking as well, butAPPEND_CXXFLAGS
does not.
maflcko commented at 8:46 am on March 13, 2025:Why change to
Debug
when there is no reason? The docs literally say in the first line:This document explains how to use clang’s source-based code coverage feature. It’s called “source-based” because it operates on AST and preprocessor information directly. This allows it to generate very precise coverage data.
https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#introduction
If it doesn’t work, at a minimum, you should provide exact steps to reproduce, not add workarounds for bugs that don’t exist.
ldionne commented at 3:17 pm on March 18, 2025:Sadly, I am not very familiar with how code coverage works in Clang (or other compilers), so I am not the best person to answer that question. However, as a general rule of thumb, I would encourage you to go with what the documentation says, and if you then notice shortcomings, report those as bugs against the project.
vasild commented at 6:02 pm on March 18, 2025:The docs at https://clang.llvm.org/docs/SourceBasedCodeCoverage.html don’t mention anything about-O
flags.
maflcko commented at 8:24 pm on March 18, 2025:don’t mention anything about
-O
flags.That is because they don’t matter, as explained in the first line of the docs and in #31933 (review).
I also confirmed this locally and couldn’t find any “bogus line numbers”.
If you find any bugs, it would be good to provide exact steps to reproduce, so that the bug can be reported upstream and fixed.
vasild commented at 11:21 am on March 19, 2025:I don’t see how from “source-based … operates on AST and preprocessor information” follows that
-O
don’t matter.I did coverage reports locally with
Debug
(-O0
) andRelease
(-O2
) and compared them. They are slightly different, which could be due to non-determinism in the tests.src/sync.cpp
is completely missing from the-O2
report.Time to complete the tests:
test Debug Release unit 60s 31s functional 186s 76s
ldionne commented at 1:52 pm on March 19, 2025:The
-O
flag controls optimization level, which shouldn’t have any impact on the AST representation of the code. The compiler builds an AST representation of the code that corresponds to the semantics of the program, and from that generates LLVM IR that implements those semantics. This is then passed on to the optimizer, which performs transformations on the LLVM IR representation (not the AST directly) to produce equivalent but more efficient LLVM IR.This is much simplified, but if code coverage is implemented based on the AST representation, then I do agree that
-O
flags shouldn’t matter. That being said, I have a hard time imagining how each line in the final IR can always be attributed to the correct line in the original sources in the presence of optimizations like inlining and especially outlining, where the same executable code may be used to implement source-different functions. But if the documentation says it, I would go with that.
vasild commented at 2:08 pm on March 19, 2025:@ldionne, that is very useful. Thanks for the clarification! With my limited knowledge I was thinking that maybe AST is generated after optimization.
Just to clarify on this particular issue - I am fine with whatever
CMAKE_BUILD_TYPE=
or omitting it and leaving the default. This is only a documentation after all. People can tweak the commands as they wish before executing them.
maflcko commented at 3:00 pm on March 19, 2025:I did coverage reports locally with
Debug
(-O0
) andRelease
(-O2
) and compared them. They are slightly different, which could be due to non-determinism in the tests.src/sync.cpp
is completely missing from the-O2
report.I can’t reproduce this locally. For me, sync.cpp is missing from both reports and -O0 and -O2 have no effect on it missing. So this could be a bug, but I don’t think optimization has anything to do with that.
in doc/developer-notes.md:536 in 1245e05325 outdated
531+``` 532+ 533+Generating the raw profile data based on `ctest` execution: 534+ 535+```shell 536+LLVM_PROFILE_FILE="default_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs.
vasild commented at 12:20 pm on March 4, 2025:Using
%m
will help reduce the chance of.profraw
files being overwritten in case the same PID is reused by the OS:0LLVM_PROFILE_FILE="default_%m_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs.
https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#running-the-instrumented-program mentions:
For a program such as the Lit testing tool which invokes other programs, it may be necessary to set LLVM_PROFILE_FILE for each invocation. The pattern strings “%p” or “%Nm” may help to avoid corruption due to concurrency.
in doc/developer-notes.md:539 in 1245e05325 outdated
534+ 535+```shell 536+LLVM_PROFILE_FILE="default_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs. 537+ 538+# merge all the raw profile data into a single file 539+find build -name "default_*.profraw" | xargs llvm-profdata merge -sparse -o build/coverage.profdata
vasild commented at 12:24 pm on March 4, 2025:Why
-sparse
?0 --sparse[=true|false] 1 Do not emit function records with 0 execution count. Can only be 2 used in conjunction with -instr. Defaults to false, since it can 3 inhibit compiler optimization during PGO.
hodlinator commented at 8:08 am on March 26, 2025:Thread #31933 (review):
With
-sparse
:0warning: 16 functions have mismatched data
Without
-sparse
(current version of PR):0warning: 65 functions have mismatched data
in doc/developer-notes.md:545 in 1245e05325 outdated
532+ 533+Generating the raw profile data based on `ctest` execution: 534+ 535+```shell 536+LLVM_PROFILE_FILE="default_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs. 537+
vasild commented at 1:04 pm on March 4, 2025:I would suggest collecting coverage from functional tests as well because that would change a bit the arguments of
llvm-cov
(see below) and because the path to the profile file has to be specified as an absolute path which is not obvious:0Generating the raw profile data based on `ctest` execution: 1 2```shell 3LLVM_PROFILE_FILE=$(pwd)/build/raw_profile_data/%m_%p.profraw build/test/functional/test_runner.py 4`` `
it has to be a full path because otherwise the
.profraw
file will be created inside the temporary directory, like/tmp/test_runner_₿_🏃_20250304_134152/...
and will be removed when the temporary directory is deleted at the end of the tests.PS: if you go with that, then maybe it would be better to also store
.profraw
files from unit tests into$(pwd)/build/raw_profile_data/
so that all are in one place.in doc/developer-notes.md:567 in 1245e05325 outdated
549+ --format=html \ 550+ --show-instantiation-summary \ 551+ --show-line-counts-or-regions \ 552+ --show-expansions \ 553+ --output-dir=build/coverage_report \ 554+ --project-title="Bitcoin Core Coverage Report"
vasild commented at 1:40 pm on March 4, 2025:If there are profile files from more than one executable, like from
test_bitcoin
and frombitcoind
then the syntax of this changes a little bit: dropbuild/src/test/test_bitcoin
and add-object=build/src/test/test_bitcoin -object=build/src/bitcoind
:0llvm-cov show 1 --object=build/src/test/test_bitcoin \ 2 --object=build/src/bitcoind \ 3 --instr-profile=build/coverage.profdata \ 4 --format=html \ 5 --show-instantiation-summary \ 6 --show-line-counts-or-regions \ 7 --show-expansions \ 8 --output-dir=build/coverage_report \ 9 --project-title="Bitcoin Core Coverage Report"
in doc/developer-notes.md:560 in 1245e05325 outdated
543+ 544+Generating the coverage report: 545+ 546+```shell 547+llvm-cov show build/src/test/test_bitcoin \ 548+ --instr-profile=build/coverage.profdata \
vasild commented at 1:43 pm on March 4, 2025:Locally, I have been using this as well:
0-ignore-filename-regex="src/crc32c/|src/leveldb/|src/minisketch/|src/secp256k1/|src/test/"
Prabhat1308 force-pushed on Mar 13, 2025Prabhat1308 commented at 2:04 am on March 13, 2025: noneAddressing the review from comment
- The coverage now includes functional tests
- Removed coverage for external deps (crc32,leveldb,minisketch,secp256k1) and
src/test
. - Now uses
-DCMAKE_BUILD_TYPE=Debug
- Notes are now in Blockquotes
- Incuding the functional tests has increased the mismatch from 2 to 61 .
in doc/developer-notes.md:534 in 7f57ad00d3 outdated
529+ -DAPPEND_CXXFLAGS="-fprofile-instr-generate -fcoverage-mapping" \ 530+ -DAPPEND_LDFLAGS="-fprofile-instr-generate -fcoverage-mapping" 531+cmake --build build # Use "-j N" here for N parallel jobs. 532+``` 533+ 534+Generating the raw profile data based on `ctest` execution:
pablomartin4btc commented at 0:22 am on March 14, 2025:small nit:
0Generating the raw profile data based on `ctest` and functional tests execution:
Or remove both and clarify it below before each execution (or any alternative you might have).
pablomartin4btc commented at 0:38 am on March 14, 2025: memberACK 7f57ad00d301b9564ece511d2e99cb0678b21174
I think the feedback about
CMAKE_BUILD_TYPE
needs to be addressed.Since the functional tests were added (for the report generation), is it worth adding the
fuzz
?DrahtBot requested review from vasild on Mar 14, 2025DrahtBot requested review from janb84 on Mar 14, 2025DrahtBot requested review from hodlinator on Mar 14, 2025in doc/developer-notes.md:542 in 7f57ad00d3 outdated
537+# Create directory for raw profile data 538+mkdir -p $(pwd)/build/raw_profile_data 539+ 540+# Run tests to generate profiles 541+LLVM_PROFILE_FILE="$(pwd)/build/raw_profile_data/%m_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs. 542+LLVM_PROFILE_FILE="$(pwd)/build/raw_profile_data/%m_%p.profraw" build/test/functional/test_runner.py
vasild commented at 11:03 am on March 14, 2025:nit:test_runner.py
also supports-j N
for parallel jobs.in doc/developer-notes.md:556 in 7f57ad00d3 outdated
551+Generating the coverage report: 552+ 553+```shell 554+llvm-cov show \ 555+ --object=build/src/test/test_bitcoin \ 556+ --object=build/src/bitcoind \
vasild approvedvasild commented at 11:20 am on March 14, 2025: contributorAlmost ACK 7f57ad00d301b9564ece511d2e99cb0678b21174, just fix the path to the binaries which were recently relocated from
src
tobin
.I would be happy to review an extension to this that also does the fuzz part. That would involve compiling into another directory for fuzzing, running the fuzz tests, gathering their
.profraw
files as well and adding--object=buildfuzz/bin/fuzz
tollvm-cov
.Prabhat1308 force-pushed on Mar 16, 2025Prabhat1308 commented at 12:34 pm on March 16, 2025: noneAddressed the comments to fix nits. On the use of
-DEBUG
, I haven’t been able to do a comparison from lcov generated reports as I don’t have any other system . Since-02
introduces optimisation , I have chosen to keep the-DEBUG
flag to use-00
as mentioned here to keep it close to gcov reports. https://stackoverflow.com/questions/36930207/code-coverage-with-optimizationit seems if I want to collect coverage information with tools like gcov, any optimization flag must be disabled
Further comments and Answers also mention using
-00
.maflcko commented at 1:26 pm on March 16, 2025: membergcov
This pull isn’t about gcov from GCC, but about clang/llvm-based coverage reports. It seems odd to apply a pessimising workaround from a different tool without documentation. If there is an issue that requires the workaround, it should be trivial to find exact steps to reproduce, or at least a note in the official docs: https://clang.llvm.org/docs/SourceBasedCodeCoverage.html
in doc/developer-notes.md:531 in 3c69b071ac outdated
524+```shell 525+# MacOS may require -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" and -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" 526+cmake -B build -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER="clang" \ 527+ -DCMAKE_CXX_COMPILER="clang++" \ 528+ -DAPPEND_CFLAGS="-fprofile-instr-generate -fcoverage-mapping" \ 529+ -DAPPEND_CXXFLAGS="-fprofile-instr-generate -fcoverage-mapping" \
janb84 commented at 12:08 pm on March 17, 2025:0 -DAPPEND_CFLAGS="-fprofile-instr-generate -fcoverage-mapping -g" \ 1 -DAPPEND_CXXFLAGS="-fprofile-instr-generate -fcoverage-mapping -g" \
NIT: In Debug builds, the compiler usually disables optimizations, but without explicit -g, some optimizations might still occur. Given that the code coverage results are indeed different when running without
-g
and with-g
, I would say some optimization still occurs interfering with the code coverage results.
vasild commented at 3:15 pm on March 17, 2025:-DCMAKE_BUILD_TYPE=Debug
adds-g
already.
janb84 commented at 4:11 pm on March 17, 2025:-DCMAKE_BUILD_TYPE=Debug
adds-g
already.Why is there a difference in the reports when manually adding the -g ?
Edit: I see that the flag is added, but I cannot explain why I keep getting different outcomes of the coverage. Still searching :)
janb84 commented at 7:30 pm on March 17, 2025:The extra-g
just gives a lot ofUnexecuted instantiation:
messages and a lot lower coverage. No sure why but don’t think this gives a good image of the actual coverage.janb84 commented at 12:40 pm on March 17, 2025: contributorWhile re-testing this PR, I found that adding theDisproven it gives a lot of-g
flag (debug symbols) impacts code coverage reports. With-g
, more source lines are mapped, lowering the coverage percentage by revealing untested code that’s hidden without it. I tested this with and without-DCMAKE_BUILD_TYPE=Debug
, and both setups give a different result.Unexecuted instantiation:
errors.I agree with the rest of the PR changes. ACK 3c69b07
vasild approvedvasild commented at 3:14 pm on March 17, 2025: contributorACK 3c69b071ac4964870abb347f9d9c15df2e60aa62
Thank you!
DrahtBot requested review from janb84 on Mar 17, 2025DrahtBot requested review from pablomartin4btc on Mar 17, 2025in doc/developer-notes.md:525 in 3c69b071ac outdated
520+The following generates a coverage report for unit tests and functional tests. 521+ 522+Configure the build with the following flags: 523+ 524+```shell 525+# MacOS may require -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" and -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang"
jonatack commented at 3:59 pm on March 17, 2025:This would be easier to understand and copy into the config command.
0# MacOS may require `-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++"` instead of `-DCMAKE_C_COMPILER="clang" -DCMAKE_CXX_COMPILER="clang++"` if the compiler was installed using brew.
Maybe also place this comment after the command, instead of before it.
Prabhat1308 commented at 9:30 pm on March 25, 2025:Added , although I prefer it being before the commands then after it.
hodlinator commented at 8:16 am on March 26, 2025:Thread #31933 (review):
nit: Would prefer not repeating the text below, and think mentioning brew is overly verbose, adding “instead” is sufficient IMO:
0# MacOS may instead require `-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++"`
in doc/developer-notes.md:526 in 3c69b071ac outdated
521+ 522+Configure the build with the following flags: 523+ 524+```shell 525+# MacOS may require -DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" and -DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" 526+cmake -B build -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER="clang" \
jonatack commented at 4:00 pm on March 17, 2025:Directory name: would it make sense to name the
build
directory in the instructions as, say,build_cov
instead?I think we’re already using directory naming like
build_fuzz
(indoc/fuzzing.md
) andbuild_dev_mode
(inCMakePresets.json
).
Prabhat1308 commented at 9:30 pm on March 25, 2025:Decided against it since the previous docs built the coverage inbuild
directory .in doc/developer-notes.md:573 in 3c69b071ac outdated
565+``` 566+ 567+> **Note:** The "functions have mismatched data" warning can be safely ignored, the coverage report will still be generated correctly despite this warning. 568+> This warning occurs due to profdata mismatch created during the merge process for shared libraries. 569+ 570+The generated coverage report can be accessed at `build/coverage_report/index.html`.
jonatack commented at 4:06 pm on March 17, 2025:0The generated coverage report is located at `build/coverage_report/index.html`. Running `open build/coverage_report/index.html` will open it in your browser.
hodlinator commented at 8:19 am on March 26, 2025:Thread #31933 (review):
open
is MacOS-specific AFAIK. I don’t have it on my Linux install. Also overly repetitive/verbose IMO.in doc/developer-notes.md:522 in 3c69b071ac outdated
514@@ -513,6 +515,60 @@ To enable test parallelism: 515 cmake -DJOBS=$(nproc) -P build/Coverage.cmake 516 ``` 517 518+#### Using LLVM/Clang toolchain 519+ 520+The following generates a coverage report for unit tests and functional tests. 521+ 522+Configure the build with the following flags:
jonatack commented at 4:10 pm on March 17, 2025:Should we mention clearing out withrm -rf build
to begin with a clean slate?jonatack commented at 4:11 pm on March 17, 2025: memberACK 3c69b071ac4964870abb347f9d9c15df2e60aa62 modulo feedback belowin doc/developer-notes.md:557 in 3c69b071ac outdated
550+ 551+Generating the coverage report: 552+ 553+```shell 554+llvm-cov show \ 555+ --object=build/bin/test_bitcoin \
vasild commented at 10:09 am on March 19, 2025:I forgot to mention one more option that is useful here:
-Xdemangler=llvm-cxxfilt
. Without that option I get:0Unexecuted instantiation: addrman_tests.cpp:_ZL17LogAcceptCategoryN5BCLog8LogFlagsENS_5LevelE
and with that option:
0Unexecuted instantiation: addrman_tests.cpp:LogAcceptCategory(BCLog::LogFlags, BCLog::Level)
ryanofsky commented at 8:13 pm on March 24, 2025: contributor@Prabhat1308 this seems ready to merge but also has a few recent comments that haven’t been responded to. You can let me know if you’d like this to be merged or if you want to make more changes.Prabhat1308 force-pushed on Mar 25, 2025Prabhat1308 commented at 9:29 pm on March 25, 2025: none- Following the discussion , I have removed the
-DEBUG
flag . - Added
-Xdemangler=llvm-cxxfilt
option for more readable outputs. - Added the note to consider with a clean state to avoid these issues #31933 (review)
- Added some suggestions by @jonatack
Prabhat1308 commented at 9:32 pm on March 25, 2025: none@ryanofsky I have addressed the remaining comments and these should be my last changes (if everyone agrees)in doc/developer-notes.md:540 in 3b5d00f65a outdated
535+ 536+Generating the raw profile data based on `ctest` and functional tests execution: 537+ 538+```shell 539+# Create directory for raw profile data 540+mkdir -p $(pwd)/build/raw_profile_data
hodlinator commented at 8:20 am on March 26, 2025:$(pwd)
is redundant formkdir
:0mkdir -p build/raw_profile_data
Prabhat1308 commented at 8:46 am on March 26, 2025:done.in doc/developer-notes.md:547 in 3b5d00f65a outdated
542+# Run tests to generate profiles 543+LLVM_PROFILE_FILE="$(pwd)/build/raw_profile_data/%m_%p.profraw" ctest --test-dir build # Use "-j N" here for N parallel jobs. 544+LLVM_PROFILE_FILE="$(pwd)/build/raw_profile_data/%m_%p.profraw" build/test/functional/test_runner.py # Use "-j N" here for N parallel jobs 545+ 546+# merge all the raw profile data into a single file 547+find build -name "*.profraw" | xargs llvm-profdata merge -o build/coverage.profdata
hodlinator commented at 8:22 am on March 26, 2025:nit: Could be more precise, use leading Capital letter like other comments.
0# Merge all the raw profile data into a single file 1find build/raw_profile_data -name "*.profraw" | xargs llvm-profdata merge -o build/coverage.profdata
Prabhat1308 commented at 8:46 am on March 26, 2025:done.hodlinator commented at 8:26 am on March 26, 2025: contributorReviewed/tested 3b5d00f65ae37b733db74d9c9c905eb35e7363e5add clang/llvm based coverage report generation
Signed-off-by: Prabhat Verma <prabhatverma329@gmail.com>
Prabhat1308 force-pushed on Mar 26, 2025Prabhat1308 renamed this:
doc: Update documentation to include Clang/llvm based coverage report generation
doc: Add Clang/LLVM based coverage report generation
on Mar 26, 2025Prabhat1308 commented at 8:48 am on March 26, 2025: noneAddressed the reviews by @hodlinatorhodlinator approvedhodlinator commented at 9:08 am on March 26, 2025: contributorre-ACK b96f1a696aa792068b5a4fa16e2d4a342e4f55b8
Thanks for addressing my feedback!
Still think it’s strange that removing
-sparse
increases the functions with mismatched data. But it’s certainly not blocking for me.DrahtBot requested review from vasild on Mar 26, 2025DrahtBot requested review from jonatack on Mar 26, 2025maflcko removed review request from janb84 on Mar 26, 2025maflcko removed review request from BrandonOdiwuor on Mar 26, 2025maflcko requested review from janb84 on Mar 26, 2025DrahtBot requested review from BrandonOdiwuor on Mar 26, 2025in doc/developer-notes.md:524 in b96f1a696a
519+ 520+The following generates a coverage report for unit tests and functional tests. 521+ 522+Configure the build with the following flags: 523+ 524+> Consider building with a clean state using `rm -rf build`
fanquake commented at 9:48 am on March 26, 2025:We don’t do this in any other docs, and as far as I’m aware, this being needed would be a bug, not sure it should be here.
maflcko commented at 10:02 am on March 26, 2025:We don’t do this in any other docs, and as far as I’m aware, this being needed would be a bug, not sure it should be here.
Maybe I am misunderstanding cmake, but all docs refer to a single build directory as
build
and I don’t think it is supported to use a single build directory without cleaning it and while using different configure flags on it on the fly. I am sure that not cleaning it will lead to more issues down the line (https://github.com/bitcoin/bitcoin/issues/31942). Maybe therm
could be dropped, if each differing configuration was put into a different folder, similar to the cmake presets in https://github.com/bitcoin/bitcoin/blob/c0b7159de47592c95c6db061de682b4af4058102/CMakePresets.json#L64.No strong opinion, but leaving the
rm
for now, as it is harmless and possibly helpful, seems fine. No need to invalidate acks, but if it is important to remove, it could be done in a follow-up.Prabhat1308 commented at 10:59 am on March 26, 2025: noneDon’t know how the runs got cancelled.Fixed.
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-03-31 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me