contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds

theStack commented at 8:12 pm on January 1, 2025: contributor

The determinstic line coverage checking script test_deterministic_coverage.sh was seemingly forgotten to be adapted in the course of switching from autotools to CMake (only the error messages containing build instructions were updated in a9964c04447745435747d9cc557165c43902783b / #30875). Since gcovr needs to access both the source and build files, the path to the latter has to be passed explicitly as they are not in the same anymore (see e.g. https://github.com/gcovr/gcovr/issues/340).

Can be tested by:

0$ cmake -B build -DCMAKE_BUILD_TYPE=Coverage
1$ cmake --build build
2$ ./contrib/devtools/test_deterministic_coverage.sh

Alternatively, if the script is not needed anymore (apparently no-one else tried to execute it for the last three months?), it could also be deleted.

DrahtBot commented at 8:13 pm on January 1, 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/31588.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko
Concept ACK	hodlinator, fjahr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31901 (contrib: Add deterministic-unittest-coverage by maflcko)

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 added the label Scripts and tools on Jan 1, 2025

in contrib/devtools/test_deterministic_coverage.sh:37 in e2404269ae outdated

33@@ -34,7 +34,7 @@ NON_DETERMINISTIC_TESTS=(
34     "wallet_tests/wallet_disableprivkeys"                     # validation.cpp: if (signals.CallbacksPending() > 10)
35 )
36 
37-TEST_BITCOIN_BINARY="src/test/test_bitcoin"
38+TEST_BITCOIN_BINARY="build/src/test/test_bitcoin"

maflcko commented at 1:35 pm on January 2, 2025:

not sure about hardcoding. It would be better to have a symbol for this. This would also allow to remove mktemp and just use the build dir.

theStack commented at 3:07 pm on January 2, 2025:

Yeah, this is currently only a minimum-diff fix, happy to refine further. I assume with “symbol” you mean just another shell script variable, e.g. BUILD_DIR? (Could even go further and allow to specify the build-dir as an optional script parameter that defaults to “build”.)

fjahr commented at 10:14 pm on February 6, 2025:

I agree, hardcoding a different path isn’t much of a fix even if it’s the one we use in all our examples. Better to have a way to pass the path in as a parameter.

fjahr commented at 10:18 pm on February 6, 2025:

In gen-manpages.py and gen-bitcoin-conf.sh we use BUILDDIR fwiw.

theStack commented at 7:38 am on February 7, 2025:

Thanks for the reviews. The build path can now specified via the BUILDDIR environment variable (set to “build” relative to the top-level-dir by default) and the script can be called from anywhere within the repository. @maflcko

This would also allow to remove mktemp and just use the build dir.

Done.

fanquake added this to the milestone 29.0 on Jan 6, 2025

fjahr commented at 10:19 pm on February 6, 2025: contributor

Concept ACK

contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds

This change also enables to run the script for anywhere within the repository.

517d30b097

theStack force-pushed on Feb 7, 2025

contrib: test_deterministic_coverage.sh: write .tmp files to build dir 459683bcfc

maflcko commented at 7:36 am on February 7, 2025: member

lgtm ACK 517d30b097d17f86d40beff679f62287776c459a

I haven’t tested this (I am using a modified script locally anyway), but the code changes look harmless and reasonable.

DrahtBot requested review from fjahr on Feb 7, 2025

in contrib/devtools/test_deterministic_coverage.sh:108 in 517d30b097 outdated

103@@ -102,7 +104,7 @@ TEST_RUN_ID=0
104 while [[ ${TEST_RUN_ID} -lt ${N_TEST_RUNS} ]]; do
105     TEST_RUN_ID=$((TEST_RUN_ID + 1))
106     echo "[$(date +"%Y-%m-%d %H:%M:%S")] Measuring coverage, run #${TEST_RUN_ID} of ${N_TEST_RUNS}"
107-    find src/ -type f -name "*.gcda" -exec rm {} \;
108+    find "$BUILDDIR/src/" -type f -name "*.gcda" -exec rm {} \;
109     if [[ $(get_file_suffix_count gcda) != 0 ]]; then

fjahr commented at 4:11 pm on February 7, 2025:

I needed to change this to an arithmetic comparison to make it work because the wc -l result includes some white space on my system.

0    if (( $(get_file_suffix_count gcda) != 0 )); then

fjahr commented at 5:08 pm on February 7, 2025: contributor

Hm, I tried to test this but wasn’t successful so far. Aside from the fix I suggest in my inline comment I still get an error that gcov is not able to find the working dir. That seems to be because the coverage files include the build paths and not the source paths. After a little digging I tried to address this by compiling with -DAPPEND_CXXFLAGS="-coverage-prefix-map=${BUILDDIR}/src=${TOPDIR}/src" but that didn’t work and it doesn’t seem like a good enough fix anyway.

Does this just work for you @theStack ? Maybe I am missing some compile flags that are not documented but necessary…

theStack commented at 5:42 pm on February 7, 2025: contributor

@fjahr: Executing the script with the commands mentioned in the PR description works for me on Ubuntu 22.04.03 LTS, didn’t try it on any other machine yet. Which operating system are you running? (Some macOS I presume?) Some versions from my machine that would be relevant for comparison:

0$ cat /etc/lsb-release | tail -n1
1DISTRIB_DESCRIPTION="Ubuntu 22.04.3 LTS"
2$ gcovr --version | head -n1
3gcovr 5.0
4$ gcc --version | head -n1
5gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
6$ bash --version
7GNU bash, version 5.1.16(1)-release (x86_64-pc-linux-gnu)
8$ wc --version
9wc (GNU coreutils) 8.32

Another potential cause for failure that comes to my mind is that you possibly compile with clang. In that case, the GCOV_EXECUTABLE variable has to be changed manually in the script: https://github.com/bitcoin/bitcoin/blob/fb0ada982a73687520c43b8fde480fa5d456f3e1/contrib/devtools/test_deterministic_coverage.sh#L11-L13 That’s ugly of course, but not sure how to fix this easily. We should detect and set this automatically in the future.

fjahr commented at 11:08 pm on February 7, 2025: contributor

Yeah, I’m using macOS Sequoia 15.3 and I am also using clang but I have already been using GCOV_EXECUTABLE from the start.

Checking some of the other versions:

0$ gcovr --version | head -n1
1gcovr 8.3
2$ clang --version | head -n1
3Homebrew clang version 19.1.6
4$ bash --version
5GNU bash, version 5.2.37(1)-release (aarch64-apple-darwin24.0.0)

The built-in wc doesn’t seem to provide version information but it is supposed to be taken from BSD tools.

I tried compiling with GCC 14 as well now. It’s not giving me any loud errors but it’s still not working either:

0$ GCOV_EXECUTABLE="gcov-14" BUILDDIR="build" contrib/devtools/test_deterministic_coverage.sh
1[2025-02-08 00:00:50] Measuring coverage, run [#1](/bitcoin-bitcoin/1/) of 2
2(INFO) Reading coverage data...
3(INFO) Writing coverage report...
4Error: Spurious gcovr output. Make sure the correct GCOV_EXECUTABLE variable is set in contrib/devtools/test_deterministic_coverage.sh ("gcov" for gcc, "llvm-cov gcov" for clang).

theStack commented at 8:13 am on February 9, 2025: contributor

@fjahr: Note that GCOV_EXECUTABLE is hardcoded in the script currently, so setting it as environment variable won’t have any effect. Can you try again by modifying it directly in the file? It’s not very user friendly that this is necessary, can add a commit that allows passing it via environment, if reviewers feel that this is still in scope of this PR.

fjahr commented at 4:40 pm on February 9, 2025: contributor

Note that GCOV_EXECUTABLE is hardcoded in the script currently, so setting it as environment variable won’t have any effect.

Ah, I had indeed missed that. I tried this for llvm but it didn’t change the outcome, seems to be the same issue. I didn’t investigate further there and instead tried again with gcc-14, given this is at least a bit closer to your tooling. I get another error there with this. It appears the compile time macros are confusing gcov and it sees secp256k1_scalar_split_lambda defined twice. I’m not even sure if it should be considering secp at all since this is just to get coverage of our code, right?

Here is the error I’m getting:

 0$ BUILDDIR="build" contrib/devtools/test_deterministic_coverage.sh
 1[2025-02-09 14:45:24] Measuring coverage, run [#1](/bitcoin-bitcoin/1/) of 2
 2(INFO) Reading coverage data...
 3(ERROR) Traceback (most recent call last):
 4  File "/opt/homebrew/Cellar/gcovr/8.3/libexec/lib/python3.13/site-packages/gcovr/formats/gcov/workers.py", line 95, in worker
 5    work(*args, **kwargs)
 6    ~~~~^^^^^^^^^^^^^^^^^
 7  File "/opt/homebrew/Cellar/gcovr/8.3/libexec/lib/python3.13/site-packages/gcovr/formats/gcov/read.py", line 471, in process_datafile
 8    done = run_gcov_and_process_files(
 9        abs_filename,
10    ...<3 lines>...
11        chdir=wd,
12    )
13  File "/opt/homebrew/Cellar/gcovr/8.3/libexec/lib/python3.13/site-packages/gcovr/formats/gcov/read.py", line 836, in run_gcov_and_process_files
14    process_gcov_json_data(gcov_filename, covdata, options)
15    ~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
16  File "/opt/homebrew/Cellar/gcovr/8.3/libexec/lib/python3.13/site-packages/gcovr/formats/gcov/read.py", line 210, in process_gcov_json_data
17    insert_file_coverage(covdata, file_cov, get_merge_mode_from_options(options))
18    ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
19  File "/opt/homebrew/Cellar/gcovr/8.3/libexec/lib/python3.13/site-packages/gcovr/merging.py", line 250, in insert_file_coverage
20    return _insert_coverage_item(
21        target.data, file.filename, file, merge_file, options, None
22    )
23  File "/opt/homebrew/Cellar/gcovr/8.3/libexec/lib/python3.13/site-packages/gcovr/merging.py", line 222, in _insert_coverage_item
24    merged_item = merge_item(target_dict[key], new_item, options, context)
25  File "/opt/homebrew/Cellar/gcovr/8.3/libexec/lib/python3.13/site-packages/gcovr/merging.py", line 295, in merge_file
26    raise AssertionError("\n".join(message)) from None
27AssertionError: Got function secp256k1_scalar_split_lambda in /Users/XXX/projects/clones/bitcoin/src/secp256k1/src/scalar_impl.h on multiple lines: 67, 142.
28	You can run gcovr with --merge-mode-functions=MERGE_MODE.
29	The available values for MERGE_MODE are described in the documentation.
30GCOV source files of merge source is/are:
31	/Users/XXX/projects/clones/bitcoin/src/tests.c##63f67a257b368dda78c4068542388473.gcov.json.gz
32and of merge target is/are:
33	/Users/XXX/projects/clones/bitcoin/src/tests_exhaustive.c##a46642766247b4021fb41c87898a129a.gcov.json.gz
34
35(ERROR) Error occurred while reading reports:
36Traceback (most recent call last):
37  File "/opt/homebrew/Cellar/gcovr/8.3/libexec/lib/python3.13/site-packages/gcovr/__main__.py", line 384, in main
38    covdata = gcovr_formats.read_reports(options)
39  File "/opt/homebrew/Cellar/gcovr/8.3/libexec/lib/python3.13/site-packages/gcovr/formats/__init__.py", line 99, in read_reports
40    covdata = GcovHandler(options).read_report()
41  File "/opt/homebrew/Cellar/gcovr/8.3/libexec/lib/python3.13/site-packages/gcovr/formats/gcov/__init__.py", line 233, in read_report
42    return read_report(self.options)
43  File "/opt/homebrew/Cellar/gcovr/8.3/libexec/lib/python3.13/site-packages/gcovr/formats/gcov/read.py", line 96, in read_report
44    contexts = pool.wait()
45  File "/opt/homebrew/Cellar/gcovr/8.3/libexec/lib/python3.13/site-packages/gcovr/formats/gcov/workers.py", line 181, in wait
46    raise RuntimeError(
47        "Worker thread raised exception, workers canceled."
48    ) from None
49RuntimeError: Worker thread raised exception, workers canceled.
50
51Error: gcovr failed. Output written to build/deterministic_coverage_gcovr_output.tmp. Exiting.

maflcko commented at 9:11 am on February 10, 2025: member

lgtm ACK 459683bcfc70b77b3dae2e70c88cf5c6a007442e

Only change is a small mktemp cleanup.

I still haven’t tested this (I am using a modified script locally anyway), but the code changes look harmless and reasonable. As for the scope, I wonder how much effort should be spent on this script, given that:

No one(?) is using it right now unmodified, or has been in the past years?
It is written in bash, causing compat issues with the ancient version of bash shipped on macOS, as well as the general bash issues (like error handling).
In light of #31394, it is unclear whether or not it would be stale again anyway.

DrahtBot requested review from fjahr on Feb 10, 2025

hodlinator commented at 2:26 pm on February 10, 2025: contributor

Concept ACK 459683bcfc70b77b3dae2e70c88cf5c6a007442e

Changes are certainly a step in the right direction unless we decide to delete the script.

Tested on NixOS

Without PR, it does indeed fail:

0₿ ./contrib/devtools/test_deterministic_coverage.sh
1Error: Executable src/test/test_bitcoin not found. Run "cmake -B build -DCMAKE_BUILD_TYPE=Coverage" and compile.

With the PR I’m getting a failure inside gcov - AssertionError: count must not be a negative value..

 0₿ ./contrib/devtools/test_deterministic_coverage.sh
 1[2025-02-10 15:02:26] Measuring coverage, run [#1](/bitcoin-bitcoin/1/) of 2
 2(INFO) Reading coverage data...
 3Traceback (most recent call last):
 4  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/workers.py", line 81, in worker
 5    work(*args, **kwargs)
 6  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/read.py", line 566, in process_datafile
 7    done = run_gcov_and_process_files(
 8           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
 9  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/read.py", line 922, in run_gcov_and_process_files
10    process_gcov_json_data(gcov_filename, covdata, options)
11  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/read.py", line 234, in process_gcov_json_data
12    LineCoverage(
13  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/coverage.py", line 410, in __init__
14    raise AssertionError("count must not be a negative value.")
15AssertionError: count must not be a negative value.
16Traceback (most recent call last):
17  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/workers.py", line 81, in worker
18    work(*args, **kwargs)
19  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/read.py", line 566, in process_datafile
20    done = run_gcov_and_process_files(
21           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
22  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/read.py", line 922, in run_gcov_and_process_files
23    process_gcov_json_data(gcov_filename, covdata, options)
24  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/read.py", line 234, in process_gcov_json_data
25    LineCoverage(
26  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/coverage.py", line 410, in __init__
27    raise AssertionError("count must not be a negative value.")
28AssertionError: count must not be a negative value.
29(ERROR) Error occurred while reading reports:
30Traceback (most recent call last):
31  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/__main__.py", line 358, in main
32    covdata: CovData = gcovr_formats.read_reports(options)
33                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
34  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/__init__.py", line 85, in read_reports
35    covdata = GcovHandler(options).read_report()
36              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
37  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/__init__.py", line 215, in read_report
38    return read_report(self.options)
39           ^^^^^^^^^^^^^^^^^^^^^^^^^
40  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/read.py", line 96, in read_report
41    with Workers(
42         ^^^^^^^^
43  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/workers.py", line 177, in __exit__
44    self.wait()
45  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/workers.py", line 168, in wait
46    raise self.exceptions[0][1]
47  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/read.py", line 103, in read_report
48    contexts = pool.wait()
49               ^^^^^^^^^^^
50  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/workers.py", line 168, in wait
51    raise self.exceptions[0][1]
52  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/workers.py", line 81, in worker
53    work(*args, **kwargs)
54  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/read.py", line 566, in process_datafile
55    done = run_gcov_and_process_files(
56           ^^^^^^^^^^^^^^^^^^^^^^^^^^^
57  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/read.py", line 922, in run_gcov_and_process_files
58    process_gcov_json_data(gcov_filename, covdata, options)
59  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/formats/gcov/read.py", line 234, in process_gcov_json_data
60    LineCoverage(
61  File "/nix/store/415rw62rhdn4di1nn41qhcsvpmjr14yx-python3.12-gcovr-8.2/lib/python3.12/site-packages/gcovr/coverage.py", line 410, in __init__
62    raise AssertionError("count must not be a negative value.")
63AssertionError: count must not be a negative value.
64
65count must not be a negative value.
66Error: gcovr failed. Output written to /home/hodlinator/b2/build/deterministic_coverage_gcovr_output.tmp. Exiting.

/home/hodlinator/b2/build/deterministic_coverage_gcovr_output.tmp is empty.

Tooling versions:

0₿ g++ -v
1...
2gcc version 14.2.1 20241116 (GCC)
3
4₿ gcov -v
5gcov (GCC) 14.2.1 20241116
6...

maflcko commented at 3:10 pm on February 10, 2025: member

Given all the GCC/lcov/gcov problems, I wonder if it wouldn’t be easier to just go with clang/llvm:

Require compilation with -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fPIC -fprofile-instr-generate -fcoverage-mapping'
apply the below diff

  0diff --git a/contrib/devtools/test_deterministic_coverage.sh b/contrib/devtools/test_deterministic_coverage.sh
  1index 885396bb25..48d331ae70 100755
  2--- a/contrib/devtools/test_deterministic_coverage.sh
  3+++ b/contrib/devtools/test_deterministic_coverage.sh
  4@@ -8,9 +8,7 @@
  5 
  6 export LC_ALL=C
  7 
  8-# Use GCOV_EXECUTABLE="gcov" if compiling with gcc.
  9-# Use GCOV_EXECUTABLE="llvm-cov gcov" if compiling with clang.
 10-GCOV_EXECUTABLE="gcov"
 11+BUILD_DIR="./bld-cmake"
 12 
 13 # Disable tests known to cause non-deterministic behaviour and document the source or point of non-determinism.
 14 NON_DETERMINISTIC_TESTS=(
 15@@ -34,7 +32,9 @@ NON_DETERMINISTIC_TESTS=(
 16     "wallet_tests/wallet_disableprivkeys"                     # validation.cpp: if (signals.CallbacksPending() > 10)
 17 )
 18 
 19-TEST_BITCOIN_BINARY="src/test/test_bitcoin"
 20+TEST_BITCOIN_BINARY="${BUILD_DIR}/src/test/test_bitcoin"
 21+LLVM_PROFRAW_FILE="${BUILD_DIR}/test_det_cov.profraw"
 22+LLVM_PROFDATA_FILE="${BUILD_DIR}/test_det_cov.profdata"
 23 
 24 print_usage() {
 25     echo "Usage: $0 [custom test filter (default: all but known non-deterministic tests)] [number of test runs (default: 2)]"
 26@@ -65,18 +65,17 @@ if [[ $# != 0 ]]; then
 27 fi
 28 if [[ ${BOOST_TEST_RUN_FILTERS} == "" ]]; then
 29     BOOST_TEST_RUN_FILTERS="$(IFS=":"; echo "!${NON_DETERMINISTIC_TESTS[*]}" | sed 's/:/:!/g')"
 30-else
 31-    echo "Using Boost test filter: ${BOOST_TEST_RUN_FILTERS}"
 32-    echo
 33 fi
 34+echo "Using Boost test filter: ${BOOST_TEST_RUN_FILTERS}"
 35+echo
 36 
 37-if ! command -v gcov > /dev/null; then
 38-    echo "Error: gcov not installed. Exiting."
 39+if ! command -v llvm-profdata > /dev/null; then
 40+    echo "Error: llvm-profdata not installed. Exiting."
 41     exit 1
 42 fi
 43 
 44-if ! command -v gcovr > /dev/null; then
 45-    echo "Error: gcovr not installed. Exiting."
 46+if ! command -v llvm-cov > /dev/null; then
 47+    echo "Error: llvm-cov not installed. Exiting."
 48     exit 1
 49 fi
 50 
 51@@ -85,52 +84,41 @@ if [[ ! -e ${TEST_BITCOIN_BINARY} ]]; then
 52     exit 1
 53 fi
 54 
 55-get_file_suffix_count() {
 56-    find src/ -type f -name "*.$1" | wc -l
 57-}
 58-
 59-if [[ $(get_file_suffix_count gcno) == 0 ]]; then
 60-    echo "Error: Could not find any *.gcno files. The *.gcno files are generated by the compiler. Run \"cmake -B build -DCMAKE_BUILD_TYPE=Coverage\" and re-compile."
 61-    exit 1
 62-fi
 63-
 64-get_covr_filename() {
 65-    echo "gcovr.run-$1.txt"
 66-}
 67+
 68+
 69+
 70+
 71 
 72 TEST_RUN_ID=0
 73 while [[ ${TEST_RUN_ID} -lt ${N_TEST_RUNS} ]]; do
 74     TEST_RUN_ID=$((TEST_RUN_ID + 1))
 75     echo "[$(date +"%Y-%m-%d %H:%M:%S")] Measuring coverage, run #${TEST_RUN_ID} of ${N_TEST_RUNS}"
 76-    find src/ -type f -name "*.gcda" -exec rm {} \;
 77-    if [[ $(get_file_suffix_count gcda) != 0 ]]; then
 78-        echo "Error: Stale *.gcda files found. Exiting."
 79-        exit 1
 80-    fi
 81-    TEST_OUTPUT_TEMPFILE=$(mktemp)
 82-    if ! BOOST_TEST_RUN_FILTERS="${BOOST_TEST_RUN_FILTERS}" ${TEST_BITCOIN_BINARY} > "${TEST_OUTPUT_TEMPFILE}" 2>&1; then
 83+
 84+    TEST_OUTPUT_TEMPFILE="$BUILD_DIR/test_det_cov_out.tmp"
 85+    LLVM_PROFILE_FILE="${LLVM_PROFRAW_FILE}" BOOST_TEST_RUN_FILTERS="${BOOST_TEST_RUN_FILTERS}" ${TEST_BITCOIN_BINARY} > "${TEST_OUTPUT_TEMPFILE}" 2>&1
 86+    if [[ $? -ne 0 ]]; then
 87         cat "${TEST_OUTPUT_TEMPFILE}"
 88         rm "${TEST_OUTPUT_TEMPFILE}"
 89         exit 1
 90     fi
 91     rm "${TEST_OUTPUT_TEMPFILE}"
 92-    if [[ $(get_file_suffix_count gcda) == 0 ]]; then
 93-        echo "Error: Running the test suite did not create any *.gcda files. The gcda files are generated when the instrumented test programs are executed. Run \"cmake -B build -DCMAKE_BUILD_TYPE=Coverage\" and re-compile."
 94-        exit 1
 95-    fi
 96-    GCOVR_TEMPFILE=$(mktemp)
 97-    if ! gcovr --gcov-executable "${GCOV_EXECUTABLE}" -r src/ > "${GCOVR_TEMPFILE}"; then
 98-        echo "Error: gcovr failed. Output written to ${GCOVR_TEMPFILE}. Exiting."
 99+
100+    if [[ ! -f ${LLVM_PROFRAW_FILE} ]]; then
101+        echo "Error: Profiling data not generated. Ensure the binary is compiled with Clang coverage flags."
102         exit 1
103     fi
104-    GCOVR_FILENAME=$(get_covr_filename ${TEST_RUN_ID})
105-    mv "${GCOVR_TEMPFILE}" "${GCOVR_FILENAME}"
106-    if grep -E "^TOTAL *0 *0 " "${GCOVR_FILENAME}"; then
107-        echo "Error: Spurious gcovr output. Make sure the correct GCOV_EXECUTABLE variable is set in $0 (\"gcov\" for gcc, \"llvm-cov gcov\" for clang)."
108+
109+    llvm-profdata merge --sparse "${LLVM_PROFRAW_FILE}" -o "${LLVM_PROFDATA_FILE}"
110+    if [[ $? -ne 0 ]]; then
111+        echo "Error: llvm-profdata failed to generate profiling data. Exiting."
112         exit 1
113     fi
114+
115+    COVERAGE_FILENAME="${BUILD_DIR}/test_det_cov.run-${TEST_RUN_ID}.txt"
116+    llvm-cov show "${TEST_BITCOIN_BINARY}" -instr-profile="${LLVM_PROFDATA_FILE}" > "${COVERAGE_FILENAME}"
117+
118     if [[ ${TEST_RUN_ID} != 1 ]]; then
119-        COVERAGE_DIFF=$(diff -u "$(get_covr_filename 1)" "${GCOVR_FILENAME}")
120+        COVERAGE_DIFF=$(diff -u "${BUILD_DIR}/test_det_cov.run-1.txt" "${COVERAGE_FILENAME}")
121         if [[ ${COVERAGE_DIFF} != "" ]]; then
122             echo
123             echo "The line coverage is non-deterministic between runs. Exiting."
124@@ -142,7 +130,7 @@ while [[ ${TEST_RUN_ID} -lt ${N_TEST_RUNS} ]]; do
125             echo "${COVERAGE_DIFF}"
126             exit 1
127         fi
128-        rm "${GCOVR_FILENAME}"
129+        rm "${COVERAGE_FILENAME}"
130     fi
131 done
132

But no strong opinion. I think anything is fine here and I’ve already reviewed this pull.

hodlinator commented at 10:08 pm on February 10, 2025: contributor

Succeeded with maflcko’s diff applied to master (modified build dir).

Got a big diff, indicating a lot of test non-determinism:

 0[2025-02-10 22:32:56] Measuring coverage, run [#1](/bitcoin-bitcoin/1/) of 2
 1[2025-02-10 22:35:26] Measuring coverage, run [#2](/bitcoin-bitcoin/2/) of 2
 2
 3The line coverage is non-deterministic between runs. Exiting.
 4
 5The test suite must be deterministic in the sense that the set of lines executed at least
 6once must be identical between runs. This is a necessary condition for meaningful
 7coverage measuring.
 8
 9--- ./build/test_det_cov.run-1.txt	2025-02-10 22:35:26.017552349 +0100
10+++ ./build/test_det_cov.run-2.txt	2025-02-10 22:37:53.626966257 +0100
11@@ -1980,19 +1980,19 @@
12   751|       |
13   752|       |    // Loop through the addrman table until we find an appropriate entry
14   753|     78|    double chance_factor = 1.0;
15-  754|  85.7k|    while (1) {
16+  754|  85.3k|    while (1) {
17... ~39k+ lines omitted

Don’t have a strong preference either really (GCC/LLVM). Seems like a cool concept to have this if only people thought to chip away at what it finds, and CI stats somewhere graphed the overall size of the diff.

fjahr commented at 10:09 pm on February 10, 2025: contributor

I wonder if it wouldn’t be easier to just go with clang/llvm

I’ve tested the suggested diff and it worked for me. llvm/clang is my default anyway so I would give this a Concept ACK if you want to follow this approach @theStack

maflcko commented at 7:36 am on February 11, 2025: member

Got a big diff, indicating a lot of test non-determinism:

I haven’t looked in detail, but I presume much is due to randomness. On one hand, one could go with #31486 for the unit tests as well. On the other hand, some unit tests are fuzz tests (drawing inputs from a random seed, which is recorded), so they’d be a bit more limited if the randomness was made deterministic. But I think either way is fine.

hodlinator commented at 1:09 pm on February 11, 2025: contributor

Got a big diff, indicating a lot of test non-determinism:

I haven’t looked in detail, but I presume much is due to randomness. On one hand, one could go with #31486 for the unit tests as well. On the other hand, some unit tests are fuzz tests (drawing inputs from a random seed, which is recorded), so they’d be a bit more limited if the randomness was made deterministic. But I think either way is fine.

This gives me an 8.4K line diff instead of 39k, so there is some hope:

0RANDOM_CTX_SEED=21 ./contrib/devtools/test_deterministic_coverage.sh

theStack commented at 8:26 pm on February 11, 2025: contributor

I wonder if it wouldn’t be easier to just go with clang/llvm

I’ve tested the suggested diff and it worked for me. llvm/clang is my default anyway so I would give this a Concept ACK if you want to follow this approach @theStack

Concept ACK on switching over to clang/llvm as well considering all the reported problems. Note though that this PR merely tries to restore the previous behaviour of the script for CMake builds (and minor improvements that are enabled by that), so I think a deeper change is out of scope. @maflcko: Do you want to open a PR? Happy to close this one and test then. I’d rather not include a diff here that I don’t understand myself, and there are seemingly additional changes needed, like documenting the needed build flags somewhere.

maflcko commented at 8:32 pm on February 11, 2025: member

Sure, but it may take some time until I get around to it.

maflcko commented at 8:34 pm on February 11, 2025: member

In any case, I am sure this script is broken or at least stale for years and shouldn’t be a blocker for 29.0 (even if it was used, it is a dev-only tool). So I’ll be removing the milestone for now.

maflcko removed this from the milestone 29.0 on Feb 11, 2025

maflcko commented at 8:30 pm on February 18, 2025: member

Done in https://github.com/bitcoin/bitcoin/pull/31901

contrib: fix test_deterministic_coverage.sh script for out-of-tree builds #31588

Code Coverage & Benchmarks

Reviews

Conflicts

Tested on NixOS

contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds #31588