build: Fix coverage builds

hebasto commented at 9:41 pm on November 20, 2024: member

This PR follows up on #30811, which inadvertently broke coverage builds:

For GCC. See #31337 (comment).
For Clang’s source-based code coverage in the OSS-Fuzz environment due to its use of other options and a third party script. See https://issues.oss-fuzz.com/issues/379122777.

The root cause of this regression is that the -ffile-prefix-map option implicitly applies:

-fprofile-prefix-map when using GCC.
-fcoverage-prefix-map when using Clang. ~~This behaviour is not explicitly documented~~ See https://github.com/llvm/llvm-project/commit/994c544c18c86cbdb6536aae5d27ef7e2f592486.

With this PR, only the -fdebug-prefix-map and -fmacro-prefix-map options are applied.

Note for reviewers: Please ensure that #30799 is not reintroduced.

hebasto added the label Build system on Nov 20, 2024

DrahtBot commented at 9:41 pm on November 20, 2024: 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/31337.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, maflcko

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

in contrib/guix/libexec/build.sh:215 in 7002a4b392 outdated

211@@ -212,6 +212,7 @@ CONFIGFLAGS="-DREDUCE_EXPORTS=ON -DBUILD_BENCH=OFF -DBUILD_GUI_TESTS=OFF -DBUILD
212 HOST_CFLAGS="-O2 -g"
213 HOST_CFLAGS+=$(find /gnu/store -maxdepth 1 -mindepth 1 -type d -exec echo -n " -ffile-prefix-map={}=/usr" \;)
214 case "$HOST" in
215+    *linux*)  HOST_CFLAGS+=" -ffile-prefix-map=${DISTSRC}/src=." ;;

maflcko commented at 8:05 am on November 21, 2024:

Why is this needed? guix never compiles with --coverage, no? See https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fprofile-prefix-map

hebasto commented at 10:10 am on November 22, 2024:

Reworked.

maflcko commented at 8:06 am on November 21, 2024: member

Good catch, but I think the upstream docs were wrong in this case. For me, they don’t mention coverage is affected as well: https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang-ffile-prefix-map

maflcko added this to the milestone 29.0 on Nov 21, 2024

fanquake commented at 9:41 am on November 21, 2024: member

As far as I’m aware, Clang based coverage builds are working outside of oss-fuzz (cc @dergoegge), so it’s not clear why this would be the right solution. If it’s an oss-fuzz only problem, shouldn’t the fix happen there?

With this PR, only the -fdebug-prefix-map and -fmacro-prefix-map options are applied only for non-release builds.

Not sure about introducing these kinds of (undocumented) distinctions. By release do you just mean Guix build? Why should these options only apply to one build and not the other (regardless of anything oss-fuzz related).

in CMakeLists.txt:444 in 7002a4b392 outdated

435@@ -436,8 +436,11 @@ configure_file(contrib/filter-lcov.py filter-lcov.py USE_SOURCE_PERMISSIONS COPY
436 # Don't allow extended (non-ASCII) symbols in identifiers. This is easier for code review.
437 try_append_cxx_flags("-fno-extended-identifiers" TARGET core_interface SKIP_LINK)
438 
439-try_append_cxx_flags("-ffile-prefix-map=A=B" TARGET core_interface SKIP_LINK
440-  IF_CHECK_PASSED "-ffile-prefix-map=${PROJECT_SOURCE_DIR}/src=."
441+try_append_cxx_flags("-fdebug-prefix-map=A=B" TARGET core_interface SKIP_LINK

fanquake commented at 9:41 am on November 21, 2024:

If this was going to be done, this would need some sort of documentation, otherwise someone would likely assume this can be consolidated back to -ffile-prefix-map.

hebasto commented at 10:22 am on November 22, 2024:

The comment has been added.

maflcko commented at 9:46 am on November 21, 2024: member

As far as I’m aware, Clang based coverage builds are working outside of oss-fuzz (cc @dergoegge), so it’s not clear why this would be the right solution. If it’s an oss-fuzz only problem, shouldn’t the fix happen there?

This also affects GCC coverage, see https://cirrus-ci.com/github/maflcko/b-c-cov/ci, so the fix here is needed and likely correct (modulo my question/nit about the guix build).

fanquake commented at 9:48 am on November 21, 2024: member

Sure, but it’d be good to get a better explanation as to how Clang builds in oss-fuzz are broken, if Clang builds outside of oss-fuzz are working.

dergoegge commented at 9:56 am on November 21, 2024: member

Here is a recent example of clang coverage working for me: http://bitcoind-fuzz.dergoegge.de:8000/bitcoin/harnesses/rpc/coverage_report/coverage/workdir/bitcoin/index.html

I’m slightly confused why oss-fuzz broke while my setup keeps working. Afaict I’m using the same flags and llvm-* tools as them…

edit: oss-fuzz uses coverage_utils.py from chromium (not sure for what) which I’m not using, which is also where the failure comes from.

hebasto commented at 10:28 am on November 21, 2024: member

I’m slightly confused why oss-fuzz broke while my setup keeps working. Afaict I’m using the same flags and llvm-* tools as them…

They use the -path-equivalence=... option:

0# Use path mapping, as $SRC directory from the builder is copied into $OUT/$SRC.
1PATH_EQUIVALENCE_ARGS="-path-equivalence=/,$OUT"

hebasto renamed this:
~~build: Use `-ffile-prefix-map` in release builds only~~
build: Fix coverage builds
on Nov 21, 2024

hebasto force-pushed on Nov 21, 2024

hebasto commented at 11:58 am on November 21, 2024: member

Reworked per feedback.

maflcko commented at 7:45 am on November 22, 2024: member

Fixed the docs in https://github.com/llvm/llvm-project/commit/994c544c18c86cbdb6536aae5d27ef7e2f592486

fanquake commented at 9:56 am on November 22, 2024: member

which inadvertently broke coverage builds for both Clang’s source-based code coverage

Can you clarify that this is only for oss-fuzz, due to its use of other options and a third party script, otherwise, source based code coverage using Clang currently still works fine with master (see: #31337 (comment)). Similarly, in the inline comment you’ve added, can you be a bit more specific than “can cause issues”; as we seemingly know what the issues are.

build: Avoid using the `-ffile-prefix-map` compiler option

The `-ffile-prefix-map` compiler option implies `-fprofile-prefix-map`
on GCC or `-fcoverage-prefix-map` on Clang, which can lead to issues
with coverage builds.

This change applies only the options necessary for build reproducibility
and accurate source location messages.

01a7298818

hebasto force-pushed on Nov 22, 2024

hebasto commented at 10:21 am on November 22, 2024: member

which inadvertently broke coverage builds for both Clang’s source-based code coverage

Can you clarify that this is only for oss-fuzz, due to its use of other options and a third party script, otherwise, source based code coverage using Clang currently still works fine with master (see: #31337 (comment)). Similarly, in the inline comment you’ve added, can you be a bit more specific than “can cause issues”; as we seemingly know what the issues are.

Thanks! Updated.

maflcko commented at 2:26 pm on November 22, 2024: member

Tested via https://github.com/maflcko/DrahtBot/blob/main/coverage_fuzz/src/main.rs (GCC coverage):

Commit 85224f92d52356483a4e45a1735db02b8612dea6~1: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/9719d373dc2fb0f2/d964b0cfa3dc534a/fuzz.coverage/index.html
Commit 01a7298818d7bd66bb8c800d7cd30ae0a4d3b1d1: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/01a7298818d7bd66/d964b0cfa3dc534a/fuzz.coverage/index.html
master: fails (see below)

 0[ 98%] Building CXX object src/test/fuzz/CMakeFiles/fuzz.dir/__/__/wallet/test/fuzz/wallet_bdb_parser.cpp.o
 1[100%] Linking CXX executable fuzz
 2[100%] Built target fuzz
 3Make coverage ...
 4Capturing coverage data from src
 5geninfo cmd: '/usr/bin/geninfo src --output-filename baseline.info --gcov-tool ./monotree/build/cov_tool_wrapper.sh --ignore-errors mismatch --ignore-errors mismatch --ignore-errors inconsistent --ignore-errors inconsistent --initial --rc branch_coverage=1 --branch-coverage'
 6Found gcov version: 14.2.0
 7Using intermediate gcov format
 8Recording 'internal' directories:
 9	./monotree/build/src
10Writing temporary data to /tmp/geninfo_datRtE7
11Scanning src for .gcno files ...
12Found 395 graph files in src
13using: chunkSize: 1, nchunks:395, intervalLength:19
14geninfo: WARNING: (gcov) GCOV did not produce any data for ./monotree/build/src/CMakeFiles/bitcoin_node.dir/policy/settings.cpp.gcno
15	(use "geninfo --ignore-errors gcov,gcov ..." to suppress this warning)
16geninfo: WARNING: (gcov) GCOV did not produce any data for ./monotree/build/src/CMakeFiles/bitcoin_node.dir/deploymentstatus.cpp.gcno
17geninfo: WARNING: (gcov) GCOV did not produce any data for ./monotree/build/src/util/CMakeFiles/bitcoin_util.dir/__/sync.cpp.gcno
18Finished processing 395 GCNO files
19Apply filtering..
20Message summary:
21  1 error message:
22    source: 1
23  3 warning messages:
24    gcov: 3
25  16389 ignore messages:
26    inconsistent: 172
27    mismatch: 16217
28geninfo: ERROR: (source) unable to open ./monotree/build/src/test/util/netaddress.h: No such file or directory
29	(use "geninfo --ignore-errors source ..." to bypass this error)
30CMake Error at build/CoverageInclude.cmake:45 (execute_process):
31  execute_process failed command indexes:
32
33    1: "Child return code: 1"
34
35Call Stack (most recent call first):
36  build/CoverageFuzz.cmake:5 (include)

maflcko commented at 3:13 pm on November 22, 2024: member

source based code coverage using Clang currently still works fine with master (see: #31337 (comment)).

Would be nice to do it for this pull as well, to see if there is any difference.

hebasto commented at 3:18 pm on November 22, 2024: member

source based code coverage using Clang currently still works fine with master (see: #31337 (comment)).

Would be nice to do it for this pull as well, to see if there is any difference.

cc @dergoegge

dergoegge approved

dergoegge commented at 3:03 pm on November 26, 2024: member

tACK 01a7298818d7bd66bb8c800d7cd30ae0a4d3b1d1

jonatack commented at 3:11 pm on November 26, 2024: member

Is this fix related to the “no coverage data” results in https://corecheck.dev (i.e. https://corecheck.dev/bitcoin/bitcoin/pulls/31337), or is that a different issue?

dergoegge commented at 3:19 pm on November 26, 2024: member

Is this fix related to the “no coverage data” results in https://corecheck.dev/ (i.e. https://corecheck.dev/bitcoin/bitcoin/pulls/31337), or is that a different issue?

I think the reason for “no coverage data” on corecheck’s site for this PR is that this PR doesn’t change any source files covered by any tests. It does show coverage changes under “Lost/Gained baseline coverage” but those are likely caused by non-determinism in the tests.

This PR was triggered by coverage builds on oss-fuzz being broken.

in CMakeLists.txt:442 in 01a7298818

435@@ -436,8 +436,16 @@ configure_file(contrib/filter-lcov.py filter-lcov.py USE_SOURCE_PERMISSIONS COPY
436 # Don't allow extended (non-ASCII) symbols in identifiers. This is easier for code review.
437 try_append_cxx_flags("-fno-extended-identifiers" TARGET core_interface SKIP_LINK)
438 
439-try_append_cxx_flags("-ffile-prefix-map=A=B" TARGET core_interface SKIP_LINK
440-  IF_CHECK_PASSED "-ffile-prefix-map=${PROJECT_SOURCE_DIR}/src=."
441+# Avoiding the `-ffile-prefix-map` compiler option because it implies
442+# `-fcoverage-prefix-map` on Clang or `-fprofile-prefix-map` on GCC,
443+# which can cause issues with coverage builds, particularly when using
444+# Clang in the OSS-Fuzz environment due to its use of other options

fanquake commented at 9:55 am on November 27, 2024:

I’m not sure this is a good inline comment, because it doesn’t actually document the problem, just the side-effects.

hebasto commented at 9:57 am on November 27, 2024:

Mind suggesting a better wording for the comment?

fanquake commented at 2:12 pm on November 27, 2024:

I don’t really have one, given it’s a somewhat obscure issue, but which is also why it’d be good to have proper documentation for. Anyone reading this in future isn’t going to have any information other than, “don’t use ffile-prefix-map otherwise it might break something in oss-fuzz”. However given how long the build has been broken now, going to merge this to unbreak it.

maflcko commented at 10:12 am on November 27, 2024: member

review ACK 01a7298818d7bd66bb8c800d7cd30ae0a4d3b1d1

fanquake merged this on Nov 27, 2024

fanquake closed this on Nov 27, 2024

hebasto deleted the branch on Nov 27, 2024

build: Fix coverage builds #31337

Code Coverage & Benchmarks

Reviews