build: Fix coverage builds #31337

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:241120-prefix-map changing 1 files +10 −2
  1. hebasto commented at 9:41 pm on November 20, 2024: member

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

    1. For GCC. See #31337 (comment).
    2. 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:

    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.

  2. hebasto added the label Build system on Nov 20, 2024
  3. 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.

  4. 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:
  5. 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
  6. maflcko added this to the milestone 29.0 on Nov 21, 2024
  7. 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).

  8. 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.
  9. 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).

  10. 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.
  11. 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.

  12. 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"
    
  13. hebasto renamed this:
    build: Use `-ffile-prefix-map` in release builds only
    build: Fix coverage builds
    on Nov 21, 2024
  14. hebasto force-pushed on Nov 21, 2024
  15. hebasto commented at 11:58 am on November 21, 2024: member
    Reworked per feedback.
  16. maflcko commented at 7:45 am on November 22, 2024: member
  17. 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.

  18. 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
  19. hebasto force-pushed on Nov 22, 2024
  20. 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.

  21. 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):

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

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

  24. dergoegge approved
  25. dergoegge commented at 3:03 pm on November 26, 2024: member
    tACK 01a7298818d7bd66bb8c800d7cd30ae0a4d3b1d1
  26. 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?
  27. 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.

  28. 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.
  29. maflcko commented at 10:12 am on November 27, 2024: member
    review ACK 01a7298818d7bd66bb8c800d7cd30ae0a4d3b1d1
  30. fanquake merged this on Nov 27, 2024
  31. fanquake closed this on Nov 27, 2024

  32. hebasto deleted the branch on Nov 27, 2024

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: 2024-12-03 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me