build: RFC Coverage build type #31047

issue fanquake openend this issue on October 7, 2024
  1. fanquake commented at 11:01 am on October 7, 2024: member

    Porting this issue from: https://github.com/hebasto/bitcoin/issues/341.

    It’s not clear if using our Coverage build type works with Clang, or not. In the linked thread there are simultaneous claims that it “works”, but also that it does not work. It seems from the discussion that even using GCC with the Coverage build type is flaky.

    There’s been suggestions to add a Coverage mode for Clang: https://github.com/hebasto/bitcoin/pull/233, however adding a second way of doing things, when the current way may not work properly, and (likely?) currently isn’t being used by anyone, doesn’t seem like a good approach, and adds even more complication to the build system. Note that devs are already using Clang for coverage regardless of if it currently exists in the build system (cc @dergoegge, @marcofleon, @vasild).

    If using Clangs native coverage mode is what is preferred/is “better”, then it’d seem better for us to replace the current, more GCC focussed implementation, with one that is geared towards LLVM/Clang, if the idea is to provide something that works out-of-the-box, and is generally used by the developers working on the project.

  2. fanquake added the label Brainstorming on Oct 7, 2024
  3. fanquake added the label Build system on Oct 7, 2024
  4. dergoegge commented at 11:28 am on October 7, 2024: member

    I haven’t tried after the transition to CMake but I always had trouble with the build in coverage build mode (e.g. make cov). Usually there was problem with path prefixes or a lcov version mismatch. The coverage reports themselves were always hard to read and I could not find any documentation on how to read them, for example:

    Screenshot from 2024-10-07 12-18-00

    How is branch coverage supposed to be interpreted from this?

    I’ve switched all my coverage reports to using LLVM’s native coverage tooling: https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#id3. I don’t use our build in command to produce coverage reports but I think it’s a useful thing to have, so devs can easily produce their own coverage reports. I can recommend switching to LLVM’s native coverage reporting, as that has been far less of a struggle in my experience.

  5. vasild commented at 11:32 am on October 7, 2024: contributor

    For me it seems best to use each compiler’s native workflow. That is: gcov+lcov if using GCC and the source based coverage if using Clang.

    Would be nice if that is integrated into the build system so it boils down to just cmake --i-want-coverage, but if not, then some good instructions in doc/ would be fine as well.

  6. fanquake commented at 11:36 am on October 7, 2024: member

    For me it seems best to use each compiler’s native workflow.

    Sure, anyone can still use whatever they like. However our build system doesn’t need to (and shouldn’t have to) handle all possible combinations of compiler + tooling. Providing a single convenience build type that is the same as what is used by the majority of the developers seems fine, however I don’t think we need to natively support anything outside of that, given it’s unlikely to be tested/used, or work properly, and just futher complicates our build system.

  7. vasild commented at 1:12 pm on October 7, 2024: contributor

    our build system doesn’t need to (and shouldn’t have to) handle all possible combinations of compiler + tooling

    It is just two:

    1. gcc compiler + gcov/lcov and
    2. clang compiler + llvm-cov (source based).

    (I think it is a waste of time and we should ignore a third option where we try to use the gcov/lcov workflow with clang).

    Providing a single convenience build type that is the same as what is used by the majority of the developers seems fine

    I do not object this, but then if only 1. or only 2. is supported then the coverage build should produce an error if the compiler is the other one.

  8. fanquake commented at 1:17 pm on October 7, 2024: member

    It is just two:

    It’s not though, because gcc compiler + gcov/lcov already encompasses more than 1 thing. i.e supporting different versions of gcov/lcov across releases.

  9. marcofleon commented at 5:40 pm on October 7, 2024: contributor
    I don’t have a strong preference for which one is supported by the build system. I happened to learn about llvm-cov before coming upon the coverage section in the docs, so that’s what I’ve been using on both macOS and Linux. It’s been working well for me so far.
  10. fanquake commented at 8:55 am on October 10, 2024: member
    cc @maflcko. Do you have an opinion here?
  11. maflcko commented at 11:54 am on October 10, 2024: member
    I liked the summary stats per folder produced by the current approach, but if others like other stuff better, I don’t mind.
  12. dergoegge commented at 12:54 pm on October 10, 2024: member

    I liked the summary stats per folder produced by the current approach, but if others like other stuff better, I don’t mind.

    The LLVM approach also supports that:

    Screenshot from 2024-10-10 13-52-58

    edit: this is possible with the -show-directory-coverage option for llvm-cov show.

  13. vasild commented at 3:43 pm on October 10, 2024: contributor

    I liked the summary stats per folder produced by the current approach, but if others like other stuff better, I don’t mind.

    Maybe I misunderstand, but somehow the above implies that we have to commit to one and give up on the other method. But one can create gcc or clang coverages regardless of what is integrated into the cmake build system. That integration would be just a convenience and we could have no integration, or just gcc (the current master), or just clang or both gcc and clang.

  14. maflcko commented at 4:27 pm on October 10, 2024: member
    Yeah. I guess I don’t really mind either way. As long as there is at least one documented, tested and working solution.
  15. maflcko commented at 11:51 am on October 28, 2024: member
    I switched https://maflcko.github.io/b-c-cov/ to GCC, just to get something working again. But the config is open source and pull requests with changes are welcome: https://github.com/maflcko/b-c-cov
  16. fanquake commented at 11:58 am on November 19, 2024: member

    Apparently code coverage on oss-fuzz has been broken since CMake. See https://issues.oss-fuzz.com/issues/379122777. Full logs (https://oss-fuzz-build-logs.storage.googleapis.com/log-6b12c5ff-d6c4-441c-a10f-2dffff96e82c.txt):

     0Step [#7](/bitcoin-bitcoin/7/): error: /workspace/out/libfuzzer-coverage-x86_64/src/bitcoin-core/build_fuzz/src/wallet/wallet/walletdb.h: No such file or directory
     1Step [#7](/bitcoin-bitcoin/7/): warning: The file '/src/bitcoin-core/build_fuzz/src/wallet/wallet/walletdb.h' isn't covered.
     2Step [#7](/bitcoin-bitcoin/7/): error: /workspace/out/libfuzzer-coverage-x86_64/src/bitcoin-core/build_fuzz/src/wallet/wallet/rpc/wallet.cpp: No such file or directory
     3Step [#7](/bitcoin-bitcoin/7/): warning: The file '/src/bitcoin-core/build_fuzz/src/wallet/wallet/rpc/wallet.cpp' isn't covered.
     4Step [#7](/bitcoin-bitcoin/7/): error: /workspace/out/libfuzzer-coverage-x86_64/src/bitcoin-core/build_fuzz/src/wallet/util/task_runner.h: No such file or directory
     5Step [#7](/bitcoin-bitcoin/7/): warning: The file '/src/bitcoin-core/build_fuzz/src/wallet/util/task_runner.h' isn't covered.
     6Step [#7](/bitcoin-bitcoin/7/): warning: 60 functions have mismatched data
     7Step [#7](/bitcoin-bitcoin/7/): [2024-11-17 06:58:59,574 DEBUG] Finished generating per-file code coverage summary.
     8Step [#7](/bitcoin-bitcoin/7/): [2024-11-17 06:58:59,574 DEBUG] Generating file view html index file as: "/workspace/out/libfuzzer-coverage-x86_64/report/linux/file_view_index.html".
     9Step [#7](/bitcoin-bitcoin/7/): Traceback (most recent call last):
    10Step [#7](/bitcoin-bitcoin/7/):   File "/opt/code_coverage/coverage_utils.py", line 829, in <module>
    11Step [#7](/bitcoin-bitcoin/7/):     sys.exit(Main())
    12Step [#7](/bitcoin-bitcoin/7/):   File "/opt/code_coverage/coverage_utils.py", line 823, in Main
    13Step [#7](/bitcoin-bitcoin/7/):     return _CmdPostProcess(args)
    14Step [#7](/bitcoin-bitcoin/7/):   File "/opt/code_coverage/coverage_utils.py", line 780, in _CmdPostProcess
    15Step [#7](/bitcoin-bitcoin/7/):     processor.PrepareHtmlReport()
    16Step [#7](/bitcoin-bitcoin/7/):   File "/opt/code_coverage/coverage_utils.py", line 577, in PrepareHtmlReport
    17Step [#7](/bitcoin-bitcoin/7/):     self.GenerateFileViewHtmlIndexFile(per_file_coverage_summary,
    18Step [#7](/bitcoin-bitcoin/7/):   File "/opt/code_coverage/coverage_utils.py", line 450, in GenerateFileViewHtmlIndexFile
    19Step [#7](/bitcoin-bitcoin/7/):     self.GetCoverageHtmlReportPathForFile(file_path),
    20Step [#7](/bitcoin-bitcoin/7/):   File "/opt/code_coverage/coverage_utils.py", line 422, in GetCoverageHtmlReportPathForFile
    21Step [#7](/bitcoin-bitcoin/7/):     assert os.path.isfile(
    22Step [#7](/bitcoin-bitcoin/7/): AssertionError: "/src/bitcoin-core/build_fuzz/crc32c/include/crc32c/crc32c.h" is not a file.
    23Step [#7](/bitcoin-bitcoin/7/): ********************************************************************************
    24Step [#7](/bitcoin-bitcoin/7/): Code coverage report generation failed.
    25Step [#7](/bitcoin-bitcoin/7/): To reproduce, run:
    26Step [#7](/bitcoin-bitcoin/7/): python infra/helper.py build_image bitcoin-core
    27Step [#7](/bitcoin-bitcoin/7/): python infra/helper.py build_fuzzers --sanitizer coverage bitcoin-core
    28Step [#7](/bitcoin-bitcoin/7/): python infra/helper.py coverage bitcoin-core
    29Step [#7](/bitcoin-bitcoin/7/): ********************************************************************************
    30Finished Step [#7](/bitcoin-bitcoin/7/)
    31ERROR
    
  17. fanquake added this to the milestone 29.0 on Nov 25, 2024
  18. glozow commented at 4:51 pm on February 13, 2025: member
    @hebasto @theuni can you open a PR to prune the gcc stuff for v29?
  19. theuni commented at 5:44 pm on February 13, 2025: member
    Will do.
  20. theuni commented at 7:54 pm on February 13, 2025: member

    Hmm, I now see that @maflcko is still using the gcc coverage, so nuking it would break https://maflcko.github.io/b-c-cov/ . @maflcko since you weren’t on today’s v29 milestone call: the consensus was to nuke the current gcc coverage support for v29 since it’s “not working”, and try to get llvm support in in its place, but that part wouldn’t be a v29 release blocker.

    But apparently it’s working enough for you so I suspect you’d disagree with that?

  21. maflcko commented at 7:22 am on February 14, 2025: member

    Looks like the discussion is a bit scattered around the place. No strong opinion, but some notes (or a summary):

    • A pull request is already open and I left a comment there to list the stuff I noticed that is using GCC coverage: #31394 (comment)
    • Coverage on debuginfo-level (post-optimization) surely is different than coverage on source-code-level, but I wouldn’t call it “broken”.
    • All of this is mostly a test-only dev-only issue, so I don’t see the connection to a release milestone.

    Again, no strong opinion, but my recommendation would be:

    • Remove the milestone from this issue (and the pull request), as there is no regression, nor a blocker, nor a user-facing issue.
    • Figure out what the coverage workflow should look like. (Should there be a coverage build type or is it sufficient and more flexible to just document the cmake configure flags as well as handling of the coverage data? Should there be gcc support at all?)
    • Implement a pull based on that, along with a quick note on the required changes to affected places
    • Review and merge the pull on its own pace
  22. fanquake commented at 10:09 am on February 14, 2025: member

    to list the stuff I noticed that is using GCC coverage:

    The only thing mentioned here is corecheck. Is there anything else (other than your use)?

    Figure out what the coverage workflow should look like. “Should there be gcc support at all?”

    If we are currently unsure what our coverage support should even look like, I’m not sure we should ship something just because it exists, maybe just to delete it in the next release; that’s why it’s attached to a milestone. It seems reasonable to come to some sort of conslusion now.

    Can you elaborate on “all the GCC/lcov/gcov problems” you mentioned here: #31588 (comment)? Seems like there is also some agreement there to switch to LLVM/Clang.

  23. vasild commented at 10:33 am on February 14, 2025: contributor
    I tried the coverage instructions from doc/developer-notes.md and gcc/gcov generated a reasonable report. It does not look broken. No reason to remove a working, documented and used feature, with urgency, in 29.
  24. maflcko commented at 10:39 am on February 14, 2025: member

    Can you elaborate on “all the GCC/lcov/gcov problems” you mentioned here: #31588 (comment)? Seems like there is also some agreement there to switch to LLVM/Clang.

    I don’t use macOS, nor have I tested this specific bash script on the different Linux distros mentioned, so I can’t vouch for the problems, but it looks like some people ran into problems and shared them in the thread. In any case that specific script seems to be a dev-only test-only contrib script with 0(?) users right now, where at least the suppression list is stale. So I removed it from the milestone in #31588 (comment). I’ll follow-up with my follow-up, but earliest I can do is next week or later.

    I’m not sure we should ship something just because it exists, maybe just to delete it in the next release; that’s why it’s attached to a milestone.

    It not only exists, but it is also used. For example:

    If those use cases are considered invalid, then removing it seems fine. However, if others think that those are useful use-cases, then my recommendation would be to first update them and then remove the feature.

    If there are any risks or downsides in removing the milestone from this issue, it would help to list them. If it is just a documentation problem that the “feature” will be removed, it should be trivial to fix:

    sed -i 's/Coverage/DangerCoverageExperimentalWillBeRemoved/g' ...

  25. fanquake commented at 10:41 am on February 14, 2025: member
    Looking at Corecheck, does it use our Coverage.cmake? I can’t find it being used anywhere there, seems like only calls to gcovr directly.
  26. maflcko commented at 4:05 pm on February 17, 2025: member
    It is using the coverage build type, which this issue is about. In any case, all mentioned projects are open-source, and I am happy to review pull requests, if there are any. Removing the cmake scripts may be fine, but I don’t see it as a release blocker.
  27. maflcko removed this from the milestone 29.0 on Feb 21, 2025
  28. maflcko commented at 10:58 am on February 21, 2025: member
    Removing the milestone for now. I am happy to review pull requests or suggestions, if there are any.
  29. hebasto commented at 3:11 pm on February 21, 2025: member

    From the OP:

    It’s not clear if using our Coverage build type works with Clang, or not.

    There are a few points to take into consideration:

    1. Modern LLVM’s coverage tool emulates GCC’s gcov version 4.2.0, which is obviously outdated.
    2. Recent versions of the lcov tool no longer support GCC/gcov versions older than 9.x. For example:
    0lcov: WARNING: (unsupported) Function begin/end line exclusions not supported with this version of GCC/gcov; require gcc/9 or newer: attempting to derive function end lines - see lcovrc man entry for 'derive_function_end_line'.
    

    Thus, I must conclude that the answer is “No.”

    The current “Coverage” build type should be considered and documented as GCC-only.


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-02-22 15:12 UTC

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