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
  1. Prabhat1308 commented at 2:20 am on February 22, 2025: contributor
    Followed up from the comment on the issue #31927 , issues have been observed building coverage reports with gcov in MacOs and NixOs. This PR adds the steps to generate a coverage report based on the default llvm/clang tooling.
  2. 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, Crypt-iQ
    Concept ACK brunoerg, BrandonOdiwuor, i-am-yuvi
    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.

  3. 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
  4. DrahtBot added the label Docs on Feb 22, 2025
  5. Prabhat1308 force-pushed on Feb 22, 2025
  6. DrahtBot added the label CI failed on Feb 22, 2025
  7. 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.

  8. DrahtBot removed the label CI failed on Feb 22, 2025
  9. 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.
  10. 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 newline
  11. maflcko approved
  12. maflcko commented at 10:29 am on February 22, 2025: member
    lgtm
  13. hebasto commented at 10:46 am on February 22, 2025: member
    cc @vasild
  14. Prabhat1308 force-pushed on Feb 22, 2025
  15. in 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 and llvm-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.
  16. jonatack commented at 6:59 pm on February 22, 2025: member
    Concept ACK
  17. brunoerg commented at 2:33 pm on February 23, 2025: contributor
    Concept ACK
  18. in 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 for llvm-cov show to point into the build/ directory.

  19. 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 that clang/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.
  20. hodlinator commented at 10:08 am on February 24, 2025: contributor
    Concept ACK 6436bdb301278abb3526eab2138b2f23f8905b5e
  21. Prabhat1308 force-pushed on Feb 25, 2025
  22. Prabhat1308 force-pushed on Feb 25, 2025
  23. Prabhat1308 commented at 4:49 pm on February 25, 2025: contributor

    Addressed the comments

    1. Change the commands so we dont have to change shell from the root of the repo.
    2. 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.
  24. Prabhat1308 force-pushed on Feb 25, 2025
  25. Prabhat1308 force-pushed on Feb 25, 2025
  26. Prabhat1308 requested review from hodlinator on Feb 25, 2025
  27. in 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
  28. 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```
  29. 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`.
  30. hodlinator commented at 10:59 am on February 26, 2025: contributor

    Reviewed 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.
  31. janb84 commented at 11:17 am on February 27, 2025: contributor

    Concept 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!

  32. Prabhat1308 force-pushed on Feb 27, 2025
  33. Prabhat1308 commented at 2:22 pm on February 27, 2025: contributor
    • 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 .

  34. janb84 commented at 2:25 pm on February 27, 2025: contributor
    re ACK 05605da
  35. DrahtBot requested review from hodlinator on Feb 27, 2025
  36. DrahtBot requested review from jonatack on Feb 27, 2025
  37. DrahtBot requested review from brunoerg on Feb 27, 2025
  38. pablomartin4btc approved
  39. pablomartin4btc commented at 3:45 pm on February 27, 2025: member

    tACK 05605dae2983c11f0ba662dbdc495f84de12724b

    image

    image

    Maybe we could advice the user about the warnings, left a comment there.

  40. BrandonOdiwuor commented at 8:24 pm on February 27, 2025: contributor
    Concept ACK
  41. in 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:

    https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/CONTRIBUTING.md?plain=1#L170-L174

    Prabhat1308 commented at 9:04 pm on February 27, 2025:
    Removed the @ symbol
  42. DrahtBot requested review from hodlinator on Feb 27, 2025
  43. Prabhat1308 force-pushed on Feb 27, 2025
  44. hodlinator approved
  45. hodlinator commented at 8:00 pm on March 1, 2025: contributor

    ACK 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.
  46. DrahtBot requested review from janb84 on Mar 1, 2025
  47. DrahtBot requested review from pablomartin4btc on Mar 1, 2025
  48. DrahtBot requested review from BrandonOdiwuor on Mar 1, 2025
  49. in 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?
  50. 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.

  51. janb84 commented at 8:21 pm on March 1, 2025: contributor

    re ACK 1245e05

    The extra comments are informative of the possible warnings and their influence on the result.

  52. 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?

    https://github.com/bitcoin/bitcoin/blob/15717f0ef3960969ee550a4a41741987b86684dc/CMakeLists.txt#L33-L37

    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" because CMAKE_CXX_FLAGS causes the flags to be used during linking as well, but APPEND_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.

    vasild commented at 10:49 am on March 14, 2025:
    @ldionne, do you know if -O2 could have any adverse effects on coverage created with clang’s -fprofile-instr-generate -fcoverage-mapping?
    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) and Release (-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) and Release (-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.

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

  54. 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
  55. 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.

  56. 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 from bitcoind then the syntax of this changes a little bit: drop build/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"
  57. 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/"
  58. vasild commented at 1:50 pm on March 4, 2025: contributor

    Approach ACK 1245e05325b41101eddc76ba9214d910489e35b6

    In the light of #31047 and #31394 I have been thinking that it would be useful to at least document the steps needed to gather coverage with clang. Thanks for the initiative!

  59. Prabhat1308 force-pushed on Mar 13, 2025
  60. Prabhat1308 commented at 2:04 am on March 13, 2025: contributor

    Addressing 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 .
  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).

  62. pablomartin4btc commented at 0:38 am on March 14, 2025: member

    ACK 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?

  63. DrahtBot requested review from vasild on Mar 14, 2025
  64. DrahtBot requested review from janb84 on Mar 14, 2025
  65. DrahtBot requested review from hodlinator on Mar 14, 2025
  66. in 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.
  67. 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 commented at 11:10 am on March 14, 2025:

    After #31161 the binaries are now in bin/:

    0    --object=build/bin/test_bitcoin \
1    --object=build/bin/bitcoind \
  68. vasild approved
  69. vasild commented at 11:20 am on March 14, 2025: contributor

    Almost ACK 7f57ad00d301b9564ece511d2e99cb0678b21174, just fix the path to the binaries which were recently relocated from src to bin.

    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 to llvm-cov.

  70. Prabhat1308 force-pushed on Mar 16, 2025
  71. Prabhat1308 commented at 12:34 pm on March 16, 2025: contributor

    Addressed 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-optimization

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

  72. maflcko commented at 1:26 pm on March 16, 2025: member

    gcov

    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

  73. 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 of Unexecuted instantiation: messages and a lot lower coverage. No sure why but don’t think this gives a good image of the actual coverage.
  74. janb84 commented at 12:40 pm on March 17, 2025: contributor

    While re-testing this PR, I found that adding the -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. Disproven it gives a lot of Unexecuted instantiation: errors.

    I agree with the rest of the PR changes. ACK 3c69b07

  75. vasild approved
  76. vasild commented at 3:14 pm on March 17, 2025: contributor

    ACK 3c69b071ac4964870abb347f9d9c15df2e60aa62

    Thank you!

  77. DrahtBot requested review from janb84 on Mar 17, 2025
  78. DrahtBot requested review from pablomartin4btc on Mar 17, 2025
  79. in 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++"`
  80. 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 (in doc/fuzzing.md) and build_dev_mode (in CMakePresets.json).

    Prabhat1308 commented at 9:30 pm on March 25, 2025:
    Decided against it since the previous docs built the coverage in build directory .
  81. 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.

  82. 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 with rm -rf build to begin with a clean slate?
  83. jonatack commented at 4:11 pm on March 17, 2025: member
    ACK 3c69b071ac4964870abb347f9d9c15df2e60aa62 modulo feedback below
  84. in 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)
  85. 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.
  86. Prabhat1308 force-pushed on Mar 25, 2025
  87. Prabhat1308 commented at 9:29 pm on March 25, 2025: contributor
    • 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
  88. Prabhat1308 commented at 9:32 pm on March 25, 2025: contributor
    @ryanofsky I have addressed the remaining comments and these should be my last changes (if everyone agrees)
  89. 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 for mkdir:

    0mkdir -p build/raw_profile_data
    Prabhat1308 commented at 8:46 am on March 26, 2025:
    done.
  90. 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.
  91. hodlinator commented at 8:26 am on March 26, 2025: contributor
    Reviewed/tested 3b5d00f65ae37b733db74d9c9c905eb35e7363e5
  92. add clang/llvm based coverage report generation 
    Signed-off-by: Prabhat Verma <prabhatverma329@gmail.com>
    b96f1a696a
  93. Prabhat1308 force-pushed on Mar 26, 2025
  94. Prabhat1308 renamed this:
    doc: Update documentation to include Clang/llvm based coverage report generation
    doc: Add Clang/LLVM based coverage report generation
    on Mar 26, 2025
  95. Prabhat1308 commented at 8:48 am on March 26, 2025: contributor
    Addressed the reviews by @hodlinator
  96. hodlinator approved
  97. hodlinator commented at 9:08 am on March 26, 2025: contributor

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

  98. DrahtBot requested review from vasild on Mar 26, 2025
  99. DrahtBot requested review from jonatack on Mar 26, 2025
  100. maflcko removed review request from janb84 on Mar 26, 2025
  101. maflcko removed review request from BrandonOdiwuor on Mar 26, 2025
  102. maflcko requested review from janb84 on Mar 26, 2025
  103. janb84 commented at 9:39 am on March 26, 2025: contributor

    Re ACK b96f1a6

    After reading the discussion and the Clang documentation, I think that the call to remove the DEBUG build type is a good call. Agree with other changes. Good work !

  104. DrahtBot requested review from BrandonOdiwuor on Mar 26, 2025
  105. in 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 the rm 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.

    fanquake commented at 5:59 am on April 2, 2025:

    but if it is important to remove, it could be done in a follow-up.

    The more I look at this, the less clear it seems to be. Clearly the dir reuse can/is causing issues, however the discussion in #31942 also claims that any need to nuke a build dir is a bug. If we are going to start adding this, we should at least be consistent, and add this same disclaimer to every bit of documentation that invokes cmake. Will followup in #31942.

  106. Prabhat1308 commented at 10:59 am on March 26, 2025: contributor
    Don’t know how the runs got cancelled. Fixed.
  107. Prabhat1308 commented at 9:17 am on April 1, 2025: contributor
    @jonatack @pablomartin4btc @vasild would appreciate some re-ACKs.
  108. i-am-yuvi commented at 8:20 pm on April 1, 2025: contributor
    Concept ACK
  109. Crypt-iQ commented at 10:01 pm on April 1, 2025: contributor

    tACK b96f1a696aa792068b5a4fa16e2d4a342e4f55b8

    I was able to produce coverage reports with llvm@18 on macOS 15.4. I wasn’t able to produce a coverage report for the fuzzer following similar steps – the raw_profile_data directory was empty.

  110. Prabhat1308 commented at 4:41 am on April 2, 2025: contributor

    tACK b96f1a6

    I was able to produce coverage reports with llvm@18 on macOS 15.4. I wasn’t able to produce a coverage report for the fuzzer following similar steps – the raw_profile_data directory was empty.

    The following steps are only for functional tests and unit tests. I will add coverage generation for fuzz tests in another follow up PR .

  111. fanquake merged this on Apr 2, 2025
  112. fanquake closed this on Apr 2, 2025
  113. Prabhat1308 deleted the branch on Apr 2, 2025
  114. in doc/developer-notes.md:558 in b96f1a696a 
    553+Generating the coverage report:
554+
555+```shell
556+llvm-cov show \
557+    --object=build/bin/test_bitcoin \
558+    --object=build/bin/bitcoind \
    maflcko commented at 7:31 am on April 2, 2025:

    I haven’t tried this, so I don’t know if it makes any difference, but why is this listing bitcoind and test_bitcoin, but not the util exe?

    They are listed in https://github.com/bitcoin/bitcoin/pull/31394

    Prabhat1308 commented at 2:36 pm on April 2, 2025:
    There are also other binaries like bitcoin-cli , bitcoin-tx including bitcoin-chainstate which are built when using specific flags. Including each requires --object={} . Adding too many felt too verbose and not needed by a general reviewer. If required he/she may add it themselves. These two I felt are most needed/ most used by a reviewer .
  115. ryanofsky referenced this in commit bb92bb36f2 on Apr 8, 2025
  116. vasild commented at 1:59 pm on April 16, 2025: contributor
    ACK b96f1a696aa792068b5a4fa16e2d4a342e4f55b8


Prabhat1308 DrahtBot maflcko hebasto jonatack pablomartin4btc brunoerg hodlinator janb84 BrandonOdiwuor vasild ldionne ryanofsky fanquake i-am-yuvi Crypt-iQ


vasild jonatack BrandonOdiwuor brunoerg pablomartin4btc

Labels
Docs

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-04-29 21:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me